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

Double-drops when control jumps out of borrow of slice literal construction #30822

Closed
pnkfelix opened this issue Jan 11, 2016 · 1 comment · Fixed by #30823
Closed

Double-drops when control jumps out of borrow of slice literal construction #30822

pnkfelix opened this issue Jan 11, 2016 · 1 comment · Fixed by #30823
Labels
A-destructors Area: Destructors (`Drop`, …) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@pnkfelix
Copy link
Member

(this is related to #30018, but it is also a distinct problem in its own right.)

We appear to be creating copies of values that should be moved when doing slice literal construction.

The copies can be observed because double-drop (of the same abstract value) can occur when there is a break of control out of the slice literal during evaluation of one of the elements.

Here is an example (playpen):

struct D(&'static str);

impl Drop for D {
    fn drop(&mut self) {
        println!("Dropping D({})", self.0);
    }
}

fn main() {
    println!("Start");
    loop {
        let _r = &[D("1"),
                   D("2"),
                   { let _tmp = D("3"); break; },
                   D("4")];
    }
    println!("Finis");
}

outputs (in release mode):

Start
Dropping D(3)
Dropping D(2)
Dropping D(1)
Dropping D(1)
Dropping D(2)
Finis

The crucial point is that we see double drops of D(1) and D(2) in the output above. This would correspond, at least in theory, to deallocating a backing buffer multiple times -- that is, it is wrong. (Types like Vec probably defend themselves against such an attack since they currently use unsafe_no_drop_flag and thus should have an embedded drop flag state that it is maintaining itself, but other types that implement Drop do not necessary take that tack.)

(In debug mode there is a sigtrap at the end of that example; that is due to #30018, but again, that is not this ticket.)

@pnkfelix pnkfelix added I-wrong A-destructors Area: Destructors (`Drop`, …) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jan 11, 2016
@pnkfelix
Copy link
Member Author

One way to solve this problem would be to set the drop-flags to "dropped" after running the destructors on each of the D's the first time through.

However, I am a little surprised by the idea that this would be necessary; in particular, I would have thought that the array of D constructed via alloca would itself have no other cleanup scope associated with it, and so I don't know why the generated code thinks/knows that it needs to run the destructor for that array


Update: The previous paragraph was wrong: the cleanup for the array itself is indeed necessary, since we might panic after we've actually finished all of the elements of the array itself. (The regression test I added with PR #30823 covers this case.) I don't know if there's any way to delay the scheduling of the array cleanup until after it has been entirely initialized ... that is, some way to make so that the array cleanup is not a parent scope during the evaluation of the individual elements... anyway, the easy way to get around this is to do what I said above: Set the drop-flags to "dropped" when dropping each of the Ds' if control breaks in the midst of the array construction. (That is what PR #30823 does)

pnkfelix added a commit to pnkfelix/rust that referenced this issue Jan 13, 2016
…st-lang#30530, rust-lang#30822.

Note that the test for rust-lang#30822 is folded into the test for rust-lang#30530 (but
the file name mentions only 30530).
Manishearth added a commit to Manishearth/rust that referenced this issue Jan 14, 2016
…r-issue-30530, r=dotdash

Put back alloca zeroing for issues rust-lang#29092, rust-lang#30018, rust-lang#30530; inject zeroing for rust-lang#30822.

----

Background context: `fn alloca_zeroed` was removed in PR rust-lang#22969, so we haven't been "zero'ing" (\*) the alloca's since at least that point, but the logic behind that PR seems sound, so its not entirely obvious how *long* the underlying bug has actually been present.  In other words, I have not yet done a survey to see when the new `alloc_ty` and `lvalue_scratch_datum` calls were introduced that should have had "zero'ing" the alloca's.

----

I first fixed rust-lang#30018, then decided to do a survey of `alloc_ty` calls to see if they needed similar treatment, which quickly led to a rediscovery of rust-lang#30530.

While making the regression test for the latter, I discovered rust-lang#30822, which is a slightly different bug (in terms of where the "zero'ing" needs to go), but still relevant.

I haven't finished the aforementioned survey of `fn alloc_ty` calls, but I decided I wanted to get this up for review in its current state (namely to see if my attempt to force developers to include a justification for passing `Uninit` can possibly fly, or if I should abandon that path of action).

----

(*): I am putting quotation marks around "zero'ing" because we no longer use zero as our "dropped" marker value.

Fix rust-lang#29092
Fix rust-lang#30018
Fix rust-lang#30530
Fix rust-lang#30822
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-destructors Area: Destructors (`Drop`, …) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant