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]> / 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
