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

[Laszlo Ersek <lersek@redhat.com>]

On 10/28/16 15:52, Leif Lindholm 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.
> 
> 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>

Yes, I agree it's an improvement if Ard spells out the "added
robustness" in the commit message. (Should not require a repost.)

In this case, we do have a comment in the function:

  // Argument is 8-character ASCII string hex representation of number
  // of bytes that will be sent in the data phase.
  // Response is "DATA" + that same 8-character string.

Honestly, I didn't trust it fully. I didn't verify where the data comes
from, so the comment could be true. But, in all such cases, as a general
principle, I re-format the string representation from the parsed integer
value. It deals nicely with leading "no-op" characters, such as space
and zeros, and it also drops any trailing garbage even if the input is
*not* overlong.

(For example, if the input is "12zzzz", then AsciiStrHexToUint64() will
return 0x12, and ignore the "zzzz" suffix. I think there's value in not
reproducing such trailing garbage.)

I guess I stick to this principle without much thinking, so I didn't ask
for the commit message to be updated. :)

... Extrapolating a bit, I think you might ask for a commit message
update on patch v2 #8 as well :) Because, in addition to the API
replacement there, the patch fixes a separate, genuine overflow as well
(present in the original code). I think mentioning that fact in passing
in the commit message of v2 #8 should suffice.

Thanks!
Laszlo


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