panic_in_drop.md

Summary

Panics which happen inside a Drop impl currently unwind as long as the drop was not itself called during unwinding (which would otherwise abort immediated due to a double-panic). This design meeting proposes to change this behavior to always abort if a panic attempts to escape a drop, even if this drop was invoked as part of normal function execution (as opposed to unwinding).

The dicussion on this issue dates back to 2014 (rust-lang/rust#14875) where the decision was made to support unwinding in drops but leave the door open to potentially reversing this decision.

Background reading

rust-lang/rust#86027

Specific issues

Here are several examples of how panicking in a Drop can cause issues.

Bugs in unsafe code

Unwinds from drops are often unexpected since unlike normal function calls, drops are invisible in the source code. This is an easy mistake to make, especially when writing unsafe code, and can lead to security vulnerabilities.

This has affect many popular crates and even the standard library:

• Double drop in the standard library: rust-lang/rust#83618
• Double drop in arrayvec: bluss/arrayvec#3
• The exact same double-drop bug in smallvec: servo/rust-smallvec#14

This easy source of bugs is one of the main reasons why C++11 switched the default behavior of destructors to be noexcept by default, despite C++ being normally very conservative with breaking changes.

Memory leaks

Even code which correctly handles unwinding from drops will often still leak memory when this happens. For example this is the case with hashbrown and other standard library collections such as LinkedList and BTreeMap.

Vec is not affected by this since it uses the compiler-generate Drop impl for slices which continues dropping elements even after a drop has panicked, at the cost of code size.

Code size and compilation time

Amanieu implemented a compiler option -Zpanic-in-drop=abort which causes core::ptr::drop_in_place to abort if a panic attempts unwinds out of it. Codegen was also updated so that all call site to drop_in_place are marked as nounwind so that the MIR passes and LLVM will eliminate unnecessary landing pads.

Performance results from rust-lang/rust#88759 show up to 10% reduction in compilation time (perf) and a 5MB (3%) reduction in the size of librustc_driver.so.

Exception object returned by catch_unwind

Issue rust-lang/rust#86027

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

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.

Example in the standard library (issue rust-lang/rust#86030):

struct Bomb;

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

std::panic::panic_any(Bomb);

https://github.com/rust-lang/rust/blob/5ea19239d9d6f49fdd76513a36386d7e83708e3f/library/std/src/rt.rs#L34-L39

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)

Concerns and drawbacks

The main concern is whether anyone is relying on the current behavior. I can see two main types of panic sources in drops:

• Assertions for things that should never happen, such as failures in close, munmap, pthread_mutex_destroy, etc. If any of these calls fail then something has gone seriously wrong (double-free?) and we are better off aborting immediately.
• Some APIs may run user-provided closures on drop. One example in the standard library is the unstable drain_filter API (rust-lang/rust#43244). This creates an iterator which returns items that were drained out of a collection, but will also eagerly continue draining to drop any remaining elements when the iterator is dropped. I would argue that this is a design mistake since Iterator is fundamentally designed around laziness.

Since -Zpanic-in-drop affects the ABI of drop_in_place, the entire crate graph must be compiled with the same value for this option. This leaves little room for compromise solutions: if we decide that this change is desirable then it would have to be done for all crates in a program by changing the default value of the -Zpanic-in-drop option.