Re: [PATCH 6/9] EmbeddedPkg/Ebl: eliminate deprecated string function calls

[Laszlo Ersek <lersek@redhat.com>]

On 10/24/16 19:41, Ard Biesheuvel wrote:
> Get rid of calls to unsafe string functions. These are deprecated and may
> be removed in the future.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  EmbeddedPkg/Ebl/Command.c   |  2 +-
>  EmbeddedPkg/Ebl/Dir.c       |  4 ++--
>  EmbeddedPkg/Ebl/EfiDevice.c | 11 ++++++-----
>  EmbeddedPkg/Ebl/Main.c      |  8 ++++----
>  EmbeddedPkg/Ebl/Variable.c  | 17 +++++++++++------
>  5 files changed, 24 insertions(+), 18 deletions(-)
> 
> diff --git a/EmbeddedPkg/Ebl/Command.c b/EmbeddedPkg/Ebl/Command.c
> index e75c6a2e5c32..4bc1f4df0ca0 100644
> --- a/EmbeddedPkg/Ebl/Command.c
> +++ b/EmbeddedPkg/Ebl/Command.c
> @@ -614,7 +614,7 @@ OutputData (
>    UINTN Spaces   = 0;
>    CHAR8 Blanks[80];
>  
> -  AsciiStrCpy (Blanks, mBlanks);
> +  AsciiStrCpyS (Blanks, sizeof Blanks, mBlanks);

The original code is so poor that it makes me weep. (Not knowing who
wrote this, and I don't want to look it up.) The mBlanks array has 43
(forty three) space characters in it.

Please drop the mBlanks array, and use a SetMem() call here.

Or... meh, I don't care. It's fine as it is.

>    for (EndAddress = Address + Length; Address < EndAddress; Offset += Line) {
>      AsciiPrint ("%08x: ", Offset);
>      for (Line = 0; (Line < 0x10) && (Address < EndAddress);) {
> diff --git a/EmbeddedPkg/Ebl/Dir.c b/EmbeddedPkg/Ebl/Dir.c
> index 36095b633019..8dd9d48ff6ac 100644
> --- a/EmbeddedPkg/Ebl/Dir.c
> +++ b/EmbeddedPkg/Ebl/Dir.c
> @@ -116,7 +116,7 @@ EblDirCmd (
>      UnicodeFileName[0] = '\0';
>      MatchSubString = &UnicodeFileName[0];
>      if (Argc > 2) {
> -      AsciiStrToUnicodeStr (Argv[2], UnicodeFileName);
> +      AsciiStrToUnicodeStrS (Argv[2], UnicodeFileName, MAX_CMD_LINE);
>        if (UnicodeFileName[0] == '*') {
>          // Handle *Name substring matching
>          MatchSubString = &UnicodeFileName[1];

Looks okay.

> @@ -231,7 +231,7 @@ EblDirCmd (
>      MatchSubString = NULL;
>      UnicodeFileName[0] = '\0';
>      if (Argc > 2) {
> -      AsciiStrToUnicodeStr (Argv[2], UnicodeFileName);
> +      AsciiStrToUnicodeStrS (Argv[2], UnicodeFileName, MAX_CMD_LINE);
>        if (UnicodeFileName[0] == '*') {
>          MatchSubString = &UnicodeFileName[1];
>        }

Ditto.

> diff --git a/EmbeddedPkg/Ebl/EfiDevice.c b/EmbeddedPkg/Ebl/EfiDevice.c
> index ec9c331b7004..f6969e7b2b05 100644
> --- a/EmbeddedPkg/Ebl/EfiDevice.c
> +++ b/EmbeddedPkg/Ebl/EfiDevice.c
> @@ -343,7 +343,7 @@ EblStartCmd (
>  
>        ImageInfo->LoadOptionsSize = (UINT32)AsciiStrSize (Argv[2]);
>        ImageInfo->LoadOptions     = AllocatePool (ImageInfo->LoadOptionsSize);
> -      AsciiStrCpy (ImageInfo->LoadOptions, Argv[2]);
> +      AsciiStrCpyS (ImageInfo->LoadOptions, ImageInfo->LoadOptionsSize, Argv[2]);
>      }
>  
>      // Transfer control to the EFI image we loaded with LoadImage()

AllocateCopyPool() would be much better, but this will do.

> @@ -741,7 +741,7 @@ EblFileCopyCmd (
>    UINTN         Size;
>    UINTN         Offset;
>    UINTN         Chunk        = FILE_COPY_CHUNK;
> -  UINTN         FileNameLen;
> +  UINTN         FileNameLen, DestFileNameLen;
>    CHAR8*        DestFileName;
>    CHAR8*        SrcFileName;
>    CHAR8*        SrcPtr;
> @@ -786,9 +786,10 @@ EblFileCopyCmd (
>      }
>  
>      // Construct the destination filepath
> -    DestFileName = (CHAR8*)AllocatePool (FileNameLen + AsciiStrLen (SrcFileName) + 1);
> -    AsciiStrCpy (DestFileName, Argv[2]);
> -    AsciiStrCat (DestFileName, SrcFileName);
> +    DestFileNameLen = FileNameLen + AsciiStrLen (SrcFileName) + 1;
> +    DestFileName = (CHAR8*)AllocatePool (DestFileNameLen);
> +    AsciiStrCpyS (DestFileName, DestFileNameLen, Argv[2]);
> +    AsciiStrCatS (DestFileName, DestFileNameLen, SrcFileName);
>    }
>  
>    Source = EfiOpen(Argv[1], EFI_FILE_MODE_READ, 0);

AsciiSPrint would be (and would have been, originally) superior, but
this will do.

> diff --git a/EmbeddedPkg/Ebl/Main.c b/EmbeddedPkg/Ebl/Main.c
> index 18b2878f69a1..4f04ae4afd33 100644
> --- a/EmbeddedPkg/Ebl/Main.c
> +++ b/EmbeddedPkg/Ebl/Main.c
> @@ -88,7 +88,7 @@ SetCmdHistory (
>      }
>  
>      // Copy the new command line into the ring buffer
> -    AsciiStrnCpy(&mCmdHistory[mCmdHistoryStart][0], Cmd, MAX_CMD_LINE);
> +    AsciiStrnCpyS (&mCmdHistory[mCmdHistoryStart][0], MAX_CMD_LINE, Cmd, MAX_CMD_LINE);
>    }
>  
>    // Reset the command history for the next up arrow press
> @@ -432,7 +432,7 @@ GetCmd (
>        }
>        AsciiPrint (History);
>        Index = AsciiStrLen (History);
> -      AsciiStrnCpy (Cmd, History, CmdMaxSize);
> +      AsciiStrnCpyS (Cmd, CmdMaxSize, History, CmdMaxSize);
>      } else {
>        Cmd[Index++] = Char;
>        if (FixedPcdGetBool(PcdEmbeddedShellCharacterEcho) == TRUE) {
> @@ -644,14 +644,14 @@ EdkBootLoaderEntry (
>  
>      Status = gRT->GetVariable(CommandLineVariableName, &VendorGuid, NULL, &CommandLineVariableSize, CommandLineVariable);
>      if (!EFI_ERROR(Status)) {
> -      UnicodeStrToAsciiStr(CommandLineVariable, CmdLine);
> +      UnicodeStrToAsciiStrS (CommandLineVariable, CmdLine, CommandLineVariableSize);
>      }

This is wrong, the last argument should be MAX_CMD_LINE.

CommandLineVariableSize is the size (incl. the terminating NUL) of the
UCS2 string read from the variable. The last argument of
UnicodeStrToAsciiStrS() is DestMax, "The maximum number of Destination
scii char, including terminating null char.". That is, MAX_CMD_LINE.

>  
>      FreePool(CommandLineVariable);
>    }
>  
>    if (EFI_ERROR(Status)) {
> -    AsciiStrCpy (CmdLine, (CHAR8 *)PcdGetPtr (PcdEmbeddedAutomaticBootCommand));
> +    AsciiStrCpyS (CmdLine, MAX_CMD_LINE, (CHAR8 *)PcdGetPtr (PcdEmbeddedAutomaticBootCommand));
>    }
>  
>    for (;;) {

This looks good.

> diff --git a/EmbeddedPkg/Ebl/Variable.c b/EmbeddedPkg/Ebl/Variable.c
> index f440c48f16dd..b94531300d07 100644
> --- a/EmbeddedPkg/Ebl/Variable.c
> +++ b/EmbeddedPkg/Ebl/Variable.c
> @@ -29,6 +29,7 @@ EblGetCmd (
>    VOID*       Value;
>    CHAR8*      AsciiVariableName = NULL;
>    CHAR16*     VariableName;
> +  UINTN       VariableNameLen;
>    UINT32      Index;
>  
>    if (Argc == 1) {
> @@ -48,8 +49,9 @@ EblGetCmd (
>      AsciiPrint("Variable name is missing.\n");
>      return Status;
>    } else {
> -    VariableName = AllocatePool((AsciiStrLen (AsciiVariableName) + 1) * sizeof (CHAR16));
> -    AsciiStrToUnicodeStr (AsciiVariableName,VariableName);
> +    VariableNameLen = (AsciiStrLen (AsciiVariableName) + 1) * sizeof (CHAR16);
> +    VariableName = AllocatePool(VariableNameLen);
> +    AsciiStrToUnicodeStrS (AsciiVariableName, VariableName, VariableNameLen);
>    }
>  
>    // Try to get the variable size.

This is wrong. VariableNameLen is the size in bytes of a CHAR16 array
(incl. the terminating L'\0'). The last argument of
AsciiStrToUnicodeStrS(), is DestMax: "The maximum number of Destination
Unicode char, including terminating null char."

> @@ -93,6 +95,7 @@ EblSetCmd (
>    CHAR8*        AsciiValue;
>    UINT32        AsciiValueLength;
>    CHAR16*       VariableName;
> +  UINTN         VariableNameLen;
>    UINT32        Index;
>    UINT32        EscapedQuotes = 0;
>    BOOLEAN       Volatile = FALSE;
> @@ -125,8 +128,9 @@ EblSetCmd (
>      //
>  
>      // Convert VariableName into Unicode
> -    VariableName = AllocatePool((AsciiStrLen (AsciiVariableSetting) + 1) * sizeof (CHAR16));
> -    AsciiStrToUnicodeStr (AsciiVariableSetting,VariableName);
> +    VariableNameLen = (AsciiStrLen (AsciiVariableSetting) + 1) * sizeof (CHAR16);
> +    VariableName = AllocatePool(VariableNameLen);
> +    AsciiStrToUnicodeStrS (AsciiVariableSetting, VariableName, VariableNameLen);
>  
>      Status = gRT->SetVariable (
>                            VariableName,

Same issue here.

> @@ -170,8 +174,9 @@ EblSetCmd (
>    }
>  
>    // Convert VariableName into Unicode
> -  VariableName = AllocatePool((AsciiStrLen (AsciiVariableName) + 1) * sizeof (CHAR16));
> -  AsciiStrToUnicodeStr (AsciiVariableName,VariableName);
> +  VariableNameLen = (AsciiStrLen (AsciiVariableName) + 1) * sizeof (CHAR16);
> +  VariableName = AllocatePool(VariableNameLen);
> +  AsciiStrToUnicodeStrS (AsciiVariableName, VariableName, VariableNameLen);
>  
>    Status = gRT->SetVariable (
>                        VariableName,
> 

And here.

Thanks
Laszlo