On 28 October 2016 at 14:52, Leif Lindholm <leif.lindh...@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.lindh...@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.biesheu...@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.lindh...@linaro.org>
>
Thanks.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
