Re: [PATCH 1/2] ArmPlatformPkg/ArmVExpressFastBootDxe: eliminate deprecated string functions

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On 26 October 2016 at 11:32, Laszlo Ersek <lersek@redhat.com> wrote:
> On 10/25/16 20:17, Ard Biesheuvel wrote:
>> Get rid of functions that are no longer available when defining
>> DISABLE_NEW_DEPRECATED_INTERFACES
>>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>>  ArmPlatformPkg/ArmVExpressPkg/ArmVExpressFastBootDxe/ArmVExpressFastBoot.c | 8 ++++----
>>  1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/ArmPlatformPkg/ArmVExpressPkg/ArmVExpressFastBootDxe/ArmVExpressFastBoot.c b/ArmPlatformPkg/ArmVExpressPkg/ArmVExpressFastBootDxe/ArmVExpressFastBoot.c
>> index 4d0811cc5eaf..6b39682948aa 100644
>> --- a/ArmPlatformPkg/ArmVExpressPkg/ArmVExpressFastBootDxe/ArmVExpressFastBoot.c
>> +++ b/ArmPlatformPkg/ArmVExpressPkg/ArmVExpressFastBootDxe/ArmVExpressFastBoot.c
>> @@ -269,7 +269,7 @@ ArmFastbootPlatformInit (
>>
>>        // Copy handle and partition name
>>        Entry->PartitionHandle = AllHandles[LoopIndex];
>> -      StrnCpy (
>> +      CopyMem (
>>          Entry->PartitionName,
>>          PartitionEntries[PartitionNode->PartitionNumber - 1].PartitionName, // Partition numbers start from 1.
>>          PARTITION_NAME_MAX_LENGTH
>
> okay
>
>> @@ -320,7 +320,7 @@ ArmFastbootPlatformFlashPartition (
>>    CHAR16                   PartitionNameUnicode[60];
>>    BOOLEAN                  PartitionFound;
>>
>> -  AsciiStrToUnicodeStr (PartitionName, PartitionNameUnicode);
>> +  AsciiStrToUnicodeStrS (PartitionName, PartitionNameUnicode, 60);
>>
>>    PartitionFound = FALSE;
>>    Entry = (FASTBOOT_PARTITION_LIST *) GetFirstNode (&(mPartitionListHead));
>
> You asked me to introduce a macro for a very similar case in one of my
> ArmPkg patches...
>

You are right, my apologies. In my defense, ArmPkg is something we
consider maintained, whereas ArmPlatformPkg is a collection of cruft
which we would like to phase out as soon as we can.

> Anyway, the change is valid.
>
>> @@ -396,7 +396,7 @@ ArmFastbootPlatformGetVar (
>>    )
>>  {
>>    if (AsciiStrCmp (Name, "product")) {
>> -    AsciiStrCpy (Value, FixedPcdGetPtr (PcdFirmwareVendor));
>> +    AsciiStrCpyS (Value, 60, FixedPcdGetPtr (PcdFirmwareVendor));
>>    } else {
>>      *Value = '\0';
>>    }
>
> This is wrong.
>
> The signature of this function does not indicate the expected size of
> the receiving buffer. However, the function is a
> FASTBOOT_PLATFORM_GETVAR implementation (==
> FASTBOOT_PLATFORM_PROTOCOL.GetVar() member implementation). The leading
> comment on that function pointer type says,
>
>   Variable names and values may not be larger than 60 bytes, excluding the
>   terminal null character. This is a limitation of the Fastboot protocol.
>
> I.e., 60 non-NUL bytes, plus the terminating NUL, is a valid variable
> value. Therefore the DestMax parameter should be 61.
>
> Just to be sure, I checked the call sites. There is only one call site
> actually, in
> "EmbeddedPkg/Application/AndroidFastboot/AndroidFastbootApp.c", function
> HandleGetVar():
>
>   CHAR8      Response[FASTBOOT_COMMAND_MAX_LENGTH + 1] = "OKAY";
> ...
>     Status = mPlatform->GetVar (CmdArg, Response + 4);
>
> FASTBOOT_COMMAND_MAX_LENGTH is 64 (same file), therefore (Response + 4)
> points to a (sub-)array of 61 characters. IOW, the call site is
> consistent with the protocol definition, and the DestMax param should be
> bumped to 61.
>

Thanks for spotting that.

>> @@ -410,7 +410,7 @@ ArmFastbootPlatformOemCommand (
>>  {
>>    CHAR16 CommandUnicode[65];
>>
>> -  AsciiStrToUnicodeStr (Command, CommandUnicode);
>> +  AsciiStrToUnicodeStrS (Command, CommandUnicode, 65);
>>
>>    if (AsciiStrCmp (Command, "Demonstrate") == 0) {
>>      DEBUG ((EFI_D_ERROR, "ARM OEM Fastboot command 'Demonstrate' received.\n"));
>>
>
> This is correct.
>

Thanks,
Ard.