From mboxrd@z Thu Jan  1 00:00:00 1970
From: Hans de Goede <hdegoede@redhat.com>
Date: Sun, 6 Mar 2016 11:08:36 +0100
Subject: [U-Boot] [PATCH] malloc: handle free() before gd is set
In-Reply-To: <56D9C7FD.2050806@wwwdotorg.org>
References: <1457079557-31419-1-git-send-email-swarren@wwwdotorg.org>
 <56D94B43.3020505@redhat.com> <56D9C7FD.2050806@wwwdotorg.org>
Message-ID: <56DC01A4.5060600@redhat.com>
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

Hi,

On 04-03-16 18:38, Stephen Warren wrote:
> 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.

You're right, since simple_malloc depends on gd being set
we should not call it when gd is not set, so the above code
is correct.

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

I was thinking along the same lines, except that I wonder if
the non-simple malloc may work without gd, or in other words
if there are platforms which call mem_malloc_init() before
setting the gd because they need it early?

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

malloc_simple depends on gd being set unlike the dlmalloc code itself,
which depends on mem_malloc_init() being called.

So in hindsight I believe that your original patch is correct,
since the malloc() simple_malloc check is correct, and we should
mirror it free(), and then indeed trust that if we get past this
check because gd == NULL, free is being called with a NULL ptr.

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

It could be that it has some cleanup-code which also gets called
on init which unconditionally does: free(foo), even if foo was never
set, since free(NULL) is a nop. And your patch makes it a nop again
even when building with malloc_simple and gd == NULL.

But if there is a matching malloc which gets called before gd gets
set then indeed there is something fishy here.

Regards,

Hans