Uh oh!
There was an error while loading. Please reload this page.
Prevent UB in child process after calling libc::fork - #102460
Merged
bors merged 6 commits intoOct 12, 2022
Merged
Conversation
rust-highfive
commented
Sep 29, 2022
Contributor
r? @thomcc (rust-highfive has picked a reviewer for you, use r? to override) |
thomcc
requested changes
Sep 29, 2022
thomcc
left a comment
Member
There was a problem hiding this comment.
I'm mostly in favor here but there are a couple changes that I'd like.
Member
There was a problem hiding this comment.
What's the reason for adding this conditional?
ContributorAuthor
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.
ContributorAuthor
There was a problem hiding this comment.
Extra message is removed as it is not very helpful and only in very rare cases.
Member
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).
Uh oh!
There was an error while loading. Please reload this page.
flba-ebforce-pushed
the
fix_85261_prevent_alloc_after_fork
branch
from
September 29, 2022 09:31
d73dd95 to
f5c0a2cCompareflba-eb
marked this pull request as ready for review
September 29, 2022 10:04
rustbot
commented
Sep 29, 2022
Collaborator
Hey! It looks like you've submitted a new PR for the library teams! If this PR contains changes to any Examples of
|
flba-ebforce-pushed
the
fix_85261_prevent_alloc_after_fork
branch
from
September 29, 2022 13:32
f5c0a2c to
fa07702Comparethomcc
commented
Sep 29, 2022
Member
Looks good to me. @bors r+ |
bors
commented
Sep 29, 2022
Collaborator
📌 Commit fa07702cb25d25c1dc9dde633d7bbe6bfd6a0545 has been approved by It is now in the queue for this repository. |
thomcc
approved these changes
Sep 29, 2022
thomcc
commented
Sep 29, 2022
Member
flba-ebforce-pushed
the
fix_85261_prevent_alloc_after_fork
branch
from
September 29, 2022 16:05
fa07702 to
aa5cadfComparethomcc
commented
Sep 29, 2022
Member
@bors r+ rollup |
bors
commented
Sep 29, 2022
Collaborator
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. Fixesrust-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.
matthiaskrgr
commented
Sep 29, 2022
Member
@bors r- rollup=iffy failed in a rollup |
bors
commented
Oct 4, 2022
Collaborator
☔ The latest upstream changes (presumably #102644) made this pull request unmergeable. Please resolve the merge conflicts. |
flba-ebforce-pushed
the
fix_85261_prevent_alloc_after_fork
branch
from
October 6, 2022 06:47
aa5cadf to
d56ebf3Comparerust-log-analyzer
commented
Oct 10, 2022
Collaborator
The job Click to see the possible cause of the failure (guessed by this bot) |
thomcc
commented
Oct 11, 2022
Member
Seems to be a spurious failure. @bors r+ |
bors
commented
Oct 11, 2022
Collaborator
💡 This pull request was already approved, no need to approve it again.
|
bors
commented
Oct 11, 2022
Collaborator
bors
commented
Oct 11, 2022
Collaborator
⌛ Testing commit 4c5d6bb with merge 84ac4ffdae5e3c4031380966ab284361cdedcc51... |
bors
commented
Oct 12, 2022
Collaborator
💥 Test timed out |
rust-log-analyzer
commented
Oct 12, 2022
Collaborator
flba-eb
commented
Oct 12, 2022
ContributorAuthor
@bors retry |
bors
commented
Oct 12, 2022
Collaborator
@flba-eb: 🔑 Insufficient privileges: not in try users |
thomcc
commented
Oct 12, 2022
Member
@bors r+ |
bors
commented
Oct 12, 2022
Collaborator
💡 This pull request was already approved, no need to approve it again.
|
bors
commented
Oct 12, 2022
Collaborator
bors
commented
Oct 12, 2022
Collaborator
bors
commented
Oct 12, 2022
Collaborator
☀️ Test successful - checks-actions |
rust-timer
commented
Oct 12, 2022
Collaborator
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 |
Mark-Simulacrum
commented
Oct 18, 2022
Member
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. Fixesrust-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.
This file contains hidden or 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
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
9 participants
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.