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

Panic safety in drop #14

Closed
bluss opened this issue Aug 20, 2015 · 2 comments
Closed

Panic safety in drop #14

bluss opened this issue Aug 20, 2015 · 2 comments

Comments

@bluss
Copy link
Contributor

bluss commented Aug 20, 2015

@Stebalien discovered panic safety issue bluss/arrayvec#3 and it applies to smallvec as well.

My understanding is: SmallVec::drop first attempts to drop every element, then it inhibits the drop of the inner array. The panic safety issue is that a panic during drop of an element means the inhibition is never reached, so the inner data can be dropped again.

Testcase adapted from @Stebalien

    #[test]
    #[should_panic]
    fn test_drop_panic_smallvec() {
        // This test should only panic once, and not double panic,
        // which would mean a double drop
        struct DropPanic;

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

        let mut v = SmallVec::<[_; 1]>::new();
        v.push(DropPanic);
    }
bluss pushed a commit to bluss/rust-smallvec that referenced this issue Aug 20, 2015
bluss pushed a commit to bluss/rust-smallvec that referenced this issue Aug 20, 2015
The summary is: SmallVec::drop first attempts to drop every element,
then it inhibits the drop of the inner array. The panic safety issue is
that a panic during drop of an element means the inhibition is never
reached, so the inner data can be dropped again.

NoDrop acts as a panic guard; its drop will trigger on panic, so the
inner array's drop will be inhibited even on panic during SmallVec::drop.

Using NoDrop incurs the overhead of its enum tag and its drop flag.

Fixes servo#14
bluss added a commit to bluss/rust-smallvec that referenced this issue Aug 20, 2015
bluss added a commit to bluss/rust-smallvec that referenced this issue Aug 20, 2015
The summary is: SmallVec::drop first attempts to drop every element,
then it inhibits the drop of the inner array. The panic safety issue is
that a panic during drop of an element means the inhibition is never
reached, so the inner data can be dropped again.

NoDrop acts as a panic guard; its drop will trigger on panic, so the
inner array's drop will be inhibited even on panic during SmallVec::drop.

Using NoDrop incurs the overhead of its enum tag and its drop flag.

Fixes servo#14
@bluss
Copy link
Contributor Author

bluss commented Aug 20, 2015

bluss/arrayvec/pull/4 shows an alternative way to solve it, but it is fragile due to bug rust-lang/rust/issues/27906

@bluss
Copy link
Contributor Author

bluss commented Aug 20, 2015

I guess any way to solve it is fragile 😦 due to panic-on-drop semantics not being completely clear.

bluss added a commit to bluss/rust-smallvec that referenced this issue Aug 21, 2015
The summary is: SmallVec::drop first attempts to drop every element,
then it inhibits the drop of the inner array. The panic safety issue is
that a panic during drop of an element means the inhibition is never
reached, so the inner data can be dropped again.

If Drop is split betweeen SmallVec and SmallVecData, this issue is
avoided because the SmallVecData drop will be called even in the panic
case.

This solution incurs the overhead of an additional drop flag on
SmallVecData.

Fixes servo#14
bors-servo pushed a commit that referenced this issue Aug 21, 2015
Fix panic safety issue in drop

The summary is: SmallVec::drop first attempts to drop every element,
then it inhibits the drop of the inner array. The panic safety issue is
that a panic during drop of an element means the inhibition is never
reached, so the inner data can be dropped again.

If Drop is split betweeen SmallVec and SmallVecData, this issue is
avoided because the SmallVecData drop will be called even in the panic
case.

This solution incurs the overhead of an additional drop flag on
SmallVecData.

Fixes #14
bors added a commit to rust-lang-ci/rust that referenced this issue Sep 12, 2021
Add -Z panic-in-drop={unwind,abort} command-line option

This PR changes `Drop` to abort if an unwinding panic attempts to escape it, making the process abort instead. This has several benefits:
- The current behavior when unwinding out of `Drop` is very unintuitive and easy to miss: unwinding continues, but the remaining drops in scope are simply leaked.
- A lot of unsafe code doesn't expect drops to unwind, which can lead to unsoundness:
  - servo/rust-smallvec#14
  - bluss/arrayvec#3
- There is a code size and compilation time cost to this: LLVM needs to generate extra landing pads out of all calls in a drop implementation. This can compound when functions are inlined since unwinding will then continue on to process drops in the callee, which can itself unwind, etc.
  - Initial measurements show a 3% size reduction and up to 10% compilation time reduction on some crates (`syn`).

One thing to note about `-Z panic-in-drop=abort` is that *all* crates must be built with this option for it to be sound since it makes the compiler assume that dropping `Box<dyn Any>` will never unwind.

cc rust-lang/lang-team#97
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant