Nested panics and `std::thread::panicking`

[purplesyringa]

struct Dropper;
impl Drop for Dropper {
    fn drop(&mut self) {
        catch_unwind(|| panic!()); // two panics alive at the same time
    }
}
let _dropper = Dropper;
panic!();

Should this abort? We often say that "double panics lead to abort", and I think we might even document that, but I've just found out this behavior is surprising to a lot of folks. So I want to ask for T-lang consensus: are double panics explicitly allowed if caught, and only unwinding across a landing pad leads to abort?

If double panics are allowed in this case, then everyone relying on std::thread::panicking is subtly broken. We encounter the same problem as std::uncaught_exception in C++. N4152 had to deprecate std::uncaught_exception and expose a counter instead. I think N4152 explains the problem well enough, but the TL;DR for Rust is that it's impossible to reliably implement something like ScopeGuard, because we'll either notice a panic when there's none (because std::uncaught_exception is set at the beginning anyway), or ignore a panic when it does happen (which is a problem for e.g. MutexGuard currently, see #143471). I have found some unsoundness in the wild (1, 2, 3) due to this.

I don't have a specific proposal to fix this, e.g. I don't think exposing std::thread::panic_count is a good idea, but I at least want this problem to be known.

@rustbot label +T-lang +T-libs-api +T-libs -C-discussion +A-panic

[Noratrieb]

This was changed in #110975. cc @Amanieu and @joshtriplett, you probably have opinions on this.

[bjorn3]

Inside of the catch_unwind you could argue that you are no longer inside a panicking context as panicking won't abort the process.

I have found some unsoundness in the wild (1, 2, 3) due to this.

How are these unsound? It seems like those are intended to be invoked iff a panic unwinds out of the scope in which these panic guards are created, which should be exactly what happens. If a user uses catch_unwind outside of the function call, that won't affect anything. And if the user uses catch_unwind inside of a function called by those functions, then the panic never escapes the user function and thus doesn't unwind out of the scope that contains the panic guard.

[purplesyringa]

How are these unsound?

defer_on_unwind declares a local with a destructor that runs user code if panicking() == true. So if panicking is already set on entry to the function, then defer_on_unwind will run, but on-success code will run as well. In all 3 examples, this will cause a double free.

This can be easily argued to be a scopeguard bug, but implementing it correctly requires to compare the panic count at creation and drop time, which we don't currently expose.

[bjorn3]

defer_on_unwind declares a local with a destructor that runs user code if panicking() == true. So if panicking is already set on entry to the function, then defer_on_unwind will run, but on-success code will run as well. In all 3 examples, this will cause a double free.

Right. That UB isn't caused by nested panics though.

This can be easily argued to be a scopeguard bug, but implementing it correctly requires to compare the panic count at creation and drop time, which we don't currently expose.

I see, makes sense. A quick workaround would be to never run the guard code when thread::panicking() is true already when constructing the panic. You have to handle the case where a panic isn't detected anyway due to foreign exceptions not updating the panic count.

[purplesyringa]

A quick workaround would be to never run the guard code when thread::panicking() is true already when constructing the panic.

That's what std's MutexGuard currently does, and that leads to the problem described in #143471. It's fine as a quick workaround, but I'm not happy about it long-term.

It doesn't help that we document panicking as a good way to detect a panic in Drop::drop docs, but we never spell out that panicking() && !self.was_panicking_at_construction is the right way. If you search for std::thread::panicking on GitHub, you'll see that absolutely everyone uses panicking directly and not the MutexGuard-like incantation.

I'm concerned that no changes to the behavior of panicking itself can fix this... this could be argued to be a user's problem, but this API is so easy to misuse that it feels like we need to at least warn the users somehow... setting panicking to false inside catch_unwind wouldn't help this in any way.

I would propose to deprecate panicking and replace it with a different API (maybe panic_count, maybe something different) if we could do that. Deprecation would automatically notify people that they are likely doing something wrong and that they need to revalidate their approach. But I'm not sure if that's realistic.

[purplesyringa]

Assuming we wanted to deprecate panicking in favor of another API (I'll leave arguing for or against that to others, I don't see any good way forward but I at least want to make them feel viable), my proposal would be to avoid exposing panic_count and instead introduce a slightly higher-level, harder to misuse API like this:

