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

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

Open
gnzlbg opened this issue May 7, 2019 · 4 comments
Open

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

gnzlbg opened this issue May 7, 2019 · 4 comments
Labels
A-destructors Area: Destructors (`Drop`, …) A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools C-enhancement Category: An issue proposing an enhancement or a PR with one. I-needs-decision Issue: In need of a decision. T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@gnzlbg
Copy link
Contributor

gnzlbg commented May 7, 2019

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
Copy link
Contributor

tesuji commented May 7, 2019

@rustbot modify labels: T-lang T-doc

@rustbot rustbot added A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools T-lang Relevant to the language team, which will review and decide on the PR/issue. labels May 7, 2019
@jonas-schievink jonas-schievink added A-destructors Area: Destructors (`Drop`, …) C-enhancement Category: An issue proposing an enhancement or a PR with one. labels May 7, 2019
@Centril
Copy link
Contributor

Centril commented May 7, 2019

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

@frewsxcv
Copy link
Member

frewsxcv commented May 8, 2019

Previous issue: #50765

And also: rust-lang/reference#348

@tmandry
Copy link
Member

tmandry commented May 16, 2019

Related: #60840 (comment)

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.

@lolbinarycat lolbinarycat added I-needs-decision Issue: In need of a decision. WG-ucg labels Sep 9, 2024
@saethlin saethlin removed the WG-ucg label Oct 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-destructors Area: Destructors (`Drop`, …) A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools C-enhancement Category: An issue proposing an enhancement or a PR with one. I-needs-decision Issue: In need of a decision. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

9 participants