This is the mail archive of the newlib@sourceware.org mailing list for the newlib project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


While working on another issue, I noticed something in nano-mallocr.c
that doesn't look right to me:

As described in nano-mallocr.c, chunks of heap are represented in memory
as follows: [ size ] [ optional padding ] [ data area ]

Note that "optional padding" refers to padding added explicitly by code,
not to any padding that the compiler could theoretically introduce into
the layout of the struct.

Annotating the code around line 310 of nano-mallocr.c (nano_malloc) with
some comments:

    /* r is either a chunk address retrieved from the free list, or a
       new address retrieved from sbrk_aligned.  */

    /* ptr points to the data area ASSUMING that no optional padding is
       being used.  */
    ptr = (char *)r + CHUNK_OFFSET;

    /* align_ptr points to the data area, taking any padding into
       account.  */
    align_ptr = (char *)ALIGN_TO((unsigned long)ptr, MALLOC_ALIGN);

    /* offset is the size of the optional padding.  */
    offset = align_ptr - ptr;

This means that r < ptr <= align_ptr

The layout can then be summarised as follows:
  (r) [ size ] [ compiler padding ]
  (ptr) [ explicit optional padding ]
  (align_ptr) [ data area ]

nano_malloc then needs to initialize any optional padding with a
negative offset to size:

    if (offset)
    {
      *(long *)((char *)r + offset) = -offset;
    }

The above code calculates the address of the optional padding as
(r + offset).  This looks incorrect to me, as offset is the size of the
padding, not the size of [ size ] + [ compiler padding ].

Similarly, it then populates the padding with -offset, when the padding
should be populated with a negative offset to [ size ].

I don't have a motivating test case for this, it is based purely on
reading of the code. I suspect that in most (all?) real cases the value
of offset is coincidentally equal to the required value.

The following patch addresses these concerns.

I've built and tested as follows:
  Configured (gcc+newlib) with: --target=msp430-elf --enable-languages=c
  gcc testsuite variations:
    msp430-sim/-mcpu=msp430
    msp430-sim/-mcpu=msp430x
    msp430-sim/-mcpu=msp430x/-mlarge/-mdata-region=either/-mcode-region=either
    msp430-sim/-mhwmult=none
    msp430-sim/-mhwmult=f5series
My testing has shown no regressions, however I don't know if the gcc
testsuite provides sufficient coverage for this patch?

In an ideal world I would propose a patch that eliminates the use of
code-managed padding in nano_malloc, unfortunately I don't have the time
to consider whether such a patch is viable.

I don't have write access, so if this patch is deemed correct and
acceptable, I would appreciate if someone would commit it on my behalf.

Thanks,

2017-01-XX  Joe Seymour  <joe.s@somniumtech.com>

	newlib/
	* libc/stdlib/nano-mallocr.c (nano_malloc): Fix initialization
	of padding. Add comments.
	(nano_memalign): Fix initialization of padding.
---
 newlib/libc/stdlib/nano-mallocr.c |    9 +++++++--
 1 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/newlib/libc/stdlib/nano-mallocr.c b/newlib/libc/stdlib/nano-mallocr.c
index 457eb88..61e6591 100644
--- a/newlib/libc/stdlib/nano-mallocr.c
+++ b/newlib/libc/stdlib/nano-mallocr.c
@@ -307,14 +307,19 @@ void * nano_malloc(RARG malloc_size_t s)
     }
     MALLOC_UNLOCK;
 
+    /* ptr points to malloc_chunk.next, ASSUMING that no alignment padding is
+       being used.  */
     ptr = (char *)r + CHUNK_OFFSET;
 
+    /* align_ptr points to malloc_chunk.next, taking alignment padding into
+       account.  */
     align_ptr = (char *)ALIGN_TO((unsigned long)ptr, MALLOC_ALIGN);
     offset = align_ptr - ptr;
 
     if (offset)
     {
-        *(long *)((char *)r + offset) = -offset;
+        /* Write the negative offset to size into alignment padding.  */
+        *(long *)(ptr) = -(CHUNK_OFFSET);
     }
 
     assert(align_ptr + size <= (char *)r + alloc_size);
@@ -587,7 +592,7 @@ void * nano_memalign(RARG size_t align, size_t s)
             /* Padding is used. Need to set a jump offset for aligned pointer
             * to get back to chunk head */
             assert(offset >= sizeof(int));
-            *(long *)((char *)chunk_p + offset) = -offset;
+            *(long *)((char *)chunk_p + CHUNK_OFFSET) = -(CHUNK_OFFSET);
         }
     }
 
-- 
1.7.1


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]