Re: [PATCH 5/9] EmbeddedPkg/AndroidFastboot: 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/Application/AndroidFastboot/AndroidBootImg.c     | 3 ++-
>  EmbeddedPkg/Application/AndroidFastboot/AndroidFastbootApp.c | 4 ++--
>  2 files changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/EmbeddedPkg/Application/AndroidFastboot/AndroidBootImg.c b/EmbeddedPkg/Application/AndroidFastboot/AndroidBootImg.c
> index bbca90fc08a2..f3e770bcc980 100644
> --- a/EmbeddedPkg/Application/AndroidFastboot/AndroidBootImg.c
> +++ b/EmbeddedPkg/Application/AndroidFastboot/AndroidBootImg.c
> @@ -84,7 +84,8 @@ ParseAndroidBootImg (
>                   + ALIGN_VALUE (Header->KernelSize, Header->PageSize));
>    }
>  
> -  AsciiStrnCpy (KernelArgs, Header->KernelArgs, BOOTIMG_KERNEL_ARGS_SIZE);
> +  AsciiStrnCpyS (KernelArgs, BOOTIMG_KERNEL_ARGS_SIZE, Header->KernelArgs,
> +    BOOTIMG_KERNEL_ARGS_SIZE);
>  
>    return EFI_SUCCESS;
>  }

This loses the zero padding, but I guess that's okay. Is fine otherwise.

> diff --git a/EmbeddedPkg/Application/AndroidFastboot/AndroidFastbootApp.c b/EmbeddedPkg/Application/AndroidFastboot/AndroidFastbootApp.c
> index 9ddc34f57cf4..960218b25241 100644
> --- a/EmbeddedPkg/Application/AndroidFastboot/AndroidFastbootApp.c
> +++ b/EmbeddedPkg/Application/AndroidFastboot/AndroidFastbootApp.c
> @@ -127,7 +127,7 @@ HandleDownload (
>    if (mDataBuffer == NULL) {
>      SEND_LITERAL ("FAILNot enough memory");
>    } else {
> -    AsciiStrnCpy (Response + 4, NumBytesString, 8);
> +    AsciiStrnCpyS (Response + 4, mNumDataBytes, NumBytesString, 8);
>      mTransport->Send (sizeof(Response), Response, &mFatalSendErrorEvent);
>  
>      mState = ExpectDataState;

I don't think this is right. Here we're trying to copy no more than 8
characters from NumBytesString into Response, after the "DATA" prefix.
mNumDataBytes is the decimal value of NumBytesString, and it's unrelated
to this formatting.

What we could do is

  AsciiStrnCpyS (Response + 4, sizeof Response - 4, NumBytesString, 8)

in order to remain "surgical". However, that's not right again, because
AsciiStrnCpyS() *always* NUL-terminates, and here we only have

  CHAR8       Response[12] = "DATA";

i.e., no room for values above 0x0FFF_FFFF. Another issue is that the
zero padding of AsciiStrnCpy() would be lost, and we actually send the
zero padding to the wire.

So, the real fix is, in my opinion:

* resize Response to 4 + 8 + 1 == 13 bytes,

* format it like this:
  ZeroMem (Response, sizeof Response);
  AsciiSPrint (Response, sizeof Response, "DATA%x",
    (UINT32)mNumDataBytes);

* send it like this:
  mTransport->Send (sizeof Response - 1, Response,
    &mFatalSendErrorEvent);


> @@ -257,7 +257,7 @@ AcceptCmd (
>    }
>  
>    // Commands aren't null-terminated. Let's get a null-terminated version.
> -  AsciiStrnCpy (Command, Data, Size);
> +  AsciiStrnCpyS (Command, sizeof Command, Data, Size);
>    Command[Size] = '\0';
>  
>    // Parse command
> 

This looks good, but the explicit NUL-termination can be dropped, as
AsciiStrnCpyS() enforces that internally.

Thanks
Laszlo