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. _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
