Crash from iterator returning borrowed reference

[tov]

With an iterator that returns a borrowed reference to its contents, it’s possible to hold the reference longer than the reference is valid. In particular, if the iterator changes its internal state, the new state can be observed via the saved reference. Check it out:

fn crash_please() {
    let mut iter = Newtype(Some(Box::new(0)));
    let saved = iter.next().unwrap();
    println!("{}", saved);
    iter.0 = None;
    println!("{}", saved);
}

struct Newtype(Option<Box<usize>>);

impl<'a> Iterator for Newtype {
    type Item = &'a Box<usize>;

    fn next(&mut self) -> Option<&Box<usize>> {
        self.0.as_ref()
    }
}

This clearly shouldn’t type check, but it does, and runs, and crashes:

0
zsh: illegal hardware instruction  target/collect_bug

Where the type error should be, I’m not sure. It seems like it’s actually a problem with traits. If we change the above code to define next in a non-trait impl, then it no longer passes the borrow checker:

impl<'a> Newtype {
    fn next(&mut self) -> Option<&Box<usize>> {
        self.0.as_ref()
    }
}

I originally observed this bug when testing an iterator that owns a String; at each iteration it modifies the string and then returns a string slice borrowed from it. My non-test code worked fine because it finished with each value from the iterator before calling next() again, but the test code collected the iterator into a vector. This meant that all the slices in the vector continued to point to the same buffer, even after it had been modified. So for example, if the strings written to the buffer were "aaa", "bb", and "c", then in the end the vector would contain "cba", "cb", and "b". Creepy.

Version

I’m using 1.0.0-alpha still, because I’m teaching a class and we don’t want to follow a moving target, but I confirmed that the Feb. 26 nightly has the same bug.

rustc 1.0.0-alpha (44a287e6e 2015-01-08 17:03:40 -0800)
binary: rustc
commit-hash: 44a287e6eb22ec3c2a687fc156813577464017f7
commit-date: 2015-01-08 17:03:40 -0800
host: x86_64-apple-darwin
release: 1.0.0-alpha

[maxsnew]

Confirmed with

rustc 1.0.0-nightly (b47aebe3f 2015-02-26) (built 2015-02-27)
binary: rustc
commit-hash: b47aebe3fc2da06c760fd8ea19f84cbc41d34831
commit-date: 2015-02-26
build-date: 2015-02-27
host: x86_64-unknown-linux-gnu
release: 1.0.0-nightly

[edwardw]

I think @nikomatsakis comment from #22077 also applies here. Quote:

The problem is that the 'a variable on the impl is unconstrained.
We should prohibit these unconstrainted lifetime parameters, either completely, or at minimum from appearing in the value of an associated type.

[tov]

Yeah, the idea that you could return a value of type &'a Box<usize> for any 'a is clearly nonsense. But I’m not sure that’s all there is to it:

fn crash_please() {
    let mut iter = Newtype {
        option: Some(Box::new(0)),
        marker: (),
    };

    let saved = iter.next().unwrap();
    println!("{}", saved);
    iter.option = None;
    println!("{}", saved);
}

struct Newtype<'a, T: 'a> {
    option: Option<Box<usize>>,
    marker: T,
}

impl<'a, T> Iterator for Newtype<'a, T> {
    type Item = &'a Box<usize>;

    fn next(&mut self) -> Option<&Box<usize>> {
        self.option.as_ref()
    }
}

[nikomatsakis]

On Sat, Feb 28, 2015 at 05:56:36AM -0800, Edward Wang wrote:

I think @nikomatsakis comment from #22077 also applies here. Quote:

The problem is that the 'a variable on the impl is unconstrained.
We should prohibit these unconstrainted lifetime parameters, either completely, or at minimum from appearing in the value of an associated type.

I'll try to make a PR for this, I've just been debating how hard core
to go. I will probably go for a complete ban, but I do know of some
use cases for sticking extra lifetimes on impls (not good ones).

[nikomatsakis]

@tov what version of rustc are you using? I don't expect that code to compile these days, because the 'a in Newtype is unconstrained.

[nikomatsakis]

@tov see e.g. http://is.gd/6ALz11 (which yields a compilation error)

this changed as part of https://github.com/rust-lang/rfcs/blob/master/text/0738-variance.md

[nikomatsakis]

Also, if we change the example to constrain the 'a, we start to see compilation errors that the method in the impl does not match the trait (as expected): http://is.gd/bNZzB5

[pnkfelix]

Duplicate of #22077. Leaving open for tracking of this thread of conversation, but removing nomination tag.

[arielb1]

This errors correctly on nightly and stable.