4

I am writing an embedded software for STM32F7 and my libc is newlib-2.4.0.20160527.

I have implemented _sbrk() as follows:

extern intptr_t g_bss_end; /* value after the last byte in .bss */
extern intptr_t g_msp_lim; /* stack buffer starts at this address */

intptr_t _sbrk(ptrdiff_t heap_incr)
{
    static intptr_t heap_end = 0;

    intptr_t prev_heap_end;
    intptr_t new_heap_end;

    if(heap_end == 0) {
        heap_end = (intptr_t)&g_bss_end;
    }

    prev_heap_end = heap_end;
    new_heap_end = prev_heap_end + heap_incr;

    if(new_heap_end >= g_msp_lim) {
        errno = ENOMEM;

        return -1;
    }

    heap_end = new_heap_end;

    return prev_heap_end;
}

Then, when I do the following:

/* total capacity of my heap is 0x40000 */
void * mem = malloc(0x40000);
free(mem); mem = 0;
mem = malloc(0x40000);

everything works fine (i.e., malloc returns non-zero twice).

But when I do the following (for testing purposes):

for(int32_t sz = 0x50000; sz >= 0; sz--) {
    void * mem = malloc(sz);

    if(mem != 0) {
        __BKPT();
        free(mem);

        break;
    }
}

every malloc() fails, even malloc(0) (i.e., __BKPT() is never reached). So, there is no allocated memory on heap in fact (I did not get any mem != 0 so I can not even free() something) and there is also no available memory.

I expected malloc() to fail for every sz > 0x40000 and succeed for every sz <= 0x40000 (assuming free() works fine after each malloc()).

Have I missed something, or this is either a bug or intended behavior in newlib?

Piotr Jedyk
  • 361
  • 1
  • 10
  • What does the debugger say? Did you step throught the code? Note: using heap-based dynamic memory allocaion like `malloc` in embedded systems is most often a bad idea and disallowed by many coding standards for good reasons. Notably deterministic behaviour and guaranteed allocation. Evaluate the use of pools or other measures like static variables before even thinking about `malloc` etc! – too honest for this site Aug 22 '16 at 21:10
  • Oh, and use the `NULL` macro with pointers. `0` as null pointer constant is valid, but a bad habit from C++ programmers. C++11 introduced `nullptr` for good reasons. (Wish C11 had followed them) – too honest for this site Aug 22 '16 at 21:11
  • I stepped through my code (I checked the values of `mem`; `__BKPT()` was also a breakpoint). To get inside the newlib's code with gdb I am recompiling it with `-g3 -O0` now. I want to have working `snprintf` which relies on `malloc`. Should I start searching `snprintf` alternatives? I do not need `malloc` for anything else. – Piotr Jedyk Aug 22 '16 at 21:26
  • Maybe you first should learn about embedded software development. The stream-IO functions are also major resource eaters. Though I wonder why `snprintf` would **require** `malloc`. – too honest for this site Aug 22 '16 at 21:30
  • I am learning right now, not doing this for work/school/etc.. I have just started to analyse `malloc()` and `snprintf()` routines line by line. – Piotr Jedyk Aug 22 '16 at 21:40
  • What happens when you change the "direction" of the loop? I.e. start counting up instead of down? – Daniel Jour Aug 22 '16 at 22:03
  • Are `g_bss_end` and `g_msp_lim` symbols defined in the linker script? Those are symbols, not pointers (I usually `extern char` them), so `if(new_heap_end >= g_msp_lim)` is wrong and should read `&g_msp_lim`. Otherwise you're reading some random stack memory. – Andrea Biondo Aug 22 '16 at 22:14
  • `g_msp_lim` is an addres, `g_bss_end` is a value. – Piotr Jedyk Aug 22 '16 at 22:59
  • what is `SIZE_MAX` on your platform – M.M Aug 22 '16 at 23:51
  • 5
    @Olaf `0` is null pointer constant in C. It isn't a "bad habit from C++", it has been used that way since well before C++ was invented – M.M Aug 22 '16 at 23:53
  • @M.M: I think I made clear that it is vaild. It still is a bad habit from C++. Most C programmer use the macro. It was one of the worst decissions not to provide a keyword for this. Apparently the C++ commitee had the same thoughts. – too honest for this site Aug 23 '16 at 11:12
  • BTW - in newlib you can use sniprintf() as an alternative to snprintf() (there are alternatives for other *printf() functions as well, just add 'i'). malloc() is needed in "normal" version of this function for some floating point stuff. The 'i' versions of these functions don't support floats, so they also don't need malloc(). And don't read all these comments about "you shouldn't do X and Y in embedded" - this is just some ancient nonsense... – Freddie Chopin Sep 20 '16 at 18:21
  • @FreddieChopin Actually its not so much ancient nonsense as it has something do with reliability. If you have an embedded system that does something security critical and you do dynamic memory allocation that could fragment your memory piece by piece. After running for a long enough time this can lead to your device running out of usable memory chunks even though the total free memory is still large enough. This can crash your device. Hence why many embedded coding guidelines discourage it and some certifications forbid it using standard malloc at least. – Blackclaws Oct 04 '16 at 17:35
  • @Blackclaws There's a huge difference between "all embedded" and "embedded system that does something security critical". Do you (and Olaf, to who I'm referring) really assume that everybody is working on life-saving medical equipment, military systems or avionics? As for "coding guidelines" I'll stick to my own judgement instead of using some rules developed 20-30 years ago - especially as I work with "normal", not "safety critical" systems. – Freddie Chopin Oct 04 '16 at 20:54

