Never allow unwinding from Drop impls

Summary

Code using catch_unwind is not typically prepared to handle an object that panics in its Drop impl. Even the standard library has had various bugs in this regard, and if the standard library doesn't consistently get it right, we can hardly expect others to do so.

This came up in @rust-lang/libs discussion.

We discussed various ways to handle this, including potential tweaks to panic_any or catch_unwind to add special handling of types that implement Drop, but on balance we felt like it would be preferable to decide at the language level to generally not allow unwind from Drop impls. (We may not be able to universally prohibit this, but we could work towards transitioning there.)

Background reading

rust-lang/rust#86027

About this issue

This issue corresponds to a lang-team design meeting proposal. It corresponds
to a possible topic of discussion that may be scheduled for deeper discussion
during one of our design meetings.

To clarify something, I'm not proposing making a backwards-incompatible change here; there may well be code relying on the ability to do this. But there are steps we could take to deprecate this for new code, such as a new panic variant that allows unwind but not from Drop, which could eventually become the default. And eventually, new code may be able to safely say "this crate just doesn't support unwind-on-Drop at all".

And eventually, new code may be able to safely say "this crate just doesn't support unwind-on-Drop at all".

Making a behavior change a property of the "current" crate is convenient to have opt-in through an edition, but what if multiple crates are involved? For example a generic function in edition A contains code expanded by a (proc-)macro in edition B that drops a value of a type parameter, which for a given instantiation is a type (and Drop impl) is in edition C.

Current panic behavior is universal, determined by the executable. There's never some crates with unwind and some with abort. This would have to be a third variant: unwind-but-not-in-drop.

Zulip discussion: https://rust-lang.zulipchat.com/#narrow/stream/213817-t-lang/topic/Never.20allow.20unwinding.20from.20Drop.20impls

Notes from today's @rust-lang/lang design planning meeting:

For this meeting to be a success, we would need some exploration of what the alternatives are and what kinds of backwards compatibility concerns exist in the wild.
- Are people relying on the current behavior?
- Can we estimate how many latent bugs exist because of the current behavior?
  - Anecdotally: Rayon for example found it too hard to manage and just aborts.
  - Libs team has found this painful in the std library.

Current plan: defer this topic until a design meeting in August, plan for summarizing proposals and usage information from Zulip by that meeting.

I opened a PR which implements -Z panic-in-drop=abort (rust-lang/rust#88759), we should have some perf results soon.

Initial perf results: up to 10% reduction in compilation time (perf) and a 5MB (3%) reduction in the size of librustc_driver.so.

Should there be finer-grain controls (e.g. type-level)?

Should there be finer-grain controls (e.g. type-level)?

This would prevent us from optimizing drops of Box<dyn Any> since we wouldn't know whether they unwind.

I feel that this is best left as a global setting and that we eventually should enable it by default (and even remove the option of disabling it). This way authors of unsafe code can rely on stronger guarantees that dropping an object never unwinds.

This would prevent us from optimizing drops of Box<dyn Any> since we wouldn't know whether they unwind.

It's limited, but we can still perform some optimizations. E.g. if the drop to Box<dyn Any> is itself called from a nounwind function, we'll still abort rather than unwind.

I feel that this is best left as a global setting and that we eventually should enable it by default (and even remove the option of disabling it). This way authors of unsafe code can rely on stronger guarantees that dropping an object never unwinds.

I believe there are still legit uses of panicking within drop. The stronger guarantee is usually about calling drop within drops; aborting in such scenario is more reasonable than a blanket "panic in drop will abort".

"significant" drop here means "has drop glue," "more than nothing (forget)," or "runs some Drop::drop code."

Any type which does not have a drop implementation and does not have any field with (transitively a field that has) a drop implementation has a trivial drop.

Making std types special as "not user code" and therefore "safe" to drop (if they only contain std types) is unfair to third party libraries and further privileges std. The alternative would be some way of specifying nounwind(nounwind(~T())) for user types to a) generate a landing pad that aborts and b) allow the type to be silently dropped even when #[warn(implicit_could_panic_drops)].

It's also worth reiterating that extern "C" will have an implicit nounwind landing pad once extern "C-unwind" is available. Making drop unwinds always abort does not plug a soundness hole here. It does make code destroying unknown types easier to make correct.

I think every drop is too much (unless I misunderstood what you mean by "significant"). But I would like a lint against running arbitrary user code, including drop. This will be very helpful for verifying correctness of unsafe functions.

My apologies; by a "significant drop", I mean any drop that calls a Drop impl, or approximately, dropping any type T where core::mem::needs_drop::<T>(). This is somewhat inspired by clippy::significant_drop_in_scrutinee, although I should probably use another name to avoid confusing the concepts.

Your idea does sound attractive, though, if it could be implemented usefully; writing generic unsafe code can be difficult when every user function or Drop impl can panic or give incorrect results. Perhaps such a lint could trigger on a few different things, such as:

Calling a trait method on a type parameter.
Calling a trait method on a dyn Trait.
Dropping a type parameter.
Dropping a dyn Trait.

The problem would be indirection; does dropping a LibraryType<T> drop T values or not? We could add attributes for types like Option<T> and Box<T> in the standard library, but this wouldn't work for third-party libraries. I'd be happy to hear of any good solutions on this front.

@CAD97

