From mboxrd@z Thu Jan  1 00:00:00 1970
From: Stephen Warren <swarren@wwwdotorg.org>
Date: Fri, 4 Mar 2016 10:38:05 -0700
Subject: [U-Boot] [PATCH] malloc: handle free() before gd is set
In-Reply-To: <56D94B43.3020505@redhat.com>
References: <1457079557-31419-1-git-send-email-swarren@wwwdotorg.org>
 <56D94B43.3020505@redhat.com>
Message-ID: <56D9C7FD.2050806@wwwdotorg.org>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

On 03/04/2016 01:45 AM, Hans de Goede wrote:
> Hi,
>
> On 04-03-16 09:19, Stephen Warren wrote:
>> On at least Ubuntu Xenial, free() can be called before main(). In this
>> case, U-Boot won't have set gd, so dereferencing it will crash. Check
>> whether gd is set before using it.
>>
>> While at it, apply the same fix to other functions.
>>
>> Signed-off-by: Stephen Warren <swarren@wwwdotorg.org>
>> ---
>>   common/dlmalloc.c | 6 +++---
>>   1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/common/dlmalloc.c b/common/dlmalloc.c
>> index 5ea37dfb6e4c..7453e63d6bf4 100644
>> --- a/common/dlmalloc.c
>> +++ b/common/dlmalloc.c
>> @@ -2453,7 +2453,7 @@ void fREe(mem) Void_t* mem;
>>
>>   #ifdef CONFIG_SYS_MALLOC_F_LEN
>>       /* free() is a no-op - all the memory will be freed on
>> relocation */
>> -    if (!(gd->flags & GD_FLG_FULL_MALLOC_INIT))
>> +    if (gd && !(gd->flags & GD_FLG_FULL_MALLOC_INIT))
>>           return;
>>   #endif
>>
>
> I believe you want:
>
> +    if (!gd || !(gd->flags & GD_FLG_FULL_MALLOC_INIT))
>
> Instead, so that you actually go into the return; path when there is no gd.

Hmm. Is the existing logic at the start of malloc() (which I copied) 
incorrect too then? Perhaps so...

#ifdef CONFIG_SYS_MALLOC_F_LEN
   if (gd && !(gd->flags & GD_FLG_FULL_MALLOC_INIT))
     return malloc_simple(bytes);
#endif

   /* check if mem_malloc_init() was run */
   if ((mem_malloc_start == 0) && (mem_malloc_end == 0)) {
     /* not initialized yet */
     return NULL;
   }

I guess that works because "if (gd && ..." prevents gd from being 
dereferenced, but doesn't actually return, and then presumably 
"(mem_malloc_start == 0) && (mem_malloc_end == 0)" is true at that 
point, so the function returns NULL immediately anyway.

For free() after my change:

#ifdef CONFIG_SYS_MALLOC_F_LEN
   /* free() is a no-op - all the memory will be freed on relocation */
   if (!(gd->flags & GD_FLG_FULL_MALLOC_INIT))
     return;
#endif

   if (mem == NULL) /* free(0) has no effect */
     return;

I guess that "mem == NULL" is always true, since malloc() always 
returned NULL, so everything works out somewhat accidentally in a 
similar way. Still, as you say it's probably better to be a bit more 
direct and add an explicit guard in malloc on gd leaving it:

+ if (!gd)
+   return NULL;
   #ifdef CONFIG_SYS_MALLOC_F_LEN
-   if (gd && !(gd->flags & GD_FLG_FULL_MALLOC_INIT))
+   if (!(gd->flags & GD_FLG_FULL_MALLOC_INIT)
       return malloc_simple(bytes);
   #endif

and free:

+ if (!gd)
+   return;

Or perhaps actually using malloc_simple() if (!gd) is the better option, 
since obviously something[1] is actually trying to allocate memory?

[1] IIRC something in the dynamic loader, but I forget the complete 
backtrace right now.