[PATCH v2 0/2] ArmPlatformPkg: remove deprecated string function calls

[PATCH v2 0/2] ArmPlatformPkg: remove deprecated string function calls

[Ard Biesheuvel]

ArmPlatformPkg no longer has its own .dsc, but we can still clean up some
uses of deprecated string functions, so that users of ArmPlatformPkg can
define DISABLE_NEW_DEPRECATED_INTERFACES

v2:
- incorporated feedback from Laszlo

Ard Biesheuvel (2):
  ArmPlatformPkg/ArmVExpressFastBootDxe: eliminate deprecated string
    functions
  ArmPlatformPkg/BootMonFs: eliminate deprecated string functions

 ArmPlatformPkg/ArmVExpressPkg/ArmVExpressFastBootDxe/ArmVExpressFastBoot.c |  9 +++++----
 ArmPlatformPkg/FileSystem/BootMonFs/BootMonFsDir.c                         |  8 +++-----
 ArmPlatformPkg/FileSystem/BootMonFs/BootMonFsEntryPoint.c                  |  3 ++-
 ArmPlatformPkg/FileSystem/BootMonFs/BootMonFsOpenClose.c                   | 19 +++++++++++--------
 4 files changed, 21 insertions(+), 18 deletions(-)

-- 
2.7.4

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

