malloc: handle free() before gd is set

PATCH New Stephen Warren 2 participants 4 messages Mar 4, 2016 – Mar 6, 2016

Share & tools

Stephen Warren

author

2016-03-04 08:19 UTC #0 lore raw

PATCH 1/1 malloc: handle free() before gd is set

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 <[email protected]>

---
 common/dlmalloc.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

common/dlmalloc.c +3 -3

index 5ea37dfb6e4c..7453e63d6bf4 100644
@@ -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

@@ -2609,7 +2609,7 @@ Void_t* rEALLOc(oldmem, bytes) Void_t* oldmem; size_t bytes;
   if (oldmem == NULL) return mALLOc(bytes);
 
 #ifdef CONFIG_SYS_MALLOC_F_LEN
-	if (!(gd->flags & GD_FLG_FULL_MALLOC_INIT)) {
+	if (gd && !(gd->flags & GD_FLG_FULL_MALLOC_INIT)) {
 		/* This is harder to support and should not be needed */
 		panic("pre-reloc realloc() is not supported");
 	}

@@ -2985,7 +2985,7 @@ Void_t* cALLOc(n, elem_size) size_t n; size_t elem_size;
   else
   {
 #ifdef CONFIG_SYS_MALLOC_F_LEN
-	if (!(gd->flags & GD_FLG_FULL_MALLOC_INIT)) {
+	if (gd && !(gd->flags & GD_FLG_FULL_MALLOC_INIT)) {
 		MALLOC_ZERO(mem, sz);
 		return mem;
 	}

Signature · 1 line

2.7.0

Hans de Goede

2016-03-04 08:45 UTC #1 lore raw

PATCH 1/1 malloc: handle free() before gd is set

Hi,

Quoted 16 lines · Stephen Warren

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 <[email protected]>
---
  common/dlmalloc.c | 6 +++---
  1 file changed, 3 insertions(+), 3 deletions(-)
common/dlmalloc.c +0 -0
index 5ea37dfb6e4c..7453e63d6bf4 100644

common/dlmalloc.c void fREe(mem) Void_t* mem; · line 2459

Quoted 8 lines

common/dlmalloc.c +1 -1

@@ -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.

Regards,

Hans

Quoted 19 lines

common/dlmalloc.c +2 -2

@@ -2609,7 +2609,7 @@ Void_t* rEALLOc(oldmem, bytes) Void_t* oldmem; size_t bytes;
    if (oldmem == NULL) return mALLOc(bytes);
 
  #ifdef CONFIG_SYS_MALLOC_F_LEN
-	if (!(gd->flags & GD_FLG_FULL_MALLOC_INIT)) {
+	if (gd && !(gd->flags & GD_FLG_FULL_MALLOC_INIT)) {
  		/* This is harder to support and should not be needed */
  		panic("pre-reloc realloc() is not supported");
  	}

@@ -2985,7 +2985,7 @@ Void_t* cALLOc(n, elem_size) size_t n; size_t elem_size;
    else
    {
  #ifdef CONFIG_SYS_MALLOC_F_LEN
-	if (!(gd->flags & GD_FLG_FULL_MALLOC_INIT)) {
+	if (gd && !(gd->flags & GD_FLG_FULL_MALLOC_INIT)) {
  		MALLOC_ZERO(mem, sz);
  		return mem;
  	}

Stephen Warren

author

2016-03-04 17:38 UTC #2 lore raw

PATCH 1/1 malloc: handle free() before gd is set

Quoted 31 lines · Hans de Goede

Hi,
Quoted 21 lines · Stephen Warren
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 <[email protected]>
---
  common/dlmalloc.c | 6 +++---
  1 file changed, 3 insertions(+), 3 deletions(-)
common/dlmalloc.c +0 -0
index 5ea37dfb6e4c..7453e63d6bf4 100644
@@ -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 */
common/dlmalloc.c void fREe(mem) Void_t* mem; · line 2458
Quoted 4 lines
common/dlmalloc.c +1 -1
-    if (!(gd->flags & GD_FLG_FULL_MALLOC_INIT))
+    if (gd && !(gd->flags & GD_FLG_FULL_MALLOC_INIT))
          return;
  #endif
I believe you want:

Quoted 3 lines

+ 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.

Hans de Goede

2016-03-06 10:08 UTC #3 lore raw

PATCH 1/1 malloc: handle free() before gd is set

Hi,

Quoted 43 lines · Stephen Warren

Quoted 31 lines · Hans de Goede
Hi,
Quoted 21 lines · Stephen Warren
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 <[email protected]>
---
  common/dlmalloc.c | 6 +++---
  1 file changed, 3 insertions(+), 3 deletions(-)
common/dlmalloc.c +0 -0
index 5ea37dfb6e4c..7453e63d6bf4 100644
@@ -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 */
common/dlmalloc.c void fREe(mem) Void_t* mem; · line 2458
Quoted 4 lines
common/dlmalloc.c +1 -1
-    if (!(gd->flags & GD_FLG_FULL_MALLOC_INIT))
+    if (gd && !(gd->flags & GD_FLG_FULL_MALLOC_INIT))
          return;
  #endif
I believe you want:
Quoted 3 lines
+ 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

Quoted 7 lines

   /* 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.

Quoted 21 lines

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)
(quoted diff) +2 -1
+   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

Quoted 5 lines

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?

Quoted 1 line

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.

Quoted 1 line

[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