2 Answers2

6

The newlib's malloc() does not work properly when allocating whole heap memory due to bad malloc_extend_top() routine in newlib/libc/stdlib/mallocr.c:2137. After a successful call to _sbrk()

  brk = (char*)(MORECORE (sbrk_size)); /* MORECORE = _sbrk */

  /* Fail if sbrk failed or if a foreign sbrk call killed our space */
  if (brk == (char*)(MORECORE_FAILURE) || 
      (brk < old_end && old_top != initial_top))
    return;

it tries to calculate the correction to fit the page alignment:

/* Guarantee alignment of first new chunk made from this space */
front_misalign = (POINTER_UINT)chunk2mem(brk) & MALLOC_ALIGN_MASK;
if (front_misalign > 0) 
{
  correction = (MALLOC_ALIGNMENT) - front_misalign;
  brk += correction;
}
else
  correction = 0;

/* Guarantee the next brk will be at a page boundary */
correction += pagesz - ((POINTER_UINT)(brk + sbrk_size) & (pagesz - 1));

The correction is always positive, because even if the allocation fits perfectly it tries to allocate the next whole page. E.g., if page size is 4096 and the brk + sbrk_size = 4096*n, expression 4096 - ((brk + sbrk_size) & 4095) will give 4096, so the next empty page is required, but there is no space for it.

The routine is handling this situation not properly and leaves just allocated data (brk value), resulting in permanent "unfreeable" whole heap allocation. Such a waste :-)

Piotr Jedyk
  • 361
  • 1
  • 10
  • 2
    Maybe you should open a bug report for this? – Daniel Jour Aug 23 '16 at 03:33
  • +1 because I use newlib malloc a lot. In this case `front_misalign` will always be zero (because chunk address and header size are both aligned). `correction` will indeed be 4096, but further down in the code it is `sbrk`ed,which will fail, so `correction` will be reset to zero and `sbrked_mem` (the actual global) will stay the same. It seems to me that the real issue is that when correction fails it sets `new_brk = brk`, so further down you'll get a zero `top_size` which get passed to `set_head`, resulting in a zero-sized head block which fails further allocations (can't be extended). – Andrea Biondo Aug 23 '16 at 08:56
2

Tested, works OK.

From 1473e08d2a16ad448afedb7036a476231a785643 Mon Sep 17 00:00:00 2001
From: Jeff Johnston <jjohnstn@redhat.com>
Date: Thu, 24 May 2018 23:53:15 -0400
Subject: [PATCH] Fix issue with malloc_extend_top

- when calculating a correction to align next brk to page boundary,
  ensure that the correction is less than a page size
- if allocating the correction fails, ensure that the top size is
  set to brk + sbrk_size (minus any front alignment made)

Signed-off-by: Jeff Johnston <jjohnstn@redhat.com>
---
 newlib/libc/stdlib/mallocr.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/newlib/libc/stdlib/mallocr.c b/newlib/libc/stdlib/mallocr.c
index ecc445f..26d1c89 100644
--- a/newlib/libc/stdlib/mallocr.c
+++ b/newlib/libc/stdlib/mallocr.c
@@ -2198,13 +2198,18 @@ static void malloc_extend_top(RARG nb) RDECL INTERNAL_SIZE_T nb;
     /* Guarantee the next brk will be at a page boundary */
     correction += pagesz - ((POINTER_UINT)(brk + sbrk_size) & (pagesz - 1));

+    /* To guarantee page boundary, correction should be less than pagesz */
+    correction &= (pagesz - 1);
+
     /* Allocate correction */
     new_brk = (char*)(MORECORE (correction));
     if (new_brk == (char*)(MORECORE_FAILURE))
       {
    correction = 0;
    correction_failed = 1;
-   new_brk = brk;
+   new_brk = brk + sbrk_size;
+   if (front_misalign > 0)
+     new_brk -= (MALLOC_ALIGNMENT) - front_misalign;
       }

     sbrked_mem += correction;
-- 
1.8.3.1
Oleksandr Kapitanenko
  • 21
  • 2