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

[Laszlo Ersek <lersek@redhat.com>]

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