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

Soundness bug? (use after move) #42729

Closed
dpc opened this issue Jun 18, 2017 · 6 comments
Closed

Soundness bug? (use after move) #42729

dpc opened this issue Jun 18, 2017 · 6 comments

Comments

@dpc
Copy link
Contributor

dpc commented Jun 18, 2017

struct MyObject {
    text: String,
}

fn do_something_owned(_o: MyObject) {}

fn main() {
    let mut my_object = MyObject { text: String::from("sometest") };

    do_something_owned(my_object);

    my_object.text = String::from("we're done");
}

Playground link: https://is.gd/3eEKsV

I would expect it to be a compile time error.

@kennytm
Copy link
Member

kennytm commented Jun 18, 2017

Seems safe. Moving my_object leaves it in an uninitialized state which can be reinitialized field-by-field.

1. If you use an additional field, you can't read that field after do_something_owned (E0382)

struct MyObject {
    text: String,
    int: u32,
}

fn do_something_owned(_o: MyObject) {}

fn main() {
    let mut my_object = MyObject { text: "1".into(), int: 2 };
    do_something_owned(my_object);
    my_object.text = "3".into();
    println!("{:?}", my_object.int);
    //~^ ERROR: use of moved value: `my_object.int`
}

2. If the type impl Drop, you can't use my_object after the do_something_owned (E0383)

struct MyObject {
    text: String,
}

impl Drop for MyObject {
    fn drop(&mut self) {}
}

fn do_something_owned(_o: MyObject) {}

fn main() {
    let mut my_object = MyObject { text: "1".into() };
    do_something_owned(my_object);
    my_object.text = "3".into();
    //~^ ERROR: partial reinitialization of uninitialized structure `my_object`
}

@ishitatsuyuki
Copy link
Contributor

While it's safe, it doesn't seem to be a well looking behavior. Assigning all fields back doesn't create a working object (which is correct because structs can't be self referencing), and it's basically like an decomposed object.

@petrochenkov
Copy link
Contributor

Not a bug, borrow and move/initialization checkers decompose structs/enums/unions into "elementary fragments" and analyze each of these fragments individually.

Assigning all fields back doesn't create a working object

This is a limitation of the current implementation and it would be useful to fix it, especially for unions.

Here's what happens in the example, step by step:

fn main() {
    // my_object becomes initialized
    // ->
    // my_object.text becomes initialized
    let mut my_object = MyObject { text: String::from("sometest") };

    // my_object becomes uninitialized
    // ->
    // my_object.text becomes uninitialized
    do_something_owned(my_object);

    // my_object.text becomes initialized
    // ->
    // my_object becomes initialized (UNIMPLEMENTED)
    my_object.text = String::from("we're done");
}

Note, that the last assignment to my_object.text doesn't run it's destructor because my_object.text transitions from "uninitialized" to "initialized", not from "initialized" to "initialized", so it's safe.

If the type impl Drop, you can't use my_object after the do_something_owned

I'm relatively sure this is an artificial restriction (probably to simplify implementation, especially dynamic drop flags), with general rule "every fully initialized fragment is dropped, all other fragments are not" it becomes redundant.

@djzin
Copy link
Contributor

djzin commented Jun 19, 2017

I will point out here that whoever wrote the text for E0383 certainly thought this should fail to compile; maybe it did at some point:

E0383
This error occurs when an attempt is made to partially reinitialize a structure that is currently uninitialized.

For example, this can happen when a drop has taken place:

struct Foo {
    a: u32,
}

let mut x = Foo { a: 1 };
drop(x); // `x` is now uninitialized
x.a = 2; // error, partial reinitialization of uninitialized structure `t`

This actually compiles today.

On a side note - these examples in the error reference probably should be tested like doctests in every release!

@kennytm
Copy link
Member

kennytm commented Jun 19, 2017

@djzin The error index does get doc-tested, but this example is ignored so the CI don't know if it failed compilation.

rustdoc is cannot test this either, since doc test doesn't support the "compile-fail" mode (only no_run and should_panic). Apparently compile_fail does exist but just undocumented #42288.

@steveklabnik
Copy link
Member

Given that this is not a bug, I'm giving it a close. Thanks!

frewsxcv added a commit to frewsxcv/rust that referenced this issue Jun 22, 2017
…bank

Remove most "```ignore" doc tests.

Unconditional ` ```ignore ` doc tests lead to outdated examples (e.g. rust-lang#42729 (comment)). This PR tries to change all existing ` ```ignore ` tests into one of the following:

* Add import and declarations to ensure the code is run-pass
* If the code is not Rust, change to ` ```text `/` ```sh `/` ```json `/` ```dot `
* If the code is expected compile-fail, change to ` ```compile_fail `
* If the code is expected run-fail, change to ` ```should_panic `
* If the code can type-check but cannot link/run, change to ` ```no_run `
* Otherwise, add an explanation after the ` ```ignore `

The `--explain` handling is changed to cope with hidden lines from the error index.

Tidy is changed to reject any unexplained ` ```ignore ` and ` ```rust,ignore `.
frewsxcv added a commit to frewsxcv/rust that referenced this issue Jun 22, 2017
…bank

