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

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On 28 October 2016 at 14:52, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> 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.
>

Indeed, I missed that. Been looking at many different variations on
this theme over the past week, so my vision got a little blurry,
sorry.

> 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>
>

Thanks.