Re: [PATCH edk2-platforms v5 0/2] add platform boot options

[Laszlo Ersek <lersek@redhat.com>]

On 04/26/18 05:12, Haojian Zhuang wrote:
> On 26 April 2018 at 00:10, Laszlo Ersek <lersek@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