Remove most "```ignore" doc tests.

Unconditional ` ```ignore ` doc tests lead to outdated examples (e.g. rust-lang#42729 (comment)). This PR tries to change all existing ` ```ignore ` tests into one of the following:

* Add import and declarations to ensure the code is run-pass
* If the code is not Rust, change to ` ```text `/` ```sh `/` ```json `/` ```dot `
* If the code is expected compile-fail, change to ` ```compile_fail `
* If the code is expected run-fail, change to ` ```should_panic `
* If the code can type-check but cannot link/run, change to ` ```no_run `
* Otherwise, add an explanation after the ` ```ignore `

The `--explain` handling is changed to cope with hidden lines from the error index.

Tidy is changed to reject any unexplained ` ```ignore ` and ` ```rust,ignore `.
bors added a commit that referenced this issue Jun 22, 2017
Remove most "```ignore" doc tests.

Unconditional ` ```ignore ` doc tests lead to outdated examples (e.g. #42729 (comment)). This PR tries to change all existing ` ```ignore ` tests into one of the following:

* Add import and declarations to ensure the code is run-pass
* If the code is not Rust, change to ` ```text `/` ```sh `/` ```json `/` ```dot `
* If the code is expected compile-fail, change to ` ```compile_fail `
* If the code is expected run-fail, change to ` ```should_panic `
* If the code can type-check but cannot link/run, change to ` ```no_run `
* Otherwise, add an explanation after the ` ```ignore `

The `--explain` handling is changed to cope with hidden lines from the error index.

Tidy is changed to reject any unexplained ` ```ignore ` and ` ```rust,ignore `.
bors added a commit that referenced this issue Jun 22, 2017
Remove most "```ignore" doc tests.

Unconditional ` ```ignore ` doc tests lead to outdated examples (e.g. #42729 (comment)). This PR tries to change all existing ` ```ignore ` tests into one of the following:

* Add import and declarations to ensure the code is run-pass
* If the code is not Rust, change to ` ```text `/` ```sh `/` ```json `/` ```dot `
* If the code is expected compile-fail, change to ` ```compile_fail `
* If the code is expected run-fail, change to ` ```should_panic `
* If the code can type-check but cannot link/run, change to ` ```no_run `
* Otherwise, add an explanation after the ` ```ignore `

The `--explain` handling is changed to cope with hidden lines from the error index.

Tidy is changed to reject any unexplained ` ```ignore ` and ` ```rust,ignore `.
Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this issue Jun 22, 2017
…bank

Remove most "```ignore" doc tests.

Unconditional ` ```ignore ` doc tests lead to outdated examples (e.g. rust-lang#42729 (comment)). This PR tries to change all existing ` ```ignore ` tests into one of the following:

* Add import and declarations to ensure the code is run-pass
* If the code is not Rust, change to ` ```text `/` ```sh `/` ```json `/` ```dot `
* If the code is expected compile-fail, change to ` ```compile_fail `
* If the code is expected run-fail, change to ` ```should_panic `
* If the code can type-check but cannot link/run, change to ` ```no_run `
* Otherwise, add an explanation after the ` ```ignore `

The `--explain` handling is changed to cope with hidden lines from the error index.

Tidy is changed to reject any unexplained ` ```ignore ` and ` ```rust,ignore `.
Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this issue Jun 23, 2017
…bank

Remove most "```ignore" doc tests.

Unconditional ` ```ignore ` doc tests lead to outdated examples (e.g. rust-lang#42729 (comment)). This PR tries to change all existing ` ```ignore ` tests into one of the following:

* Add import and declarations to ensure the code is run-pass
* If the code is not Rust, change to ` ```text `/` ```sh `/` ```json `/` ```dot `
* If the code is expected compile-fail, change to ` ```compile_fail `
* If the code is expected run-fail, change to ` ```should_panic `
* If the code can type-check but cannot link/run, change to ` ```no_run `
* Otherwise, add an explanation after the ` ```ignore `

The `--explain` handling is changed to cope with hidden lines from the error index.

Tidy is changed to reject any unexplained ` ```ignore ` and ` ```rust,ignore `.
spikespaz pushed a commit to spikespaz/dotwalk-rs that referenced this issue Aug 29, 2024
Remove most "```ignore" doc tests.

Unconditional ` ```ignore ` doc tests lead to outdated examples (e.g. rust-lang/rust#42729 (comment)). This PR tries to change all existing ` ```ignore ` tests into one of the following:

* Add import and declarations to ensure the code is run-pass
* If the code is not Rust, change to ` ```text `/` ```sh `/` ```json `/` ```dot `
* If the code is expected compile-fail, change to ` ```compile_fail `
* If the code is expected run-fail, change to ` ```should_panic `
* If the code can type-check but cannot link/run, change to ` ```no_run `
* Otherwise, add an explanation after the ` ```ignore `

The `--explain` handling is changed to cope with hidden lines from the error index.

Tidy is changed to reject any unexplained ` ```ignore ` and ` ```rust,ignore `.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants