Re: [PATCH 3/6] ArmPkg/LinuxLoader: 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/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);

Apparently nothing in this file checks if the tags being added still
actually fit in the preallocated space (which is EFI_SIZE_TO_PAGES
(ATAG_MAX_SIZE)). The change does preserve the previous behavior ("copy
the full string").

> 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);
>    }
>  
>    //
> 

I prefer to call character counts without the terminating NUL "Length",
and character counts with the terminating NUL "Size", but that's just a
personal preference. (And the rest of this code uses Length differently
already, for example in "LineLength" itself, near the top.)

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

Thanks
Laszlo