From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-yw0-x232.google.com (mail-yw0-x232.google.com [IPv6:2607:f8b0:4002:c05::232]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id AB9C081DA0 for ; Fri, 28 Oct 2016 07:04:37 -0700 (PDT) Received: by mail-yw0-x232.google.com with SMTP id p22so71365120ywe.0 for ; Fri, 28 Oct 2016 07:04:38 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=4VsN3GaRJ/ULoDjAKzWt/r+bxOPfSVUjXelLQPvOHIM=; b=kLoUFWhQ8whXeCftslDncMSV6HRpNvHjBQzxky1lpl97YaaqIAMctve3MwIAK9n33t YOqHVBfieE3cl7oPNJ1B/+LYP2a+wIbmn/+nVDKW3Oi1uZ/vib5SGtAioDvLOsUEt2+j 7L/BHTeqAVDnDCJgPUijNqFiyjn0+ljrzrMho= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=4VsN3GaRJ/ULoDjAKzWt/r+bxOPfSVUjXelLQPvOHIM=; b=axEYY8+4sU5xftikLOMEjyz30HyHWmhhzKWnsEHmQhpeWyREcKRx/cEgx3m6hkOIF2 B4fwZD1IY5FoPH8CbTa6pgGIqSArWtWMRaQjl0PrZ9svD/HmX2xc45+DKgOrrJRpj8UN nCpPukAGPBZD9cIgpV64tDOxqvurPwCgK9AojStiQfS+5QT6NYyeTAQe4fiTfueERsPl ogKKmUTHz4IhlR4LftWpLCmzFT69PMrWE5i3K3AqFelopQq5EfKVLmBMVX8H5m2wQRxr 6/CSXzXBImSS1GF/Q12zgDsJmtJ6hsMZE6Ou5avXc6rLBc1sQCLAXv9XUQfwfmaO5oEk zV5g== X-Gm-Message-State: ABUngvdwJ+8kZm7jhp7+a6juPScFF8TnomAP8zAVgodRR65ZKvYQ/Oxsht1/LeINqGO7oeniLebpslcMnmBMEoEv X-Received: by 10.36.146.84 with SMTP id l81mr2646540itd.37.1477663477216; Fri, 28 Oct 2016 07:04:37 -0700 (PDT) MIME-Version: 1.0 Received: by 10.107.5.139 with HTTP; Fri, 28 Oct 2016 07:04:36 -0700 (PDT) In-Reply-To: <20161028135214.GN1161@bivouac.eciton.net> 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> From: Ard Biesheuvel Date: Fri, 28 Oct 2016 15:04:36 +0100 Message-ID: To: Leif Lindholm Cc: edk2-devel-01 , Laszlo Ersek , Ryan Harkin 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:04:37 -0000 Content-Type: text/plain; charset=UTF-8 On 28 October 2016 at 14: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. > 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 > Thanks.