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

Expressions in block tail supposed to outlive block variables dropped early. #33490

Closed
eddyb opened this Issue May 7, 2016 · 8 comments

Comments

Projects
None yet
4 participants
@eddyb
Copy link
Member

eddyb commented May 7, 2016

Found while investigating the remaining wrong case in #32433. Effectively use-after-free.

Can be used to cause a segmentation fault (check out on playpen):

struct Fine<T: std::fmt::Debug>(T);

impl<T: std::fmt::Debug> Drop for Fine<T> {
    fn drop(&mut self) {
        print!("Fine({:?}) ", self.0);
    }
}

enum Evil<'a, T: 'a+std::fmt::Debug> {
    None,
    Some(&'a Fine<T>)
}

impl<'a, T: std::fmt::Debug> Drop for Evil<'a, T> {
    fn drop(&mut self) {
        if let Evil::Some(x) = *self {
            print!("Evil({:?}) ", x.0);
        }
    }
}

// Allocates a new 3-usize Box on drop.
struct Dirty;
impl Drop for Dirty {
    fn drop(&mut self) {
        let _ = Box::new((1usize, 1usize, 1usize));
    }
}

fn assign<'a, T: std::fmt::Debug>(x: &mut Evil<'a, T>, _: &Dirty, y: &'a Fine<T>) {
    *x = Evil::Some(y);
}

fn main() {
    let mut _x = Evil::None;
    assign(&mut _x, &Dirty, &Fine(Box::new(vec![1, 2, 3])))
}

The Fine pointee drops before the Evil holding of the pointer, and in between those the destructor of Dirty overwrites the same heap location of the deallocated Box<Vec<i32>> with a different 3-usize Box, causing a segmentation fault on Debug mode and garbage to be printed on Release mode.

EDIT: Works fine even with old trans if there is any sort of block nesting (i.e. only the outermost block of a function has the broken behavior) - check out on playpen.

cc @rust-lang/compiler @rust-lang/lang

@eddyb

This comment has been minimized.

Copy link
Member Author

eddyb commented May 9, 2016

Apparently the rule we check is intentional, see #21114.
That means this is an old trans bug, as @arielb1 suspected.

@eddyb eddyb changed the title Expressions in block tail incorrectly assumed to outlive block variables. Expressions in block tail supposed to outlive block variables dropped early. May 11, 2016

@eddyb

This comment has been minimized.

Copy link
Member Author

eddyb commented May 11, 2016

Updated to reflect that borrowck enforces the semantics we want to support, even though old trans doesn't respect them.
#33239 brings MIR to the correct behavior, so it's not an immediate priority to fix old trans.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented May 23, 2016

@eddyb is the remaining bug here just that old trans does it wrong?

@eddyb

This comment has been minimized.

Copy link
Member Author

eddyb commented May 24, 2016

@nikomatsakis Correct, as everyone seems to agree that while the scoping semantics regionck/borrowck enforce may appear strange, they're intended.

@mbrubeck

This comment has been minimized.

Copy link
Contributor

mbrubeck commented Nov 3, 2016

is the remaining bug here just that old trans does it wrong?

Correct

Then since old trans doesn't exist anymore, should this be closed?

@eddyb

This comment has been minimized.

Copy link
Member Author

eddyb commented Nov 3, 2016

Indeed.

@eddyb eddyb closed this Nov 3, 2016

@Eh2406

This comment has been minimized.

Copy link
Contributor

Eh2406 commented Sep 3, 2017

There is a FIXME related to this issue,
https://github.com/rust-lang/rust/blob/master/src/test/run-pass/issue-23338-ensure-param-drop-order.rs#L67
Now that the issue is closed can the FIXME be fix, or made more specific?

@eddyb

This comment has been minimized.

Copy link
Member Author

eddyb commented Sep 4, 2017

Eh2406 added a commit to Eh2406/rust that referenced this issue Sep 5, 2017

Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this issue Sep 6, 2017

Rollup merge of rust-lang#44326 - Eh2406:FIXME#44590, r=arielb1
rust-lang#33490 is closed remove the FIXME

let's see if this can be cleaned up.

rust-lang#33490 (comment)

Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this issue Sep 7, 2017

Rollup merge of rust-lang#44326 - Eh2406:FIXME#44590, r=arielb1
rust-lang#33490 is closed remove the FIXME

let's see if this can be cleaned up.

rust-lang#33490 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.