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

Undetected use after move #18571

Closed
pepijndevos opened this issue Nov 3, 2014 · 6 comments · Fixed by #22219
Closed

Undetected use after move #18571

pepijndevos opened this issue Nov 3, 2014 · 6 comments · Fixed by #22219
Assignees
Milestone

Comments

@pepijndevos
Copy link

So I was curious how Drop handles cycles, because this could lead to dangling pointers, which is why __del__ might not be called in Python. And indeed the below example does not call Drop.

But t is moved into u, so why does this compile at all?

struct Test {
    a: int,
    b: Option<Box<Test>>,
}

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

fn stuff() {
    let mut t = Test { a: 1, b: None};
    let mut u = Test { a: 2, b: Some(box t)};    
    t.b = Some(box u);
    println!("done");
}

fn main() {
    stuff();
    println!("Hello, world!")
}
@alexcrichton
Copy link
Member

Nominating, seems like a soundness hole.

@arielb1
Copy link
Contributor

arielb1 commented Nov 3, 2014

This is an issue with zeroing dynamic drops (which are going to be removed: rust-lang/rfcs#320) – when t is moved, its drop flag is cleared, which prevents the destructor of t.b from running.

A clearer example is:

struct Test;

struct Test2 {
    b: Option<Test>,
}

impl Drop for Test {
    fn drop(&mut self) {
        println!("Dropping");
    }
}

#[cfg(leaks)]
impl Drop for Test2 {
    fn drop(&mut self) {}
}

fn stuff() {
    let mut t = Test2 { b: None };
    let u = Test;
    drop(t);
    t.b = Some(u);
}

fn main() {
    stuff();
}

@nikomatsakis
Copy link
Contributor

Structs that implement drop are not supposed to be in a partially initialized state. We already prevents moves out from individual fields, but we also need to prevent individual fields from being re-initialized.

cc @zwarich since (iirc?) he was working on the patches that made us more accepting around re-initializing fields and so forth, and maybe this bug is of interest to him.

@pnkfelix
Copy link
Member

pnkfelix commented Nov 6, 2014

So we are not supposed to allow structs that implement drop to be partially initialized.

We already disallow going from fully-initialized to partially initialized.

But it seems (from the first example) that we missed disallowing going from fully-uninitialized to partially initializd. That needs to be fixed.

Assigning P-backcompat-lang, 1.0.

@pnkfelix pnkfelix added this to the 1.0 milestone Nov 6, 2014
pcwalton added a commit to pcwalton/rust that referenced this issue Nov 14, 2014
enumerations that implement the `Drop` trait.

This breaks code like:

    struct Struct {
        f: String,
        g: String,
    }

    impl Drop for Struct { ... }

    fn main() {
        let x = Struct { ... };
        drop(x);
        x.f = ...;
    }

Change this code to not create partially-initialized structures. For
example:

    struct Struct {
        f: String,
        g: String,
    }

    impl Drop for Struct { ... }

    fn main() {
        let x = Struct { ... };
        drop(x);
        x = Struct {
            f: ...,
            g: ...,
        }
    }

Closes rust-lang#18571.

[breaking-change]
@pnkfelix
Copy link
Member

pnkfelix commented Feb 8, 2015

While this will hopefully be fixed soon, as there is a PR up, it seems to have fallen through the cracks as a P-backcompat-lang issue that is only on the 1.0 milestone and not 1.0 beta.

Nominating for 1.0 beta. (Also, I guess I will try to run through and find any other such overlooked issues.)

pnkfelix pushed a commit to pnkfelix/rust that referenced this issue Feb 12, 2015
enumerations that implement the `Drop` trait.

This breaks code like:

    struct Struct {
        f: String,
        g: String,
    }

    impl Drop for Struct { ... }

    fn main() {
        let x = Struct { ... };
        drop(x);
        x.f = ...;
    }

Change this code to not create partially-initialized structures. For
example:

    struct Struct {
        f: String,
        g: String,
    }

    impl Drop for Struct { ... }

    fn main() {
        let x = Struct { ... };
        drop(x);
        x = Struct {
            f: ...,
            g: ...,
        }
    }

Closes rust-lang#18571.

[breaking-change]

----

(Joint authorship by pcwalton and Ryman; thanks all!)
Ryman pushed a commit to Ryman/rust that referenced this issue Feb 12, 2015
enumerations that implement the `Drop` trait.

This breaks code like:

    struct Struct {
        f: String,
        g: String,
    }

    impl Drop for Struct { ... }

    fn main() {
        let x = Struct { ... };
        drop(x);
        x.f = ...;
    }

Change this code to not create partially-initialized structures. For
example:

    struct Struct {
        f: String,
        g: String,
    }

    impl Drop for Struct { ... }

    fn main() {
        let x = Struct { ... };
        drop(x);
        x = Struct {
            f: ...,
            g: ...,
        }
    }

Closes rust-lang#18571.

[breaking-change]
@pnkfelix
Copy link
Member

Assigning 1.0 beta.

@pnkfelix pnkfelix modified the milestones: 1.0 beta, 1.0 Feb 12, 2015
@pnkfelix pnkfelix self-assigned this Feb 12, 2015
bors added a commit that referenced this issue Feb 13, 2015
borrowck: Prevent partial reinitialization of uninitialized structures

This is a pnkfelix-swiped squash of #22079, which was a rebase and revision of #18963

Fixes #18571.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment