rvalue destructors inside of a bare block run at the wrong time

[sfackler]

** UPDATE by @nikomatsakis **

This is almost-but-not-quite a dup of #8861, see this comment for details.

** ORIGINAL POST **

This program:

struct Foo { a: int }

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

fn main() {
    {
        let _1 = Foo { a: 1 };
        let _2 = Foo { a: 2 };
        match Foo { a: 3 } {
            _ => {}
        }
    }
    let _4 = Foo { a: 4 };
}

should output

3
2
1
4

but instead outputs

2
1
3
4

It appears that if the bare block around the first part of main is removed, everything runs in the correct order. Moving the "3" Foo into a variable also makes everything run in the right order.

This was originally reported to me in sfackler/rust-postgres#31 as a segfault, but it looks like that's probably just due to heap corruption from a use-after-free caused by this bug.

Update

IR:

join:                                             ; preds = %case_body
  call void @_ZN3Foo14glue_drop.146917h02f639dd107d06daE(%struct.Foo* %_2)
  call void @_ZN3Foo14glue_drop.146917h02f639dd107d06daE(%struct.Foo* %_1)
  call void @_ZN3Foo14glue_drop.146917h02f639dd107d06daE(%struct.Foo* %0) ; <-- 3
  %7 = getelementptr inbounds %struct.Foo* %_4, i32 0, i32 1
  store i8 1, i8* %7
  %8 = getelementptr inbounds %struct.Foo* %_4, i32 0, i32 0
  store i64 4, i64* %8
  call void @_ZN3Foo14glue_drop.146917h02f639dd107d06daE(%struct.Foo* %_4)
  ret void

[flaper87]

This sounds bad. Nominating

EDIT: Looks like something may be wrong in rustc::middle::region::resolve_expr for ExprMatch.

[flaper87]

cc @nikomatsakis

[sfackler]

This also pops up in for loops (see the original issue on rust-postgres), so I don't believe it's an issue exclusively with ExprMatch. Nevermind, I forgot that the for sugar wraps in a single branch match.

[nikomatsakis]

This is actually the expected behavior (if you fully grok the rules), but it's an interesting side-effect.

[nikomatsakis]

@sfackler I'd like to know more about the details of the use-after-free.

[nikomatsakis]

The reason this occurs is that:

1. The match introduces a temporary.
2. The match occurs as the tail expression in a block.
3. The lifetime of temporaries in the tail expression of a block is equal to the innermost enclosing statement -- in this case, that's the outer block.

[sfackler]

@nikomatsakis In the example in sfackler/rust-postgres#31, the iterator has a reference to the statement, which in turn has a reference to the connection. The destructor for the iterator uses the connection to clean up the associated state on the server. But, since the connection has already been dropped, it ends up writing to a freed buffer, and then tries to send on a closed socket.

[sfackler]

If the destructor order is working as intended, it seems like the bug may actually be in the lifetime verification infrastructure, since the lifetime of the iterator type should not be allowed to outlive the statement, and transitively the connection.

[nikomatsakis]

That's what I'm trying to understand. If there is an unsound interaction here. I can imagine where something confusing might crop up, I have to read and better understand the example.

[sfackler]

Here's a very simple version of the types used in that example:

struct Connection {
    conn: BufferedStream<TcpStream>
}

struct Statement<'conn> {
    conn: &'conn Connection
}

struct Rows<'stmt> {
    stmt: &'stmt Statement<'stmt>
}

impl<'stmt> Drop for Rows<'stmt> {
    fn drop(&mut self) {
        self.stmt.conn.conn.write("drop my server side state please".as_bytes());
        self.stmt.conn.conn.flush();
    }
}

[nikomatsakis]

@sfackler right, so yes I think I see the problem. It is a shortcoming of regionck basically. We need to say something like "if we produce a value whose destructor will run at the end of scope 'a, then all references within must outlive 'a". cc #8861, since this is basically what the rules I propose are there.

In general, things with destructors that contain references are currently tagged as #[unsafe] for a reason :) but I agree this should be fixed. (And once it is, we can hopefully remove all these #[unsafe_dtor] attributes)

[pnkfelix]

Given that #8861 is already on the 1.0 milestone as P-backcompat-lang, this is "just a bug", and need not be given a milestone marking itself.

(if we were to revisit the decision regarding #8861, then we might need to revisit this bug as well, but I am assuming that will not happen.)

Not marking with any triage markers.

[brson]

Nominating.

[pnkfelix]

Same reasoning as April 3rd applies (@nikomatsakis says it is in fact a dupe); not marking with any milestone marker.

[nikomatsakis]

Closing as dup of #8861