mod thread {
    /// A primitive "on-panic" guard.
    ///
    /// When this type is used as a `struct` field, it lets the `Drop` implementation detect whether
    /// unhandled panics have occured between the creation and destruction of the object. This can
    /// be used to run error handlers only on panic. For example, `MutexGuard` uses this API to
    /// implement mutex poisoning.
    struct PanicTracker { /* private fields */ }

    impl PanicTracker {
        /// Start tracking panics occuring while this object exists.
        fn new() -> Self;

        /// Determine whether an unhandled panic has occurred since the creation of the tracker.
        fn panicking(&self) -> bool;
    }

    impl !Send for PanicTracker {}
}

PanicTracker could internally store the saved panic_count, and panicking could just compare it to the current one. But the bonus would be that people wouldn't be able to write panic_count() > 0, which is as incorrect as panicking(), and that you wouldn't be able to accidentally send it to a different thread, where panic_count could have a different value. Migrating from panicking() to PanicTracker::panicking() should be intuitive enough, and more readable than comparing panic_count by hand. (Though now that I'm aware of uncaught_exceptions, maybe panic_count can be argued to be just as intuitive due to prior art... I'm not sure.)

Another reason not to expose panic_count directly is that we might want to tune the PanicTracker implementation. For example, with the AsyncDrop proposal, panics in futures would need to be handled in a special way, and I'm not sure if we could make that work just with panic_count. Keeping panic count tracking as an implementation detail would give us some wiggle room here.

[purplesyringa]

Another thing to keep in mind is how we'd want to handle foreign exceptions. Now that C++ exceptions are allowed to unwind through Rust code, how should panic_count/PanicTracker react to that? Assuming that scopeguard is fixed to rely on panic_count, the following snippet always outputs exactly one of two strings.

fn check(f: impl FnOnce()) {
    defer_on_unwind! {
        println!("panicked");
    };
    f();
    println!("returned");
}

But if f threw a C++ exception, this would output nothing. That's a lot better than printing both, but it can still lead to a memory leak, and if the defer_on_unwind! block was meant to, say, join a background thread that refers to locals, this could still be problematic. (I think we guarantee that destructors of locals are run on scope exit, so this is a new problem specific to panicking.) Should we integrate uncaught_exceptions into panic_count or not?

[purplesyringa]

There's also the async story. The check function in the previous post would at least be guaranteed to print at most one line; and in absence of foreign exceptions, it'd be guaranteed to print exactly one line. Now consider this:

async fn check(f: impl AsyncFnOnce()) {
    defer_on_unwind! {
        println!("panicked");
    };
    f().await;
    println!("returned");
}

It sure seems like it should behave equivalently, but that's not quite true. A future can be polled multiple times from different contexts -- say, from a panic hook or inside unwinding Drop::drop, but also normally. That would be stupid, sure, but it's safe, and we've never spelled out that something might go wrong if you do this. Under these conditions, 0 or 2 strings could be printed. So scopeguard would still be unreliable in async functions.

We could fix this by treating the panic_count TLS entry sort of like part of a coroutine's context, kind of like registers are handled when switching contexts between threads. So coroutines would store the value of panic_count as recorded just before the previous suspension, and resuming the coroutine would store this value into the TLS. (And then suspending will save the new TLS value into the coroutine and restore the pre-poll value into the TLS, etc.) This would make PanicTracker reliable, but at the cost of adding TLS access at each suspension. Maybe a single TLS access is not that bad, async runtimes do a lot of things on suspend anyway... but euuuuuugh.

Keep in mind that ScopeGuard is a completely synchronous type, so we wouldn't even be able to lint against use of PanicTracker in such types. We could warn if types containing PanicTracker are stored across a suspension point, much like we do with MutexGuard currently, but that feels so hacky, and GitHub search demonstrates that people currently often use types with panicking in async fn. And then there's also AsyncDrop...

For this and other reasons, panicking/panic_count/PanicTracker seem like the wrong tool for the job, and if I could roll back time, I wouldn't provide panicking in stdlib at all. There's always going to be nuances if we keep such a mechanism. But we've made our bed, so now we have to make some kind of decision...