-
-
Notifications
You must be signed in to change notification settings - Fork 714
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Prevent malloc from overwriting the stack #681
base: master
Are you sure you want to change the base?
Prevent malloc from overwriting the stack #681
Conversation
This is a linker option, but was passed when compiling c or cpp files, so it was effectively ignored by gcc (and passing it to the linker actually breaks the build, so it is really not needed here).
When allocating memory with malloc, this calls _sbrk() to expand the heap. The current implementation always succeeds, which makes malloc always succeed, potentially returning memory that overlaps the stack, or is even outside of the available RAM. This commit fixes this by replacing the _sbrk() function by one which fails rather than allocating memory too close to the stack (with configurable margin). = Background & history = The malloc implementation used by this core is provided by newlib, which is shipped along gcc in the toolchain package and is a libc implementation intended for embedded systems. It implements the entire user-facing API, but relies on a number of "syscalls", to be provided by the underlying system to do the real work. See https://sourceware.org/newlib/libc.html#Syscalls These syscalls mostly concern handling of processes, and files, which are not applicable here, so dummy implementations that simply returned failure were originally provided by syscalls.c in this core. In addition, the _sbrk() syscall *is* relevant, since it is used to expand the heap when malloc runs out of memory to (re)use. Originally, the samd core provided a very simple version that always succeeded, which was later changed to fail when trying to allocate beyond the end of memory (but would still happily overwrite the stack). In commit 93c6355 (Resolving syscalls/_sbrk issue), the syscalls were removed, and (among a lot of other flag changes) `--specs=nosys.specs` was passed to the linker. Unlike what you might think, this "nosys" does not instruct the linker to omit any system library, but it tells the linker that no syscalls will be provided and makes it include the libgloss "nosys" library. This "nosys" library also provides dummy syscall implementations that always fail, except for _sbrk(), which always succeeds (like the original samd implementation). So this left the samd core with a malloc that *always* succeeds without regard for the stack or the end of RAM. = Solution = This is fixed by adding a _sbrk() implmentation that does check the current stack location and fails allocation if that would overwrite the stack. This implementation of _sbrk() is based on the STM32 core, except with some different variable and type names and with the minimum stack size check omitted (since the linker scripts do not define such a minimum size): https://github.com/stm32duino/Arduino_Core_STM32/blob/616258269f7e5a2ef8b2f71bb2a55616b25d07db/libraries/SrcWrapper/src/syscalls.c#L29-L51 Because libgloss nosys defines its dummy _sbrk() as a weak symbol, it can be overridden by a non-weak symbol in the samd core. However, because the core is linked through an archive file, it will only be included in the link if there is an unresolved reference to it (or anything else in the same file, which is nothing) at the time the core is linked. Normally, this undefined reference to _sbrk() is only introduced when linking newlib at the end of the link, which is then satisfied by libgloss (which is linked after newlib) rather than the samd core. To ensure that the samd core version is included instead, an artificial unresolved reference is created in main.c (which *is* always included because it defines main). This ensures that sbrk.c is pulled into the link instead of the libgloss version.
The _sbrk() implementation added previously refuses to overwrite the stack, but will still happily allocate all other memory, leaving none to expand the stack into. Since the stack can expand over the heap undetected, this changes sbrk() to leave a safety margin between the heap and the stack (at the time of the malloc/_sbrk call). The size of this margin can be configured (by the sketch) by changing the `__malloc_margin` global variable, it defaults to 64 bytes. This approach (both having a margin and making it configurable with the `__malloc_margin`) is copied from the avr-libc malloc implementation, see: https://onlinedocs.microchip.com/pr/GUID-317042D4-BCCE-4065-BB05-AC4312DBC2C4-en-US-2/index.html?GUID-27757112-8BE1-49C2-B023-CD94AD06C5E2 avr-libc uses a default margin of 32 bytes, but given the bigger integer and pointer size on ARM, this is doubled for this core.
✅ Build completed. ⬇️ Build URL: ℹ️ To test this build:
|
Memory usage change @ 4460dbc
Click for full report table
|
Below is the sketch I used to test this. It just allocates various types of memory using operator new, including some allocations that would overwrite the stack, or even extend past the end of RAM so must fail. There is a bit of disabled code at the end that checks non-no-throw do not return null on failure, but that's a different issue that I'll try to discuss separately later, but I included the test code just FYI.
With the PR applied, output is as it should be:
Without the patch (samd 1.8.13 downloaded through board manager), this produces incorrect results for the last testcases:
The hard fault is because, I think, the stack margin tests messed up the heap. If you disable those, the hard fault is delayed one testcase (but still occurs because the next allocation is a struct that is automatically zero'd, so the |
The current implementation of malloc (or really, the underlying
sbrk()
implementation) happily allocates any amount of memory, even when that overwrites the stack or even beyond the end of RAM. This is fixed by implementing a customsbrk()
function that does proper checking. The code is based on thesbrk()
from the STM32 Arduino core, but with an additional margin added (to make it fail when it comes close to the stack, instead of just when it would actually overwrite the stack).This margin approach is copied from avr-libc's malloc implementation. I considered also copying more of avr-libc's configurability (e.g.
__malloc_heap_start
and__malloc_heap_end
), but that ended up just adding complexity without a very clear usecase, so I left that out.See the d232b59 commit message for much more detail on this problem and the solution.
This PR also has a somewhat unrelated commit removing the
-nostdlib
compilation option, which was in the wrong place and thus effectively unused (and also unneeded).