[Ard Biesheuvel]

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 | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/ArmPlatformPkg/ArmVExpressPkg/ArmVExpressFastBootDxe/ArmVExpressFastBoot.c b/ArmPlatformPkg/ArmVExpressPkg/ArmVExpressFastBootDxe/ArmVExpressFastBoot.c
index 4d0811cc5eaf..64b25f8a8c45 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,8 @@ ArmFastbootPlatformFlashPartition (
   CHAR16                   PartitionNameUnicode[60];
   BOOLEAN                  PartitionFound;
 
-  AsciiStrToUnicodeStr (PartitionName, PartitionNameUnicode);
+  AsciiStrToUnicodeStrS (PartitionName, PartitionNameUnicode,
+    ARRAY_SIZE (PartitionNameUnicode));
 
   PartitionFound = FALSE;
   Entry = (FASTBOOT_PARTITION_LIST *) GetFirstNode (&(mPartitionListHead));
@@ -396,7 +397,7 @@ ArmFastbootPlatformGetVar (
   )
 {
   if (AsciiStrCmp (Name, "product")) {
-    AsciiStrCpy (Value, FixedPcdGetPtr (PcdFirmwareVendor));
+    AsciiStrCpyS (Value, 61, FixedPcdGetPtr (PcdFirmwareVendor));
   } else {
     *Value = '\0';
   }
@@ -410,7 +411,7 @@ ArmFastbootPlatformOemCommand (
 {
   CHAR16 CommandUnicode[65];
 
-  AsciiStrToUnicodeStr (Command, CommandUnicode);
+  AsciiStrToUnicodeStrS (Command, CommandUnicode, ARRAY_SIZE (CommandUnicode));
 
   if (AsciiStrCmp (Command, "Demonstrate") == 0) {
     DEBUG ((EFI_D_ERROR, "ARM OEM Fastboot command 'Demonstrate' received.\n"));
-- 
2.7.4

[PATCH v2 2/2] ArmPlatformPkg/BootMonFs: eliminate deprecated string functions

[Ard Biesheuvel]

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/FileSystem/BootMonFs/BootMonFsDir.c        |  8 +++-----
 ArmPlatformPkg/FileSystem/BootMonFs/BootMonFsEntryPoint.c |  3 ++-
 ArmPlatformPkg/FileSystem/BootMonFs/BootMonFsOpenClose.c  | 19 +++++++++++--------
 3 files changed, 16 insertions(+), 14 deletions(-)

diff --git a/ArmPlatformPkg/FileSystem/BootMonFs/BootMonFsDir.c b/ArmPlatformPkg/FileSystem/BootMonFs/BootMonFsDir.c
index 450a707f183c..64ea0ec68048 100644
--- a/ArmPlatformPkg/FileSystem/BootMonFs/BootMonFsDir.c
+++ b/ArmPlatformPkg/FileSystem/BootMonFs/BootMonFsDir.c
@@ -304,7 +304,6 @@ SetFileName (
   IN  CONST CHAR16         *FileName
   )
 {
-  CHAR16           TruncFileName[MAX_NAME_LENGTH];
   CHAR8            AsciiFileName[MAX_NAME_LENGTH];
   BOOTMON_FS_FILE  *SameFile;
 
@@ -314,9 +313,7 @@ SetFileName (
     FileName++;
   }
 
-  StrnCpy (TruncFileName, FileName, MAX_NAME_LENGTH - 1);
-  TruncFileName[MAX_NAME_LENGTH - 1] = 0;
-  UnicodeStrToAsciiStr (TruncFileName, AsciiFileName);
+  UnicodeStrToAsciiStrS (FileName, AsciiFileName, MAX_NAME_LENGTH);
 
   if (BootMonGetFileFromAsciiFileName (
         File->Instance,
@@ -327,7 +324,8 @@ SetFileName (
     return EFI_ACCESS_DENIED;
   } else {
     // OK, change the filename.
-    AsciiStrToUnicodeStr (AsciiFileName, File->Info->FileName);
+    AsciiStrToUnicodeStrS (AsciiFileName, File->Info->FileName,
+      (File->Info->Size - SIZE_OF_EFI_FILE_INFO) / sizeof (CHAR16));
     return EFI_SUCCESS;
   }
 }
diff --git a/ArmPlatformPkg/FileSystem/BootMonFs/BootMonFsEntryPoint.c b/ArmPlatformPkg/FileSystem/BootMonFs/BootMonFsEntryPoint.c
index 3d71760fef99..a1150856f6ba 100644
--- a/ArmPlatformPkg/FileSystem/BootMonFs/BootMonFsEntryPoint.c
+++ b/ArmPlatformPkg/FileSystem/BootMonFs/BootMonFsEntryPoint.c
@@ -98,7 +98,8 @@ BootMonGetFileFromAsciiFileName (
   {
     FileEntry = BOOTMON_FS_FILE_FROM_LINK_THIS (Entry);
     if (FileEntry->Info != NULL) {
-      UnicodeStrToAsciiStr (FileEntry->Info->FileName, OpenFileAsciiFileName);
+      UnicodeStrToAsciiStrS (FileEntry->Info->FileName, OpenFileAsciiFileName,
+        MAX_NAME_LENGTH);
       AsciiFileNameToCompare = OpenFileAsciiFileName;
     } else {
       AsciiFileNameToCompare = FileEntry->HwDescription.Footer.Filename;
diff --git a/ArmPlatformPkg/FileSystem/BootMonFs/BootMonFsOpenClose.c b/ArmPlatformPkg/FileSystem/BootMonFs/BootMonFsOpenClose.c
index af2fe514f044..ae10055255ff 100644
--- a/ArmPlatformPkg/FileSystem/BootMonFs/BootMonFsOpenClose.c
+++ b/ArmPlatformPkg/FileSystem/BootMonFs/BootMonFsOpenClose.c
@@ -101,7 +101,8 @@ WriteFileDescription (
   Description->Attributes = 1;
   Description->BlockStart = FileStart / BlockSize;
   Description->BlockEnd   = Description->BlockStart + (FileSize / BlockSize);
-  AsciiStrCpy (Description->Footer.Filename, FileName);
+  AsciiStrCpyS (Description->Footer.Filename,
+    sizeof Description->Footer.Filename, FileName);
 
 #ifdef MDE_CPU_ARM
   Description->Footer.Offset  = HW_IMAGE_FOOTER_OFFSET;
@@ -294,7 +295,7 @@ BootMonFsFlushFile (
   DiskIo    = Instance->DiskIo;
   BlockSize = Media->BlockSize;
 
-  UnicodeStrToAsciiStr (Info->FileName, AsciiFileName);
+  UnicodeStrToAsciiStrS (Info->FileName, AsciiFileName, MAX_NAME_LENGTH);
 
   // If the file doesn't exist then find a space for it
   if (File->HwDescription.RegionCount == 0) {
@@ -513,6 +514,7 @@ BootMonFsOpenFile (
   CHAR16               *Separator;
   CHAR8                *AsciiFileName;
   EFI_FILE_INFO        *Info;
+  UINTN                AsciiFileNameSize;
 
   if (This == NULL) {
     return EFI_INVALID_PARAMETER;
@@ -621,15 +623,16 @@ BootMonFsOpenFile (
   //
   // BootMonFs interface requires ASCII filenames
   //
-  AsciiFileName = AllocatePool (StrLen (Path) + 1);
+  AsciiFileNameSize = StrLen (Path) + 1;
+  if (AsciiFileNameSize > MAX_NAME_LENGTH) {
+    AsciiFileNameSize = MAX_NAME_LENGTH;
+  }
+  AsciiFileName = AllocatePool (AsciiFileNameSize);
   if (AsciiFileName == NULL) {
     Status = EFI_OUT_OF_RESOURCES;
     goto Error;
   }
-  UnicodeStrToAsciiStr (Path, AsciiFileName);
-  if (AsciiStrSize (AsciiFileName) > MAX_NAME_LENGTH) {
-   AsciiFileName[MAX_NAME_LENGTH - 1] = '\0';
-  }
+  UnicodeStrToAsciiStrS (Path, AsciiFileName, AsciiFileNameSize);
 
   if ((AsciiFileName[0] == '\0') ||
       (AsciiFileName[0] == '.' )    ) {
@@ -688,7 +691,7 @@ BootMonFsOpenFile (
 
     Info->FileSize     = BootMonFsGetImageLength (File);
     Info->PhysicalSize = BootMonFsGetPhysicalSize (File);
-    AsciiStrToUnicodeStr (AsciiFileName, Info->FileName);
+    AsciiStrToUnicodeStrS (AsciiFileName, Info->FileName, MAX_NAME_LENGTH);
 
     File->Info = Info;
     Info = NULL;
-- 
2.7.4

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

[Laszlo Ersek]

On 10/28/16 12:48, 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 | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/ArmPlatformPkg/ArmVExpressPkg/ArmVExpressFastBootDxe/ArmVExpressFastBoot.c b/ArmPlatformPkg/ArmVExpressPkg/ArmVExpressFastBootDxe/ArmVExpressFastBoot.c
> index 4d0811cc5eaf..64b25f8a8c45 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,8 @@ ArmFastbootPlatformFlashPartition (
>    CHAR16                   PartitionNameUnicode[60];
>    BOOLEAN                  PartitionFound;
>  
> -  AsciiStrToUnicodeStr (PartitionName, PartitionNameUnicode);
> +  AsciiStrToUnicodeStrS (PartitionName, PartitionNameUnicode,
> +    ARRAY_SIZE (PartitionNameUnicode));

Congratulations on the first genuine use of the now-central ARRAY_SIZE()
macro! :)

>  
>    PartitionFound = FALSE;
>    Entry = (FASTBOOT_PARTITION_LIST *) GetFirstNode (&(mPartitionListHead));
> @@ -396,7 +397,7 @@ ArmFastbootPlatformGetVar (
>    )
>  {
>    if (AsciiStrCmp (Name, "product")) {
> -    AsciiStrCpy (Value, FixedPcdGetPtr (PcdFirmwareVendor));
> +    AsciiStrCpyS (Value, 61, FixedPcdGetPtr (PcdFirmwareVendor));
>    } else {
>      *Value = '\0';
>    }

Right.

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

I knew ARRAY_SIZE() would be especially useful with these "safe string"
functions! :)

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Re: [PATCH v2 2/2] ArmPlatformPkg/BootMonFs: eliminate deprecated string functions

[Laszlo Ersek]

On 10/28/16 12:48, 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/FileSystem/BootMonFs/BootMonFsDir.c        |  8 +++-----
>  ArmPlatformPkg/FileSystem/BootMonFs/BootMonFsEntryPoint.c |  3 ++-
>  ArmPlatformPkg/FileSystem/BootMonFs/BootMonFsOpenClose.c  | 19 +++++++++++--------
>  3 files changed, 16 insertions(+), 14 deletions(-)
> 
> diff --git a/ArmPlatformPkg/FileSystem/BootMonFs/BootMonFsDir.c b/ArmPlatformPkg/FileSystem/BootMonFs/BootMonFsDir.c
> index 450a707f183c..64ea0ec68048 100644
> --- a/ArmPlatformPkg/FileSystem/BootMonFs/BootMonFsDir.c
> +++ b/ArmPlatformPkg/FileSystem/BootMonFs/BootMonFsDir.c
> @@ -304,7 +304,6 @@ SetFileName (
>    IN  CONST CHAR16         *FileName
>    )
>  {
> -  CHAR16           TruncFileName[MAX_NAME_LENGTH];
>    CHAR8            AsciiFileName[MAX_NAME_LENGTH];
>    BOOTMON_FS_FILE  *SameFile;
>  
> @@ -314,9 +313,7 @@ SetFileName (
>      FileName++;
>    }
>  
> -  StrnCpy (TruncFileName, FileName, MAX_NAME_LENGTH - 1);
> -  TruncFileName[MAX_NAME_LENGTH - 1] = 0;
> -  UnicodeStrToAsciiStr (TruncFileName, AsciiFileName);
> +  UnicodeStrToAsciiStrS (FileName, AsciiFileName, MAX_NAME_LENGTH);
>  
>    if (BootMonGetFileFromAsciiFileName (
>          File->Instance,
> @@ -327,7 +324,8 @@ SetFileName (
>      return EFI_ACCESS_DENIED;
>    } else {
>      // OK, change the filename.
> -    AsciiStrToUnicodeStr (AsciiFileName, File->Info->FileName);
> +    AsciiStrToUnicodeStrS (AsciiFileName, File->Info->FileName,
> +      (File->Info->Size - SIZE_OF_EFI_FILE_INFO) / sizeof (CHAR16));
>      return EFI_SUCCESS;
>    }
>  }
> diff --git a/ArmPlatformPkg/FileSystem/BootMonFs/BootMonFsEntryPoint.c b/ArmPlatformPkg/FileSystem/BootMonFs/BootMonFsEntryPoint.c
> index 3d71760fef99..a1150856f6ba 100644
> --- a/ArmPlatformPkg/FileSystem/BootMonFs/BootMonFsEntryPoint.c
> +++ b/ArmPlatformPkg/FileSystem/BootMonFs/BootMonFsEntryPoint.c
> @@ -98,7 +98,8 @@ BootMonGetFileFromAsciiFileName (
>    {
>      FileEntry = BOOTMON_FS_FILE_FROM_LINK_THIS (Entry);
>      if (FileEntry->Info != NULL) {
> -      UnicodeStrToAsciiStr (FileEntry->Info->FileName, OpenFileAsciiFileName);
> +      UnicodeStrToAsciiStrS (FileEntry->Info->FileName, OpenFileAsciiFileName,
> +        MAX_NAME_LENGTH);
>        AsciiFileNameToCompare = OpenFileAsciiFileName;
>      } else {
>        AsciiFileNameToCompare = FileEntry->HwDescription.Footer.Filename;
> diff --git a/ArmPlatformPkg/FileSystem/BootMonFs/BootMonFsOpenClose.c b/ArmPlatformPkg/FileSystem/BootMonFs/BootMonFsOpenClose.c
> index af2fe514f044..ae10055255ff 100644
> --- a/ArmPlatformPkg/FileSystem/BootMonFs/BootMonFsOpenClose.c
> +++ b/ArmPlatformPkg/FileSystem/BootMonFs/BootMonFsOpenClose.c
> @@ -101,7 +101,8 @@ WriteFileDescription (
>    Description->Attributes = 1;
>    Description->BlockStart = FileStart / BlockSize;
>    Description->BlockEnd   = Description->BlockStart + (FileSize / BlockSize);
> -  AsciiStrCpy (Description->Footer.Filename, FileName);
> +  AsciiStrCpyS (Description->Footer.Filename,
> +    sizeof Description->Footer.Filename, FileName);
>  
>  #ifdef MDE_CPU_ARM
>    Description->Footer.Offset  = HW_IMAGE_FOOTER_OFFSET;
> @@ -294,7 +295,7 @@ BootMonFsFlushFile (
>    DiskIo    = Instance->DiskIo;
>    BlockSize = Media->BlockSize;
>  
> -  UnicodeStrToAsciiStr (Info->FileName, AsciiFileName);
> +  UnicodeStrToAsciiStrS (Info->FileName, AsciiFileName, MAX_NAME_LENGTH);
>  
>    // If the file doesn't exist then find a space for it
>    if (File->HwDescription.RegionCount == 0) {
> @@ -513,6 +514,7 @@ BootMonFsOpenFile (
>    CHAR16               *Separator;
>    CHAR8                *AsciiFileName;
>    EFI_FILE_INFO        *Info;
> +  UINTN                AsciiFileNameSize;
>  
>    if (This == NULL) {
>      return EFI_INVALID_PARAMETER;
> @@ -621,15 +623,16 @@ BootMonFsOpenFile (
>    //
>    // BootMonFs interface requires ASCII filenames
>    //
> -  AsciiFileName = AllocatePool (StrLen (Path) + 1);
> +  AsciiFileNameSize = StrLen (Path) + 1;
> +  if (AsciiFileNameSize > MAX_NAME_LENGTH) {
> +    AsciiFileNameSize = MAX_NAME_LENGTH;
> +  }
> +  AsciiFileName = AllocatePool (AsciiFileNameSize);
>    if (AsciiFileName == NULL) {
>      Status = EFI_OUT_OF_RESOURCES;
>      goto Error;
>    }
> -  UnicodeStrToAsciiStr (Path, AsciiFileName);
> -  if (AsciiStrSize (AsciiFileName) > MAX_NAME_LENGTH) {
> -   AsciiFileName[MAX_NAME_LENGTH - 1] = '\0';
> -  }
> +  UnicodeStrToAsciiStrS (Path, AsciiFileName, AsciiFileNameSize);
>  
>    if ((AsciiFileName[0] == '\0') ||
>        (AsciiFileName[0] == '.' )    ) {
> @@ -688,7 +691,7 @@ BootMonFsOpenFile (
>  
>      Info->FileSize     = BootMonFsGetImageLength (File);
>      Info->PhysicalSize = BootMonFsGetPhysicalSize (File);
> -    AsciiStrToUnicodeStr (AsciiFileName, Info->FileName);
> +    AsciiStrToUnicodeStrS (AsciiFileName, Info->FileName, MAX_NAME_LENGTH);
>  
>      File->Info = Info;
>      Info = NULL;
> 

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

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

[Leif Lindholm]

On Fri, Oct 28, 2016 at 11:48:40AM +0100, 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 | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/ArmPlatformPkg/ArmVExpressPkg/ArmVExpressFastBootDxe/ArmVExpressFastBoot.c b/ArmPlatformPkg/ArmVExpressPkg/ArmVExpressFastBootDxe/ArmVExpressFastBoot.c
> index 4d0811cc5eaf..64b25f8a8c45 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,8 @@ ArmFastbootPlatformFlashPartition (
>    CHAR16                   PartitionNameUnicode[60];
>    BOOLEAN                  PartitionFound;
>  
> -  AsciiStrToUnicodeStr (PartitionName, PartitionNameUnicode);
> +  AsciiStrToUnicodeStrS (PartitionName, PartitionNameUnicode,
> +    ARRAY_SIZE (PartitionNameUnicode));
>  
>    PartitionFound = FALSE;
>    Entry = (FASTBOOT_PARTITION_LIST *) GetFirstNode (&(mPartitionListHead));
> @@ -396,7 +397,7 @@ ArmFastbootPlatformGetVar (
>    )
>  {
>    if (AsciiStrCmp (Name, "product")) {
> -    AsciiStrCpy (Value, FixedPcdGetPtr (PcdFirmwareVendor));
> +    AsciiStrCpyS (Value, 61, FixedPcdGetPtr (PcdFirmwareVendor));

I'm totally OK with the reason for hard-coding this, but could you add
the comment from the feedback on previous version?:
  // FASTBOOT_COMMAND_MAX_LENGTH - 4 + NULL terminator
(Or if there's a better way of putting it.)

>    } else {
>      *Value = '\0';
>    }
> @@ -410,7 +411,7 @@ ArmFastbootPlatformOemCommand (
>  {
>    CHAR16 CommandUnicode[65];
>  
> -  AsciiStrToUnicodeStr (Command, CommandUnicode);
> +  AsciiStrToUnicodeStrS (Command, CommandUnicode, ARRAY_SIZE (CommandUnicode));
>  
>    if (AsciiStrCmp (Command, "Demonstrate") == 0) {
>      DEBUG ((EFI_D_ERROR, "ARM OEM Fastboot command 'Demonstrate' received.\n"));
> -- 
> 2.7.4
> 

Re: [PATCH v2 2/2] ArmPlatformPkg/BootMonFs: eliminate deprecated string functions

[Leif Lindholm]

On Fri, Oct 28, 2016 at 11:48:41AM +0100, 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>

Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>

> ---
>  ArmPlatformPkg/FileSystem/BootMonFs/BootMonFsDir.c        |  8 +++-----
>  ArmPlatformPkg/FileSystem/BootMonFs/BootMonFsEntryPoint.c |  3 ++-
>  ArmPlatformPkg/FileSystem/BootMonFs/BootMonFsOpenClose.c  | 19 +++++++++++--------
>  3 files changed, 16 insertions(+), 14 deletions(-)
> 
> diff --git a/ArmPlatformPkg/FileSystem/BootMonFs/BootMonFsDir.c b/ArmPlatformPkg/FileSystem/BootMonFs/BootMonFsDir.c
> index 450a707f183c..64ea0ec68048 100644
> --- a/ArmPlatformPkg/FileSystem/BootMonFs/BootMonFsDir.c
> +++ b/ArmPlatformPkg/FileSystem/BootMonFs/BootMonFsDir.c
> @@ -304,7 +304,6 @@ SetFileName (
>    IN  CONST CHAR16         *FileName
>    )
>  {
> -  CHAR16           TruncFileName[MAX_NAME_LENGTH];
>    CHAR8            AsciiFileName[MAX_NAME_LENGTH];
>    BOOTMON_FS_FILE  *SameFile;
>  
> @@ -314,9 +313,7 @@ SetFileName (
>      FileName++;
>    }
>  
> -  StrnCpy (TruncFileName, FileName, MAX_NAME_LENGTH - 1);
> -  TruncFileName[MAX_NAME_LENGTH - 1] = 0;
> -  UnicodeStrToAsciiStr (TruncFileName, AsciiFileName);
> +  UnicodeStrToAsciiStrS (FileName, AsciiFileName, MAX_NAME_LENGTH);
>  
>    if (BootMonGetFileFromAsciiFileName (
>          File->Instance,
> @@ -327,7 +324,8 @@ SetFileName (
>      return EFI_ACCESS_DENIED;
>    } else {
>      // OK, change the filename.
> -    AsciiStrToUnicodeStr (AsciiFileName, File->Info->FileName);
> +    AsciiStrToUnicodeStrS (AsciiFileName, File->Info->FileName,
> +      (File->Info->Size - SIZE_OF_EFI_FILE_INFO) / sizeof (CHAR16));
>      return EFI_SUCCESS;
>    }
>  }
> diff --git a/ArmPlatformPkg/FileSystem/BootMonFs/BootMonFsEntryPoint.c b/ArmPlatformPkg/FileSystem/BootMonFs/BootMonFsEntryPoint.c
> index 3d71760fef99..a1150856f6ba 100644
> --- a/ArmPlatformPkg/FileSystem/BootMonFs/BootMonFsEntryPoint.c
> +++ b/ArmPlatformPkg/FileSystem/BootMonFs/BootMonFsEntryPoint.c
> @@ -98,7 +98,8 @@ BootMonGetFileFromAsciiFileName (
>    {
>      FileEntry = BOOTMON_FS_FILE_FROM_LINK_THIS (Entry);
>      if (FileEntry->Info != NULL) {
> -      UnicodeStrToAsciiStr (FileEntry->Info->FileName, OpenFileAsciiFileName);
> +      UnicodeStrToAsciiStrS (FileEntry->Info->FileName, OpenFileAsciiFileName,
> +        MAX_NAME_LENGTH);
>        AsciiFileNameToCompare = OpenFileAsciiFileName;
>      } else {
>        AsciiFileNameToCompare = FileEntry->HwDescription.Footer.Filename;
> diff --git a/ArmPlatformPkg/FileSystem/BootMonFs/BootMonFsOpenClose.c b/ArmPlatformPkg/FileSystem/BootMonFs/BootMonFsOpenClose.c
> index af2fe514f044..ae10055255ff 100644
> --- a/ArmPlatformPkg/FileSystem/BootMonFs/BootMonFsOpenClose.c
> +++ b/ArmPlatformPkg/FileSystem/BootMonFs/BootMonFsOpenClose.c
> @@ -101,7 +101,8 @@ WriteFileDescription (
>    Description->Attributes = 1;
>    Description->BlockStart = FileStart / BlockSize;
>    Description->BlockEnd   = Description->BlockStart + (FileSize / BlockSize);
> -  AsciiStrCpy (Description->Footer.Filename, FileName);
> +  AsciiStrCpyS (Description->Footer.Filename,
> +    sizeof Description->Footer.Filename, FileName);
>  
>  #ifdef MDE_CPU_ARM
>    Description->Footer.Offset  = HW_IMAGE_FOOTER_OFFSET;
> @@ -294,7 +295,7 @@ BootMonFsFlushFile (
>    DiskIo    = Instance->DiskIo;
>    BlockSize = Media->BlockSize;
>  
> -  UnicodeStrToAsciiStr (Info->FileName, AsciiFileName);
> +  UnicodeStrToAsciiStrS (Info->FileName, AsciiFileName, MAX_NAME_LENGTH);
>  
>    // If the file doesn't exist then find a space for it
>    if (File->HwDescription.RegionCount == 0) {
> @@ -513,6 +514,7 @@ BootMonFsOpenFile (
>    CHAR16               *Separator;
>    CHAR8                *AsciiFileName;
>    EFI_FILE_INFO        *Info;
> +  UINTN                AsciiFileNameSize;
>  
>    if (This == NULL) {
>      return EFI_INVALID_PARAMETER;
> @@ -621,15 +623,16 @@ BootMonFsOpenFile (
>    //
>    // BootMonFs interface requires ASCII filenames
>    //
> -  AsciiFileName = AllocatePool (StrLen (Path) + 1);
> +  AsciiFileNameSize = StrLen (Path) + 1;
> +  if (AsciiFileNameSize > MAX_NAME_LENGTH) {
> +    AsciiFileNameSize = MAX_NAME_LENGTH;
> +  }
> +  AsciiFileName = AllocatePool (AsciiFileNameSize);
>    if (AsciiFileName == NULL) {
>      Status = EFI_OUT_OF_RESOURCES;
>      goto Error;
>    }
> -  UnicodeStrToAsciiStr (Path, AsciiFileName);
> -  if (AsciiStrSize (AsciiFileName) > MAX_NAME_LENGTH) {
> -   AsciiFileName[MAX_NAME_LENGTH - 1] = '\0';
> -  }
> +  UnicodeStrToAsciiStrS (Path, AsciiFileName, AsciiFileNameSize);
>  
>    if ((AsciiFileName[0] == '\0') ||
>        (AsciiFileName[0] == '.' )    ) {
> @@ -688,7 +691,7 @@ BootMonFsOpenFile (
>  
>      Info->FileSize     = BootMonFsGetImageLength (File);
>      Info->PhysicalSize = BootMonFsGetPhysicalSize (File);
> -    AsciiStrToUnicodeStr (AsciiFileName, Info->FileName);
> +    AsciiStrToUnicodeStrS (AsciiFileName, Info->FileName, MAX_NAME_LENGTH);
>  
>      File->Info = Info;
>      Info = NULL;
> -- 
> 2.7.4
> 

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

[Ard Biesheuvel]

On 28 October 2016 at 15:48, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> On Fri, Oct 28, 2016 at 11:48:40AM +0100, 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 | 9 +++++----
>>  1 file changed, 5 insertions(+), 4 deletions(-)
>>
>> diff --git a/ArmPlatformPkg/ArmVExpressPkg/ArmVExpressFastBootDxe/ArmVExpressFastBoot.c b/ArmPlatformPkg/ArmVExpressPkg/ArmVExpressFastBootDxe/ArmVExpressFastBoot.c
>> index 4d0811cc5eaf..64b25f8a8c45 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,8 @@ ArmFastbootPlatformFlashPartition (
>>    CHAR16                   PartitionNameUnicode[60];
>>    BOOLEAN                  PartitionFound;
>>
>> -  AsciiStrToUnicodeStr (PartitionName, PartitionNameUnicode);
>> +  AsciiStrToUnicodeStrS (PartitionName, PartitionNameUnicode,
>> +    ARRAY_SIZE (PartitionNameUnicode));
>>
>>    PartitionFound = FALSE;
>>    Entry = (FASTBOOT_PARTITION_LIST *) GetFirstNode (&(mPartitionListHead));
>> @@ -396,7 +397,7 @@ ArmFastbootPlatformGetVar (
>>    )
>>  {
>>    if (AsciiStrCmp (Name, "product")) {
>> -    AsciiStrCpy (Value, FixedPcdGetPtr (PcdFirmwareVendor));
>> +    AsciiStrCpyS (Value, 61, FixedPcdGetPtr (PcdFirmwareVendor));
>
> I'm totally OK with the reason for hard-coding this, but could you add
> the comment from the feedback on previous version?:
>   // FASTBOOT_COMMAND_MAX_LENGTH - 4 + NULL terminator
> (Or if there's a better way of putting it.)
>

What if I decorate each entry point with the comment block from the
associated protocol header file instead? (in a follow up patch)
That is common practice in Tianocore anyway, and looks like this for
this particular protocol method:

/*
  If the variable referred to by Name exists, copy it (as a null-terminated
  string) into Value. If it doesn't exist, put the Empty string in Value.

  Variable names and values may not be larger than 60 bytes, excluding the
  terminal null character. This is a limitation of the Fastboot protocol.

  The Fastboot application will handle platform-nonspecific variables
  (Currently "version" is the only one of these.)

  @param[in]  Name   Null-terminated name of Fastboot variable to retrieve.
  @param[out] Value  Caller-allocated buffer for null-terminated value of
                     variable.

  @retval EFI_SUCCESS       The variable was retrieved, or it doesn't exist.
  @retval EFI_DEVICE_ERROR  There was an error looking up the variable. This
                            does _not_ include the variable not existing.
*/

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

[Laszlo Ersek]

On 10/28/16 17:02, Ard Biesheuvel wrote:
> On 28 October 2016 at 15:48, Leif Lindholm <leif.lindholm@linaro.org> wrote:
>> On Fri, Oct 28, 2016 at 11:48:40AM +0100, 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 | 9 +++++----
>>>  1 file changed, 5 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/ArmPlatformPkg/ArmVExpressPkg/ArmVExpressFastBootDxe/ArmVExpressFastBoot.c b/ArmPlatformPkg/ArmVExpressPkg/ArmVExpressFastBootDxe/ArmVExpressFastBoot.c
>>> index 4d0811cc5eaf..64b25f8a8c45 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,8 @@ ArmFastbootPlatformFlashPartition (
>>>    CHAR16                   PartitionNameUnicode[60];
>>>    BOOLEAN                  PartitionFound;
>>>
>>> -  AsciiStrToUnicodeStr (PartitionName, PartitionNameUnicode);
>>> +  AsciiStrToUnicodeStrS (PartitionName, PartitionNameUnicode,
>>> +    ARRAY_SIZE (PartitionNameUnicode));
>>>
>>>    PartitionFound = FALSE;
>>>    Entry = (FASTBOOT_PARTITION_LIST *) GetFirstNode (&(mPartitionListHead));
>>> @@ -396,7 +397,7 @@ ArmFastbootPlatformGetVar (
>>>    )
>>>  {
>>>    if (AsciiStrCmp (Name, "product")) {
>>> -    AsciiStrCpy (Value, FixedPcdGetPtr (PcdFirmwareVendor));
>>> +    AsciiStrCpyS (Value, 61, FixedPcdGetPtr (PcdFirmwareVendor));
>>
>> I'm totally OK with the reason for hard-coding this, but could you add
>> the comment from the feedback on previous version?:
>>   // FASTBOOT_COMMAND_MAX_LENGTH - 4 + NULL terminator
>> (Or if there's a better way of putting it.)
>>
> 
> What if I decorate each entry point with the comment block from the
> associated protocol header file instead? (in a follow up patch)
> That is common practice in Tianocore anyway,

Indeed it is.

Laszlo

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

[Leif Lindholm]

On Fri, Oct 28, 2016 at 04:02:21PM +0100, Ard Biesheuvel wrote:
> On 28 October 2016 at 15:48, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> > On Fri, Oct 28, 2016 at 11:48:40AM +0100, 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 | 9 +++++----
> >>  1 file changed, 5 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/ArmPlatformPkg/ArmVExpressPkg/ArmVExpressFastBootDxe/ArmVExpressFastBoot.c b/ArmPlatformPkg/ArmVExpressPkg/ArmVExpressFastBootDxe/ArmVExpressFastBoot.c
> >> index 4d0811cc5eaf..64b25f8a8c45 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,8 @@ ArmFastbootPlatformFlashPartition (
> >>    CHAR16                   PartitionNameUnicode[60];
> >>    BOOLEAN                  PartitionFound;
> >>
> >> -  AsciiStrToUnicodeStr (PartitionName, PartitionNameUnicode);
> >> +  AsciiStrToUnicodeStrS (PartitionName, PartitionNameUnicode,
> >> +    ARRAY_SIZE (PartitionNameUnicode));
> >>
> >>    PartitionFound = FALSE;
> >>    Entry = (FASTBOOT_PARTITION_LIST *) GetFirstNode (&(mPartitionListHead));
> >> @@ -396,7 +397,7 @@ ArmFastbootPlatformGetVar (
> >>    )
> >>  {
> >>    if (AsciiStrCmp (Name, "product")) {
> >> -    AsciiStrCpy (Value, FixedPcdGetPtr (PcdFirmwareVendor));
> >> +    AsciiStrCpyS (Value, 61, FixedPcdGetPtr (PcdFirmwareVendor));
> >
> > I'm totally OK with the reason for hard-coding this, but could you add
> > the comment from the feedback on previous version?:
> >   // FASTBOOT_COMMAND_MAX_LENGTH - 4 + NULL terminator
> > (Or if there's a better way of putting it.)
> >
> 
> What if I decorate each entry point with the comment block from the
> associated protocol header file instead? (in a follow up patch)
> That is common practice in Tianocore anyway, and looks like this for
> this particular protocol method:
> 
> /*
>   If the variable referred to by Name exists, copy it (as a null-terminated
>   string) into Value. If it doesn't exist, put the Empty string in Value.
> 
>   Variable names and values may not be larger than 60 bytes, excluding the
>   terminal null character. This is a limitation of the Fastboot protocol.
> 
>   The Fastboot application will handle platform-nonspecific variables
>   (Currently "version" is the only one of these.)
> 
>   @param[in]  Name   Null-terminated name of Fastboot variable to retrieve.
>   @param[out] Value  Caller-allocated buffer for null-terminated value of
>                      variable.
> 
>   @retval EFI_SUCCESS       The variable was retrieved, or it doesn't exist.
>   @retval EFI_DEVICE_ERROR  There was an error looking up the variable. This
>                             does _not_ include the variable not existing.
> */

That works for me.

/
    Leif

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

[Ard Biesheuvel]

On 28 October 2016 at 16:13, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> On Fri, Oct 28, 2016 at 04:02:21PM +0100, Ard Biesheuvel wrote:
>> On 28 October 2016 at 15:48, Leif Lindholm <leif.lindholm@linaro.org> wrote:
>> > On Fri, Oct 28, 2016 at 11:48:40AM +0100, 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 | 9 +++++----
>> >>  1 file changed, 5 insertions(+), 4 deletions(-)
>> >>
>> >> diff --git a/ArmPlatformPkg/ArmVExpressPkg/ArmVExpressFastBootDxe/ArmVExpressFastBoot.c b/ArmPlatformPkg/ArmVExpressPkg/ArmVExpressFastBootDxe/ArmVExpressFastBoot.c
>> >> index 4d0811cc5eaf..64b25f8a8c45 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,8 @@ ArmFastbootPlatformFlashPartition (
>> >>    CHAR16                   PartitionNameUnicode[60];
>> >>    BOOLEAN                  PartitionFound;
>> >>
>> >> -  AsciiStrToUnicodeStr (PartitionName, PartitionNameUnicode);
>> >> +  AsciiStrToUnicodeStrS (PartitionName, PartitionNameUnicode,
>> >> +    ARRAY_SIZE (PartitionNameUnicode));
>> >>
>> >>    PartitionFound = FALSE;
>> >>    Entry = (FASTBOOT_PARTITION_LIST *) GetFirstNode (&(mPartitionListHead));
>> >> @@ -396,7 +397,7 @@ ArmFastbootPlatformGetVar (
>> >>    )
>> >>  {
>> >>    if (AsciiStrCmp (Name, "product")) {
>> >> -    AsciiStrCpy (Value, FixedPcdGetPtr (PcdFirmwareVendor));
>> >> +    AsciiStrCpyS (Value, 61, FixedPcdGetPtr (PcdFirmwareVendor));
>> >
>> > I'm totally OK with the reason for hard-coding this, but could you add
>> > the comment from the feedback on previous version?:
>> >   // FASTBOOT_COMMAND_MAX_LENGTH - 4 + NULL terminator
>> > (Or if there's a better way of putting it.)
>> >
>>
>> What if I decorate each entry point with the comment block from the
>> associated protocol header file instead? (in a follow up patch)
>> That is common practice in Tianocore anyway, and looks like this for
>> this particular protocol method:
>>
>> /*
>>   If the variable referred to by Name exists, copy it (as a null-terminated
>>   string) into Value. If it doesn't exist, put the Empty string in Value.
>>
>>   Variable names and values may not be larger than 60 bytes, excluding the
>>   terminal null character. This is a limitation of the Fastboot protocol.
>>
>>   The Fastboot application will handle platform-nonspecific variables
>>   (Currently "version" is the only one of these.)
>>
>>   @param[in]  Name   Null-terminated name of Fastboot variable to retrieve.
>>   @param[out] Value  Caller-allocated buffer for null-terminated value of
>>                      variable.
>>
>>   @retval EFI_SUCCESS       The variable was retrieved, or it doesn't exist.
>>   @retval EFI_DEVICE_ERROR  There was an error looking up the variable. This
>>                             does _not_ include the variable not existing.
>> */
>
> That works for me.
>

Good. Does that mean you are happy with this patch as-is then?

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

[Leif Lindholm]

On Fri, Oct 28, 2016 at 04:14:43PM +0100, Ard Biesheuvel wrote:
> On 28 October 2016 at 16:13, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> > On Fri, Oct 28, 2016 at 04:02:21PM +0100, Ard Biesheuvel wrote:
> >> On 28 October 2016 at 15:48, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> >> > On Fri, Oct 28, 2016 at 11:48:40AM +0100, 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 | 9 +++++----
> >> >>  1 file changed, 5 insertions(+), 4 deletions(-)
> >> >>
> >> >> diff --git a/ArmPlatformPkg/ArmVExpressPkg/ArmVExpressFastBootDxe/ArmVExpressFastBoot.c b/ArmPlatformPkg/ArmVExpressPkg/ArmVExpressFastBootDxe/ArmVExpressFastBoot.c
> >> >> index 4d0811cc5eaf..64b25f8a8c45 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,8 @@ ArmFastbootPlatformFlashPartition (
> >> >>    CHAR16                   PartitionNameUnicode[60];
> >> >>    BOOLEAN                  PartitionFound;
> >> >>
> >> >> -  AsciiStrToUnicodeStr (PartitionName, PartitionNameUnicode);
> >> >> +  AsciiStrToUnicodeStrS (PartitionName, PartitionNameUnicode,
> >> >> +    ARRAY_SIZE (PartitionNameUnicode));
> >> >>
> >> >>    PartitionFound = FALSE;
> >> >>    Entry = (FASTBOOT_PARTITION_LIST *) GetFirstNode (&(mPartitionListHead));
> >> >> @@ -396,7 +397,7 @@ ArmFastbootPlatformGetVar (
> >> >>    )
> >> >>  {
> >> >>    if (AsciiStrCmp (Name, "product")) {
> >> >> -    AsciiStrCpy (Value, FixedPcdGetPtr (PcdFirmwareVendor));
> >> >> +    AsciiStrCpyS (Value, 61, FixedPcdGetPtr (PcdFirmwareVendor));
> >> >
> >> > I'm totally OK with the reason for hard-coding this, but could you add
> >> > the comment from the feedback on previous version?:
> >> >   // FASTBOOT_COMMAND_MAX_LENGTH - 4 + NULL terminator
> >> > (Or if there's a better way of putting it.)
> >> >
> >>
> >> What if I decorate each entry point with the comment block from the
> >> associated protocol header file instead? (in a follow up patch)
> >> That is common practice in Tianocore anyway, and looks like this for
> >> this particular protocol method:
> >>
> >> /*
> >>   If the variable referred to by Name exists, copy it (as a null-terminated
> >>   string) into Value. If it doesn't exist, put the Empty string in Value.
> >>
> >>   Variable names and values may not be larger than 60 bytes, excluding the
> >>   terminal null character. This is a limitation of the Fastboot protocol.
> >>
> >>   The Fastboot application will handle platform-nonspecific variables
> >>   (Currently "version" is the only one of these.)
> >>
> >>   @param[in]  Name   Null-terminated name of Fastboot variable to retrieve.
> >>   @param[out] Value  Caller-allocated buffer for null-terminated value of
> >>                      variable.
> >>
> >>   @retval EFI_SUCCESS       The variable was retrieved, or it doesn't exist.
> >>   @retval EFI_DEVICE_ERROR  There was an error looking up the variable. This
> >>                             does _not_ include the variable not existing.
> >> */
> >
> > That works for me.
> >
> 
> Good. Does that mean you are happy with this patch as-is then?

If it goes after the one I just acked, sure:
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>