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

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