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

TLS dtor panics abort the process #24479

Open
alexcrichton opened this issue Apr 15, 2015 · 3 comments
Open

TLS dtor panics abort the process #24479

alexcrichton opened this issue Apr 15, 2015 · 3 comments
Labels
A-destructors Area: destructors (Drop, ..) A-thread-locals Area: Thread local storage (TLS) C-bug Category: This is a bug. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@alexcrichton
Copy link
Member

Today a destructor in TLS which panics will abort the process:

struct Foo;

impl Drop for Foo {
    fn drop(&mut self) { panic!() }
}

thread_local!(static FOO: Foo = Foo);

pub fn main() {
    FOO.with(|_| {});
}

When compiled and run (note that it must be run on a recent system due to #19776):

thread '<main>' panicked at 'explicit panic', foo.rs:4
fatal runtime error: Could not unwind stack, error = 5
zsh: illegal hardware instruction  ./foo

The reason behind this is that the context in which TLS destructors are running is different than the execution of the main thread itself (e.g. there is no try/catch block).

Is this (a) desirable and (b) should we change it?

@steveklabnik
Copy link
Member

Triage: no change in behavior here.

@steveklabnik steveklabnik added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. and removed A-libs labels Mar 24, 2017
@Mark-Simulacrum Mark-Simulacrum added the C-bug Category: This is a bug. label Jul 22, 2017
@alexcrichton alexcrichton added the A-thread-locals Area: Thread local storage (TLS) label Aug 25, 2017
@eddyb
Copy link
Member

eddyb commented Aug 23, 2019

It would be nice if this was a clean and intentional abort, which would probably be achieved whenever we make extern "C" functions abort to avoid UB (but we can do it sooner manually).

The fatal runtime error: Could not unwind stack, error = 5 message could lean one to believe this is some sort of bug elsewhere (i.e. in the implementation of panics).

For example, in #63804 (comment) I noticed that the message doesn't have anything to do with the way proc macros and libstd('s panic runtime) interact, which is what I initially assumed.

@andersk
Copy link
Contributor

andersk commented Jan 24, 2021

An especially bad side effect is that, when this happens in a test, the test doesn’t always fail.

struct Foo;

impl Drop for Foo {
    fn drop(&mut self) {
        panic!()
    }
}

thread_local!(static FOO: Foo = Foo);

#[test]
pub fn test() {
    FOO.with(|_| {});
}
$ cargo test
    Finished test [unoptimized + debuginfo] target(s) in 0.00s
     Running target/debug/deps/panicking_test-85130fa46b54f758

running 1 test
thread 'test test ... test' panicked at 'explicit panic', src/main.rs:5:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
ok

test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s

fatal runtime error: failed to initiate panic, error 5

$ echo $?
0

The exit status of cargo test is either 0 or 101 nondeterministically, and seems to be more likely to be 0 in a large project, which means that the bug that caused this panic can easily remain uncaught.

Edit: This turns out to be an independent problem, due to libtest failing to actually wait for its test threads to exit; I opened a PR as #81367.

bors added a commit to rust-lang-ci/rust that referenced this issue Jan 26, 2021
libtest: Wait for test threads to exit after they report completion

Otherwise we can miss bugs where a test reports that it succeeded but then panics within a TLS destructor.

Example:

```rust
use std::thread::sleep;
use std::time::Duration;

struct Foo;

impl Drop for Foo {
    fn drop(&mut self) {
        sleep(Duration::from_secs(1));
        panic!()
    }
}

thread_local!(static FOO: Foo = Foo);

#[test]
pub fn test() {
    FOO.with(|_| {});
}
```

Before this fix, `cargo test` incorrectly reports success.

```console
$ cargo test
    Finished test [unoptimized + debuginfo] target(s) in 0.01s
     Running target/debug/deps/panicking_test-85130fa46b54f758

running 1 test
test test ... ok

test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s

$ echo $?
0
```

After this fix, the failure is visible. (The entire process is aborted due to rust-lang#24479.)

```console
$ cargo test
    Finished test [unoptimized + debuginfo] target(s) in 0.01s
     Running target/debug/deps/panicking_test-76180625bc2ee3c9

running 1 test
thread 'test' panicked at 'explicit panic', src/main.rs:9:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
fatal runtime error: failed to initiate panic, error 5
error: test failed, to rerun pass '--bin panicking-test'

Caused by:
  process didn't exit successfully: `/tmp/panicking-test/target/debug/deps/panicking_test-76180625bc2ee3c9 --nocapture` (signal: 6, SIGABRT: process abort signal)

$ echo $?
101
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-destructors Area: destructors (Drop, ..) A-thread-locals Area: Thread local storage (TLS) C-bug Category: This is a bug. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

6 participants