On 04/26/18 05:12, Haojian Zhuang wrote:
> On 26 April 2018 at 00:10, Laszlo Ersek <ler...@redhat.com> wrote:
>> (4) In both patches, in both of the CreatePlatformBootOptionFromPath()
>> and CreatePlatformBootOptionFromGuid() helper functions, the
>> "DevicePath" variable is leaked when
>> EfiBootManagerInitializeLoadOption() fails.
>>
>> "DevicePath" should be freed unconditionally at that point, in both
>> patches and in both helper functions (4 instances in total). Simply
>> eliminate the following:
>>
>> if (EFI_ERROR (Status)) {
>> return Status;
>> }
>>
>> and you will be left with:
>>
>> Status = EfiBootManagerInitializeLoadOption (
>> ...
>> );
>> FreePool (DevicePath);
>> return Status;
>>
>> I believe I need not separately review this update either.
>>
>
> For this one, I have a different opinion.
>
> EFI_STATUS
> EFIAPI
> EfiBootManagerInitializeLoadOption (
> ...
> )
> {
> if ((Option == NULL) || (Description == NULL) || (FilePath == NULL)) {
> return EFI_INVALID_PARAMETER;
> }
>
> if (((OptionalData != NULL) && (OptionalDataSize == 0)) ||
> ((OptionalData == NULL) && (OptionalDataSize != 0))) {
> return EFI_INVALID_PARAMETER;
> }
> if ((UINT32) OptionType >= LoadOptionTypeMax) {
> return EFI_INVALID_PARAMETER;
> }
>
> ZeroMem (Option, sizeof (EFI_BOOT_MANAGER_LOAD_OPTION));
> Option->OptionNumber = OptionNumber;
> Option->OptionType = OptionType;
> Option->Attributes = Attributes;
> Option->Description = AllocateCopyPool (StrSize
> (Description), Description);
> Option->FilePath = DuplicateDevicePath (FilePath);
> if (OptionalData != NULL) {
> Option->OptionalData = AllocateCopyPool (OptionalDataSize,
> OptionalData);
> Option->OptionalDataSize = OptionalDataSize;
> }
>
> return EFI_SUCCESS;
> }
>
> We can find that no memory is allocated to "DevicePath" if it returns
> with error.
>
> So we shouldn't free memory on "DevicePath" if "Status" is error code.
I disagree. We have:
CreatePlatformBootOptionFromPath()
DevicePath = ConvertTextToDevicePath(...) #1
EfiBootManagerInitializeLoadOption(... DevicePath ...)
DuplicateDevicePath(FilePath=DevicePath) #2
The ConvertTextToDevicePath() function returns the binary representation
of the device path in a dynamically allocated area. That is dynamic
object #1, tracked by the "DevicePath" variable. Then,
EfiBootManagerInitializeLoadOption() creates a deep copy, by calling the
DuplicateDevicePath() function. That creates dynamic object #2, tracked
by Option->FilePath.
- If EfiBootManagerInitializeLoadOption() returns with failure, then
dynamic object #2 does not exist, and we no longer need dynamic object
#1, so we have to free dynamic object #1.
- If EfiBootManagerInitializeLoadOption() returns with success, then
dynamic object #2 exists, and is correctly tracked by Option->FilePath.
We no longer need dynamic object #1, so we have to free it.
In other words, regardless of the return status of
EfiBootManagerInitializeLoadOption(), we must release dynamic object #1.
We need dynamic object #1 only temporarily, so we can call
EfiBootManagerInitializeLoadOption(), and let it make a copy.
The same applies to CreatePlatformBootOptionFromGuid(), except replace
ConvertTextToDevicePath() with AppendDevicePathNode():
CreatePlatformBootOptionFromGuid()
DevicePath = AppendDevicePathNode(...) #1
EfiBootManagerInitializeLoadOption (... DevicePath ...)
DuplicateDevicePath(FilePath=DevicePath) #2
Again, AppendDevicePathNode() produces dynamic object #1, similarly to
ConvertTextToDevicePath().
Thanks
Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
