Prevent UB in child process after calling libc::fork #102460
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
rustbot
added
the
T-libs
|
r? @thomcc (rust-highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
thomcc
requested changes
Sep 29, 2022
library/std/src/panicking.rs
Outdated
| @@ -644,7 +659,13 @@ fn rust_panic_with_hook( | |||
| if panics > 2 { | |||
| // Don't try to print the message in this case | |||
| // - perhaps that is causing the recursive panics. | |||
| rtprintpanic!("thread panicked while processing panic. aborting.\n"); | |||
| if must_abort { | |||
There was a problem hiding this comment.
What's the reason for adding this conditional?
There was a problem hiding this comment.
The only reason is to print out a different message so that the user is aware about the uncertainty. I thought it might be helpful but is not required.
There was a problem hiding this comment.
Extra message is removed as it is not very helpful and only in very rare cases.
There was a problem hiding this comment.
From zulip (talking about the case where this branch is hit)
Thom Chiovoloni: there's a slight race where
- T1 panics, calls increase
- T2 forks, panics in child before exec
- T1 (in parent) calls decrease
Thom Chiovoloni: T2 can't look at the local panic count and only global, so it can't if this is different from
- some thread panics
- while unwinding, the same thread forks and the child panics before exec
right?
I think this means that we ideally would handle this case the must_abort case in here the same as below (but without the panic info). In other words: rtprintpanic!("panicked after panic::always_abort(), aborting.\n");
Or is there some other case where this matters?
(All that said, I'm kind of fine leaving this with the existing message, since this is suuuuch an edge case. That said, giving a slightly better message can't hurt).
flba-eb
force-pushed
the
fix_85261_prevent_alloc_after_fork
branch
from
d73dd95
to
f5c0a2c
Compare
|
Hey! It looks like you've submitted a new PR for the library teams! If this PR contains changes to any Examples of
|
flba-eb
force-pushed
the
fix_85261_prevent_alloc_after_fork
branch
from
f5c0a2c
to
fa07702
Compare
|
Looks good to me. @bors r+ |
|
📌 Commit fa07702cb25d25c1dc9dde633d7bbe6bfd6a0545 has been approved by It is now in the queue for this repository. |
bors
added
S-waiting-on-bors
thomcc
approved these changes
Sep 29, 2022
bors
added
S-waiting-on-author
flba-eb
force-pushed
the
fix_85261_prevent_alloc_after_fork
branch
from
fa07702
to
aa5cadf
Compare
|
@bors r+ rollup |
bors
added
S-waiting-on-bors
matthiaskrgr
added a commit
to matthiaskrgr/rust
that referenced
this pull request
Sep 29, 2022
…ter_fork, r=thomcc Prevent UB in child process after calling libc::fork After calling libc::fork, the child process tried to access a TLS variable when processing a panic. This caused a memory allocation which is UB in the child. To prevent this from happening, the panic handler will not access the TLS variable in case `panic::always_abort` was called before. Fixes rust-lang#85261 (not only on Android systems, but also on Linux/QNX with TLS disabled, see issue for more details) Main drawbacks of this fix: * Panic messages can incorrectly omit `core::panic::PanicInfo` struct in case several panics (of multiple threads) occur at the same time. The handler cannot distinguish between multiple panics in different threads or recursive ones in the same thread, but the message will contain a hint about the uncertainty. * `panic_count::increase()` will be a bit slower as it has an additional `if`, but this should be irrelevant as it is only called in case of a panic.
|
@bors r- rollup=iffy failed in a rollup |
bors
added
S-waiting-on-author
|
☔ The latest upstream changes (presumably #102644) made this pull request unmergeable. Please resolve the merge conflicts. |
flba-eb
force-pushed
the
fix_85261_prevent_alloc_after_fork
branch
from
aa5cadf
to
d56ebf3
Compare
bors
added
the
S-waiting-on-review
|
The job Click to see the possible cause of the failure (guessed by this bot) |
Seems to be a spurious failure. @bors r+ |
|
💡 This pull request was already approved, no need to approve it again.
|
bors
added
S-waiting-on-bors
|
⌛ Testing commit 4c5d6bb with merge 84ac4ffdae5e3c4031380966ab284361cdedcc51... |
|
💥 Test timed out |
bors
added
S-waiting-on-review
|
@bors retry |
|
@flba-eb: 🔑 Insufficient privileges: not in try users |
|
@bors r+ |
|
💡 This pull request was already approved, no need to approve it again.
|
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions |
|
Finished benchmarking commit (50f6d33): comparison URL. Overall result: ✅ improvements - no action needed@rustbot label: -perf-regression Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)ResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Footnotes |
|
This wasn't actually a instruction count improvement, just noise from diesel. |
Aaron1011
pushed a commit
to Aaron1011/rust
that referenced
this pull request
Jan 6, 2023
…r_fork, r=thomcc Prevent UB in child process after calling libc::fork After calling libc::fork, the child process tried to access a TLS variable when processing a panic. This caused a memory allocation which is UB in the child. To prevent this from happening, the panic handler will not access the TLS variable in case `panic::always_abort` was called before. Fixes rust-lang#85261 (not only on Android systems, but also on Linux/QNX with TLS disabled, see issue for more details) Main drawbacks of this fix: * Panic messages can incorrectly omit `core::panic::PanicInfo` struct in case several panics (of multiple threads) occur at the same time. The handler cannot distinguish between multiple panics in different threads or recursive ones in the same thread, but the message will contain a hint about the uncertainty. * `panic_count::increase()` will be a bit slower as it has an additional `if`, but this should be irrelevant as it is only called in case of a panic.
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.
After calling libc::fork, the child process tried to access a TLS variable when processing a panic. This caused a memory allocation which is UB in the child.
To prevent this from happening, the panic handler will not access the TLS variable in case
panic::always_abortwas called before.Fixes #85261 (not only on Android systems, but also on Linux/QNX with TLS disabled, see issue for more details)
Main drawbacks of this fix:
core::panic::PanicInfostruct in case several panics (of multiple threads) occur at the same time. The handler cannot distinguish between multiple panics in different threads or recursive ones in the same thread, but the message will contain a hint about the uncertainty.panic_count::increase()will be a bit slower as it has an additionalif, but this should be irrelevant as it is only called in case of a panic.