Change lifetime bounds to allow copying references

[vbkaisetsu]

This branch changes lifetime bounds of BorrowedMutFields to allow copying references. (fixes #118)

Struct definition:

#[self_referencing]
struct CopyRef {
    data: String,
    #[borrows(data)]
    #[covariant]
    ref1: Option<&'this str>,
    #[borrows(data)]
    #[covariant]
    ref2: Option<&'this str>,
    other: String,
}

Generated code:

pub(super) struct BorrowedMutFields<'outer_borrow, 'this2, 'this1, 'this0>
where
    'static: 'this0,
    'static: 'this1,
    'static: 'this2,
    'this2: 'this1,
    'this1: 'this2,
    'this2: 'this0,
    'this0: 'this2,
{
    pub(super) other: &'outer_borrow mut String,
    pub(super) ref2: &'outer_borrow mut Option<&'this0 str>,
    pub(super) ref1: &'outer_borrow mut Option<&'this1 str>,
    pub(super) data: &'this2 String,
}

Test:

let mut s = CopyRefBuilder {
    data: "test".to_string(),
    ref1_builder: |_| None,
    ref2_builder: |x| Some(x),
    other: "other".to_string(),
}.build();

s.with_mut(|f| {
    *f.ref2 = *f.ref1;
});
s.with_mut(|f| {
    *f.ref1 = *f.ref2;
});
s.with_mut(|f| {
    *f.ref1 = Some(f.data);
});
/* compile error
s.with_mut(|f| {
    *f.ref1 = Some(f.other);
});
*/

[someguynamedjosh]

Your example produces these errors:

error[E0597]: `s` does not live long enough
  --> scratch/src/main.rs:27:5
   |
16 |       let mut s = CopyRefBuilder {
   |           ----- binding `s` declared here
...
27 |       s.with_mut(|f| {
   |       ^ borrowed value does not live long enough
   |  _____|
   | |
28 | |         *f.ref1 = *f.ref2;
29 | |     });
   | |______- argument requires that `s` is borrowed for `'static`
...
36 |   }
   |   - `s` dropped here while still borrowed

error[E0499]: cannot borrow `s` as mutable more than once at a time
  --> scratch/src/main.rs:30:5
   |
27 |       s.with_mut(|f| {
   |       - first mutable borrow occurs here
   |  _____|
   | |
28 | |         *f.ref1 = *f.ref2;
29 | |     });
   | |______- argument requires that `s` is borrowed for `'static`
30 |       s.with_mut(|f| {
   |       ^ second mutable borrow occurs here

Try updating your test case to reflect the usage in your example.

[vbkaisetsu]

@someguynamedjosh I added a new test case.

[vbkaisetsu]

$ cargo test
...
test ok_tests::copy_ref1 ... ok
test ok_tests::copy_ref2 ... ok

[someguynamedjosh]

My bad, I forgot to switch away from the main branch after cloning your repo.

[someguynamedjosh]

With this change, I can do the following:

use ouroboros::self_referencing;

#[self_referencing]
struct CopyRef {
    data: String,
    #[borrows(data)]
    #[covariant]
    ref1: Option<&'this str>,
    other: String,
    #[borrows(data, other)]
    #[covariant]
    ref2: Option<&'this str>,
}
fn main() {
    println!("Hello, world!");
    let mut s = CopyRefBuilder {
        data: "test".to_string(),
        ref1_builder: |_| None,
        other: "other".to_string(),
        ref2_builder: |x, _| Some(x),
    }
    .build();

    s.with_mut(|f| {
        *f.ref2 = Some(f.other);
        *f.ref1 = *f.ref2;
    });

    drop(s);
}

Which triggers a use-after-free.

[someguynamedjosh]

It's worth noting that these lifetime bounds:

    'this2: 'this1,
    'this1: 'this2,

Require that 'this1 and 'this2 are exactly the same lifetime, so you should just use a single 'this if that is your intention.

[vbkaisetsu]

@someguynamedjosh I fixed lifetime bounds as follows:

If the set of variables borrowed by reference a is a subset of the set of variables borrowed by reference b, then 'a: 'b.

For example:

#[self_referencing]
struct CopyRef {
    data: String,
    #[borrows(data)]
    ref1: &'this str,
    other: String,
    #[borrows(data, other)]
    ref2: &'this str,
}

In this case, {data} is a subset of {data, other}, so *ref2 = *ref1 is ok, but *ref2 = *ref1 is invalid.

[someguynamedjosh]

Thank you for continuing to look into this. It looks like this update allows the original issue again:

use ouroboros::self_referencing;

#[self_referencing]
struct CopyRef {
    data: String,
    #[borrows(data)]
    #[covariant]
    ref1: Option<&'this str>,
    other: String,
    #[borrows(other)]
    #[covariant]
    ref2: Option<&'this str>,
}

fn main() {
    println!("Hello, world!");
    let mut s = CopyRefBuilder {
        data: "test".to_string(),
        ref1_builder: |_| None,
        other: "other".to_string(),
        ref2_builder: |_| None,
    }
    .build();

    s.with_mut(|f| {
        *f.ref1 = Some(f.other);
    });

    drop(s);
}

[vbkaisetsu]

@someguynamedjosh I fixed a bug and added a new fail test. I think the code you mentioned no longer works.

[someguynamedjosh]

Sorry for the delay. Your fix does indeed solve the above issue.

I've now done some more testing and found that mutably borrowing a field that itself borrows other fields yields a compiler error since that causes the intermediate field to no longer be considered a "tail" field, excluding it from the BorrowedMutFields struct:

#[self_referencing]
struct Chained {
    data: String,
    #[borrows(data)]
    ref_a: &'this str,
    #[borrows(mut ref_a)]
    ref_b: &'this mut &'this str,
}

error[E0392]: lifetime parameter `'this1` is never used
 --> scratch/src/main.rs:3:1
  |
3 | #[self_referencing]
  | ^^^^^^^^^^^^^^^^^^^ unused lifetime parameter
  |
  = help: consider removing `'this1`, referring to it in a field, or using a marker such as `PhantomData`
  = note: this error originates in the attribute macro `self_referencing` (in Nightly builds, run with -Z macro-backtrace for more info)

If I go in and add a PhantomData whenever this comes up, I am able to do some trickery with mutable references to trigger another use-after-free:

#[self_referencing]
struct CopyRef {
    #[borrows()]
    the_ref: &'this str,
    later: String,
    #[borrows(later, mut the_ref)]
    target: &'this mut &'this str,
}

fn main() {
    let mut s = CopyRefBuilder {
        the_ref: "static",
        later: "test".to_string(),
        target_builder: |_, the_ref| the_ref,
    }
    .build();

    s.with_mut(|f| {
        **f.target = f.later;
    });
    drop(s); // "later" dropped before "the_ref".
}

This is the field I added to the generated BorrowMutFields struct to get this particular example to compile:

phantom: ::core::marker::PhantomData<(&'this0 mut (), &'this1 mut (), &'this2 mut ())>,