I think I should also point out explicitly (I think @CAD97 mentioned about this as AbortOnUnwindBomb but doesn't go into much detail) is that for unsafe code it's fairly easy to prevent unwinding from escaping a function (and works in stable):

struct PanicGuard;
impl Drop for PanicGuard {
	fn drop(&mut self) {
		std::process::abort();
		// panic!("unwind escapes nounwind context"); // This also works, as it causes double-unwind, so trigger abort
		// core::intrinsics::abort(); // Or this with unstable
		// rtabort!("unwind escapes nounwind context"); // Or this when used inside std
	}
}

let guard = PanicGuard;
// Your code that does not allow unwinding
std::mem::forget(guard);

When C-unwind is stablised majority of FFI code will automatically get their free abort-upon-unwind behaviour, and the remaining non-extern "C" ones could use this pattern to explicit prohibit unwind.

"significant" drop here means "has drop glue," "more than nothing (forget)," or "runs some Drop::drop code."

My apologies; by a "significant drop", I mean any drop that calls a Drop impl, or approximately, dropping any type T where core::mem::needs_drop::<T>(). This is somewhat inspired by clippy::significant_drop_in_scrutinee, although I should probably use another name to avoid confusing the concepts.

This is what I thought; I just wanted to make sure you didn't mean something like "runs user code on drop".

Making std types special as "not user code" and therefore "safe" to drop (if they only contain std types) is unfair to third party libraries and further privileges std.

I didn't mean to implement this only for std types. I want an (allow-by-default) lint that warns when you run untrusted code, i.e. user code. As long as you know the types everything is fine (unsafe Rust can trust known safe code). Generics are indeed a problem, but even using heuristics will be a big help.

@nbdd0121

Rather than specifying "user code" (from a compiler perspective, that's any Rust code not emitted by the compiler), it's better to say "upstream" or "downstream" for the relationship of the library dependency graph.

There should still be a lint for implicit drop glue of upstream types, but I do agree that if such a lint is added, splitting it into two tiers of downstream and all types would indeed be useful.

Also, @nbdd0121: abort on unwind can be made even simpler, skipping the need to forget with a cheap runtime check:

impl Drop for AbortOnUnwind {
    fn drop(&mut self) {
        if std::thread::panicking() { panic!("unwound through nounwind context") }
    }
}

@nbdd0121

Also, @nbdd0121: abort on unwind can be made even simpler, skipping the need to forget with a cheap runtime check:
impl Drop for AbortOnUnwind {
    fn drop(&mut self) {
        if std::thread::panicking() { panic!("unwound through nounwind context") }
    }
}

I think it's desirable in the future to allow multiple concurrent panics -- as long as the inner panic is caught and does not escape. This code will disallow nested panics.

Also, checking std::thread::panicking would not protect your against a foreign exception; std::thread::panicking() would return false when, for example, a C++ exception causes the current in-progress unwind.

C++ exceptions crossing Rust frames with drop glue are already UB, and there is no current plan to make them not UB. panic! was an option you gave and the one I chose for the example; you can just as easily use abort. Also, this code has no impact on whether unwinding inside of a drop while already unwinding needs to immediately abort or only abort when hitting the drop glue edge, as this panic clearly does hit the edge of the destructor.

But this is off topic noise in an already noisy thread.

C++ exceptions crossing Rust frames with drop glue are already UB, and there is no current plan to make them not UB.

That's not true with C-unwind.

panic! was an option you gave and the one I chose for the example; you can just as easily use abort.

No, I mean your code would abort when it is used while unwinding is already in place, not only when a new unwind propagates out from the function body.

But this is off topic noise in an already noisy thread.

Well, I think this is on-topic, since other ways to prevent the footgun would be alternative solutions to the proposed approach.

C++ exceptions crossing Rust frames with drop glue are already UB, and there is no current plan to make them not UB.

That's not true with C-unwind.

Ah, apologies, I misremembered the section about forced unwinds as applying to all foreign unwinds.

I wrote an RFC to argue for turning -Z panic-in-drop=abort on by default: rust-lang/rfcs#3288

I assume you mean "=abort"?

Oops, fixed.

There is now an RFC, I don't think we ever had the meeting, going to close this meeting request.

Navigation Menu

Repository navigation

Never allow unwinding from Drop impls #97

Summary

Background reading

About this issue

109 remaining items

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Never allow unwinding from Drop impls #97

Description

Summary

Background reading

About this issue

Activity

joshtriplett commented on Jun 9, 2021

SimonSapin commented on Jun 10, 2021

Lokathor commented on Jun 10, 2021

danielhenrymantilla commented on Jun 10, 2021

joshtriplett commented on Jul 7, 2021

Amanieu commented on Sep 8, 2021

Amanieu commented on Sep 9, 2021

nbdd0121 commented on Sep 10, 2021

Amanieu commented on Sep 10, 2021

nbdd0121 commented on Sep 10, 2021

109 remaining items

CAD97 commented on Jun 23, 2022

LegionMammal978 commented on Jun 23, 2022

nbdd0121 commented on Jun 23, 2022

ChayimFriedman2 commented on Jun 23, 2022

CAD97 commented on Jun 23, 2022

nbdd0121 commented on Jun 23, 2022

CAD97 commented on Jun 23, 2022

nbdd0121 commented on Jun 23, 2022

CAD97 commented on Jun 23, 2022

Amanieu commented on Jul 5, 2022

RalfJung commented on Jul 5, 2022

Amanieu commented on Jul 5, 2022

nikomatsakis commented on Oct 12, 2022

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions