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 6CA9E1A1E60 for ; Tue, 25 Oct 2016 05:40:47 -0700 (PDT) Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) (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 F1B46C056790; Tue, 25 Oct 2016 12:40:46 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-71.phx2.redhat.com [10.3.116.71]) by int-mx11.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id u9PCejo5031365; Tue, 25 Oct 2016 08:40:46 -0400 To: Ard Biesheuvel , edk2-devel@ml01.01.org, leif.lindholm@linaro.org References: <1477330907-13733-1-git-send-email-ard.biesheuvel@linaro.org> <1477330907-13733-6-git-send-email-ard.biesheuvel@linaro.org> From: Laszlo Ersek Message-ID: Date: Tue, 25 Oct 2016 14:40:44 +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: <1477330907-13733-6-git-send-email-ard.biesheuvel@linaro.org> X-Scanned-By: MIMEDefang 2.68 on 10.5.11.24 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.32]); Tue, 25 Oct 2016 12:40:47 +0000 (UTC) Subject: Re: [PATCH 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: Tue, 25 Oct 2016 12:40:47 -0000 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit On 10/24/16 19:41, 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 | 4 ++-- > 2 files changed, 4 insertions(+), 3 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; > } This loses the zero padding, but I guess that's okay. Is fine otherwise. > diff --git a/EmbeddedPkg/Application/AndroidFastboot/AndroidFastbootApp.c b/EmbeddedPkg/Application/AndroidFastboot/AndroidFastbootApp.c > index 9ddc34f57cf4..960218b25241 100644 > --- a/EmbeddedPkg/Application/AndroidFastboot/AndroidFastbootApp.c > +++ b/EmbeddedPkg/Application/AndroidFastboot/AndroidFastbootApp.c > @@ -127,7 +127,7 @@ HandleDownload ( > if (mDataBuffer == NULL) { > SEND_LITERAL ("FAILNot enough memory"); > } else { > - AsciiStrnCpy (Response + 4, NumBytesString, 8); > + AsciiStrnCpyS (Response + 4, mNumDataBytes, NumBytesString, 8); > mTransport->Send (sizeof(Response), Response, &mFatalSendErrorEvent); > > mState = ExpectDataState; I don't think this is right. Here we're trying to copy no more than 8 characters from NumBytesString into Response, after the "DATA" prefix. mNumDataBytes is the decimal value of NumBytesString, and it's unrelated to this formatting. What we could do is AsciiStrnCpyS (Response + 4, sizeof Response - 4, NumBytesString, 8) in order to remain "surgical". However, that's not right again, because AsciiStrnCpyS() *always* NUL-terminates, and here we only have CHAR8 Response[12] = "DATA"; i.e., no room for values above 0x0FFF_FFFF. Another issue is that the zero padding of AsciiStrnCpy() would be lost, and we actually send the zero padding to the wire. So, the real fix is, in my opinion: * resize Response to 4 + 8 + 1 == 13 bytes, * format it like this: ZeroMem (Response, sizeof Response); AsciiSPrint (Response, sizeof Response, "DATA%x", (UINT32)mNumDataBytes); * send it like this: mTransport->Send (sizeof Response - 1, Response, &mFatalSendErrorEvent); > @@ -257,7 +257,7 @@ AcceptCmd ( > } > > // Commands aren't null-terminated. Let's get a null-terminated version. > - AsciiStrnCpy (Command, Data, Size); > + AsciiStrnCpyS (Command, sizeof Command, Data, Size); > Command[Size] = '\0'; > > // Parse command > This looks good, but the explicit NUL-termination can be dropped, as AsciiStrnCpyS() enforces that internally. Thanks Laszlo