tests: Fix thread return with local message queue #17502
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
chrysn
requested review from
aabadie,
fjmolinas,
leandrolanzieri,
MichelRottleuthner and
smlng
as code owners
chrysn
added
Impact: minor
github-actions
bot
added
Area: core
chrysn
added
Process: needs >1 ACK
maribu
reviewed
Jan 11, 2022
|
Yes they can be static, and probably should (not that it'd matter a lot in the test applications); going with your newlines as well. |
maribu
approved these changes
Jan 11, 2022
When a message queue is configured from the stack, that main function must never return -- otherwise, during sched_task_exit (which the thread's function "returns" to), message senders might still send messages into already freed stack space (which would be reused by sched_task_exit). Co-authored-by: Marian Buschsieweke <maribu@users.noreply.github.com>
chrysn
force-pushed
the
queue-on-stack-never-quit
branch
from
12c8d63
to
b4a1851
Compare
chrysn
added
CI: ready for build
maribu
removed
the
Process: needs >1 ACK
|
No need for a second ACK IMO. If you insist on a 2nd ACK, speak up now. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Contribution description
When a message queue is configured from the stack, that main function
must never return -- otherwise, during sched_task_exit (which the
thread's function "returns" to), message senders might still send
messages into already freed stack space (which would be reused by
sched_task_exit).
[new] A follow-up commit adds a line on this to the msg_init_queue documentation.
Most occurrences of queue initialization throughout RIOT are either static or followed by an endless loop without breaks / returns; the few examples not matching either pattern are fixed here.
Testing procedure
I don't see any way to make the fixed behavior actually happen; this is more about best practice. (Especially given that sched_task_exit currently starts with a debug statement followed by
irq_disable, so the message would need to hit right into the execution of debug printing).The two altered tesets that don't depend on hardware can be and were successfully run (in tests/thread_msg_block_w_queue and tests/posix_semaphore) on native.
Issues/PRs references
This came up when nailing down the safety criteria of
msg_init_queuein Rust.[edit: see [new]]