From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 673F181D99 for ; Fri, 28 Oct 2016 07:05:58 -0700 (PDT) Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 75F7E8FCFB; Fri, 28 Oct 2016 14:05:58 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-38.phx2.redhat.com [10.3.116.38]) by int-mx10.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id u9SE5u2c007768; Fri, 28 Oct 2016 10:05:57 -0400 To: Leif Lindholm , Ard Biesheuvel References: <1477651478-16830-1-git-send-email-ard.biesheuvel@linaro.org> <1477651478-16830-6-git-send-email-ard.biesheuvel@linaro.org> <20161028133625.GM1161@bivouac.eciton.net> <20161028135214.GN1161@bivouac.eciton.net> Cc: edk2-devel-01 , Ryan Harkin From: Laszlo Ersek Message-ID: <021d7ae5-d846-3aad-d3ad-a5c39f2e2b21@redhat.com> Date: Fri, 28 Oct 2016 16:05:56 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: <20161028135214.GN1161@bivouac.eciton.net> X-Scanned-By: MIMEDefang 2.68 on 10.5.11.23 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.26]); Fri, 28 Oct 2016 14:05:58 +0000 (UTC) Subject: Re: [PATCH v2 5/9] EmbeddedPkg/AndroidFastboot: eliminate deprecated string function calls X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 28 Oct 2016 14:05:58 -0000 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit 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 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 >>>> --- >>>> 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 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 >>>>