Footgun with `catch_unwind` when catching panic-on-drop types

[m-ou-se]

Originally reported by @SabrinaJewson in #85927

---

catch_unwind(code) is often used to make sure no panics from code can cause further unwinding/panics. However, when catching a panic with a payload that panics on Drop, most usages of catch_unwind(code) will still result in further unwinding and often unsoundness.

struct Bomb;

impl Drop for Bomb {
    fn drop(&mut self) {
        panic!();
    }
}

std::panic::panic_any(Bomb);

Example in rustc (found by @mystor):

rust/compiler/rustc_ast/src/mut_visit.rs

Lines 299 to 300 in 5ea1923

	let new_t = panic::catch_unwind(panic::AssertUnwindSafe(|| f(old_t)))
	.unwrap_or_else(|_| process::abort());

Here, the Result containing the panic payload is dropped before abort() is called, which might cause a panic.

Edit: Looks like the _ doesn't cause an immediate drop as a parameter, so this case works fine, possibly by accident.

Another example in the standard library:

rust/library/std/src/rt.rs

Lines 34 to 39 in 5ea1923

	let exit_code = panic::catch_unwind(main);
	sys_common::rt::cleanup();
	exit_code.unwrap_or(101) as isize
	}

fn main() {
    std::panic::panic_any(Bomb);
}

thread 'main' panicked at 'Box<Any>', src/main.rs:12:5
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
thread 'main' panicked at 'explicit panic', src/main.rs:7:9
fatal runtime error: failed to initiate panic, error 5
abort (core dumped)

And another case in the proc_macro bridge:

rust/library/proc_macro/src/bridge/server.rs

Lines 115 to 116 in 5ea1923

	panic::catch_unwind(panic::AssertUnwindSafe(call_method))
	.map_err(PanicMessage::from)

#[proc_macro]
pub fn hey(_: proc_macro::TokenStream) -> proc_macro::TokenStream {
    std::panic::panic_any(Bomb);
}

thread 'rustc' panicked at 'explicit panic', src/lib.rs:5:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

error: internal compiler error: unexpected panic

note: the compiler unexpectedly panicked. this is a bug.

note: we would appreciate a bug report: https://github.com/rust-lang/rust/issues/new?labels=C-bug%2C+I-ICE%2C+T-compiler&template=ice.md

[m-ou-se]

#85927 adds drop_unwind as an alternative to catch_unwind which safely aborts() when dropping the payload panics. While that does provide a safer alternative, it doesn't solve the easy mistake which we've apparently made ourselves already in quite a few places.

[m-ou-se]

cc @rust-lang/libs @rust-lang/compiler

[Mark-Simulacrum]

One option might be to somehow intervene (via an intrinsic or whatever) in the destructor of the Any trait object such that the destructor decrements the panic counter after running (i.e., the caught panic is still "panicking" until it's destructor runs). That would make any panics in that destructor cause an abort.

It's not clear to me that this behavior is what's desired in all cases, but I'm not sure how much flexibility we have... it seems unlikely that someone actually wants alternative behavior and can't get it with downcasting or similar...

[mystor]

Unfortunately I don't know how possible that will be to do in this particular situation, as resume_unwind will allow user code to provide arbitrary Box<dyn Any + Send + 'static> types, and it's probably not practical for rustc to change the vtable at that point.

(edit: I suppose it might be possible to use a flag in the low bits of the vtable pointer to indicate that drop may not panic without making a duplicate vtable for every dyn Any, but that adds code to every callsite using trait objects)

[m-ou-se]

We could deprecate catch_unwind and have a new catch_panic or something that doesn't give a Box, but gives it wrapped in a PanicPayloadThingBikeshed with the abort-on-panic-in-drop behaviour.

[BoxyUwU]

Can we just change the return type to be Result<T, UnwindError> and have UnwindError abort in the drop impl if the Box panics when dropped, it is basically a soundness fix. Maybe a crater run? :)

[bjorn3]

You can use catch_unwind in combination with resume_unwind on the panic result. For example https://github.com/bevyengine/bevy/pull/2222/files#diff-30fd76e28519c7fa11f2990e8f7a70c6fc09227f30ea6ffb46ccb243cdb9f15eR42-R50 Changing both at the same time also won't work as compiletest uses resume_unwind to fail a test without a panic message and backtrace.

[Amanieu]

I personally feel that the long-term solution to this and other issues related to panic-on-dropis is to do what C++ did in C++11: destructors were changed to be noexcept by default, which causes an immediate abort if an exception is thrown from a destructor. One of the primary motivations for this was that unwinding from destructors is a huge footgun that can very easily lead to memory leaks or undefined behavior. In fact the collection types in the C++ standard library explicitly say that objects placed in a collection must not throw from their destructor, otherwise behavior is undefined.

[mystor]

A potentially less-invasive version of @Amanieu's proposal would be to only require trait object destructors to abort if they panic, and continue to allow panicking from other Drop impls.