Segfault (use-after-free) in ty::lookup_field_type when pattern-matching structs

[bblum]

In very lucky situations (dependent on file size, file name, comments, compiler flags), when compiling a struct pattern, rustc will segfault outright. In all situations, it will use-after-free.

Run rustc under valgrind on this code:

// #1: "x: T" produces 3x invalid reads.
// #2: "x: ()" produces 1x invalid read (only the first one listed).
// #3: Making everything monomorphic at "()" is the same as #2.
struct Crash<T> { x: T; }

fn unwrap_crash<T>(+c: Crash<T>) {
    let Crash { x: _ } = c;
    // Exactly the same with: match c { Crash { x: _ } => { } }
    // If you write "Crash { _ }" it doesn't do any invalid reads
}

fn main() { } 

You should see:

==26425== Invalid read of size 8
==26425==    at 0x691AB35: middle::ty::subst
==26425==    by 0x68DEFE6: middle::ty::lookup_field_type
==26425==    by 0x6AAFA5E: middle::typeck::check::alt::check_pat
==26425==    by 0x6AA7188: _ZN6middle6typeck5check3alt9check_alt4anonE410
==26425==    by 0x6B42D96: middle::typeck::check::check_expr_with_unifier
==26425==  Address 0xa4ae580 is 32 bytes inside a block of size 56 free'd
==26425== 
==26425== Invalid read of size 8
==26425==    at 0x6A22C6A: middle::ty::subst::do_subst
==26425==    by 0x691AB62: middle::ty::subst
==26425==    by 0x68DEFE6: middle::ty::lookup_field_type
==26425==    by 0x6AAFA5E: middle::typeck::check::alt::check_pat
==26425==    by 0x6AA7188: _ZN6middle6typeck5check3alt9check_alt4anonE410
==26425==    by 0x6B42D96: middle::typeck::check::check_expr_with_unifier
==26425==  Address 0xa4ae580 is 32 bytes inside a block of size 56 free'd
==26425== 
==26425== Invalid read of size 8
==26425==    at 0x6A22C74: middle::ty::subst::do_subst
==26425==    by 0x691AB62: middle::ty::subst
==26425==    by 0x68DEFE6: middle::ty::lookup_field_type
==26425==    by 0x6AAFA5E: middle::typeck::check::alt::check_pat
==26425==    by 0x6AA7188: _ZN6middle6typeck5check3alt9check_alt4anonE410
==26425==    by 0x6B42D96: middle::typeck::check::check_expr_with_unifier
==26425==  Address 0xa4ae590 is 48 bytes inside a block of size 56 free'd

[bblum]

here is disassembly that shows where the first invalid read is: http://pastebin.mozilla.org/1760806
here is disassembly for the second and third: http://pastebin.mozilla.org/1760805

[bblum]

I wonder if we ought to back out any snapshots that have been taken since the new struct syntax was introduced, if that's really what it is. I wouldn't trust any code compiled with arbitrary values read from the heap.

[nikomatsakis]

Do we have any idea what the bug is, really?

[eholk]

I had a use after free bug the other day that was related to moving out of an enum doing the wrong thing. I don't know if that would be at all related here though.

[bblum]

this reminds me of the use after free we ran into in icfp when using eric's unsafe move_it! macro. it could be that, or something related with move.

@eholk what was it? maybe rustc is doing the same thing.

[bblum]

The first use-after-free comes on the line if substs.tps.len() == 0u { in substs_is_noop. Somehow the argument substs: &substs to do_subst outlived its loan.

[bblum]

Found the root of it. In middle/typeck/check/alt.rs, we have:

237         let substitutions;
238         match structure_of(fcx, pat.span, expected) {
239             ty::ty_class(cid, ref substs) => {
241                 substitutions = substs;
243             }
251         }

And then later:

280         for fields.each |field| {
281             match field_map.find(field.ident) {
282                 some(index) => {
284                     let field_type = ty::lookup_field_type(tcx,
285                                                            class_id,
286                                                            class_field.id,
287                                                            substitutions);

This shouldn't have compiled. Or maybe it should have, and the ty::ty_class should have been forced to live longer?

[nikomatsakis]

actually, I think this is the question that @graydon and I have been wrestling with. borrowck right now permits rvalues to live longer than you might expect---up to the surrounding fn or loop, but I don't think trans respects it (as I mentioned). The question is, who is wrong? I was going to go and patch up trans, but perhaps the bug is that borrowck is too liberal.

[bblum]

minimized test case - valgrind reports this also does a use-after-free:

fn main() {
    let msg;
    match some(~"Hello") {
        some(ref m) => {
            msg = m;
        },  
        none => { fail }
    }   
    io::println(*msg);
}

[bblum]

Opened #3217 so I can close this one with a workaround.