Re: [PATCH v2 5/9] EmbeddedPkg/AndroidFastboot: eliminate deprecated string function calls

[Leif Lindholm <leif.lindholm@linaro.org>]

On Fri, Oct 28, 2016 at 02:40:59PM +0100, Ard Biesheuvel wrote:
> On 28 October 2016 at 14:36, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> > On Fri, Oct 28, 2016 at 11:44:34AM +0100, 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 | 11 ++++++-----
> >>  2 files changed, 8 insertions(+), 6 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;
> >>  }
> >> diff --git a/EmbeddedPkg/Application/AndroidFastboot/AndroidFastbootApp.c b/EmbeddedPkg/Application/AndroidFastboot/AndroidFastbootApp.c
> >> index 9ddc34f57cf4..c5e8a7e34af2 100644
> >> --- a/EmbeddedPkg/Application/AndroidFastboot/AndroidFastbootApp.c
> >> +++ b/EmbeddedPkg/Application/AndroidFastboot/AndroidFastbootApp.c
> >> @@ -99,7 +99,7 @@ HandleDownload (
> >>    IN CHAR8 *NumBytesString
> >>    )
> >>  {
> >> -  CHAR8       Response[12] = "DATA";
> >> +  CHAR8       Response[13];
> >>    CHAR16      OutputString[FASTBOOT_STRING_MAX_LENGTH];
> >>
> >>    // Argument is 8-character ASCII string hex representation of number of bytes
> >> @@ -127,8 +127,10 @@ HandleDownload (
> >>    if (mDataBuffer == NULL) {
> >>      SEND_LITERAL ("FAILNot enough memory");
> >>    } else {
> >> -    AsciiStrnCpy (Response + 4, NumBytesString, 8);
> >> -    mTransport->Send (sizeof(Response), Response, &mFatalSendErrorEvent);
> >> +    ZeroMem (Response, sizeof Response);
> >> +    AsciiSPrint (Response, sizeof Response, "DATA%x",
> >> +      (UINT32)mNumDataBytes);
> >
> > I'll try to keep the bikeshedding to a minimum, but since
> > mNumDataBytes is generated from NumBytesString in the first place, why
> > not do
> >   "DATA%s", NumBytesString
> > ?
> >
> 
> Are you asking me? Or the author of the original code?

Well, the original code used NumBytesString, and your updated version
does not.

As per Laszlo's comment - the implementation of
AsciiStrHexToUint64 means that an arbitrarily long string of leading
zeroes could be handled by this version that would not previously have
been handled.

If that is desired behaviour, then that makes this change a bugfix
rather than just an API cleanup. Which should be mentioned in the
commit message. If you do that:

Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>

/
    Leif

> >> +    mTransport->Send (sizeof Response - 1, Response, &mFatalSendErrorEvent);
> >>
> >>      mState = ExpectDataState;
> >>      mBytesReceivedSoFar = 0;
> >> @@ -257,8 +259,7 @@ AcceptCmd (
> >>    }
> >>
> >>    // Commands aren't null-terminated. Let's get a null-terminated version.
> >> -  AsciiStrnCpy (Command, Data, Size);
> >> -  Command[Size] = '\0';
> >> +  AsciiStrnCpyS (Command, sizeof Command, Data, Size);
> >>
> >>    // Parse command
> >>    if (MATCH_CMD_LITERAL ("getvar", Command)) {
> >> --
> >> 2.7.4
> >>