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.

[nagisa]

Why is this categorized I-unsound? There is no inherent unsoundness in the catch_unwind function (which appears the primary focus of this issue), and any unsound code (such as the libstd example) appears to be a bug in the code that uses said function incorrectly when also interacting with unsafe code otherwise.

Those unsoundness instances should be tracked and fixed separately, and this issue, I think, should remain as a plain footgun issue, rather than an I-unsound one.

(As to why: I-unsound issues are usually prioritized differently, interpreted by passersby differently, and tend to be subject to various time consuming processes. I think given these things this nitpick is quite important ^^)

---

Not allowing the destructors to panic would be great to simplify codegen significantly too, and would mostly remove another class of footguns (panic-while-panicking). That said, knowing how abundant destructors that run fallible code are, it would likely be quite difficult to justify this kind of change?

[m-ou-se]

If we make the Bomb panic with another Bomb in drop, the proc_macro case causes a fun triple ICE:

thread 'rustc' panicked at 'Box<Any>', 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

note: rustc 1.54.0-nightly (dbe459ded 2021-06-02) running on x86_64-unknown-linux-gnu

note: compiler flags: -C embed-bitcode=no -C debuginfo=2 -C incremental --crate-type bin

note: some of the compiler flags provided by cargo are hidden

query stack during panic:
end of query stack
thread 'main' panicked at 'Box<Any>', src/lib.rs:5:9

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

note: rustc 1.54.0-nightly (dbe459ded 2021-06-02) running on x86_64-unknown-linux-gnu

query stack during panic:
end of query stack
fatal runtime error: failed to initiate panic, error 5
zsh: abort (core dumped)

[SabrinaJewson]

There are additional reasons why we may want to add an UnwindError type:

• It is shorter and less noisy to type than Box<dyn Any + Send>. I have used type Panic = Box<dyn Any + Send> multiple times in the past.
• We can add utility methods for panic payloads specifically, such as fn as_str(&self) -> Option<&str> and fn into_string(self) -> Result<Cow<'static, str>, Self>.
• It can provide a more helpful Debug implementation that makes use of the above methods.
• If we want, we can make it Sync.

[dtolnay]

@rustbot modify labels +T-lang -I-nominated

We discussed this topic in today's @rust-lang/libs meeting. There is some traction behind #86027 (comment) (aborting on any unwind from a Drop impl) but that would be for the lang team to decide. @joshtriplett will follow up with the lang team -- rust-lang/lang-team#97.

I promised to write up a counterproposal for how to proceed without any language change necessary. The approach I'd like is that catch_unwind also catches unwinds from the payload's Drop impl. That sounds impossible but here is how:

The dyn Any vtable is currently something like this:

    vtable struct for dyn Any
+------------------------------+
| size: usize                  |
+------------------------------+
| align: usize                 |
+------------------------------+
| type_id: fn(&self) -> TypeId |
+------------------------------+
| drop: fn(&mut self)          |
+------------------------------+

with an example instance:

Box::new(Bomb) as Box<dyn Any>                    impl Any for Bomb vtable
  +--------------------+           +-------> +---------------------------------+
  | ptr: 0xfoobar      |           |         | size: 0                         |
  +--------------------+           |         +---------------------------------+
  | vtable: -----------------------+         | align: 1                        |
  +--------------------+                     +---------------------------------+
                                             | type_id: <Bomb as Any>::type_id |
                                             +---------------------------------+
                                             | drop: <Bomb as Drop>::drop      |
                                             +---------------------------------+

The implementation I'd like looks like:

     impl Any for Bomb vtable         impl Any for Bomb (nonpanicking-drop vtable)
+---------------------------------+   +-> +---------------------------------+ <-+
| size: 0                         |   |   | size: 0                         |   |
+---------------------------------+   |   +---------------------------------+   |
| align: 1                        |   |   | align: 1                        |   |
+---------------------------------+   |   +---------------------------------+   |
| type_id: <Bomb as Any>::type_id |   |   | type_id: <Bomb as Any>::type_id |   |
+---------------------------------+   |   +---------------------------------+   |
| nonpanicking_drop_vtable: ----------+   | nonpanicking_drop_vtable: ----------+
+---------------------------------+       +---------------------------------+
| drop: <Bomb as Drop>::drop      |       | drop: catch_unwind(Bomb::drop)  |
+---------------------------------+       +---------------------------------+

This is a circular linked list of vtables. During the initial unsize coercion from Box<T> to Box<dyn Any>, the vtable would be the type's ordinary (potentially panicking Drop) dyn Any vtable. However, catch_unwind (and all other panicking uses of dyn Any, like resume_unwind) would take one step along this linked list, resulting in a payload Box<dyn Any> whose Drop is non-panicking (it aborts instead).

Pseudocode:

// https://doc.rust-lang.org/std/panic/fn.resume_unwind.html
pub fn resume_unwind(payload: Box<dyn Any + Send>) -> ! {
    payload.vtable = payload.vtable.nonpanicking_drop_vtable;
    /* now resume using `payload` as the Box<dyn Any>... */
}

[Diggsey]

@dtolnay That seems very magical, and while equality on vtables is already broken, this would mean that there could never be a 1:1 correspondence between (trait/type pair) and (vtable pointer).

Also, the way that this "abort on panic" property sticks around indefinitely on the value, even outside of the catch_unwind code is quite surprising.

IMO, the best option is just to change existing code to fix the problem (without excluding #86027 (comment) as a future possibility).

To assist with that change, we should introduce a new catch_unwind function which returns Result<T, AbortOnPanic<Box<dyn Any>>>.

[danielhenrymantilla]

FWIW, I have submitted #99032 which offers one way to fix or at least palliate this

[RalfJung]

(Replying here since it's not what the PR does.)

FWIW, another approach to achieve the same idea would be to create one's own tweaked version of the vtable of Any, and directly use it to manually unsize a Box to a Box<dyn Any…> (using ptr::from_raw_parts_mut) to achieve what this PR automagically does with Box<Wrapper>. That being said, since the exact vtable layout of Any, much like the vtable layout of any trait, is only known to the compiler, this other approach could only be achieved by exploiting stdlib magic knowledge of lang stuff. For instance, it would be brittle: Any's definition, the vtable generation algorithm of the compiler, and this hand-rolled vtable would all three have to remain in sync at all times.

I would be rather opposed to that. I think we want to specify vtables in a way that it becomes entirely impossible for Rust code to actually generate a valid vtable -- any attempt of doing so will yield UB. vtables are "magic" and recognized by the Abstract Machine and that magic can not be replicated just by putting the same bits into some other memory.

I plan to implement this in Miri in the near future. Under that version of Miri, that alternative implementation quoted above would be diagnosed as UB, notwithstanding the fact that it is in the stdlib. The stdlib is "privileged" to exploit implementation-specific behavior, but it cannot do anything about undefined behavior.

[SoniEx2]

btw this may be a hot take but has anyone tried documenting the current behavior with catch_unwind payload dropping?

because it's currently not https://doc.rust-lang.org/stable/std/panic/fn.catch_unwind.html

[safinaskar]

Maybe we should deny unwinding panic from drop in edition 2024? I. e. such drop should always abort, similarly to C++ default behavior?

[RalfJung]

While #99032, which would conclusively solve this, issue has been sitting inactive for years, we should definitely add a note of this behavior to our docs. Fixing this properly is hard, but almost 3 years after learning about the problem we should definitely begin telling our users about it. (We should have done that long ago, but alas.)