| Message ID | 1478194274-16524-1-git-send-email-ard.biesheuvel@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 11/03/16 18:31, Ard Biesheuvel wrote: > Unlike other string functions in this library, ZeroMem () does not > return early when the length of the input buffer is 0. So add the > same to ZeroMem () as well. > > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > --- > MdePkg/Library/BaseMemoryLibOptDxe/ZeroMemWrapper.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/MdePkg/Library/BaseMemoryLibOptDxe/ZeroMemWrapper.c b/MdePkg/Library/BaseMemoryLibOptDxe/ZeroMemWrapper.c > index 2a0a038fd6c5..fbc2f5742c8c 100644 > --- a/MdePkg/Library/BaseMemoryLibOptDxe/ZeroMemWrapper.c > +++ b/MdePkg/Library/BaseMemoryLibOptDxe/ZeroMemWrapper.c > @@ -46,6 +46,10 @@ ZeroMem ( > IN UINTN Length > ) > { > + if (Length == 0) { > + return Buffer; > + } > + > ASSERT (!(Buffer == NULL && Length > 0)); > ASSERT (Length <= (MAX_ADDRESS - (UINTN)Buffer + 1)); > return InternalMemZeroMem (Buffer, Length); > 1. Why is this necessary? 2. After the new check, Length is guaranteed to be positive. The first ASSERT() should be updated (simplified), I think: ASSERT (Buffer != NULL); Thanks Laszlo _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Is it worth modifying the assert after to no longer check length being garter than 0? Jaben > On Nov 3, 2016, at 10:33 AM, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > > Unlike other string functions in this library, ZeroMem () does not > return early when the length of the input buffer is 0. So add the > same to ZeroMem () as well. > > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > --- > MdePkg/Library/BaseMemoryLibOptDxe/ZeroMemWrapper.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/MdePkg/Library/BaseMemoryLibOptDxe/ZeroMemWrapper.c b/MdePkg/Library/BaseMemoryLibOptDxe/ZeroMemWrapper.c > index 2a0a038fd6c5..fbc2f5742c8c 100644 > --- a/MdePkg/Library/BaseMemoryLibOptDxe/ZeroMemWrapper.c > +++ b/MdePkg/Library/BaseMemoryLibOptDxe/ZeroMemWrapper.c > @@ -46,6 +46,10 @@ ZeroMem ( > IN UINTN Length > ) > { > + if (Length == 0) { > + return Buffer; > + } > + > ASSERT (!(Buffer == NULL && Length > 0)); > ASSERT (Length <= (MAX_ADDRESS - (UINTN)Buffer + 1)); > return InternalMemZeroMem (Buffer, Length); > -- > 2.7.4 > > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 3 November 2016 at 17:38, Laszlo Ersek <lersek@redhat.com> wrote: > On 11/03/16 18:31, Ard Biesheuvel wrote: >> Unlike other string functions in this library, ZeroMem () does not >> return early when the length of the input buffer is 0. So add the >> same to ZeroMem () as well. >> >> Contributed-under: TianoCore Contribution Agreement 1.0 >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> >> --- >> MdePkg/Library/BaseMemoryLibOptDxe/ZeroMemWrapper.c | 4 ++++ >> 1 file changed, 4 insertions(+) >> >> diff --git a/MdePkg/Library/BaseMemoryLibOptDxe/ZeroMemWrapper.c b/MdePkg/Library/BaseMemoryLibOptDxe/ZeroMemWrapper.c >> index 2a0a038fd6c5..fbc2f5742c8c 100644 >> --- a/MdePkg/Library/BaseMemoryLibOptDxe/ZeroMemWrapper.c >> +++ b/MdePkg/Library/BaseMemoryLibOptDxe/ZeroMemWrapper.c >> @@ -46,6 +46,10 @@ ZeroMem ( >> IN UINTN Length >> ) >> { >> + if (Length == 0) { >> + return Buffer; >> + } >> + >> ASSERT (!(Buffer == NULL && Length > 0)); >> ASSERT (Length <= (MAX_ADDRESS - (UINTN)Buffer + 1)); >> return InternalMemZeroMem (Buffer, Length); >> > > 1. Why is this necessary? > The 32-bit accelerated ARM code writes at least one byte, and given that the other string functions take the same shortcut, this seemed the most appropriate way to fix that. > 2. After the new check, Length is guaranteed to be positive. The first > ASSERT() should be updated (simplified), I think: > > ASSERT (Buffer != NULL); > Good point. I will change that _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 11/03/16 19:05, Ard Biesheuvel wrote: > On 3 November 2016 at 17:38, Laszlo Ersek <lersek@redhat.com> wrote: >> On 11/03/16 18:31, Ard Biesheuvel wrote: >>> Unlike other string functions in this library, ZeroMem () does not >>> return early when the length of the input buffer is 0. So add the >>> same to ZeroMem () as well. >>> >>> Contributed-under: TianoCore Contribution Agreement 1.0 >>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> >>> --- >>> MdePkg/Library/BaseMemoryLibOptDxe/ZeroMemWrapper.c | 4 ++++ >>> 1 file changed, 4 insertions(+) >>> >>> diff --git a/MdePkg/Library/BaseMemoryLibOptDxe/ZeroMemWrapper.c b/MdePkg/Library/BaseMemoryLibOptDxe/ZeroMemWrapper.c >>> index 2a0a038fd6c5..fbc2f5742c8c 100644 >>> --- a/MdePkg/Library/BaseMemoryLibOptDxe/ZeroMemWrapper.c >>> +++ b/MdePkg/Library/BaseMemoryLibOptDxe/ZeroMemWrapper.c >>> @@ -46,6 +46,10 @@ ZeroMem ( >>> IN UINTN Length >>> ) >>> { >>> + if (Length == 0) { >>> + return Buffer; >>> + } >>> + >>> ASSERT (!(Buffer == NULL && Length > 0)); >>> ASSERT (Length <= (MAX_ADDRESS - (UINTN)Buffer + 1)); >>> return InternalMemZeroMem (Buffer, Length); >>> >> >> 1. Why is this necessary? >> > > The 32-bit accelerated ARM code writes at least one byte, Does that conform to the InternalMemZeroMem() contract? > and given > that the other string functions take the same shortcut, this seemed > the most appropriate way to fix that. I don't disagree, but then the commit message should explain this -- the circumstances where the missing shortcut actually caused a problem. > >> 2. After the new check, Length is guaranteed to be positive. The first >> ASSERT() should be updated (simplified), I think: >> >> ASSERT (Buffer != NULL); >> > > Good point. I will change that > Thanks! Laszlo _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
diff --git a/MdePkg/Library/BaseMemoryLibOptDxe/ZeroMemWrapper.c b/MdePkg/Library/BaseMemoryLibOptDxe/ZeroMemWrapper.c index 2a0a038fd6c5..fbc2f5742c8c 100644 --- a/MdePkg/Library/BaseMemoryLibOptDxe/ZeroMemWrapper.c +++ b/MdePkg/Library/BaseMemoryLibOptDxe/ZeroMemWrapper.c @@ -46,6 +46,10 @@ ZeroMem ( IN UINTN Length ) { + if (Length == 0) { + return Buffer; + } + ASSERT (!(Buffer == NULL && Length > 0)); ASSERT (Length <= (MAX_ADDRESS - (UINTN)Buffer + 1)); return InternalMemZeroMem (Buffer, Length);
Unlike other string functions in this library, ZeroMem () does not return early when the length of the input buffer is 0. So add the same to ZeroMem () as well. Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> --- MdePkg/Library/BaseMemoryLibOptDxe/ZeroMemWrapper.c | 4 ++++ 1 file changed, 4 insertions(+) -- 2.7.4 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel