Re: [PATCH 7/9] EmbeddedPkg/EfiFileLib: 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/Library/EfiFileLib/EfiFileLib.c | 42 +++++++++++---------
>  1 file changed, 23 insertions(+), 19 deletions(-)
> 
> diff --git a/EmbeddedPkg/Library/EfiFileLib/EfiFileLib.c b/EmbeddedPkg/Library/EfiFileLib/EfiFileLib.c
> index 4d58c830861c..d3b65aa5a3e0 100644
> --- a/EmbeddedPkg/Library/EfiFileLib/EfiFileLib.c
> +++ b/EmbeddedPkg/Library/EfiFileLib/EfiFileLib.c
> @@ -384,9 +384,9 @@ EblFileDevicePath (
>  
>  
>    if ( *FileName != 0 ) {
> -    AsciiStrToUnicodeStr (FileName, UnicodeFileName);
> +    AsciiStrToUnicodeStrS (FileName, UnicodeFileName, MAX_PATHNAME);

Seems okay.

>    } else {
> -    AsciiStrToUnicodeStr ("\\", UnicodeFileName);
> +    AsciiStrToUnicodeStrS ("\\", UnicodeFileName, MAX_PATHNAME);
>    }

Ditto.

>  
>    Size = StrSize (UnicodeFileName);
> @@ -589,7 +589,7 @@ EblFvFileDevicePath (
>            &AuthenticationStatus
>            );
>          if (!EFI_ERROR (Status)) {
> -          UnicodeStrToAsciiStr (Section, AsciiSection);
> +          UnicodeStrToAsciiStrS (Section, AsciiSection, MAX_PATHNAME);
>            if (AsciiStriCmp (FileName, AsciiSection) == 0) {
>              FreePool (Section);
>              break;

Seems fine.

> @@ -674,6 +674,7 @@ EfiOpen (
>    CHAR8                     *CwdPlusPathName;
>    UINTN                     Index;
>    EFI_SECTION_TYPE          ModifiedSectionType;
> +  UINTN                     AsciiLength;
>  
>    EblUpdateDeviceLists ();
>  
> @@ -706,7 +707,8 @@ EfiOpen (
>        }
>  
>        // We could add a current working directory concept
> -      CwdPlusPathName = AllocatePool (AsciiStrSize (gCwd) + AsciiStrSize (PathName));
> +      AsciiLength = AsciiStrSize (gCwd) + AsciiStrSize (PathName);
> +      CwdPlusPathName = AllocatePool (AsciiLength);
>        if (CwdPlusPathName == NULL) {
>          return NULL;
>        }
> @@ -723,14 +725,14 @@ EfiOpen (
>            }
>          }
>        } else {
> -        AsciiStrCpy (CwdPlusPathName, gCwd);
> +        AsciiStrCpyS (CwdPlusPathName, AsciiLength, gCwd);

okay

>          StrLen = AsciiStrLen (gCwd);
>          if ((*PathName != '/') && (*PathName != '\\') && (gCwd[StrLen-1] != '/') && (gCwd[StrLen-1] != '\\')) {
> -          AsciiStrCat (CwdPlusPathName, "\\");
> +          AsciiStrCatS (CwdPlusPathName, AsciiLength, "\\");
>          }
>        }

okay

>  
> -      AsciiStrCat (CwdPlusPathName, PathName);
> +      AsciiStrCatS (CwdPlusPathName, AsciiLength, PathName);
>        if (AsciiStrStr (CwdPlusPathName, ":") == NULL) {
>          // Extra error check to make sure we don't recurse and blow stack
>          return NULL;

Also fine.

(I strongly dislike the trickery in the original allocation, that is,
summing two full sizes (each including the terminating NUL separately),
and then letting one of those NULs account for the pathname separator
'\\' that gets inserted in the middle. It happens to be correct, but
seriously...)

> @@ -745,7 +747,7 @@ EfiOpen (
>    }
>  
>    File->DeviceName = AllocatePool (StrLen);

StrLen was initially assigned like this:

  StrLen = AsciiStrSize (PathName);

(Q: what would you call a variable storing the retval of a function
named XxxxSize()? A: XxxxLen, obviously!

Never mind that there is a function named XxxxLen() as well; we'll store
the retval of *that* in XxxxSize. For clarity.)

... Just to be clear, this is criticism for the original code.

> -  AsciiStrCpy (File->DeviceName, PathName);
> +  AsciiStrCpyS (File->DeviceName, StrLen, PathName);

okay.

>    File->DeviceName[FileStart - 1] = '\0';
>    File->FileName = &File->DeviceName[FileStart];
>    if (File->FileName[0] == '\0') {
> @@ -1611,7 +1613,7 @@ ExpandPath (
>  {
>    CHAR8   *NewPath;
>    CHAR8   *Work, *Start, *End;
> -  UINTN   StrLen;
> +  UINTN   StrLen, AllocLen;
>    INTN    i;
>  
>    if (Cwd == NULL || Path == NULL) {
> @@ -1625,11 +1627,12 @@ ExpandPath (
>    }
>  
>    StrLen = AsciiStrSize (Path);
> -  NewPath = AllocatePool (AsciiStrSize (Cwd) + StrLen + 1);
> +  AllocLen = AsciiStrSize (Cwd) + StrLen + 1;
> +  NewPath = AllocatePool (AllocLen);
>    if (NewPath == NULL) {
>      return NULL;
>    }
> -  AsciiStrCpy (NewPath, Cwd);
> +  AsciiStrCpyS (NewPath, AllocLen, Cwd);
>  
>    End = Path + StrLen;
>    for (Start = Path ;;) {
> @@ -1640,7 +1643,7 @@ ExpandPath (
>      }
>  
>      // append path prior to ..
> -    AsciiStrnCat (NewPath, Start, Work - Start);
> +    AsciiStrnCatS (NewPath, AllocLen, Start, Work - Start);
>      StrLen = AsciiStrLen (NewPath);
>      for (i = StrLen; i >= 0; i--) {
>        if (NewPath[i] == ':') {
> @@ -1663,7 +1666,7 @@ ExpandPath (
>    }
>  
>    // Handle the path that remains after the ..
> -  AsciiStrnCat (NewPath, Start, End - Start);
> +  AsciiStrnCatS (NewPath, AllocLen, Start, End - Start);
>  
>    return NewPath;
>  }

looks good

> @@ -1686,7 +1689,7 @@ EfiSetCwd (
>    )
>  {
>    EFI_OPEN_FILE *File;
> -  UINTN         Len;
> +  UINTN         Len, AllocLen;
>    CHAR8         *Path;
>  
>    if (Cwd == NULL) {
> @@ -1729,17 +1732,18 @@ EfiSetCwd (
>  
>    // Use the info returned from EfiOpen as it can add in CWD if needed. So Cwd could be
>    // relative to the current gCwd or not.
> -  gCwd = AllocatePool (AsciiStrSize (File->DeviceName) + AsciiStrSize (File->FileName) + 10);
> +  AllocLen = AsciiStrSize (File->DeviceName) + AsciiStrSize (File->FileName) + 10;
> +  gCwd = AllocatePool (AllocLen);
>    if (gCwd == NULL) {
>      return EFI_INVALID_PARAMETER;
>    }
>  
> -  AsciiStrCpy (gCwd, File->DeviceName);
> +  AsciiStrCpyS (gCwd, AllocLen, File->DeviceName);
>    if (File->FileName == NULL) {
> -    AsciiStrCat (gCwd, ":\\");
> +    AsciiStrCatS (gCwd, AllocLen, ":\\");
>    } else {
> -    AsciiStrCat (gCwd, ":");
> -    AsciiStrCat (gCwd, File->FileName);
> +    AsciiStrCatS (gCwd, AllocLen, ":");
> +    AsciiStrCatS (gCwd, AllocLen, File->FileName);
>    }
>  
>  
> 

Looks correct.

(The constant 10 in the addition, for the allocation, represents how
carefully the original code was written.)

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

I need some brain bleach now.