sys/newlib_syscalls_default: fix race condition in __sinit()#20392
sys/newlib_syscalls_default: fix race condition in __sinit()#20392benpicco merged 1 commit intoRIOT-OS:masterfrom
Conversation
maribu
added
CI: ready for build
|
@mchesser: Could you give this a try? I'm aware that it still is a pain in the ass that RIOT doesn't support (at least optional) reentrancy for platforms other than ESP. That is in issue that stills needs to be addressed. However, even people desperate for saving RAM don't like crashes, and this hopefully makes the concurrent use of stdio crash-free. |
This eagerly calls `__sinit()` instead of lazy initialization upon the first call to stdio (e.g. `puts()`, `printf()`). The issue is that without locking (as is currently the case for all RIOT platforms but ESP) two concurrent "first calls" may result in concurrent initialization of the same structure and data corruption. Fixes RIOT-OS#20067
maribu
force-pushed
the
sys/newlib_syscalls_default
branch
from
ea98597 to
375aed1
Compare
| /* Also, make an exception for riotboot, which does not use stdio | ||
| * at all. This would pull in stdio and increase .text size | ||
| * significantly there */ | ||
| if (!IS_USED(MODULE_RIOTBOOT)) { |
There was a problem hiding this comment.
I was under the impression that riotboot would already do
DISBALE_MODULE += core_thread
but apparently not. That would save us some special casing in other places too.
There was a problem hiding this comment.
Would you open a PR to just do that? I could wait and rebase on top of that.
There was a problem hiding this comment.
riotboot_dfu uses a thread (and thread flags), so might not be so easy.
But how about
| if (!IS_USED(MODULE_RIOTBOOT)) { | |
| if (!IS_USED(MODULE_STDIO_NULL)) { |
There was a problem hiding this comment.
Sadly that would not work: stdio_null just throws away the output after it has gone the whole way through newlib. So there is no way for newlib to know whether the output is actually printed or just thrown away.
It would be possible to implement stdio_null to wrap all calls to printf() and friends to a mockup implementation. (And if we assume that callers of printf() don't care about the return value, those could be lean.) But as of know, we still need to init that with stdio_null.
benpicco
left a comment
benpicco
left a comment
There was a problem hiding this comment.
I don't like hard-coding riotboot, but let's get this in since we don't have a better solution.
benpicco
added
CI: ready for build
maribu
added
CI: ready for build
|
OK, full CI run with tests passed. I guess this is OK to merge. Note: The CI run failed twice before due to:
|
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
|
Thx :) |
4 participants
Contribution description
This eagerly calls
__sinit()instead of lazy initialization upon the first call to stdio (e.g.puts(),printf()). The issue is that without locking (as is currently the case for all RIOT platforms but ESP) two concurrent "first calls" may result in concurrent initialization of the same structure and data corruption.Testing procedure
Have two threads concurrently use stdio and the boot message disabled. This may result in data corruptions and crashes.
See #20067 for details on a reproducible crashing app.
Issues/PRs references
Fixes #20067