[edk2] MdePkg/BaseMemoryLibOptDxe: check for zero length in ZeroMem ()

Message ID 1478194274-16524-1-git-send-email-ard.biesheuvel@linaro.org
State New
Headers show

Commit Message

Ard Biesheuvel Nov. 3, 2016, 5:31 p.m.
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

Comments

Laszlo Ersek Nov. 3, 2016, 5:38 p.m. | #1
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
Carsey, Jaben Nov. 3, 2016, 5:38 p.m. | #2
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
Ard Biesheuvel Nov. 3, 2016, 6:05 p.m. | #3
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
Laszlo Ersek Nov. 3, 2016, 6:10 p.m. | #4
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

Patch

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);