Limit dynamic memory allocation to stack + reserve

[justinschoeman]

The current _sbrk() implementation allows dynamic allocations all the way to the top of RAM - overwriting the stack and crashing the board.

There does not seem to be a way to hardware limit the stack, but this small change provides a minimal amount of protection, and a mechanism to tune it for the application being developed.

The 'ret' variable should be allocated at the very bottom of the stack - so its address is the top of the free space for the heap. __sbrk_stack_reserve is a tunable parameter to allow the application developer to leave additional space for further stack growth.

[justinschoeman]

Oops - I got it wrong. Seems like it posted all the diffs to the main fork, even if they are 0 change diffs...

The actual diff is 707ebdf...

[rogerclarkmelbourne]

@RickKimball

This looks like its your area of expertise.

Could you let me know what you think ?

[RickKimball]

malloc isn't ever going to be perfect, You are only looking at a snapshot of the stack usage when you call malloc. In the next call, the stack might walk all over the heap.

A better strategy is to allocate all the memory you need up front so that it isn't changing. You should also monitor your maximum stack usage by coloring the stack and then monitoring it.
Anotther strategy is to use a debugger and a watchpoint to monitor stack and heap usage.

[justinschoeman]

As noted, the ARM7m series lacks hardware support for stack protection, so there can be no perfect solution on this hardware. The proposal is a small change that at least prevents malloc from immediately trashing the stack.

For advanced users, __sbrk_stack_reserve is exposed so they can provide an estimate of stack requirements for additional safety.

Pre-allocating memory is a worse strategy, as it is allocated on the heap anyway, and will still be trashed if the stack grows. Using malloc with a stack reserve will at least fail the malloc, if there is not enough space for the estimated stack requirements.

While my proposal is not perfect (it can not be perfect due to hardware limitations), it adds virtually no overhead and at least provides a degree of protection.

[RickKimball]

what do you plan to do when your malloc fails?

[RickKimball]

The alternative to the approach suggested is to move the STACK pointer to say 0x2000 0000+1024 at the start. Move the start of heap to 0x2000 0000+1024 and let it grow to 0x2000 5000. If the stack grows too much it will trigger a fault. they will never clash. *Assuming BluepIll

You can override the Reset_handler and memory routines I think they are all weak symbols.

[RickKimball]

Thinking about it more you would also have to adjust the start of RAM to be above the stack .. so 0x2000 0000+1024 and heap to start at start of RAM + size of data + size of bss

[justinschoeman]

malloc() will fail at the moment, if you attempt to allocate beyond the end of RAM. A well behaved application will detect the null return and either attempt a smaller malloc(), or fail in a sane way. A badly behaved application will always fail no matter what.

The current sbrk() implementation will cause malloc() to fail and return NULL when allocating beyond the end of RAM. It will succeed, but crash the board if you allocate below the end of RAM, but above the bottom of the stack. It will succeed and work normally if you allocate below the bottom of the stack. The proposed change causes the allocation to fail gracefully in the conditions that would have resulted in an immediate hang, at least giving the application developer the option of handling it gracefully.

I did consider your other proposals, but found them unworkable/impractical.

Moving the stack to the bottom of RAM is a great solution, as it is the only way to get hardware stack protection on this mcu. But it has problems. It will be impossible to pick an initial stack size that will be compatible with all existing apps/libraries (I have seen some that use big automatic variables, and some that use big dynamic allocations), so the target stack size would need to be modified on a per-application basis. Bootloader RAM would need to be above this configurable stack (probably only practical to locate it at the end of RAM) - but all bootloaders will need to be recompiled and reflashed to make the boards compatible with this change.

Pre-allocating memory and dishing it out is just an all round bad idea. You need to add your own memory manager. Overload new() and delete() to use your new memory manager. Avoid every libc call which uses malloc() internally (printf and many others). And in the end all you achieve is that the compiler tells you how much stack space you have available.

That is why I finally proposed this change. It is not ideal (no hardware stack protection). Is fully compatible with all well behaved programs (all programs, if you set __sbrk_stack_reserve to 0). Removes the case where malloc() currently crashes. And for the more advanced programmers, allows them to specify the required stack size (so you get all the advantages of pre-allocated memory, without any disadvantages).

[RickKimball]

I personally feel you should suffer failure if you are using malloc()/new in an embedded application. Memory fragmentation will eventually cause your device to fail at the least opportune moment. So I'm probably the wrong person to be commenting on this.

[rogerclarkmelbourne]

I'm afraid I agree with Rick.

Changing the core to handle the over allocation of RAM could potentially cause problems for existing users of the core as it appears to change the amount of RAM available slightly
(__sbrk_stack_reserve = 256 )

IMHO, embedded applications should be written so that they don't allocate more RAM than is available.

