[PATCH 4/4] malloc: Fix sbrk clearing memory after freeing it instead of before

[Sean Anderson]

On 5/4/21 11:26 AM, Simon Glass wrote:
> Hi Sean,
> 
> On Sun, 2 May 2021 at 20:55, Sean Anderson <seanga2 at gmail.com> wrote:
>>
>> This fixes memory being cleared after releasing it. Instead, clear memory
>> before releasing it. In addition, suppress valgrind warnings about writing
>> to free'd memory.
>>
>> Signed-off-by: Sean Anderson <seanga2 at gmail.com>
>> ---
>>
>>   common/dlmalloc.c | 10 ++++++----
>>   1 file changed, 6 insertions(+), 4 deletions(-)
>>
>> diff --git a/common/dlmalloc.c b/common/dlmalloc.c
>> index 05c8fd87e7..ea51bdf6a6 100644
>> --- a/common/dlmalloc.c
>> +++ b/common/dlmalloc.c
>> @@ -592,11 +592,13 @@ void *sbrk(ptrdiff_t increment)
>>          ulong new = old + increment;
>>
>>          /*
>> -        * if we are giving memory back make sure we clear it out since
>> -        * we set MORECORE_CLEARS to 1
>> +        * if we are allocating memory make sure we clear it out since we set
>> +        * MORECORE_CLEARS to 1
>>           */
>> -       if (increment < 0)
>> -               memset((void *)new, 0, -increment);
>> +       if (increment > 0) {
>> +               VALGRIND_MAKE_MEM_UNDEFINED(old, increment);
>> +               memset((void *)old, 0, increment);
>> +       }
> 
> Can you explain this a bit more? What is the difference?

As it turns out, this patch is wrong. We need to clear memory when we
release it if SYS_MALLOC_CLEAR_ON_INIT is set, since calloc assumes that
memory has already been cleared if it gets it from sbrk.

> Do you need the cast?

Yes (but this is moot)

common/dlmalloc.c: In function ‘sbrk’:
common/dlmalloc.c:600:10: warning: passing argument 1 of ‘memset’ makes pointer from integer without a cast [-Wint-conversion]
   600 |   memset(old, 0, increment);
       |          ^~~
       |          |
       |          ulong {aka long unsigned int}
In file included from include/common.h:21,
                  from common/dlmalloc.c:1:
include/linux/string.h:111:22: note: expected ‘void *’ but argument is of type ‘ulong’ {aka ‘long unsigned int’}
   111 | extern void * memset(void *,int,__kernel_size_t);
       |                      ^~~~~~

--Sean

> 
>>
>>          if ((new < mem_malloc_start) || (new > mem_malloc_end))
>>                  return (void *)MORECORE_FAILURE;
>> --
>> 2.31.0
>>
> 
> Regards,
> Simon
>