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

std::thread::JoinHandle::join panics with seccomp-killed threads #112521

Open
inikulin opened this issue Jun 11, 2023 · 2 comments
Open

std::thread::JoinHandle::join panics with seccomp-killed threads #112521

inikulin opened this issue Jun 11, 2023 · 2 comments
Labels
A-thread Area: `std::thread` C-bug Category: This is a bug. O-linux Operating system: Linux T-libs Relevant to the library team, which will review and decide on the PR/issue.

Comments

@inikulin
Copy link

I tried this code:

use libseccomp_sys::{seccomp_init, seccomp_load, SCMP_ACT_KILL};

fn main() {
    let _ = std::thread::spawn(|| {
        init_seccomp_deny_all();

        let _ = std::fs::read("foobar");
    })
    .join();
}

fn init_seccomp_deny_all() {
    // Kill thread on all syscalls
    let ctx = unsafe { seccomp_init(SCMP_ACT_KILL) };

    if ctx.is_null() {
        panic!("failed ot init seccomp");
    }

    if unsafe { seccomp_load(ctx) } != 0 {
        panic!("failed to load seccomp");
    }
}

I expected to see this happen: join() call return an error, gracefully reporting the killed thread

Instead, this happened: join() panics internally

Meta

rustc --version --verbose:

rustc 1.70.0 (90c541806 2023-05-31)
binary: rustc
commit-hash: 90c541806f23a127002de5b4038be731ba1458ca
commit-date: 2023-05-31
host: aarch64-unknown-linux-gnu
release: 1.70.0
LLVM version: 16.0.2
Backtrace

thread 'main' panicked at 'called `Option::unwrap()` on a `None` value', /rustc/90c541806f23a127002de5b4038be731ba1458ca/library/std/src/thread/mod.rs:1426:40
stack backtrace:
   0: rust_begin_unwind
             at /rustc/90c541806f23a127002de5b4038be731ba1458ca/library/std/src/panicking.rs:578:5
   1: core::panicking::panic_fmt
             at /rustc/90c541806f23a127002de5b4038be731ba1458ca/library/core/src/panicking.rs:67:14
   2: core::panicking::panic
             at /rustc/90c541806f23a127002de5b4038be731ba1458ca/library/core/src/panicking.rs:117:5
   3: core::option::Option<T>::unwrap
             at /rustc/90c541806f23a127002de5b4038be731ba1458ca/library/core/src/option.rs:950:21
   4: std::thread::JoinInner<T>::join
             at /rustc/90c541806f23a127002de5b4038be731ba1458ca/library/std/src/thread/mod.rs:1426:40
   5: std::thread::JoinHandle<T>::join
             at /rustc/90c541806f23a127002de5b4038be731ba1458ca/library/std/src/thread/mod.rs:1558:9
   6: std_thread_bug::main
             at ./src/main.rs:4:13
   7: core::ops::function::FnOnce::call_once
             at /rustc/90c541806f23a127002de5b4038be731ba1458ca/library/core/src/ops/function.rs:250:5

@inikulin inikulin added the C-bug Category: This is a bug. label Jun 11, 2023
@inikulin inikulin changed the title std::thread::JoinHandle::join panics with seccomp-killed threads std::thread::JoinHandle::join panics with seccomp-killed threads Jun 11, 2023
@bjorn3
Copy link
Member

bjorn3 commented Jun 12, 2023

What happens here is that .join() will first use pthread_join to wait for the thread which returns a success and then it tries to read the return value that the thread should have written right before finishing:

self.native.join();
Arc::get_mut(&mut self.packet).unwrap().result.get_mut().take().unwrap()
When a thread is killed, it won't have a chance to write the return value, thus causing the unwrap to fail. Note that killing individual threads without letting them unwind their stack is UB. It doesn't matter if this is through pthread_exit/pthread_kill or seccomp killing the thread. See rust-lang/unsafe-code-guidelines#211 Maybe the .unwrap() should be replaced with an rtabort!() which aborts the process without panicking as the process is in an inconsistent state where continuing may be a security issue?

@inikulin
Copy link
Author

@bjorn3 thank you for the clarification, killed threads being UB is a useful revelation. As for solution - yeah, aborting sounds like a good measure here.

@ChrisDenton ChrisDenton added the needs-triage-legacy Old issue that were never triaged. Remove this label once the issue has been sufficiently triaged. label Jul 16, 2023
@ChrisDenton ChrisDenton added T-libs Relevant to the library team, which will review and decide on the PR/issue. A-thread Area: `std::thread` O-linux Operating system: Linux and removed needs-triage-legacy Old issue that were never triaged. Remove this label once the issue has been sufficiently triaged. labels Nov 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-thread Area: `std::thread` C-bug Category: This is a bug. O-linux Operating system: Linux 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

3 participants