Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ACP: panic::abort_unwind #441

Closed
CAD97 opened this issue Sep 12, 2024 · 5 comments
Closed

ACP: panic::abort_unwind #441

CAD97 opened this issue Sep 12, 2024 · 5 comments
Labels
ACP-accepted API Change Proposal is accepted (seconded with no objections) api-change-proposal A proposal to add or alter unstable APIs in the standard libraries T-libs-api

Comments

@CAD97
Copy link

CAD97 commented Sep 12, 2024

Proposal

Problem statement

Various use cases exist that can't or don't want to support being unwound through. Today, there are multiple ways of coverting an in-progress unwind into an abort, each with subtly different user experience when the abort occurs. By providing a function abort_unwind as part of core, we can standardize the experience.

Motivating examples or use cases

Perhaps the most notorious case of unwind-abort is the take_mut/replace_with pattern. The take_mut crate uses catch_unwind to respond to unwinds. The replace_with crate uses drop flag conditional drop glue to respond to unwinds. Both crates simply call abort on an unwind and don't show a more specific reason for what caused the abort.

Also highly notable is the soundness footgun that dropping the panic payload returned from catch_unwind can itself cause an unwind. std necessarily uses catch_unwind1 and responds to an unwind with rtabort!.

I myself have used impl Drop types to cause a deliberate double panic (and thus an abort) in code that cannot soundly repair state when it gets unwound. This strategy generally creates a more useful crash message, as the runtime can see that the unwind will not be completed.

Solution sketch

The newly stabilized abort on extern "C" edges (“panic in a function that cannot unwind”) is the exact handling that we would like to have for all code regions that cannot be unwound. We also have catch_unwind already establishing the API shape of establishing an unwind handler.

/// Invokes a closure, aborting if the closure unwinds.
///
/// When compiled with aborting panics, this function is effectively a no-op.
/// With unwinding panics, an unwind results in another call into the panic
/// hook followed by a process abort.
///
/// # Notes
///
/// Instead of using this function, code should attempt to support unwinding.
/// Implementing [`Drop`] allows you to restore invariants uniformly in both
/// return and unwind paths.
///
/// If an unwind can lead to logical issues but not soundness issues, you
/// should allow the unwind. Opting out of [`UnwindSafe`] indicates to your
/// consumers that they need to consider correctness in the face of unwinds.
///
/// If an unwind would be unsound, then this function should be used in order
/// to prevent unwinds. However, note that `extern "C" fn` will automatically
/// convert unwinds to aborts, so using this function isn't necessary for FFI.
pub fn abort_unwind<F: FnOnce() -> R, R>(f: F) -> R {
    // extern "C" converts unwinding edges to unwind abort(abi)
    extern "C" fn abort_unwind_inner<F: FnOnce() -> R, R>(f: F) -> R {
        f()
    }

    abort_unwind_inner(f)
}

Alternatives

We can always do nothing; existing solutions in the ecosystem are sufficient. However, the way that this is usually done results in suboptimal diagnostics on an abort, and the required user code required to be sound is “obvious” enough that pulling in a 3rd party crate is unlikely.

I've chosen to use an inner function with extern "C" such that the public API signature doesn't show this implementation choice. It would be a valid choice to skip this step, and/or to use the #[rustc_nounwind] attribute instead of ABI.

Similarly, the language could provide a public #[nounwind] attribute to prevent unwinding instead of abort_unwind being library API. I argue that providing the library API is the better choice since the added friction discouraging the use of nounwind shims is a good thing overall.

Links and related work

I have participated in wg-unwind, but this is my own individual idea.

What happens now?

This issue contains an API change proposal (or ACP) and is part of the libs-api team feature lifecycle. Once this issue is filed, the libs-api team will review open proposals as capability becomes available. Current response times do not have a clear estimate, but may be up to several months.

Possible responses

