[U-Boot] [PATCH V2 1/2] malloc: use hidden visibility

[Simon Glass]

On 5 March 2016 at 10:30, Stephen Warren <swarren at wwwdotorg.org> wrote:
> When running sandbox, the following phases occur, each with different
> malloc implementations or behaviors:
>
> 1) Dynamic linker execution, using the dynamic linker's own malloc()
> implementation. This is fully functional.
>
> 2) After U-Boot's malloc symbol has been hooked into the GOT, but before
> any U-Boot code has run. This phase is entirely non-functional, since
> U-Boot's gd symbol is NULL and U-Boot's initf_malloc() and
> mem_malloc_init() have not been called.
>
> At least on Ubuntu Xenial, the dynamic linker does make both malloc() and
> free() calls during this phase. Currently these free() calls crash since
> they dereference gd, which is NULL.
>
> U-Boot itself makes no use of malloc() during this phase.
>
> 3) U-Boot execution after gd is set and initf_malloc() has been called.
> This is fully functional, albeit via a very simple malloc()
> implementation.
>
> 4) U-Boot execution after mem_malloc_init() has been called. This is fully
> functional with a complete malloc() implementation.
>
> Furthermore, if code that called malloc() during phase 1 calls free() in
> phase 3 or later, it is likely that heap corruption will occur, since
> U-Boot's malloc implementation will assume the pointer is part of its own
> heap, although it isn't. I have not actively observed this happening.
>
> To prevent phase 2 from happening, this patch makes all of U-Boot's malloc
> library public symbols have hidden visibility. This prevents them from
> being hooked into the GOT, so only code in the U-Boot binary itself
> actually calls them; any other code will call into the standard C library
> malloc(). This also avoids the "furthermore" issue mentioned above.
>
> I have seen references to this GCC pragma in blog posts from 2008, and
> RHEL5's ancient gcc appears to accept it fine, so I believe it's quite
> safe to use it without checking gcc version.
>
> Cc: Rabin Vincent <rabin at rab.in>
> Signed-off-by: Stephen Warren <swarren at wwwdotorg.org>
> ---
> v2: A whole different approach.
> ---
>  include/malloc.h | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/include/malloc.h b/include/malloc.h
> index f20e4d3d2a6b..8175c75920cf 100644
> --- a/include/malloc.h
> +++ b/include/malloc.h
> @@ -914,6 +914,7 @@ int initf_malloc(void);
>  /* Simple versions which can be used when space is tight */
>  void *malloc_simple(size_t size);
>
> +#pragma GCC visibility push(hidden)
>  # if __STD_C
>
>  Void_t* mALLOc(size_t);
> @@ -945,6 +946,7 @@ int     mALLOPt();
>  struct mallinfo mALLINFo();
>  # endif
>  #endif
> +#pragma GCC visibility pop
>
>  /*
>   * Begin and End of memory area for malloc(), and current "brk"
> --
> 2.7.0
>

This seems better than what we have. Thanks for digging into it.

Reviewed-by: Simon Glass <sjg at chromium.org>