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

A thread_local! with a const initializer and a panicking drop impl unwinds through an extern "C" frame #112285

Closed
Tracked by #110897
saethlin opened this issue Jun 4, 2023 · 3 comments
Assignees
Labels
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-high High priority T-libs Relevant to the library team, which will review and decide on the PR/issue.

Comments

@saethlin
Copy link
Member

saethlin commented Jun 4, 2023

Miri detects UB when running the following program:

pub struct NoisyDrop {}

impl Drop for NoisyDrop {
    fn drop(&mut self) {
        panic!("ow");
    }
}

thread_local! {
    pub static NOISY: NoisyDrop = const { NoisyDrop {} };
}

fn main() {
    NOISY.with(|_| ());
}

With --target=x86_64-unknown-linux-gnu and --target=aarch64-apple-darwin

thread '<unnamed>' panicked at 'ow', src/main.rs:5:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
error: Undefined Behavior: unwinding past a stack frame that does not allow unwinding
  --> /home/ben/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/sys_common/thread_local_dtor.rs:43:17
   |
43 |                 dtor(ptr);
   |                 ^^^^^^^^^ unwinding past a stack frame that does not allow unwinding
   |
   = 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: BACKTRACE:
   = note: inside `std::sys_common::thread_local_dtor::register_dtor_fallback::run_dtors` at /home/ben/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/sys_common/thread_local_dtor.rs:43:17: 43:26

With --target=x86_64-pc-windows-msvc

thread 'main' panicked at 'ow', src/main.rs:5:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
error: Undefined Behavior: unwinding past a stack frame that does not allow unwinding
  --> /home/ben/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/sys/windows/thread_local_dtor.rs:28:9
   |
28 |         (dtor)(ptr);
   |         ^^^^^^^^^^^ unwinding past a stack frame that does not allow unwinding
   |
   = 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: BACKTRACE:
   = note: inside `std::sys::windows::thread_local_dtor::run_keyless_dtors` at /home/ben/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/sys/windows/thread_local_dtor.rs:28:9: 28:20
   = note: inside `std::sys::windows::thread_local_key::on_tls_callback` at /home/ben/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/sys/windows/thread_local_key.rs:246:9: 246:54
@saethlin saethlin added I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness C-bug Category: This is a bug. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jun 4, 2023
@rustbot rustbot added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Jun 4, 2023
@thomcc thomcc self-assigned this Jun 4, 2023
@thomcc
Copy link
Member

thomcc commented Jun 4, 2023

I see the bug. We have #![feature(c_unwind)] (which means extern "C" fns in std automatically trap aborts) but that doesn't apply to extern "C" functions which are present in the macro expansion (some of which can't really be moved outside the macro).

Ill fix in like an hour or two. We just need to guard panics in a couple places.

In practice I think this should not call problems and we'd just hit an abort guard in an extern "C" defined in libstd a few frames up, although perhaps that's not true in 100% of cases (I can update on this once I implement the fix).

@workingjubilee workingjubilee added P-high High priority and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Jun 5, 2023
@workingjubilee
Copy link
Member

The apparent consensus is that the fix to this may need to be backported thus I am tagging this with P-high.

bors added a commit to rust-lang-ci/rust that referenced this issue Jun 8, 2023
Avoid unwind across `extern "C"` in `thread_local::fast_local`

This is a minimal fix for rust-lang#112285, in case we want a simple patch that can be easily to backported if that's desirable.

*(Note: I have another broader cleanup which I've mostly omitted from here to avoid clutter, except for the `Cell` change, which isn't needed to fix UB, but simplifies safety comments).*

The only tier-1 target that this occurs on in a way that seems likely to cause problems in practice linux-gnu, although I believe some folks care about that platform somewhat 😉. I'm unsure how big of an issue this is. I've seen stuff like this behave quite badly, but there's a number of reasons to think this might actually be "fine in practice".

I've hedged my bets and assumed we'll backport this at least to beta but my feeling is that there's not enough evidence this is a problem worth backporting further than that.

### More details

This issue seems to have existed since `thread_local!`'s `const` init functionality was added. It occurs if you have a `const`-initialized thread local for a type that `needs_drop`, the drop panics, and you're on a target with support for static thread locals. In this case, we will end up defining an `extern "C"` function in the user crate rather than in libstd, and because the user crate will not have `#![feature(c_unwind)]` enabled, their panic will not be caught by an auto-inserted abort guard.

In practice, the actual situation where problems are likely[^ub] is somewhat narrower.

On most targets with static thread locals, we manage the TLS dtor list by hand (for reentrancy reasons among others). In these cases, while the users code may panic, we're calling it inside our own `extern "C"` (or `extern "system"`) function, which seems to (at least in practice) catch the panic and convert it to an abort.

However, on a few targets, most notably linux-gnu with recent glibc (but also fuchsia and redox), a tls dtor registration mechanism exists which we can actually use directly, [`__cxa_thread_atexit_impl`](https://github.com/rust-lang/rust/blob/master/library/std/src/sys/unix/thread_local_dtor.rs#L26-L36).

This is the case that seems most likely to be a cause for concern, as now we're passing a function to the system library and panicking out of it in a case where there are may not be Rust frames above it on the call stack (since it's running thread shutdown), and even if there were, it may not be prepared to handle such unwinding. If that's the case, it'd be bad.

Is it? Dunno. The fact that it's a `__cxa_*` function makes me think they probably have considered that the callback could throw but I have no evidence here and it doesn't seem to be written down anywhere, so it's just a guess. (I would not be surprised if someone comes into this thread to tell me how definitely-bad-news it is).

That said, as I said, all this is actually UB! If this isn't a "technically UB but fine in practice", but all bets are off if this is the kind of thing we are telling LLVM about.

[^ub]: This is UB so take that with a grain of salt -- I'm absolutely making assumptions about how the UB will behave "in practice" here, which is almost certainly a mistake.
@Noratrieb
Copy link
Member

the fix was merged and a backport was rejected: #112292 (comment)

bors added a commit to rust-lang/miri that referenced this issue Jun 16, 2023
add tests for panicky drop in thread_local destructor

Adds a test for rust-lang/rust#112285
bors added a commit to rust-lang/miri that referenced this issue Jun 16, 2023
add tests for panicky drop in thread_local destructor

Adds a test for rust-lang/rust#112285
RalfJung pushed a commit to RalfJung/rust that referenced this issue Jun 29, 2023
add tests for panicky drop in thread_local destructor

Adds a test for rust-lang#112285
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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-high High priority T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

5 participants