On 11/22/18 19:14, Ard Biesheuvel wrote: > On Thu, 22 Nov 2018 at 19:09, Laszlo Ersek <[email protected]> wrote: >> >> On 11/22/18 18:26, Ard Biesheuvel wrote: >>> BdsLoadImage () is part of the BdsLib library API and is not documented >>> as modifying its DevicePath argument, but does so nonetheless. So take >>> a copy instead, and free it after use. >>> >>> Contributed-under: TianoCore Contribution Agreement 1.1 >>> Signed-off-by: Ard Biesheuvel <[email protected]> >>> --- >>> Platform/ARM/Library/BdsLib/BdsFilePath.c | 13 ++++++++++++- >>> 1 file changed, 12 insertions(+), 1 deletion(-) >>> >>> diff --git a/Platform/ARM/Library/BdsLib/BdsFilePath.c >>> b/Platform/ARM/Library/BdsLib/BdsFilePath.c >>> index 67dafa4f3651..74fdbbee773d 100644 >>> --- a/Platform/ARM/Library/BdsLib/BdsFilePath.c >>> +++ b/Platform/ARM/Library/BdsLib/BdsFilePath.c >>> @@ -1351,5 +1351,16 @@ BdsLoadImage ( >>> OUT UINTN *FileSize >>> ) >>> { >>> - return BdsLoadImageAndUpdateDevicePath (&DevicePath, Type, Image, >>> FileSize); >>> + EFI_DEVICE_PATH *Path; >>> + EFI_STATUS Status; >>> + >>> + Path = DuplicateDevicePath (DevicePath); >>> + if (Path == NULL) { >>> + return EFI_OUT_OF_RESOURCES; >>> + } >> >> This introduces a minor change in behavior. >> >> Previously, if BdsLoadImage() got DevicePath==NULL, then >> BdsLoadImageAndUpdateDevicePath() -> BdsConnectAndUpdateDevicePath() >> would hit (*DevicePath == NULL), and return EFI_INVALID_PARAMETER. >> >> Now, (DevicePath==NULL) causes DuplicateDevicePath() to return NULL, and >> we translate that to EFI_OUT_OF_RESOURCES. >> >> Can you check for (DevicePath==NULL) first, and preserve >> EFI_INVALID_PARAMETER? >> >>> + >>> + Status = BdsLoadImageAndUpdateDevicePath (&Path, Type, Image, FileSize); >>> + FreePool (Path); >> >> This is not safe; BdsLoadImageAndUpdateDevicePath() may change Path. >> Namely, in BdsConnectAndUpdateDevicePath(), we have at one location, >> >> *DevicePath = NewDevicePath; >> >> ... Which, in fact, makes me wonder whether we need this patch at all. I >> believe BdsLoadImageAndUpdateDevicePath() -- and >> BdsConnectAndUpdateDevicePath() -- are supposed to update the caller's >> *pointer* to the device path, and not the pointed-to device path itself. >> >> Do you agree? >> > > Indeed. > > EFI_STATUS > BdsLoadImage ( > IN EFI_DEVICE_PATH *DevicePath, > > vs > > EFI_STATUS > BdsLoadImageAndUpdateDevicePath ( > IN OUT EFI_DEVICE_PATH **DevicePath, > > and I didn't spot the diference in * vs ** > > So you are right: BdsConnectAndUpdateDevicePath() assigns to > *DevicePath, which means it updates BdsLoadImage()'s local copy of the > pointer, but not the memory it points to. > > The IN/OUT notation makes this a bit ambiguous, though. Having > something like EFI_DEVICE_PATH CONST ** vs EFI_DEVICE_PATH * CONST * > is not necessarily easier to read, but less ambiguous. >
Exactly! Laszlo _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