The libs team may respond in various different ways. First, the team will consider the problem (this doesn't require any concrete solution or alternatives to have been proposed):

  • We think this problem seems worth solving, and the standard library might be the right place to solve it.
  • We think that this probably doesn't belong in the standard library.

Second, if there's a concrete solution:

  • We think this specific solution looks roughly right, approved, you or someone else should implement this. (Further review will still happen on the subsequent implementation PR.)
  • We're not sure this is the right solution, and the alternatives or other materials don't give us enough information to be sure about that. Here are some questions we have that aren't answered, or rough ideas about alternatives we'd want to see discussed.

Footnotes

  1. There needs to be at least one catch frame on the stack to ensure that raising an unwind can actually unwind the stack.

@CAD97 CAD97 added api-change-proposal A proposal to add or alter unstable APIs in the standard libraries T-libs-api labels Sep 12, 2024
@dtolnay
Copy link
Member

dtolnay commented Sep 12, 2024

LGTM. I think this is worth having.

One thing that makes this strange is libcore does not currently expose any stable fn abort() -> !. It would be unfortunate if core::panic::abort_unwind(|| panic!()) needs to become the preferred no-std way to express an abort.

@dtolnay
Copy link
Member

dtolnay commented Sep 15, 2024

Accepting this together with #442. (But they can be implemented and tracked independently, and perhaps stabilized independently.)

I wonder if there will be any reason to have separate abort_immediate vs abort flavors of abort_unwind.

@dtolnay dtolnay added the ACP-accepted API Change Proposal is accepted (seconded with no objections) label Sep 15, 2024
@dtolnay dtolnay closed this as completed Sep 15, 2024
@CAD97
Copy link
Author

CAD97 commented Sep 15, 2024

I wonder if there will be any reason to have separate abort_immediate vs abort flavors of abort_unwind.

This seems highly unlikely. Existing usages of intrinsics::abort are micro-optimizations for code size (ud2 is cheaper than call abort@GOTPCREL) and/or compile time (intrinsic is cheaper than a function call edge). The impact of [unwind abort(abi)] edges is already tuned to have minimal impact, and the case of wanting to avoid the panic machinery entirely is served by panic_immediate_abort.

If someone absolutely must abort_immediate on unwind while also still supporting unwinding panics normally elsewhere, using a manual drop guard to do so seems like a reasonable solution for this edge case of a requirement.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Sep 15, 2024
Add `core::panic::abort_unwind`

`abort_unwind` is like `catch_unwind` except that it aborts the process if it unwinds, using the `#[rustc_nounwind]` mechanism also used by `extern "C" fn` to abort unwinding. The docs attempt to make it clear when to (rarely) and when not to (usually) use the function.

Although usage of the function is discouraged, having it available will help to normalize the experience when abort_unwind shims are hit, as opposed to the current ecosystem where there exist multiple common patterns for converting unwinding into a process abort.

For further information and justification, see the linked ACP.

- Tracking issue: rust-lang#130338
- ACP: rust-lang/libs-team#441
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Sep 15, 2024
Rollup merge of rust-lang#130339 - CAD97:unwind-choice, r=dtolnay

Add `core::panic::abort_unwind`

`abort_unwind` is like `catch_unwind` except that it aborts the process if it unwinds, using the `#[rustc_nounwind]` mechanism also used by `extern "C" fn` to abort unwinding. The docs attempt to make it clear when to (rarely) and when not to (usually) use the function.

Although usage of the function is discouraged, having it available will help to normalize the experience when abort_unwind shims are hit, as opposed to the current ecosystem where there exist multiple common patterns for converting unwinding into a process abort.

For further information and justification, see the linked ACP.

- Tracking issue: rust-lang#130338
- ACP: rust-lang/libs-team#441
@RalfJung
Copy link
Member

std necessarily uses catch_unwind

Couldn't it use abort_unwind now?

@CAD97
Copy link
Author

CAD97 commented Sep 24, 2024

lang_start needs to use catch_unwind because it's not only UB to unwind out of generic system main, it's effectively UB to start an unwind without a catch somewhere on the stack. (C++ says that std::terminate() is called, but impl-defined whether unwinding takes place.)

On Windows, using abort_unwind would likely work; an unwind without any catch will unwind out of main then abort (experimental proof), meaning we can intercept the unwind with our abort messaging.

On Itanium EH targets (e.g. Linux), an unwind without any catch target will fail to start (by the spec). Currently Rust reserves the right to abort before the unwind when an unwind would end with a runtime-introduced abort, but this isn't a great user experience. It could be acceptable for unwinding from the drop of a panic payload, if the error improves from “fatal runtime error: failed to initiate panic, code 5”. (Unfortunately, _Unwind_RaiseException is allowed to return _URC_FATAL_PHASE2_ERROR indicating an arbitrarily corrupted stack and that all registers are clobbered, making immediate rtabort! the only somewhat safe option in that case. But we don't even handle this correctly today, so…)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ACP-accepted API Change Proposal is accepted (seconded with no objections) api-change-proposal A proposal to add or alter unstable APIs in the standard libraries T-libs-api
Projects
None yet
Development

No branches or pull requests

3 participants