Nested panics and std::thread::panicking

@rustbot

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

@Amanieu

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

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.

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.

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.

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.

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.

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?

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...

We discussed this again the @rust-lang/libs-api meeting. We considered the panic_count function proposed here as an alternative to the drop changes that were proposed last time. If it can be made to work properly then we could deprecate std::thread::panicking in favor of it.

The more difficult part is making sure that panic_count takes foreign exceptions into account, which is needed for scopeguard to be sound. It's unclear if this is possible to achieve (Perhaps with a custom personality function? Would that work with SEH unwinding?).

The more difficult part is making sure that panic_count takes foreign exceptions into account, which is needed for scopeguard to be sound. It's unclear if this is possible to achieve (Perhaps with a custom personality function? Would that work with SEH unwinding?).

If all we want to count is Rust panics and foreign C++ exceptions, we can just call __uncaught_exceptions, which is exported by vcruntime. That won't count other SEH exceptions, but neither will C++, and they won't trigger Rust or C++ destructors anyway.

Took me a while to write this down, sorry. First of all, let me provide real-world examples of uses of panicking that I've shallowly listed above.

Recovery from panics: real-world sample.
Sanity checks in Drop: playground sample, real-world sample, another one. The semantics we want here are: a sanity check only needs to panic/abort the program/report an error/whatever if no panic has occurred since the creation of the object.
Best-effort handling of errors within Drop: playground sample, real-world sample, another sample. The semantics we want here are: we want to report an error noisily, but not too noisily to abort the program.
Poisoning: real-world sample, another one, another one, something borderline reasonable in hyper. The semantics we want here are: if a panic has occurred since the creation of the object, we want to mark the object as unusable in the future, otherwise we can assume it's in a pristine and reusable state.

There's another category that I hasn't listed earlier because it's not a good use of panicking, but I want to note it anyway because it looks popular:

Cancelling operations on error. This is necessary when a function wants to perform side effects, such as resource allocation, mutex locking, or directory creation, and retain those side effects on successful completion, but cancel them on error. This is usually better expressed with RAII (1, 2, 3, 4). Sometimes RAII cannot be used because the function's return type does not own those resources semantically (real-world example where a function deletes a directory if not run to completion, but returns ()). Nevertheless, using panicking here is almost guaranteed to be buggy: in the linked example, resources aren't cleaned up if the function returns Err via ?.

In my opinion, our changes to the panic infrastructure should overall keep categories 1-4 either implementable or unnecessary in the first place, and be harder to misuse for category 5 if possible.

My gripe with something like a defer mechanism is that it's control-flow based, rather than type-based, and so it's relevant to 1 and 5, but not 2, 3, or 4, which are by far most common. Moreover, it makes the anti-pattern in 5 easier to write, and I don't think that's a good idea. WithDrop wouldn't help here either.

Another problem is that the semantics we want in 3 are subtly different from all other uses, and that doesn't seem like a good thing. The existence of rust-lang/rfcs#3288 indicates that we might want to make panics in Drop always abort the program, which would imply that devs who don't want the program to abort on error in Drop would have to use eprintln! regardless of panicking. It seems that it might be worthwhile to revisit that RFC before deciding on the course here.

1, 2, and 4 can all be implemented with PanicTracker, panic_count, or unwind_drop. I'll explain my thoughts on those mechanisms in the next comment.

A major contention point here is FFI. An important question here is:

Do we want non-C++, non-Rust exceptions unwinding through Rust code to run destructors?

(The C++ case is already answered as "yes" on both SEH and Itanium.) The answer seems to be "yes", otherwise we'd hard-code C++ and that doesn't seem good. Even if we did hard-code C++, I don't know if we could weakly dynamically link to libstdc++ so as to keep working if C++ is not used in the project. The rest of this comment assumes the answer to this is "yes".

In this case, we want panicking to return true in destructors executing due to unwinding from any language, and so the responsibility of tracking panic_count should be shifted to landing pads and the personality function.

Now, this part will be weird, so please bear with me for a moment.

In SEH, cleanup pads are "subfunctions" that use ret to keep unwinding. The SEH personality function is responsible for invoking the cleanup pad as a normal function. On SEH, it would be trivial to increment panic_count before this invocation and decrementing it after, both operations to be performed by the personality function directly.

In Itanium, cleanup pads end with call _Unwind_Resume. The Itanium personality function is responsible for setting the program counter to the address of the cleanup pad, and the system unwinder does the rest. This means that we could increment panic_count in the personality function, but decrementing it would have to occur in the landing pad itself.

This is bad. For one thing, it's code bloat in every binary for a rarely used feature. It's also inconsistent and it feels like that might cause problems.

But I think we can avoid this. My proposal -- if we want to go down this road, of course -- is to change the format of the Itanium cleanup pads to use ret, just like in SEH. This will require modifications to LLVM, sure, but I hope it's not impossibly difficult. Our personality function would then invoke the cleanup method just like in SEH and both increment and decrement panic_count. We would tell the unwinder there's no more work to do in this frame instead of installing a context.

We'd need to modify personality functions for both Itanium and SEH, so there'd be quite a bit of support code to upstream to LLVM, but what's new under the sun.

I think my main problem with panic_count / PanicTracker is that it keeps defer_on_unwind unsound in async functions, at least without serious modifications to futures.

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

This feels like it should have the same guarantees as

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

but in reality, a bad-faith executor could force two strings to be printed. I've outlined a possible solution in #143612 (comment), but it feels hacky and too costly to me.

In contrast, unwind_drop would require no changes to the personality function and would automatically handle this case correctly, so although it doesn't work well with containers, I think it's at least worthwhile to explore.

Rollup merge of rust-lang#144236 - yoshuawuyts:drop-guard, r=Mark-Sim…

Unrolled build for #144236

Rollup merge of rust-lang#144236 - yoshuawuyts:drop-guard, r=Mark-Sim…

Rollup merge of #144236 - yoshuawuyts:drop-guard, r=Mark-Simulacrum

Navigation Menu

Repository navigation

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

10 remaining items

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Nested panics and std::thread::panicking #143612

Description

Activity

Noratrieb commented on Jul 7, 2025

bjorn3 commented on Jul 7, 2025

purplesyringa commented on Jul 7, 2025

bjorn3 commented on Jul 7, 2025

purplesyringa commented on Jul 7, 2025

purplesyringa commented on Jul 7, 2025

purplesyringa commented on Jul 7, 2025

purplesyringa commented on Jul 7, 2025

10 remaining items

Amanieu commented on Jul 15, 2025

Fulgen301 commented on Jul 18, 2025

purplesyringa commented on Jul 18, 2025

purplesyringa commented on Jul 18, 2025

purplesyringa commented on Jul 18, 2025

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions

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