[PATCH v2 0/6] ArmPkg: eliminate calls to deprecated functions

[PATCH v2 0/6] ArmPkg: eliminate calls to deprecated functions

[Ard Biesheuvel]

This series prepares ArmPkg for building with the preprocess symbol
DISABLE_NEW_DEPRECATED_INTERFACES defined, by adding missing components
to ArmPkg (#1), fixing broken code or code that relies on deprecated
functionality (#2 - #5), and finally adds -DDISABLE_NEW_DEPRECATED_INTERFACES
to the CC flags for all build types, toolchains and architectures.

v2:
- incorporate feedback from Laszlo
- add tags from Ryan, Leif and Laszlo

Bug: https://bugzilla.tianocore.org/show_bug.cgi?id=164

Ard Biesheuvel (6):
  ArmPkg: add missing components
  ArmPkg/ArmCortexA9Lib RVCT: remove incompatible GCC include
  ArmPkg/LinuxLoader: eliminate calls to deprecated string functions
  ArmPkg/SemihostFs: eliminate calls to deprecated string functions
  ArmPkg/BdsLib: eliminate calls to deprecated string functions
  ArmPkg: enable -DDISABLE_NEW_DEPRECATED_INTERFACES

 ArmPkg/Application/LinuxLoader/Arm/LinuxAtag.c                |  2 +-
 ArmPkg/Application/LinuxLoader/LinuxLoader.c                  |  6 ++++--
 ArmPkg/ArmPkg.dsc                                             | 13 +++++++++++++
 ArmPkg/Drivers/ArmCpuLib/ArmCortexA9Lib/ArmCortexA9Helper.asm |  4 ----
 ArmPkg/Filesystem/SemihostFs/Arm/SemihostFs.c                 | 20 ++++++++++++--------
 ArmPkg/Library/BdsLib/BdsFilePath.c                           |  8 +++++---
 6 files changed, 35 insertions(+), 18 deletions(-)

-- 
2.7.4

[PATCH v2 1/6] ArmPkg: add missing components

[Ard Biesheuvel]

ArmPkg.dsc was a bit out of date, and some modules added over the past
years had not been added to its [Components] section yet.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Tested-by: Ryan Harkin <ryan.harkin@linaro.org>
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
---
 ArmPkg/ArmPkg.dsc | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/ArmPkg/ArmPkg.dsc b/ArmPkg/ArmPkg.dsc
index ed7e482ddd62..8b2bfa2338f0 100644
--- a/ArmPkg/ArmPkg.dsc
+++ b/ArmPkg/ArmPkg.dsc
@@ -142,6 +142,16 @@ [Components.common]
 
   ArmPkg/Library/ArmMmuLib/ArmMmuBaseLib.inf
 
+  ArmPkg/Drivers/ArmPciCpuIo2Dxe/ArmPciCpuIo2Dxe.inf
+  ArmPkg/Library/ArmArchTimerLib/ArmArchTimerLib.inf
+  ArmPkg/Library/ArmGicArchLib/ArmGicArchLib.inf
+  ArmPkg/Library/ArmGicArchSecLib/ArmGicArchSecLib.inf
+  ArmPkg/Library/ArmLib/ArmBaseLib.inf
+  ArmPkg/Library/ArmSoftFloatLib/ArmSoftFloatLib.inf
+  ArmPkg/Library/DefaultExceptionHandlerLib/DefaultExceptionHandlerLibBase.inf
+  ArmPkg/Library/PeiServicesTablePointerLib/PeiServicesTablePointerLib.inf
+  ArmPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
+
 [Components.ARM]
   ArmPkg/Drivers/ArmCpuLib/ArmCortexA8Lib/ArmCortexA8Lib.inf
   ArmPkg/Drivers/ArmCpuLib/ArmCortexA9Lib/ArmCortexA9Lib.inf
@@ -150,3 +160,4 @@ [Components.ARM]
 [Components.AARCH64]
   ArmPkg/Drivers/ArmCpuLib/ArmCortexAEMv8Lib/ArmCortexAEMv8Lib.inf
   ArmPkg/Drivers/ArmCpuLib/ArmCortexA5xLib/ArmCortexA5xLib.inf
+  ArmPkg/Library/ArmMmuLib/ArmMmuPeiLib.inf
-- 
2.7.4

[PATCH v2 2/6] ArmPkg/ArmCortexA9Lib RVCT: remove incompatible GCC include

[Ard Biesheuvel]

Drop the include of AsmMacroIoLib.h, which contains GCC preprocessor macros
that RVCT does not use or require, given it has its own AsmMacroIoLib.inc

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Tested-by: Ryan Harkin <ryan.harkin@linaro.org>
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
---
 ArmPkg/Drivers/ArmCpuLib/ArmCortexA9Lib/ArmCortexA9Helper.asm | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/ArmPkg/Drivers/ArmCpuLib/ArmCortexA9Lib/ArmCortexA9Helper.asm b/ArmPkg/Drivers/ArmCpuLib/ArmCortexA9Lib/ArmCortexA9Helper.asm
index 1417c9a78204..882f25b780cd 100644
--- a/ArmPkg/Drivers/ArmCpuLib/ArmCortexA9Lib/ArmCortexA9Helper.asm
+++ b/ArmPkg/Drivers/ArmCpuLib/ArmCortexA9Lib/ArmCortexA9Helper.asm
@@ -11,10 +11,6 @@
 //
 //
 
-#include <AsmMacroIoLib.h>
-#include <Library/ArmCpuLib.h>
-#include <Chipset/ArmCortexA9.h>
-
   INCLUDE AsmMacroExport.inc
   INCLUDE AsmMacroIoLib.inc
 
-- 
2.7.4

[PATCH v2 3/6] ArmPkg/LinuxLoader: eliminate calls to deprecated string functions

[Ard Biesheuvel]

Remove calls to deprecated string functions like AsciiStrCpy() and
UnicodeStrToAsciiStr()

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Tested-by: Ryan Harkin <ryan.harkin@linaro.org>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
---
 ArmPkg/Application/LinuxLoader/Arm/LinuxAtag.c | 2 +-
 ArmPkg/Application/LinuxLoader/LinuxLoader.c   | 6 ++++--
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/ArmPkg/Application/LinuxLoader/Arm/LinuxAtag.c b/ArmPkg/Application/LinuxLoader/Arm/LinuxAtag.c
index fd7ee9c8624d..0b3e2489c758 100644
--- a/ArmPkg/Application/LinuxLoader/Arm/LinuxAtag.c
+++ b/ArmPkg/Application/LinuxLoader/Arm/LinuxAtag.c
@@ -72,7 +72,7 @@ SetupCmdlineTag (
     mLinuxKernelCurrentAtag->header.type = ATAG_CMDLINE;
 
     /* place CommandLine into tag */
-    AsciiStrCpy (mLinuxKernelCurrentAtag->body.cmdline_tag.cmdline, CmdLine);
+    AsciiStrCpyS (mLinuxKernelCurrentAtag->body.cmdline_tag.cmdline, LineLength, CmdLine);
 
     // move pointer to next tag
     mLinuxKernelCurrentAtag = next_tag_address (mLinuxKernelCurrentAtag);
diff --git a/ArmPkg/Application/LinuxLoader/LinuxLoader.c b/ArmPkg/Application/LinuxLoader/LinuxLoader.c
index 70b960b66f0e..76697c3a8c9d 100644
--- a/ArmPkg/Application/LinuxLoader/LinuxLoader.c
+++ b/ArmPkg/Application/LinuxLoader/LinuxLoader.c
@@ -61,6 +61,7 @@ LinuxLoaderEntryPoint (
   LIST_ENTRY                          *ResourceLink;
   SYSTEM_MEMORY_RESOURCE              *Resource;
   EFI_PHYSICAL_ADDRESS                SystemMemoryBase;
+  UINTN                               Length;
 
   Status = gBS->LocateProtocol (
                   &gEfiDevicePathFromTextProtocolGuid,
@@ -182,12 +183,13 @@ LinuxLoaderEntryPoint (
   }
 
   if (LinuxCommandLine != NULL) {
-    AsciiLinuxCommandLine = AllocatePool ((StrLen (LinuxCommandLine) + 1) * sizeof (CHAR8));
+    Length = StrLen (LinuxCommandLine) + 1;
+    AsciiLinuxCommandLine = AllocatePool (Length);
     if (AsciiLinuxCommandLine == NULL) {
       Status = EFI_OUT_OF_RESOURCES;
       goto Error;
     }
-    UnicodeStrToAsciiStr (LinuxCommandLine, AsciiLinuxCommandLine);
+    UnicodeStrToAsciiStrS (LinuxCommandLine, AsciiLinuxCommandLine, Length);
   }
 
   //
-- 
2.7.4

[PATCH v2 4/6] ArmPkg/SemihostFs: eliminate calls to deprecated string functions

[Ard Biesheuvel]

Remove calls to deprecated string functions like AsciiStrCpy() and
UnicodeStrToAsciiStr()

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 ArmPkg/Filesystem/SemihostFs/Arm/SemihostFs.c | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/ArmPkg/Filesystem/SemihostFs/Arm/SemihostFs.c b/ArmPkg/Filesystem/SemihostFs/Arm/SemihostFs.c
index 6efdad9ebcce..cf94ecd5d56f 100644
--- a/ArmPkg/Filesystem/SemihostFs/Arm/SemihostFs.c
+++ b/ArmPkg/Filesystem/SemihostFs/Arm/SemihostFs.c
@@ -207,11 +207,12 @@ FileOpen (
     return EFI_WRITE_PROTECTED;
   }
 
-  AsciiFileName = AllocatePool (StrLen (FileName) + 1);
+  Length = StrLen (FileName) + 1;
+  AsciiFileName = AllocatePool (Length);
   if (AsciiFileName == NULL) {
     return EFI_OUT_OF_RESOURCES;
   }
-  UnicodeStrToAsciiStr (FileName, AsciiFileName);
+  UnicodeStrToAsciiStrS (FileName, AsciiFileName, Length);
 
   // Opening '/', '\', '.', or the NULL pathname is trying to open the root directory
   if ((AsciiStrCmp (AsciiFileName, "\\") == 0) ||
@@ -463,7 +464,7 @@ FileDelete (
     NameSize = AsciiStrLen (Fcb->FileName);
     FileName = AllocatePool (NameSize + 1);
 
-    AsciiStrCpy (FileName, Fcb->FileName);
+    AsciiStrCpyS (FileName, NameSize + 1, Fcb->FileName);
 
     // Close the file if it's open.  Disregard return status,
     // since it might give an error if the file isn't open.
@@ -828,8 +829,10 @@ GetFilesystemInfo (
   EFI_FILE_SYSTEM_INFO  *Info;
   EFI_STATUS            Status;
   UINTN                 ResultSize;
+  UINTN                 StringSize;
 
-  ResultSize = SIZE_OF_EFI_FILE_SYSTEM_INFO + StrSize (mSemihostFsLabel);
+  StringSize = StrSize (mSemihostFsLabel);
+  ResultSize = SIZE_OF_EFI_FILE_SYSTEM_INFO + StringSize;
 
   if (*BufferSize >= ResultSize) {
     ZeroMem (Buffer, ResultSize);
@@ -843,7 +846,7 @@ GetFilesystemInfo (
     Info->FreeSpace  = 0;
     Info->BlockSize  = 0;
 
-    StrCpy (Info->VolumeLabel, mSemihostFsLabel);
+    CopyMem (Info->VolumeLabel, mSemihostFsLabel, StringSize);
   } else {
     Status = EFI_BUFFER_TOO_SMALL;
   }
@@ -903,7 +906,7 @@ FileGetInfo (
     ResultSize = StrSize (mSemihostFsLabel);
 
     if (*BufferSize >= ResultSize) {
-      StrCpy (Buffer, mSemihostFsLabel);
+      CopyMem (Buffer, mSemihostFsLabel, *BufferSize);
       Status = EFI_SUCCESS;
     } else {
       Status = EFI_BUFFER_TOO_SMALL;
@@ -963,11 +966,12 @@ SetFileInfo (
     return EFI_ACCESS_DENIED;
   }
 
-  AsciiFileName = AllocatePool (StrLen (Info->FileName) + 1);
+  Length = StrLen (Info->FileName) + 1;
+  AsciiFileName = AllocatePool (Length);
   if (AsciiFileName == NULL) {
     return EFI_OUT_OF_RESOURCES;
   }
-  UnicodeStrToAsciiStr (Info->FileName, AsciiFileName);
+  UnicodeStrToAsciiStrS (Info->FileName, AsciiFileName, Length);
 
   FileSizeIsDifferent = (Info->FileSize != Fcb->Info.FileSize);
   FileNameIsDifferent = (AsciiStrCmp (AsciiFileName, Fcb->FileName) != 0);
-- 
2.7.4

[PATCH v2 5/6] ArmPkg/BdsLib: eliminate calls to deprecated string functions

[Ard Biesheuvel]

Remove calls to deprecated string functions like AsciiStrCpy() and
UnicodeStrToAsciiStr()

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 ArmPkg/Library/BdsLib/BdsFilePath.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/ArmPkg/Library/BdsLib/BdsFilePath.c b/ArmPkg/Library/BdsLib/BdsFilePath.c
index aefeaed4ffb5..f9d8c4c205bf 100644
--- a/ArmPkg/Library/BdsLib/BdsFilePath.c
+++ b/ArmPkg/Library/BdsLib/BdsFilePath.c
@@ -964,7 +964,7 @@ Mtftp4CheckPacket (
       Step      = (Context->DownloadedNbOfBytes   * TFTP_PROGRESS_SLIDER_STEPS) / Context->FileSize;
       if (Step > LastStep) {
         Print (mTftpProgressDelete);
-        StrCpy (Progress, mTftpProgressFrame);
+        CopyMem (Progress, mTftpProgressFrame, sizeof mTftpProgressFrame);
         for (Index = 1; Index < Step; Index++) {
           Progress[Index] = L'=';
         }
@@ -1044,6 +1044,7 @@ BdsTftpLoadImage (
   UINT64                   FileSize;
   UINT64                   TftpBufferSize;
   BDS_TFTP_CONTEXT         *TftpContext;
+  UINTN                    PathNameLen;
 
   ASSERT(IS_DEVICE_PATH_NODE (RemainingDevicePath, MESSAGING_DEVICE_PATH, MSG_IPv4_DP));
   IPv4DevicePathNode = (IPv4_DEVICE_PATH*)RemainingDevicePath;
@@ -1187,8 +1188,9 @@ BdsTftpLoadImage (
 
   // The Device Path might contain multiple FilePath nodes
   PathName      = ConvertDevicePathToText ((EFI_DEVICE_PATH_PROTOCOL*)(IPv4DevicePathNode + 1), FALSE, FALSE);
-  AsciiFilePath = AllocatePool (StrLen (PathName) + 1);
-  UnicodeStrToAsciiStr (PathName, AsciiFilePath);
+  PathNameLen   = StrLen (PathName) + 1;
+  AsciiFilePath = AllocatePool (PathNameLen);
+  UnicodeStrToAsciiStrS (PathName, AsciiFilePath, PathNameLen);
 
   //
   // Try to get the size of the file in bytes from the server. If it fails,
-- 
2.7.4

[PATCH v2 6/6] ArmPkg: enable -DDISABLE_NEW_DEPRECATED_INTERFACES

[Ard Biesheuvel]

Define DISABLE_NEW_DEPRECATED_INTERFACES on the compiler command line by
default, to prevent deprecated interfaces from being used in core EDK2
code.

Bug: https://bugzilla.tianocore.org/show_bug.cgi?id=164
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Acked-by: Laszlo Ersek <lersek@redhat.com>
---
 ArmPkg/ArmPkg.dsc | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/ArmPkg/ArmPkg.dsc b/ArmPkg/ArmPkg.dsc
index 8b2bfa2338f0..64bc799b8e56 100644
--- a/ArmPkg/ArmPkg.dsc
+++ b/ArmPkg/ArmPkg.dsc
@@ -3,6 +3,7 @@
 #
 # Copyright (c) 2009 - 2010, Apple Inc. All rights reserved.<BR>
 # Copyright (c) 2011 - 2015, ARM Ltd. All rights reserved.<BR>
+# Copyright (c) 2016, Linaro Ltd. All rights reserved.<BR>
 #
 #    This program and the accompanying materials
 #    are licensed and made available under the terms and conditions of the BSD License
@@ -36,6 +37,7 @@ [BuildOptions]
   RVCT:*_*_ARM_PLATFORM_FLAGS  == --cpu Cortex-A15
 
   RELEASE_*_*_CC_FLAGS  = -DMDEPKG_NDEBUG
+  *_*_*_CC_FLAGS  = -DDISABLE_NEW_DEPRECATED_INTERFACES
 
 [LibraryClasses.common]
   BaseLib|MdePkg/Library/BaseLib/BaseLib.inf
-- 
2.7.4

Re: [PATCH v2 4/6] ArmPkg/SemihostFs: eliminate calls to deprecated string functions

[Laszlo Ersek]

On 10/26/16 10:17, Ard Biesheuvel wrote:
> Remove calls to deprecated string functions like AsciiStrCpy() and
> UnicodeStrToAsciiStr()
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  ArmPkg/Filesystem/SemihostFs/Arm/SemihostFs.c | 20 ++++++++++++--------
>  1 file changed, 12 insertions(+), 8 deletions(-)
> 
> diff --git a/ArmPkg/Filesystem/SemihostFs/Arm/SemihostFs.c b/ArmPkg/Filesystem/SemihostFs/Arm/SemihostFs.c
> index 6efdad9ebcce..cf94ecd5d56f 100644
> --- a/ArmPkg/Filesystem/SemihostFs/Arm/SemihostFs.c
> +++ b/ArmPkg/Filesystem/SemihostFs/Arm/SemihostFs.c
> @@ -207,11 +207,12 @@ FileOpen (
>      return EFI_WRITE_PROTECTED;
>    }
>  
> -  AsciiFileName = AllocatePool (StrLen (FileName) + 1);
> +  Length = StrLen (FileName) + 1;
> +  AsciiFileName = AllocatePool (Length);
>    if (AsciiFileName == NULL) {
>      return EFI_OUT_OF_RESOURCES;
>    }
> -  UnicodeStrToAsciiStr (FileName, AsciiFileName);
> +  UnicodeStrToAsciiStrS (FileName, AsciiFileName, Length);
>  
>    // Opening '/', '\', '.', or the NULL pathname is trying to open the root directory
>    if ((AsciiStrCmp (AsciiFileName, "\\") == 0) ||
> @@ -463,7 +464,7 @@ FileDelete (
>      NameSize = AsciiStrLen (Fcb->FileName);
>      FileName = AllocatePool (NameSize + 1);
>  
> -    AsciiStrCpy (FileName, Fcb->FileName);
> +    AsciiStrCpyS (FileName, NameSize + 1, Fcb->FileName);
>  
>      // Close the file if it's open.  Disregard return status,
>      // since it might give an error if the file isn't open.
> @@ -828,8 +829,10 @@ GetFilesystemInfo (
>    EFI_FILE_SYSTEM_INFO  *Info;
>    EFI_STATUS            Status;
>    UINTN                 ResultSize;
> +  UINTN                 StringSize;
>  
> -  ResultSize = SIZE_OF_EFI_FILE_SYSTEM_INFO + StrSize (mSemihostFsLabel);
> +  StringSize = StrSize (mSemihostFsLabel);
> +  ResultSize = SIZE_OF_EFI_FILE_SYSTEM_INFO + StringSize;
>  
>    if (*BufferSize >= ResultSize) {
>      ZeroMem (Buffer, ResultSize);
> @@ -843,7 +846,7 @@ GetFilesystemInfo (
>      Info->FreeSpace  = 0;
>      Info->BlockSize  = 0;
>  
> -    StrCpy (Info->VolumeLabel, mSemihostFsLabel);
> +    CopyMem (Info->VolumeLabel, mSemihostFsLabel, StringSize);
>    } else {
>      Status = EFI_BUFFER_TOO_SMALL;
>    }
> @@ -903,7 +906,7 @@ FileGetInfo (
>      ResultSize = StrSize (mSemihostFsLabel);
>  
>      if (*BufferSize >= ResultSize) {
> -      StrCpy (Buffer, mSemihostFsLabel);
> +      CopyMem (Buffer, mSemihostFsLabel, *BufferSize);

This is still wrong; here *BufferSize is the size of the recipient
buffer, passed in from the caller. As written, the code can overrun the
*source* buffer. Please use

      CopyMem (Buffer, mSemihostFsLabel, ResultSize);

instead.

With that update:

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

Thanks
Laszlo

>        Status = EFI_SUCCESS;
>      } else {
>        Status = EFI_BUFFER_TOO_SMALL;
> @@ -963,11 +966,12 @@ SetFileInfo (
>      return EFI_ACCESS_DENIED;
>    }
>  
> -  AsciiFileName = AllocatePool (StrLen (Info->FileName) + 1);
> +  Length = StrLen (Info->FileName) + 1;
> +  AsciiFileName = AllocatePool (Length);
>    if (AsciiFileName == NULL) {
>      return EFI_OUT_OF_RESOURCES;
>    }
> -  UnicodeStrToAsciiStr (Info->FileName, AsciiFileName);
> +  UnicodeStrToAsciiStrS (Info->FileName, AsciiFileName, Length);
>  
>    FileSizeIsDifferent = (Info->FileSize != Fcb->Info.FileSize);
>    FileNameIsDifferent = (AsciiStrCmp (AsciiFileName, Fcb->FileName) != 0);
> 

Re: [PATCH v2 5/6] ArmPkg/BdsLib: eliminate calls to deprecated string functions

[Laszlo Ersek]

On 10/26/16 10:17, Ard Biesheuvel wrote:
> Remove calls to deprecated string functions like AsciiStrCpy() and
> UnicodeStrToAsciiStr()
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  ArmPkg/Library/BdsLib/BdsFilePath.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/ArmPkg/Library/BdsLib/BdsFilePath.c b/ArmPkg/Library/BdsLib/BdsFilePath.c
> index aefeaed4ffb5..f9d8c4c205bf 100644
> --- a/ArmPkg/Library/BdsLib/BdsFilePath.c
> +++ b/ArmPkg/Library/BdsLib/BdsFilePath.c
> @@ -964,7 +964,7 @@ Mtftp4CheckPacket (
>        Step      = (Context->DownloadedNbOfBytes   * TFTP_PROGRESS_SLIDER_STEPS) / Context->FileSize;
>        if (Step > LastStep) {
>          Print (mTftpProgressDelete);
> -        StrCpy (Progress, mTftpProgressFrame);
> +        CopyMem (Progress, mTftpProgressFrame, sizeof mTftpProgressFrame);
>          for (Index = 1; Index < Step; Index++) {
>            Progress[Index] = L'=';
>          }

This is fine now; the relevant definitions are

STATIC CONST CHAR16 mTftpProgressFrame[] = ...

#define TFTP_PROGRESS_MESSAGE_SIZE  ((sizeof (mTftpProgressFrame) /
sizeof (CHAR16)) + 12)

  CHAR16            Progress[TFTP_PROGRESS_MESSAGE_SIZE];

Because TFTP_PROGRESS_MESSAGE_SIZE always results in a larger count of
UCS2 characters, it is safe to copy mTftpProgressFrame into Progress.
(IOW, the original code was safe.)

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

Thanks
Laszlo

> @@ -1044,6 +1044,7 @@ BdsTftpLoadImage (
>    UINT64                   FileSize;
>    UINT64                   TftpBufferSize;
>    BDS_TFTP_CONTEXT         *TftpContext;
> +  UINTN                    PathNameLen;
>  
>    ASSERT(IS_DEVICE_PATH_NODE (RemainingDevicePath, MESSAGING_DEVICE_PATH, MSG_IPv4_DP));
>    IPv4DevicePathNode = (IPv4_DEVICE_PATH*)RemainingDevicePath;
> @@ -1187,8 +1188,9 @@ BdsTftpLoadImage (
>  
>    // The Device Path might contain multiple FilePath nodes
>    PathName      = ConvertDevicePathToText ((EFI_DEVICE_PATH_PROTOCOL*)(IPv4DevicePathNode + 1), FALSE, FALSE);
> -  AsciiFilePath = AllocatePool (StrLen (PathName) + 1);
> -  UnicodeStrToAsciiStr (PathName, AsciiFilePath);
> +  PathNameLen   = StrLen (PathName) + 1;
> +  AsciiFilePath = AllocatePool (PathNameLen);
> +  UnicodeStrToAsciiStrS (PathName, AsciiFilePath, PathNameLen);
>  
>    //
>    // Try to get the size of the file in bytes from the server. If it fails,
> 

Re: [PATCH v2 0/6] ArmPkg: eliminate calls to deprecated functions

[Ryan Harkin]

Hi Ard,

You don't have my Tested-by tags in all of the v2 patches.  Do I need
to retest?  I thought they changes were trivial, but I'm happy to
retest if it helps.

Cheers,
Ryan.

On 26 October 2016 at 09:17, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> This series prepares ArmPkg for building with the preprocess symbol
> DISABLE_NEW_DEPRECATED_INTERFACES defined, by adding missing components
> to ArmPkg (#1), fixing broken code or code that relies on deprecated
> functionality (#2 - #5), and finally adds -DDISABLE_NEW_DEPRECATED_INTERFACES
> to the CC flags for all build types, toolchains and architectures.
>
> v2:
> - incorporate feedback from Laszlo
> - add tags from Ryan, Leif and Laszlo
>
> Bug: https://bugzilla.tianocore.org/show_bug.cgi?id=164
>
> Ard Biesheuvel (6):
>   ArmPkg: add missing components
>   ArmPkg/ArmCortexA9Lib RVCT: remove incompatible GCC include
>   ArmPkg/LinuxLoader: eliminate calls to deprecated string functions
>   ArmPkg/SemihostFs: eliminate calls to deprecated string functions
>   ArmPkg/BdsLib: eliminate calls to deprecated string functions
>   ArmPkg: enable -DDISABLE_NEW_DEPRECATED_INTERFACES
>
>  ArmPkg/Application/LinuxLoader/Arm/LinuxAtag.c                |  2 +-
>  ArmPkg/Application/LinuxLoader/LinuxLoader.c                  |  6 ++++--
>  ArmPkg/ArmPkg.dsc                                             | 13 +++++++++++++
>  ArmPkg/Drivers/ArmCpuLib/ArmCortexA9Lib/ArmCortexA9Helper.asm |  4 ----
>  ArmPkg/Filesystem/SemihostFs/Arm/SemihostFs.c                 | 20 ++++++++++++--------
>  ArmPkg/Library/BdsLib/BdsFilePath.c                           |  8 +++++---
>  6 files changed, 35 insertions(+), 18 deletions(-)
>
> --
> 2.7.4
>

Re: [PATCH v2 0/6] ArmPkg: eliminate calls to deprecated functions

[Ard Biesheuvel]

On 26 October 2016 at 13:11, Ryan Harkin <ryan.harkin@linaro.org> wrote:
> Hi Ard,
>
> You don't have my Tested-by tags in all of the v2 patches.  Do I need
> to retest?  I thought they changes were trivial, but I'm happy to
> retest if it helps.
>

If you don't mind, yes please. Given that they are changes that look
trivial on the face of it but could potentially cause hard to diagnose
problems if implemented incorrectly, I thought keeping the T-b was
perhaps not appropriate in this case.

Re: [PATCH v2 0/6] ArmPkg: eliminate calls to deprecated functions

[Ryan Harkin]

On 26 October 2016 at 13:13, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> On 26 October 2016 at 13:11, Ryan Harkin <ryan.harkin@linaro.org> wrote:
>> Hi Ard,
>>
>> You don't have my Tested-by tags in all of the v2 patches.  Do I need
>> to retest?  I thought they changes were trivial, but I'm happy to
>> retest if it helps.
>>
>
> If you don't mind, yes please. Given that they are changes that look
> trivial on the face of it but could potentially cause hard to diagnose
> problems if implemented incorrectly, I thought keeping the T-b was
> perhaps not appropriate in this case.

No problem, I'll do it now before I forget.


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

Re: [PATCH v2 4/6] ArmPkg/SemihostFs: eliminate calls to deprecated string functions

[Ryan Harkin]

On 26 October 2016 at 12:55, Laszlo Ersek <lersek@redhat.com> wrote:
> On 10/26/16 10:17, Ard Biesheuvel wrote:
>> Remove calls to deprecated string functions like AsciiStrCpy() and
>> UnicodeStrToAsciiStr()
>>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>>  ArmPkg/Filesystem/SemihostFs/Arm/SemihostFs.c | 20 ++++++++++++--------
>>  1 file changed, 12 insertions(+), 8 deletions(-)
>>
>> diff --git a/ArmPkg/Filesystem/SemihostFs/Arm/SemihostFs.c b/ArmPkg/Filesystem/SemihostFs/Arm/SemihostFs.c
>> index 6efdad9ebcce..cf94ecd5d56f 100644
>> --- a/ArmPkg/Filesystem/SemihostFs/Arm/SemihostFs.c
>> +++ b/ArmPkg/Filesystem/SemihostFs/Arm/SemihostFs.c
>> @@ -207,11 +207,12 @@ FileOpen (
>>      return EFI_WRITE_PROTECTED;
>>    }
>>
>> -  AsciiFileName = AllocatePool (StrLen (FileName) + 1);
>> +  Length = StrLen (FileName) + 1;
>> +  AsciiFileName = AllocatePool (Length);
>>    if (AsciiFileName == NULL) {
>>      return EFI_OUT_OF_RESOURCES;
>>    }
>> -  UnicodeStrToAsciiStr (FileName, AsciiFileName);
>> +  UnicodeStrToAsciiStrS (FileName, AsciiFileName, Length);
>>
>>    // Opening '/', '\', '.', or the NULL pathname is trying to open the root directory
>>    if ((AsciiStrCmp (AsciiFileName, "\\") == 0) ||
>> @@ -463,7 +464,7 @@ FileDelete (
>>      NameSize = AsciiStrLen (Fcb->FileName);
>>      FileName = AllocatePool (NameSize + 1);
>>
>> -    AsciiStrCpy (FileName, Fcb->FileName);
>> +    AsciiStrCpyS (FileName, NameSize + 1, Fcb->FileName);
>>
>>      // Close the file if it's open.  Disregard return status,
>>      // since it might give an error if the file isn't open.
>> @@ -828,8 +829,10 @@ GetFilesystemInfo (
>>    EFI_FILE_SYSTEM_INFO  *Info;
>>    EFI_STATUS            Status;
>>    UINTN                 ResultSize;
>> +  UINTN                 StringSize;
>>
>> -  ResultSize = SIZE_OF_EFI_FILE_SYSTEM_INFO + StrSize (mSemihostFsLabel);
>> +  StringSize = StrSize (mSemihostFsLabel);
>> +  ResultSize = SIZE_OF_EFI_FILE_SYSTEM_INFO + StringSize;
>>
>>    if (*BufferSize >= ResultSize) {
>>      ZeroMem (Buffer, ResultSize);
>> @@ -843,7 +846,7 @@ GetFilesystemInfo (
>>      Info->FreeSpace  = 0;
>>      Info->BlockSize  = 0;
>>
>> -    StrCpy (Info->VolumeLabel, mSemihostFsLabel);
>> +    CopyMem (Info->VolumeLabel, mSemihostFsLabel, StringSize);
>>    } else {
>>      Status = EFI_BUFFER_TOO_SMALL;
>>    }
>> @@ -903,7 +906,7 @@ FileGetInfo (
>>      ResultSize = StrSize (mSemihostFsLabel);
>>
>>      if (*BufferSize >= ResultSize) {
>> -      StrCpy (Buffer, mSemihostFsLabel);
>> +      CopyMem (Buffer, mSemihostFsLabel, *BufferSize);
>
> This is still wrong; here *BufferSize is the size of the recipient
> buffer, passed in from the caller. As written, the code can overrun the
> *source* buffer. Please use
>
>       CopyMem (Buffer, mSemihostFsLabel, ResultSize);
>
> instead.
>
> With that update:
>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>

I'll apply this update locally before I test the v2 series.


> Thanks
> Laszlo
>
>>        Status = EFI_SUCCESS;
>>      } else {
>>        Status = EFI_BUFFER_TOO_SMALL;
>> @@ -963,11 +966,12 @@ SetFileInfo (
>>      return EFI_ACCESS_DENIED;
>>    }
>>
>> -  AsciiFileName = AllocatePool (StrLen (Info->FileName) + 1);
>> +  Length = StrLen (Info->FileName) + 1;
>> +  AsciiFileName = AllocatePool (Length);
>>    if (AsciiFileName == NULL) {
>>      return EFI_OUT_OF_RESOURCES;
>>    }
>> -  UnicodeStrToAsciiStr (Info->FileName, AsciiFileName);
>> +  UnicodeStrToAsciiStrS (Info->FileName, AsciiFileName, Length);
>>
>>    FileSizeIsDifferent = (Info->FileSize != Fcb->Info.FileSize);
>>    FileNameIsDifferent = (AsciiStrCmp (AsciiFileName, Fcb->FileName) != 0);
>>
>

Re: [PATCH v2 0/6] ArmPkg: eliminate calls to deprecated functions

[Ryan Harkin]

On 26 October 2016 at 13:21, Ryan Harkin <ryan.harkin@linaro.org> wrote:
> On 26 October 2016 at 13:13, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>> On 26 October 2016 at 13:11, Ryan Harkin <ryan.harkin@linaro.org> wrote:
>>> Hi Ard,
>>>
>>> You don't have my Tested-by tags in all of the v2 patches.  Do I need
>>> to retest?  I thought they changes were trivial, but I'm happy to
>>> retest if it helps.
>>>
>>
>> If you don't mind, yes please. Given that they are changes that look
>> trivial on the face of it but could potentially cause hard to diagnose
>> problems if implemented incorrectly, I thought keeping the T-b was
>> perhaps not appropriate in this case.
>
> No problem, I'll do it now before I forget.
>

As before, tested on Juno R0/1/2, TC2 and FVP AEMv8 and Foundation models.

Tested-by: Ryan Harkin <ryan.harkin@linaro.org>


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