diff mbox

[edk2,1/2] ArmPlatformPkg/ArmVExpressFastBootDxe: eliminate deprecated string functions

Message ID 1477419424-22235-2-git-send-email-ard.biesheuvel@linaro.org
State Superseded
Headers show

Commit Message

Ard Biesheuvel Oct. 25, 2016, 6:17 p.m. UTC 
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(-)

-- 
2.7.4

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Comments

Laszlo Ersek Oct. 26, 2016, 10:32 a.m. UTC | #1 
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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Ard Biesheuvel Oct. 26, 2016, 10:34 a.m. UTC | #2 
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.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Laszlo Ersek Oct. 26, 2016, 11:28 a.m. UTC | #3 
On 10/26/16 12:34, Ard Biesheuvel wrote:
> 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.


Ah, okay; I didn't realize that. No need to change this hunk then (I
didn't request that anyway). It is correct after all (and beauty we
don't insist upon here, then).

Thanks
Laszlo

> 

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

> 


_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
diff mbox

Patch

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
@@ -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));
@@ -396,7 +396,7 @@  ArmFastbootPlatformGetVar (
   )
 {
   if (AsciiStrCmp (Name, "product")) {
-    AsciiStrCpy (Value, FixedPcdGetPtr (PcdFirmwareVendor));
+    AsciiStrCpyS (Value, 60, FixedPcdGetPtr (PcdFirmwareVendor));
   } else {
     *Value = '\0';
   }
@@ -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"));