Add std::panic::drop_unwind

[SabrinaJewson]

A footgun in std::panic::catch_unwind is that panic payloads can panic when dropped. For example, this code:

let _ = panic::catch_unwind(user_code);

does not guarantee that unwinds will not occur in the outer function, because the user code could look like this:

struct PanicOnDrop;
impl Drop for PanicOnDrop {
    fn drop(&mut self) {
        panic!();
    }
}
panic_any(PanicOnDrop);

This has caused many soundness bugs for me in the past, and I have encountered soundness bugs in other crates because of this.

The solution is to add a new function, std::panic::drop_unwind, that instead of returning a Result<T, Box<dyn Any + Send>> returns a Result<T, ()>. Panic payloads are safely dropped within the function by converting panics to aborts (it's such a rare case there's no reason to try and recover).

[rust-highfive]

r? @m-ou-se

(rust-highfive has picked a reviewer for you, use r? to override)

[m-ou-se]

That's quite a subtle but significant issue! I've opened #86027 for the issue.

Adding drop_unwind makes it easier to do it correctly, but won't fix all the cases where catch_unwind is used. Maybe we should find a way to work around this, or produce a warning for the issue or something.

[bors]

☔ The latest upstream changes (presumably #84662) made this pull request unmergeable. Please resolve the merge conflicts.

[JohnCSimon]

Ping from triage:
@KaiJewson can you please address the merge conflict?

After you've done that please set the PR to S-waiting-on-review

[bjorn3]

@rustbot ready

[rustbot]

@m-ou-se

[dtolnay]

Marking as S-blocked on the discussion that is ongoing in #86027. A few alternatives are proposed. I don't think we would want to land this PR until there is some semblance of consensus around the alternative approaches.

[CAD97]

This is unstable, so it seems okay to land such that it can be used (at least by std itself) until further consensus on #86027/rust-lang/lang-team#97 is reached.

As an aside, it might be a good idea to call crate::panicking::panic_count::increase() and crate::panicking::panic_count::decrease() around the panic payload drop to get the "panicked while panicking" message rather than just an abort when hitting AbortOnDrop.

How I wrote drop_unwind

pub fn drop_unwind<F: FnOnce() -> R + UnwindSafe, R>(f: F) -> Option<R> {
    match catch_unwind(f) {
        Ok(r) => Some(r),
        Err(e) => {
            // Dropping the panic payload could panic, so prevent that.
            // Treat the thread as still panicking.
            crate::panicking::panic_count::increase();
            let e = AssertUnwindSafe(e);
            // Mark the drop closure as rustc_allocator_nounwind. This tells
            // rustc to emit its own shim to turn unwinds into aborts.
            match catch_unwind(
                #[rustc_allocator_nounwind]
                || drop(e),
            ) {
                Ok(()) => {}
                Err(_dont_drop) => {
                    // We still unwound again somehow; just abort directly.
                    rtprintpanic!("thread panicked while dropping panic payload. aborting.\n");
                    crate::sys::abort_internal();
                }
            }
            // Dropping did not panic.
            crate::panicking::panic_count::decrease();
            None
        }
    }
}

(I'm unsure if this is correct usage of #[rustc_allocator_nounwind].)

[bors]

☔ The latest upstream changes (presumably #106228) made this pull request unmergeable. Please resolve the merge conflicts.

[JohnCSimon]

@SabrinaJewson
Ping from triage: I'm closing this due to inactivity, Please reopen when you are ready to continue with this.
Note: if you do please open the PR BEFORE you push to it, else you won't be able to reopen - this is a quirk of github.
Thanks for your contribution.

@rustbot label: +S-inactive