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

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

Changes in v2:
- Add new patch to add os_realloc()

 arch/sandbox/cpu/os.c | 38 ++++++++++++++++++++++++++++++++++++++
 include/os.h          | 12 +++++++++++-
 2 files changed, 49 insertions(+), 1 deletion(-)

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

In the code I am missing a comment why we are not simply using malloc(),
realloc(), free() instead of mmap(), munmap().

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

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.

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?

For the current patch:

Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>

> ---
>
> Changes in v2:
> - Add new patch to add os_realloc()
>
>   arch/sandbox/cpu/os.c | 38 ++++++++++++++++++++++++++++++++++++++
>   include/os.h          | 12 +++++++++++-
>   2 files changed, 49 insertions(+), 1 deletion(-)
>
> diff --git a/arch/sandbox/cpu/os.c b/arch/sandbox/cpu/os.c
> index 80996a91ce7..69ce6afdfac 100644
> --- a/arch/sandbox/cpu/os.c
> +++ b/arch/sandbox/cpu/os.c
> @@ -269,6 +269,8 @@ void *os_malloc(size_t length)
>   	int page_size = getpagesize();
>   	struct os_mem_hdr *hdr;
>
> +	if (!length)
> +		return NULL;
>   	/*
>   	 * Use an address that is hopefully available to us so that pointers
>   	 * to this memory are fairly obvious. If we end up with a different
> @@ -295,6 +297,42 @@ void os_free(void *ptr)
>   	}
>   }
>
> +/* These macros are from kernel.h but not accessible in this file */
> +#define ALIGN(x, a)		__ALIGN_MASK((x), (typeof(x))(a) - 1)
> +#define __ALIGN_MASK(x, mask)	(((x) + (mask)) & ~(mask))
> +
> +void *os_realloc(void *ptr, size_t length)
> +{
> +	int page_size = getpagesize();
> +	struct os_mem_hdr *hdr;
> +	void *new_ptr;
> +
> +	/* Reallocating a NULL pointer is just an alloc */
> +	if (!ptr)
> +		return os_malloc(length);
> +
> +	/* Changing a length to 0 is just a free */
> +	if (length) {
> +		os_free(ptr);
> +		return NULL;
> +	}
> +
> +	/*
> +	 * If the new size is the same number of pages as the old, nothing to
> +	 * do. There isn't much point in shrinking things
> +	 */
> +	hdr = ptr - page_size;
> +	if (ALIGN(length, page_size) <= ALIGN(hdr->length, page_size))
> +		return ptr;
> +
> +	/* We have to grow it, so allocate something new */
> +	new_ptr = os_malloc(length);
> +	memcpy(new_ptr, ptr, hdr->length);
> +	os_free(ptr);
> +
> +	return new_ptr;
> +}
> +
>   void os_usleep(unsigned long usec)
>   {
>   	usleep(usec);
> diff --git a/include/os.h b/include/os.h
> index 0913b47b3a8..b4c5dd727c4 100644
> --- a/include/os.h
> +++ b/include/os.h
> @@ -114,7 +114,7 @@ void os_fd_restore(void);
>    * os_malloc() - aquires some memory from the underlying os.
>    *
>    * @length:	Number of bytes to be allocated
> - * Return:	Pointer to length bytes or NULL on error
> + * Return:	Pointer to length bytes or NULL if @length is 0 or on error
>    */
>   void *os_malloc(size_t length);
>
> @@ -127,6 +127,16 @@ void *os_malloc(size_t length);
>    */
>   void os_free(void *ptr);
>
> +/**
> + * os_realloc() - reallocate memory
> + *
> + * This follows the semantics of realloc(), so can perform an os_malloc() or
> + * os_free() depending on @ptr and @length.
> + *
> + * Return:	Pointer to reallocated memory or NULL if @length is 0
> + */
> +void *os_realloc(void *ptr, size_t length);
> +
>   /**
>    * os_usleep() - access to the usleep function of the os
>    *
>

Hi Heinrich,

On Sun, 24 Jan 2021 at 15:15, Heinrich Schuchardt <xypron.glpk@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@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@gmx.de>
>

Regards,
Simon