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

[Laszlo Ersek <lersek@redhat.com>]

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...

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.

> @@ -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
Laszlo