Skip to content
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

lang_start in std/src/rt.rs is unsound in presence of panic payload that panics on drop #86030

Closed
nagisa opened this issue Jun 5, 2021 · 3 comments · Fixed by #86034
Closed
Assignees
Labels
A-runtime Area: std's runtime and "pre-main" init for handling backtraces, unwinds, stack overflows C-bug Category: This is a bug. I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness P-critical Critical priority T-libs Relevant to the library team, which will review and decide on the PR/issue.

Comments

@nagisa
Copy link
Member

nagisa commented Jun 5, 2021

See #86027 for an example of the problem.

fn main() {
    std::panic::panic_any(Bomb);
}
thread 'main' panicked at 'Box<Any>', src/main.rs:12:5
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
thread 'main' panicked at 'explicit panic', src/main.rs:7:9
fatal runtime error: failed to initiate panic, error 5
abort (core dumped)

Here we panic without any landing pads available upstack, because we are already the top-most(-ish) frame in the first place. This is UB.

@nagisa nagisa added I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness I-prioritize Issue: Indicates that prioritization has been requested for this issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jun 5, 2021
@nagisa nagisa self-assigned this Jun 5, 2021
@nagisa nagisa changed the title lang_start in std/src/rt.rs is unsound in presence of panic payload that panics lang_start in std/src/rt.rs is unsound in presence of panic payload that panics on drop Jun 5, 2021
@jonas-schievink jonas-schievink added A-runtime Area: std's runtime and "pre-main" init for handling backtraces, unwinds, stack overflows C-bug Category: This is a bug. labels Jun 5, 2021
@apiraino
Copy link
Contributor

apiraino commented Jun 9, 2021

Assigning priority as discussed in the Zulip thread of the Prioritization Working Group.

@rustbot label -I-prioritize +P-critical

@rustbot rustbot added P-critical Critical priority and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Jun 9, 2021
nagisa added a commit to nagisa/rust that referenced this issue Jun 15, 2021
Guard against panic payloads panicking within entrypoints, where it is
UB to do so.

Note that there are a number of implementation approaches to consider.
Some simpler, some more complicated. This particular solution is nice in
that it also guards against accidental implementation issues in
various pieces of runtime code, something we cannot prevent statically
right now.

Fixes rust-lang#86030
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Jun 16, 2021
Change entry point to 🛡️ against 💥 💥-payloads

Guard against panic payloads panicking within entrypoints, where it is
UB to do so.

Note that there are a number of tradeoffs to consider. For instance, I
considered guarding against accidental panics inside the `rt::init` and
`rt::cleanup` code as well, as it is not all that obvious these may not
panic, but doing so would mean that we initialize certain thread-local
slots unconditionally, which has its own problems.

Fixes rust-lang#86030
r? `@m-ou-se`
@RalfJung
Copy link
Member

@RalfJung
Copy link
Member

FWIW, Miri does detect this. :)

error: Undefined Behavior: unwinding past the topmost frame of the stack
  --> /playground/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/rt.rs:43:1
   |
43 | / fn lang_start<T: crate::process::Termination + 'static>(
44 | |     main: fn() -> T,
45 | |     argc: isize,
46 | |     argv: *const *const u8,
...  |
52 | |     )
53 | | }
   | |_^ unwinding past the topmost frame of the stack
   |
   = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
   = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information
           
   = note: inside `std::rt::lang_start::<()>` at /playground/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/rt.rs:43:1

error: aborting due to previous error

bors added a commit to rust-lang-ci/rust that referenced this issue Jun 19, 2021
Change entry point to 🛡️ against 💥 💥-payloads

Guard against panic payloads panicking within entrypoints, where it is
UB to do so.

Note that there are a number of tradeoffs to consider. For instance, I
considered guarding against accidental panics inside the `rt::init` and
`rt::cleanup` code as well, as it is not all that obvious these may not
panic, but doing so would mean that we initialize certain thread-local
slots unconditionally, which has its own problems.

Fixes rust-lang#86030
r? `@m-ou-se`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-runtime Area: std's runtime and "pre-main" init for handling backtraces, unwinds, stack overflows C-bug Category: This is a bug. I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness P-critical Critical priority T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants