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

[Laszlo Ersek <lersek@redhat.com>]

On 10/24/16 18:06, 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..e79b5cc5cf39 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) ||

Sort of muddles the purpose of the preexistent Length variable, but it's
manageable I think.

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

Haha, this uses exactly the opposite meanings for Size and Length of
what I do :) Okay.

> @@ -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);
> +    StrCpyS (Info->VolumeLabel, StringSize, mSemihostFsLabel);
>    } else {
>      Status = EFI_BUFFER_TOO_SMALL;
>    }

This is wrong:

- StrSize() "Returns the size of a Null-terminated Unicode string in
bytes, including the Null terminator",

- but for StrCpyS(), "DestMax" is "The maximum number of Destination
Unicode char, including terminating null char."

I suggest to use CopyMem() (rather than divide).

> @@ -903,7 +906,7 @@ FileGetInfo (
>      ResultSize = StrSize (mSemihostFsLabel);
>  
>      if (*BufferSize >= ResultSize) {
> -      StrCpy (Buffer, mSemihostFsLabel);
> +      StrCpyS (Buffer, *BufferSize, mSemihostFsLabel);
>        Status = EFI_SUCCESS;
>      } else {
>        Status = EFI_BUFFER_TOO_SMALL;

Also wrong; please use CopyMem().

> @@ -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);
> 

This hunk looks okay.

Thanks
Laszlo