Unclear safety of `std::thread::catch_panic`

[tbu-]

If I get it correctly, this function cannot be safe as is, due to everyone in a thread implicitely having a reference to TLS.

Created this issue because I didn't want discussion about this to be lost in IRC.

[lilyball]

If RefCell were changed to poison itself on panic, would that fix this issue? Immutable TLS isn't a problem, and offhand I believe that mutable TLS requires either RefCell or some inter-thread data primitive (such as Mutex, which already handles this case).

[steveklabnik]

/cc @alexcrichton

[Stebalien]

@kballard I'm not entirely convinced that would be safe but I can't prove it unsafe either. In the multithreaded case, data protected by a lock must not violate any invariants any time the lock isn't held because one generally can't make statements about the state of other threads. However, in single threaded environments one can reasonably make scope-based invariants. For example, my horrorshow template macro emulates scoped TLS as follows:

pub fn __with_template_reentrant<F: FnMut(&mut Template)>(mut f: F) {
    // The scoped variant is unstable so we do this ourselves...
    __TEMPLATE.with(|template| {
        let mut local_template = None;
        ::std::mem::swap(&mut *template.borrow_mut(), &mut local_template);
        (f)(local_template.as_mut().unwrap());
        ::std::mem::swap(&mut *template.borrow_mut(), &mut local_template);
    });
}```

If the last swap isn't run, the invariant is violated and my library will likely panic at some future point.


However, this example doesn't violate memory safety and I can't find a way to do so without using `unsafe`.

[lilyball]

@Stebalien You're effectively poisoning the TLS value while that code is being run. If the thread panics and avoids the last swap, a subsequent attempt to access it will panic. And that's perfectly fine (although you might want to consider the idea of explicit poisoning so you can let the user detect this case without panicking). Panicking is not memory unsafety.

In the case of RefCell, a panic will run the destructors on the extant borrows, which will return the RefCell to the unborrowed state, but the value it contains may be violating invariants (in a manner that can lead to memory unsafety). This is the same issue that Mutex had, which is why Mutex poisons itself if the thread panics while a lock is held. RefCell doesn't have poisoning because it can't be shared between multiple threads without some thread synchronization primitive (e.g. Mutex). But std::thread::catch_panic() plus TLS does give rise to the same situation.

[bluss]

Can't you use the binaryheap again? It was used to show unsoundness off thread scoped (the first time)

[bluss]

Ok, so I put myself to work since it was my suggestion.

Do you remember soundness issue the first time with std::thread::scoped (issue #20807) and why we made it stop catching panics? The following is (possibly?) a reason: It allows violating invariants in a type, and still go back to using that same value.

The following code example segfaults in safe rust. I'm using a mutex and ignoring poison (this is safe rust). I think we have other ways to share memory that would avoid mutex altogether, so I think we can probably ignore mutex in the discussion(?).

Note: This segfault works just as well with thread::spawn!

rustc version: rustc 1.2.0-nightly (0cc99f9 2015-05-17) (built 2015-05-18)

(playpen link)

#![feature(catch_panic, collections, std_misc)]
use std::cmp::Ordering;
use std::collections::BinaryHeap;
use std::collections::BTreeSet;
use std::thread;
use std::sync::{Arc, Mutex};

#[derive(PartialEq, Eq, Ord, Debug, Clone)]
struct Panicker<T>(T);

impl<T: PartialEq> PartialOrd for Panicker<T>
{
    fn partial_cmp(&self, _: &Self) -> Option<Ordering>
    {
        panic!()
    }
}

fn main() {
    let heap = Arc::new(Mutex::new(BinaryHeap::new()));
    let heap_ref = heap.clone();
    let guard = thread::catch_panic(move || {
        let mut local_heap = heap.lock().unwrap();
        local_heap.push(Panicker(BTreeSet::<i32>::new()));
        local_heap.push(Panicker(BTreeSet::<i32>::new()));
    });
    println!("Thread result: {:?}", guard);

    let mutex_guard = match heap_ref.lock() {
        Ok(inner) => inner,
        Err(e) => e.into_inner(),
    };

    let vector_from_binary_heap = mutex_guard.clone().into_vec();
    println!("{:?}", vector_from_binary_heap);
}

Q: Why use BTreeSet as the element type?
A: BinaryHeap will mark an element as dropped (zeroing or filling) before starting its sift down algorithm and comparing values. We catch the panic to get the binaryheap in this state. BTreeSet is used for a value that we know is really corrupted / memory unsafe when used in zeroed/filled state.

You have to ask the question: Is it catching panics or BinaryHeap that is broken? If we fix BinaryHeap, what other type invariants can we break for fun & unsoundness?

Edit: as tomaka says below, is it ignoring poisoning that is unsafe?

[bluss]

Time to call in the cavalry, cc @nikomatsakis @aturon

[bluss]

oh and cc @tomaka

[tomaka]

std::sync::PoisonError::into_inner() should definitely be unsafe.

[bluss]

I want to be sure we solve the fundamental issues and don't just patch the holes in an unsound bag. I don't know which of the two categories ignoring poisoning belongs to.

[tomaka]

The golden rule is that when a panic occurs, any object that is mutably borrowed must no longer be used in the future (except for its destructor) because its internal state is possibly messed up.

Rust cleans up local variables inside the closure passed to catch_panic and spawn, so the only problem comes when accessing variables from the outside:

• References/mutable references.
• Rc/Arc. (Gc in the future?)
• Anything static or static-like such as thread local storage.
• Raw pointers, but these are unsafe anyway so we don't care.

References are explicitely forbidden because the closure is required to be Send and 'static (by the way I don't see why Send is required for catch_panic, but I may be missing something).

Only Rc, Arc and statics are problematic, but they only allow immutable access to their content. Therefore the problem lies in types that allow interior mutability: Cell, RefCell, Mutex, Atomic.
Cell and Atomic can't panic during their mutations, and RefCell and Mutex use/should use poisoning.

[tomaka]

One thing that I neglected in the previous post are destructors. Maybe with a lot of hacky (but safe) code it could possible to trigger a crash by writing a destructor that is being run during a panic (that's just a supposition, I have absolutely no idea how). Destructors are some kind of gray area when it comes to exception-safety.

[tomaka]

This problem is really specific to TLS, because RefCells can't be put inside Arcs and that thread::spawn requires a closure that implements Send.

That's probably the reason why catch_panic requires Send as well, but it didn't account for TLS.

[bluss]

For completeness, the TLS + RefCell version of the same segfault.