Nested return expressions can cause drop to run on incorrect values.

[luqmana]

Came across this while working on inlining enum constructors but I could reproduce on master as well:

#![allow(dead_code)]

struct Foo {
    a: int
}

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

struct Bar {
    x: Foo,
    y: int
}

fn foo() -> Bar {
    return Bar {
        x: Foo { a: 23 },
        y: return Bar {
            x: Foo { a: 32 },
            y: 0
        }
    };
}

fn main() {
    unsafe {
        std::mem::forget(foo());
    }
}

When compiled and run this will print out:

Dropping Foo: 32

This is wrong since the Foo we're actually dropping at that point (before returning in foo) is the one with it's field set to 23. The problem is that we translate return expressions to write directly into the return pointer (either a hidden outpointer or a temporary alloca which we'll return from). So with the outer return we start writing out the struct but then we encounter another return in one of the fields and overwrite the previous result so now when we pass a pointer (which is just some offset into the return pointer) to the drop glue for what should be the initial Foo we're actually calling drop on the second one.

[apoelstra]

Hi, I have encountered this bug in the wild. Here it is causing a segfault:

use std::io::IoResult;
use std::io::{standard_error, InvalidInput};

fn build_vec_fail() -> IoResult<Vec<uint>> {
  Err(standard_error(InvalidInput))
}

fn build_vec() -> IoResult<Vec<uint>> {
  Ok(vec![])
}

struct MyStruct {
  field1: Vec<uint>,
  field2: Vec<uint>
}

fn real_main() -> IoResult<MyStruct> {
  // This is fine if we assign it to a variable then return the
  // variable. As written this segfaults.
  Ok(MyStruct {
    field1: try!(build_vec()),
    field2: try!(build_vec_fail()),
  })
}

fn main() { real_main(); }

A simple workaround is to assign the return value of real_main to a variable, which you return in the next line.

Edit: Oops, I posted the wrong source code! Fixed.

[huonw]

This bug causes code like

extern crate serialize;
use serialize::json;

#[deriving(Decodable)]
struct Foo {
    field1: String,
    field2: String
}

fn main() {
    let data = r##"
    {
        "field1": "something",
        "non_existing_field": "something_else"
    }
    "##;
    let _ = json::decode::<Foo>(data.as_slice());
}

to segfault. It expands to the following, which contains a return inside a struct initialiser (causing the String destructor of field1 to misbehave):

#![feature(phase)]
#![no_std]
#![feature(globs)]
#[phase(plugin, link)]
extern crate std;
extern crate native;
extern crate serialize;
use std::prelude::*;
use serialize::json;

struct Foo {
    field1: String,
    field2: String,
}
#[automatically_derived]
impl <__D: ::serialize::Decoder<__E>, __E> ::serialize::Decodable<__D, __E>
     for Foo {
    fn decode(__arg_0: &mut __D) -> ::std::result::Result<Foo, __E> {
        __arg_0.read_struct("Foo", 2u,
                            |_d|
                                ::std::result::Ok(Foo{field1:
                                                          match _d.read_struct_field("field1",
                                                                                     0u,
                                                                                     |_d|
                                                                                         ::serialize::Decodable::decode(_d))
                                                              {
                                                              Ok(__try_var) =>
                                                              __try_var,
                                                              Err(__try_var)
                                                              =>
                                                              return Err(__try_var)
                                                          },
                                                      field2:
                                                          match _d.read_struct_field("field2",
                                                                                     1u,
                                                                                     |_d|
                                                                                         ::serialize::Decodable::decode(_d))
                                                              {
                                                              Ok(__try_var) =>
                                                              __try_var,
                                                              Err(__try_var)
                                                              =>
                                                              return Err(__try_var)
                                                          },}))
    }
}

fn main() {
    let data =
        r##"
    {
        "field1": "something",
        "non_existing_field": "something_else"
    }
    "##;
    let _ = json::decode::<Foo>(data.as_slice());
}

[alexcrichton]

Nominating, decoders are pretty important!

[pcwalton]

Note that this is not P-backcompat-lang.

[pnkfelix]

Assigning P-high, 1.0 milestone.

[apoelstra]

Same story with tuples:

use std::io::{IoResult, InvalidInput, standard_error};

fn deserialize() -> IoResult<(Box<uint>, ())> {
  Ok((try!(Ok(box 0)),
      try!(Err(standard_error(InvalidInput)))))
}

fn main() {
  println!("{}", deserialize());
}

also segfaults.

Edit: Here is a maybe more enlightening test case:

struct MyStruct(Box<int>);

impl Drop for MyStruct {
  fn drop(&mut self) {
    let &MyStruct(ref n) = self;
    println!("dropping {}", n);
  }
}

fn foo() -> Result<(MyStruct, MyStruct), MyStruct> {
  Ok((MyStruct(box 0), try!(Err(MyStruct(box 1)))))
}

fn main() {
  foo();
}

This outputs dropping 1 twice and never dropping 0. Valgrind shows a double-free.

[apoelstra]

@luqmana can you rebase and retry this one? It should pass the tests with the recent TCP changes.

[luqmana]

@apoelstra yup! works now!