| Message ID | 1477651478-16830-6-git-send-email-ard.biesheuvel@linaro.org |
|---|---|
| State | Accepted |
| Commit | 310908760f5314c4a6d421ebcc6e1370bd2a1177 |
| Headers | show |
On 10/28/16 12:44, 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 <ard.biesheuvel@linaro.org> > --- > 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); > + 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)) { > Reviewed-by: Laszlo Ersek <lersek@redhat.com> _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
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 <ard.biesheuvel@linaro.org> > --- > 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 ? > + 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 edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 28 October 2016 at 14:36, Leif Lindholm <leif.lindholm@linaro.org> 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 <ard.biesheuvel@linaro.org> >> --- >> 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? >> + 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 edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 10/28/16 15: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 <ard.biesheuvel@linaro.org> >> --- >> 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 > ? Because of "0000000000000000000000000000000000000000000000012". Thanks Laszlo _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On Fri, Oct 28, 2016 at 02:40:59PM +0100, Ard Biesheuvel wrote: > On 28 October 2016 at 14:36, Leif Lindholm <leif.lindholm@linaro.org> 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 <ard.biesheuvel@linaro.org> > >> --- > >> 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.lindholm@linaro.org> / 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 edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 28 October 2016 at 14:52, Leif Lindholm <leif.lindholm@linaro.org> wrote: > On Fri, Oct 28, 2016 at 02:40:59PM +0100, Ard Biesheuvel wrote: >> On 28 October 2016 at 14:36, Leif Lindholm <leif.lindholm@linaro.org> 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 <ard.biesheuvel@linaro.org> >> >> --- >> >> 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 <leif.lindholm@linaro.org> > Thanks. _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
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 <leif.lindholm@linaro.org> 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 <ard.biesheuvel@linaro.org> >>>> --- >>>> 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.lindholm@linaro.org> 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 >>>> _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
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); + 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)) {
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 <ard.biesheuvel@linaro.org> --- EmbeddedPkg/Application/AndroidFastboot/AndroidBootImg.c | 3 ++- EmbeddedPkg/Application/AndroidFastboot/AndroidFastbootApp.c | 11 ++++++----- 2 files changed, 8 insertions(+), 6 deletions(-) -- 2.7.4 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel