From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-x234.google.com (mail-wm0-x234.google.com [IPv6:2a00:1450:400c:c09::234]) (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 7675981D99 for ; Fri, 28 Oct 2016 06:52:23 -0700 (PDT) Received: by mail-wm0-x234.google.com with SMTP id 140so76859257wmv.0 for ; Fri, 28 Oct 2016 06:52:23 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=siF44TYhjAM4Kqm4+zGKi9FD9XtD6MkwT+Rn0cC56fQ=; b=JwX22go2z1IMPYnE4KoO4kqAGalFrl9ooHD96N26TtzKn7nbpcjCF+/xqj5uC4oeGv t7CHD7HD0G9lbprHo6UO3ltb9NNQzzQ6fvL18Rpr5V01PJAMQ6/I79SbNAYeZjLjmsy3 gBM+ilVW48qcGKQjC4A6LF/TG8j6kd5HlCHR0= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=siF44TYhjAM4Kqm4+zGKi9FD9XtD6MkwT+Rn0cC56fQ=; b=RLTz1v+P0LFMgTWe51s+GUDVsX//NolOb005naz8iOeCVSpZ9EPYHihKMiAtQbl0II dP/74kRZRM3MbjFjpmlG2Rv2emAV9/tmTuLq/7/O0VoxwpYQuckKKjZbGeaQCnB7dZrj TEOn+pCKxCqjEj83kOfTXXqhf/mEtnfBQUdSe1+GTujK6uaF04XVi0lG+Upppib8Eifi QAYHB64tS3zWKp3Skk/jw9qd3eH9kGjM2KY0wvHhdu3U9Q22lWvKH0d917Rv7pkYb7UJ aeq/vp1wLUmx5H2O9yDHdK2u7Y+wIdeLNwWLJTsnCMjNk/77S9RNOXevBb+5Uefu21pB rliQ== X-Gm-Message-State: ABUngvdRSEeHRLpwepZanrUdLgwGyUDufsDX7KdZU5s3MbUPxhe283XiFsyfOK5j4WKGbccI X-Received: by 10.194.148.77 with SMTP id tq13mr11603016wjb.101.1477662737168; Fri, 28 Oct 2016 06:52:17 -0700 (PDT) Received: from bivouac.eciton.net (bivouac.eciton.net. [2a00:1098:0:86:1000:23:0:2]) by smtp.gmail.com with ESMTPSA id l19sm9080870wmg.5.2016.10.28.06.52.16 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Fri, 28 Oct 2016 06:52:16 -0700 (PDT) Date: Fri, 28 Oct 2016 14:52:14 +0100 From: Leif Lindholm To: Ard Biesheuvel Cc: edk2-devel-01 , Laszlo Ersek , Ryan Harkin Message-ID: <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> MIME-Version: 1.0 In-Reply-To: User-Agent: Mutt/1.5.23 (2014-03-12) 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 13:52:23 -0000 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline 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 / 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 > >>