[PATCH v2 5/8] sandbox: Add os_realloc()

[Simon Glass]

Hi Heinrich,

On Sun, 24 Jan 2021 at 15:15, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
>
> On 1/23/21 6:36 PM, Simon Glass wrote:
> > We provide os_malloc() and os_free() but not os_realloc(). Add this,
> > following the usual semantics. Also update os_malloc() to behave correctly
> > when passed a zero size.
> >
> > Signed-off-by: Simon Glass <sjg at chromium.org>
>
> In the code I am missing a comment why we are not simply using malloc(),
> realloc(), free() instead of mmap(), munmap().

OK I will add one.

>
> mmap() creates page size aligned addresses while mALLOc() in non-sandbox
> U-Boot will creates MALLOC_ALIGNMENT aligned addresses. This may result
> in errors due to misalignment not being caught on the sandbox.
>
> Hence I would prefer if os_malloc() would add a prime number times
> MALLOC_ALIGNMENT to the address returned by mmap() instead of page_size
> to match the alignment guarantee of U-Boot. E.g.
>
>      #define MALLOC_ALIGNMENT (2 * sizeof(size_t))
>
>      void *os_malloc(size_t length)
>      {
>          ...
>          return (void *)hdr + 19 * MALLOC_ALIGNMENT;
>      }

But these allocations are for internal sandbox use, so are never used
outside that code. For what you suggest I think the ram_buf could
perhaps be set up unaligned, but of course the U-Boot code uses
addresses so would be oblivious to the actual alignment and unable to
use memalign(), etc. to achieve any alignment at the hardware level.

>
> We should put a magic string into the currently unused first bytes of
> the memory provided by mmap() and check if the string is present in
> os_free(). Crash the system if the magic is not found like U-Boot's
> malloc() does.

Well you mean glibc malloc(), but the U-Boot malloc() doesn't seem to
do that. Again, this is all internal to sandbox.

>
> POSIX requires that mmap() zeros any memory that it allocates. The
> memory returned by mmap() should be filled with random bytes to catch
> more errors.
>
> What are your thought on these topics?

See above.

As an aside I would like to have a new malloc() function, something like:

malloc_tagged(size, const char *tag)

which stores the tag string with the allocation, so that it is
possible to figure out who allocated it. Good for memory leaks, double
frees, etc. Then the malloc space could be meaningfully dumped. It
would also be nice to have a way to store the function name and line
that did the malloc(), so perhaps malloc_logged()..

>
> For the current patch:
>
> Reviewed-by: Heinrich Schuchardt <xypron.glpk at gmx.de>
>

Regards,
Simon