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 <[email protected]> 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 <[email protected]>
>>>> ---
>>>> 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 <[email protected]>
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
>>>>
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.01.org/mailman/listinfo/edk2-devel
