Lifetimes can escape in traits / objects

[nikomatsakis]

This code from the io library is definitely wrong as the lifetime of the parameter s is lost:

pub fn with_str_reader<T>(s: &str, f: &fn(@Reader) -> T) -> T {
    str::byte_slice(s, |bytes| with_bytes_reader(bytes, f))
}

The lifetime of the s is lost. I have a fix for this in a branch I want to land (currently commented out with a FIXME) but I don't want to turn on the check because to do this right we need & objects working. I'm not even sure where to insert strategic transmutes to get with_str_reader working.

This signature for example should really be:

pub fn with_str_reader<T>(s: &str, f: &fn(&Reader) -> T) -> T {...}

or even

pub fn str_reader<'a, T>(s: &'a str) -> &'a Reader {...}

There is a test that was supposed to be defending against this case, but it was basically accidentally reporting an error.

[metajack]

Is this bug still relevant? Did the fix get into the new borrowck?

cc: @brson

[nikomatsakis]

I suspect it is still relevant. I'll have to dig back into the details to remember.

[pnkfelix]

I want to elaborate a little for future bug reviewers, because I did not understand what Niko meant by "the lifetime of the s is lost" in the description.

High-level explanation:

Consider:

let s = ~"input"; return with_str_reader(s, consumer);

This will invoke consumer with a reader object that should read the input s. Presumably that reader will not copy the state of the string to its internal state, but instead will hold onto a reference to the string. The problem is that since the signature says that reader argument is a @Reader, nothing is keeping the consumer closure from stashing a reference to that reader somewhere else that will outlive the lifetime of s. (Or just returning the reader outright.)

In theory, the implementation of with_str_reader could copy the contents of the string in order to enable the reader to outlive the s; it would be silly, but the point is that the signature itself here is not fundamentally "wrong" in a way that the borrow-checker could detect. Rather, the signature is wrong in the sense that it makes the obvious implementation unsound; it [over-]promises to provide a @Reader when it really should provide something more restricted.

Concrete example

fn main() {
    fn its97<T>(f: &fn(@Reader) -> T) -> T {
        let s = std::str::from_bytes([97]);
        std::io::with_str_reader(s, f)
    }

    fn consumer(r: @std::io::Reader) -> int { r.read_byte() }
    println(fmt!("%d", its97(consumer)));

    let long_lived_reader = its97(|r| r);
    println(fmt!("%d", consumer(long_lived_reader)));
}

Output:

% ~/Dev/Rust/Issues/iss5723-a
97
0

(We're "lucky" that it didn't crash, rather than returning the arbitrary value 0.)

When this bug is fixed, the above example code should be rejected, as well as any variant that lets the reader flow to the long_lived_reader binding beyond the lifetime of the string created within its97.

The solutions

Niko's suggested alternative signatures properly reflect the intended implementation.

In the first,

pub fn with_str_reader<T>(s: &str, f: &fn(&Reader) -> T) -> T {...}

the reader is itself passed as a &ref, so its dynamic extent is bounded, and it should be impossible for the consumer closure to store that value somewhere else. In the second,

pub fn str_reader<'a, T>(s: &'a str) -> &'a Reader {...}

the reader is again a borrowed reference; this time the reader's lifetime explicitly tied to the lifetime of s, so again it will be impossible for one to retain a reference to the reader that outlives the dynamic extent of s.

Implementation-oriented details:

I had thought, after reading the description and talking to Niko, the implementation of with_str_reader must be using an unsafe block to let the @Reader it creates hold a reference to the bytes via a &ref into the borrowed s. But upon inspection, I see no unsafe block in the implementation:

pub fn with_bytes_reader<T>(bytes: &[u8], f: &fn(@Reader) -> T) -> T {
    f(@BytesReader {
        bytes: bytes,
        pos: @mut 0
    } as @Reader)
}

pub fn with_str_reader<T>(s: &str, f: &fn(@Reader) -> T) -> T {
    with_bytes_reader(s.as_bytes(), f)
}

I plan to check with Niko about this. My current hypothesis is that the borrow-checker should be rejecting the code above, and when Niko says "I have a fix for this in a branch", I believe is is referring to a fix to the borrow-checker so that this code is rejected. But since & objects did not work at the time when he posted the bug (and may still not work), we cannot switch to the variants listed in "The solutions", which both use & objects (the &Reader).

[nikomatsakis]

One minor correction: the bug is not, technically, in the borrow checker, but rather the regionck code, whose job it is to guarantee that regions do not "leak" into closure and object types.

[nikomatsakis]

See this comment on the cause: https://github.com/mozilla/rust/pull/7248/files#r4805965

[bblum]

the best exploit i can do with this is reading bytes off the heap.

20:04:55 < bblum> rusti: use std::task; use std::io; fn f() -> @io::Reader { let mut y = None;
                  let x = ~[0xcdu8, ..64]; do io::with_bytes_reader(x) |r| { y = Some(r); };
                  y.unwrap() } let y = f(); do task::spawn {}; let mut z = [0, ..32];
                  y.read(z, 32); z
20:04:57 -rusti:#rust- [1, 0, 0, 0, 0, 0, 0, 0, 128, 207, 141, 113, 98, 127, 0, 0, 64, 27, 32, 104,
                        98, 127, 0, 0, 0, 9, 32, 104, 98, 127, 0, 0]

i suspect you could get it to crash if you handed it a slice to a vector that was on a page that later becomes unmapped from the address space. almost certainly not coerce.

[pnkfelix]

Sigh. Some piece of the infrastructure only saw the "fix NNNN" in the phrase "does not fix NNNN".

[bblum]

wow sighhhh

[emberian]

triage bump; nothing to add.

[bblum]

At this point it is just the with_bytes_reader interface that needs to be fixed, and not the type system.

[glaebhoerl]

@bblum really? I can still do this:

fn main() {
    let blah;
    {
        let ss: &[~str] = ~[~"sup?"];
        blah = ~ss as ~ToStr:;
    }
    let oh_noes = ~[666];
    println!("{}", blah.to_str());
}

$ rustc 5723.rs && echo Running: && ./5723
5723.rs:7:8: 7:15 warning: unused variable: `oh_noes` [-W unused-variable (default)]
5723.rs:7     let oh_noes = ~[666];
                  ^~~~~~~
5723.rs:4:26: 4:36 warning: unnecessary allocation, the sigil can be removed [-W unnecessary-allocation (default)]
5723.rs:4         let ss: &[~str] = ~[~"sup?"];
                                    ^~~~~~~~~~
Running:
Segmentation fault

[bblum]

oh you are right, my mistake. there is still the lifetimes problem, where traits need to be able to express someething like ~Trait:'a.

[alexcrichton]

Nominating for the same treatment of #9745 (high, 1.0)

I would close #9745 if this gets on a milestone.

[nikomatsakis]

Been working on this. Highly related to #8861

[nikomatsakis]

This seems to be coming up a lot. I'll try to get a writeup of my thoughts shortly. Lots to write lately!

[nikomatsakis]

Closing in favor of #16462