Threadpool implementation is unsound

[Sp00ph]

The following test case fails with a SIGSEGV, due to a use after drop on a local variable:

#[test]
fn use_after_drop() {
    let mut global = String::new();
    let thread = make_worker_thread();
    {
        let local = String::from("test");
        std::thread::scope(|s| {
            drop(s.spawn_into(
                || {
                    std::thread::sleep(std::time::Duration::from_secs(1));
                    global = local.clone();
                },
                &thread,
            ));
        });
    }
    thread.join();

    assert_eq!(global, "test");
}

[Sp00ph]

Sorry, needs to be std::mem::forget instead of drop in the test case. With drop it panics as intended.

[Sp00ph]

Here's the miri output for cargo miri test -- use_after_drop, showing it's UB:

   Compiling viridithas v19.0.0 (/home/anon/git/viridithas)
    Finished `test` profile [unoptimized + debuginfo] target(s) in 0.12s
     Running unittests src/main.rs (target/miri/x86_64-unknown-linux-gnu/debug/deps/viridithas-daa637ed2e9cd22a)
error: Undefined Behavior: not granting access to tag <232205> because that would remove [SharedReadOnly for <235860>] which is strongly protected
   --> /home/anon/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ptr/mod.rs:804:1
    |
804 | pub unsafe fn drop_in_place<T: PointeeSized>(to_drop: *mut T) {
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Undefined Behavior occurred here
    |
    = help: this indicates a potential bug in the program: it performed an invalid operation, but the Stacked Borrows rules it violated are still experimental
    = help: see https://github.com/rust-lang/unsafe-code-guidelines/blob/master/wip/stacked-borrows.md for further information
help: <232205> was created here, as the root tag for alloc171721
   --> src/threadpool.rs:165:25
    |
165 |             let local = String::from("test");
    |                         ^^^^^^^^^^^^^^^^^^^^
help: <235860> is this argument
   --> src/threadpool.rs:168:21
    |
168 | /                     || {
169 | |                         std::thread::sleep(std::time::Duration::from_secs(1));
170 | |                         global = local.clone();
171 | |                     },
    | |_____________________^
    = note: BACKTRACE (of the first span) on thread `threadpool::tes`:
    = note: inside `std::ptr::drop_in_place::<std::string::String> - shim(Some(std::string::String))` at /home/anon/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ptr/mod.rs:804:1: 804:62
note: inside `threadpool::tests::use_after_drop`
   --> src/threadpool.rs:175:9
    |
175 |         }
    |         ^
note: inside closure
   --> src/threadpool.rs:161:24
    |
160 |     #[test]
    |     ------- in this attribute macro expansion
161 |     fn use_after_drop() {
    |                        ^

note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace

error: aborting due to 1 previous error

error: test failed, to rerun pass `--bin viridithas`

Caused by:
  process didn't exit successfully: `/home/anon/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/bin/cargo-miri runner /home/anon/git/viridithas/target/miri/x86_64-unknown-linux-gnu/debug/deps/viridithas-daa637ed2e9cd22a use_after_drop` (exit status: 1)
note: test exited abnormally; to see the full output pass --no-capture to the harness.

[Sp00ph]

And additionally to Use-After-Free, it's trivial to create data races in this (e.g. by just accessing global in the snipped above without calling thread.join()).

[cosmobobak]

So, the equivalent stdlib code deadlocks:

#[test]
fn stdlib() {
    let mut global = String::new();
    {
        let local = String::from("test");
        std::thread::scope(|s| {
            std::mem::forget(s.spawn(|| {
                std::thread::sleep(std::time::Duration::from_secs(1));
                global = local.clone();
            }));
        });
    }

    assert_eq!(global, "test");
}

miri output: https://gist.github.com/cosmobobak/b9be311337b4379e0a32603d4efc6438

I'm embarrassed that this fails in this way, but I didn't expect the implementation to be particularly good (I'd never put this up as a crate)

I do wonder what the relevant difference is.

[cosmobobak]

ah. seems like std::thread::scope waits for its threads to be finished, which obviously my impl doesn't hook into. probably needs a total rewrite, sad face

[Sp00ph]

Yes, the reason that std::thread::scope is able to access locals is because it guarantees that all spawned threads join before scope() returns. The way it guarantees this is by keeping a list of spawned threads internally, in the &Scope argument passed to the closure. Viri's spawn_into instead returns an owned ReceiverHandle, with the soundness of the API relying on the handle's destructor being called. But because you can always leak objects in safe Rust, the destructor may never be called, leading to this unsoundness.

Side note: Pre-1.0 Rust had thread::scoped() which had this exact issue: https://faultlore.com/blah/everyone-poops/

To actually get a sound API, you'd probably have to copy the approach of thread::scope: Only spawn worker tasks through a shared reference (akin to &Scope), of which you keep ownership. This way, you can guarantee that the scope object gets destroyed, at which point you can join all the worker threads.

[Sp00ph]

More specifically, the reason the code deadlocks with the stdlib version can be found right here: https://github.com/rust-lang/rust/blob/140044cffad5ff8bad8d4a5d2d936bd1ed578db2/library/std/src/thread/scoped.rs#L154-L157

It parks the main thread until all spawned threads have stopped running. It doesn't keep an explicit list of running threads, but instead just the number of spawned threads that haven't yet terminated. Each of those threads carries a Packet which holds a pointer back to the Scope data: https://github.com/rust-lang/rust/blob/140044cffad5ff8bad8d4a5d2d936bd1ed578db2/library/std/src/thread/mod.rs#L1755-L1759 . When dropping the ScopedJoinHandle returned by Scope::spawn, the inner thread is joined, and the running thread count of the packet is decremented: https://github.com/rust-lang/rust/blob/140044cffad5ff8bad8d4a5d2d936bd1ed578db2/library/std/src/thread/mod.rs#L1789-L1795

When mem::forgetting the ScopedJoinHandle, then because the thread is never joined, it also never drops its packet, and therefore doesn't decrement the scope's running thread count. The running thread count then never reaches 0, which leads to the deadlock of the main thread infinitely waiting for forgotten thread to join.

[cosmobobak]

Yeah, I was aware of leakgate or whatever it was called, I just didn't have a sufficiently crisp mental model of exactly which properties of std::thread::scope keep it safe. Obviously this impl works fine for viri atm, but I agree that it's nasty as hell, and I want rid of it. When I find the time, I suppose.