Re: [U-Boot] [PATCH] malloc: handle free() before gd is set
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
---
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
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] malloc: handle free() before gd is set
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
---
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.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] malloc: handle free() before gd is set
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
---
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.
Regards,
Hans
@@ -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;
}
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
[U-Boot] [PATCH] 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
---
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
@@ -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;
}
--
2.7.0
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
