Re: [PATCH 2/2] Fix padding initialization in nano_malloc?

[Joe Seymour <joe.s@somniumtech.com>]

Hi Joey,

On 10/01/2017 03:45, Joey Ye wrote:
> Thanks for reading the code and starting this discussion. But IMHO you
> misunderstood the data structure thus this patch is not valid.

Thanks for the review! I agree and withdraw that patch.

I was thinking of the padding as being like a variable-length field in the
structure, which led me to assume that the offset was being read/written from
the beginning of the padding, instead of the end.

To try and stop anyone else making the same mistake I've prepared the following
patch, which adds some more comments to nano-mallocr.c. Note that I haven't
verified that the existing code ensures that the size of padding is at least
CHUNK_OFFSET.

What do you think?

Thanks,

Joe Seymour

---
 newlib/libc/stdlib/nano-mallocr.c |   23 ++++++++++++++++++++---
 1 files changed, 20 insertions(+), 3 deletions(-)

diff --git a/newlib/libc/stdlib/nano-mallocr.c b/newlib/libc/stdlib/nano-mallocr.c
index 457eb88..4497eb6 100644
--- a/newlib/libc/stdlib/nano-mallocr.c
+++ b/newlib/libc/stdlib/nano-mallocr.c
@@ -128,7 +128,7 @@ typedef struct malloc_chunk
      *          ------------------
      *          | Padding for    |
      *          | alignment      |
-     *          | holding neg    |
+     *          | ending with neg|
      *          | offset to size |
      *          ------------------
      * mem_ptr->| point to next  |
@@ -187,9 +187,23 @@ extern void * nano_pvalloc(RARG size_t s);
 
 static inline chunk * get_chunk_from_ptr(void * ptr)
 {
+    /* Assume that there is no explicit padding in the
+       chunk, and that the chunk starts at ptr - CHUNK_OFFSET.  */
     chunk * c = (chunk *)((char *)ptr - CHUNK_OFFSET);
-    /* Skip the padding area */
-    if (c->size < 0) c = (chunk *)((char *)c + c->size);
+
+    if (c->size < 0) {
+      /* c->size being negative indicates that there is explicit padding in
+         the chunk; c->size is actually the last CHUNK_OFFSET bytes in that
+         padding.
+
+         To find the true start of the chunk we need to subtract the size of
+         any remaining padding, and then CHUNK_OFFSET from c.  This is
+         equivalent to subtracting the total size of the padding from c,
+         because the size of any remaining padding is the total size of the
+         padding minus CHUNK_OFFSET.  The end of the padding should have been
+         initialized with this negative offset.  */
+      c = (chunk *)((char *)c + c->size);
+    }
     return c;
 }
 
@@ -314,6 +328,9 @@ void * nano_malloc(RARG malloc_size_t s)
 
     if (offset)
     {
+        /* Initialize the end of the padding with a negative offset to the
+           start of the chunk.  The rest of the padding is not initialized.
+           Note that the size of the padding must be at least CHUNK_OFFSET.  */
         *(long *)((char *)r + offset) = -offset;
     }
 
-- 
1.7.1