rust-lang / rust

Empowering everyone to build reliable and efficient software.
https://www.rust-lang.org
Other
97.96k stars 12.69k forks source link

Behavior of panicking Drop::drop is not properly documented #60611

Open gnzlbg opened 5 years ago

gnzlbg commented 5 years ago

It was decided in, I think, #14875, that Drop::drop can panic, and if this happens, the value must be leaked (at least in a generic context), that is, it cannot be re-dropped again and doing that could invoke UB (that's at least what generic unsafe code needs to assume).

This does not appear to be documented anywhere. These semantics make the following snippet have undefined behavior due to double-drops (playground uses T = Vec<HasDrop>):

let mut a: T;
let a_raw = &mut a as *mut  T;

match std::panic::catch_unwind(|| {
    // Drop `a` in place:
    unsafe { std::ptr::drop_in_place(a_raw) };
}) {
    // For exposition only, if the value isn't leaked,
    // it will be re-dropped, but one could also try to 
    // explicitly re-drop on drop failure.
    // UB: this will double-drop some previously dropped fields of a
    Err(_) => std::mem::drop(a),
    // If dropping succeeds, leak a
    Ok(()) => std::mem::forget(a),
}

To avoid UB, that snippet must be changed to unconditionally leak the value independently of whether drop_in_place succeeded or failed:

let mut a: T;
let a_raw = &mut *a as *mut T;

std::panic::catch_unwind(|| {
    // Drop `a` in place:
    unsafe { std::ptr::drop_in_place(a_raw) };
});
// Always leak `a`: if dropping failed, some elements will be leaked,
// but there is no way to properly drop them and trying to re-drop
// a could invoke UB
 std::mem::forget(a);

cc @Centril - this might be a T-lang issue, I don't know the best way to word this, and I can't find any RFC designing this part of the language.

tesuji commented 5 years ago

@rustbot modify labels: T-lang T-doc

Centril commented 5 years ago

cc @rust-lang/wg-unsafe-code-guidelines @rust-lang/lang

frewsxcv commented 5 years ago

Previous issue: https://github.com/rust-lang/rust/issues/50765

And also: https://github.com/rust-lang-nursery/reference/issues/348

tmandry commented 5 years ago

Related: https://github.com/rust-lang/rust/pull/60840#issuecomment-492435720

The assumption this PR is making is that once [MIR] drop returns to a function, whether it succeeds or panics, it is UB for the function to then access that local.