And using multiple dynamic allocations and de-allocations, would potentially cause memory fragmentation and lead to there not being enough RAM for the application to run.

I think that Arduino's String class may use dynamic allocation, but I think the jury is out on whether the inclusion of the String class as part of the API was a good idea or not.
(I think the ESP8266 core may just use a static pool for String allocation, but I could be wrong)

[justinschoeman]

While I agree that you should not really use dynamic memory in an embedded application, you do need to take the Arduino environment into account. C++ itself relies on dynamic memory for many constructs. And the libc packaged with the development environment depends on malloc for more than 50% of its functions (especially when you start using network related stuff).

If you want to move away from dynamic memory, the first thing to do is replace the default libc (which is really not intended for embedding) with one designed for embedding (eg ucLibc or its descendants). Otherwise anything but the most trivial applications will be using dynamic memory anyway.

Since the development environment as it is at the moment relies on dynamic memory, I feel that some effort should be made to ensure that it is as safe as it can be.

[stevstrong]

I am totally an amateur regarding the dynamic memory allocation. But...
If I would be the repo owner, I would like to see a practical example where this change would bring the presented advantage.

[justinschoeman]

If you want examples, the only reason I looked at this was due to two posts on the forum, both of which generated unexpected crashes (instead of NULL returns):
http://www.stm32duino.com/viewtopic.php?f=3&t=4014
http://www.stm32duino.com/viewtopic.php?f=3&t=4009

When I looked at these cases I noticed what could only be called a bug in the _sbrk() implementation, and proposed a minimal fix.

[justinschoeman]

I have been trying various alternatives, so that there can be some scratch space for libc and libstdc++ (which all use dynamic memory for various functions).

As a variant on Rick's suggestion, I modified the current _sbrk() to '__weak' and provided my own one in my application, which allocates from a static array.

This works perfectly, but I still saw throb() for various test cases. A little digging shows that C++ relies on exceptions for allocation failures. So there is no way dynamic memory can ever be really safe in this environment.

The best we can hope for is to make it safer... With that in mind, how do you feel about making _sbrk() weak so that application writers can provide their own implementation when required?

[stevstrong]

Although the changes are moderate, the effect is unpredictable.
Additionally, the forum post with examples to demonstrate the fix are not available anymore.
So, after a long time of inactivity, is there something we can conclude on this?

[victorpv]

This is old, but to provide some conclusion I think declaring _sbrk() weak should not affect anything at all, so why not?
Either that, or include this changes but setting __sbrk_stack_reserve to 0 in syscalls.c, that way nothing changes in the default behavior (but consumes 4 extra bytes of ram for __sbrk_stack_reserve ), but a user can still change the value in his code to provide some protection.

[dewhisna]

Actually, you can already limit dynamic memory allocation by defining the CONFIG_HEAP_END on the command-line to the compiler. For example, on my application that uses a STM32F103VG micro, I am passing this to the compiler:

-DCONFIG_HEAP_END='((void *)0x20016000)'

The code in syscalls.c has this:

/* If CONFIG_HEAP_START (or CONFIG_HEAP_END) isn't defined, then
 * assume _lm_heap_start (resp. _lm_heap_end) is appropriately set by
 * the linker */
#ifndef CONFIG_HEAP_START
extern char _lm_heap_start;
#define CONFIG_HEAP_START               ((void *)&_lm_heap_start)
#endif
#ifndef CONFIG_HEAP_END
extern char _lm_heap_end;
#define CONFIG_HEAP_END                 ((void *)&_lm_heap_end)
#endif

That currently sets the upper heap limit to whatever the linker defines as _lm_heap_end, which would normally be the end of RAM. But it properly checks to see if CONFIG_HEAP_END isn't already defined. Therefore, you can easily redefine it when compiling.

The STM32F103VG has 96K of RAM so _lm_heap_end is normally 0x20018000 on my target. But that would allow it to completely collide with the stack and crash if it ever allocated that much memory. So as a safeguard, I'm setting it to 0x20016000 with the compiler flag mentioned above. That gives me an entire 8K (or 0x2000) of stack space. If the heap tries to encroach on that space, then the malloc calls will start returning null pointers, allowing my code to gracefully detect when it's running out of dynamic space.

I'm not opposed to making _sbrk() weak. In fact, I think it's a good idea to define it as weak. But I just wanted to point out that it's just not actually necessary in order to limit the dynamic memory allocation size.

And yes, before someone says something about the STM32F103VG not being a supported target. I know the "official core" doesn't support it, but I've added support locally on my fork. However, the same define also works for the other variants/targets, since syscalls.c correctly checks to make sure it's not already defined. You would, of course, have to use other values for your memory limit on the other targets based on their RAM memory size.