Unsound cyclic re-borrow

[steffahn]

I’ll try to explain (and compare to ouroboros) tomorrow

use std::cell::RefCell;

use self_cell::self_cell;

struct Bar<'a>(RefCell<(Option<&'a Bar<'a>>, String)>);

self_cell! {
    struct Foo {
        owner: (),
        #[not_covariant]
        dependent: Bar,
    }
}

fn main() {
    let mut x = Foo::new((), |_| Bar(RefCell::new((None, "Hello".to_owned()))));
    x.with_dependent(|_, dep| {
        dep.0.borrow_mut().0 = Some(dep);
    });
    x.with_dependent_mut(|_, dep| {
        let r1 = dep.0.get_mut();
        let string_ref_1 = &mut r1.1;
        let mut r2 = r1.0.unwrap().0.borrow_mut();
        let string_ref_2 = &mut r2.1;

        let s = &string_ref_1[..];
        string_ref_2.clear();
        string_ref_2.shrink_to_fit();
        println!("{}", s); // prints garbage
    });
}

[steffahn]

Or shall we say, I’m comparing to ouroboros right now. And it’s not directly applicable, yet with some additional effort, there’s a bug to be found in ouroboros, too 🎉

use std::cell::RefCell;

use ouroboros::self_referencing;

struct Bar<'a>(RefCell<(Option<&'a Bar<'a>>, String)>);

#[self_referencing]
struct Foo {
    owner: (),
    #[borrows(owner)]
    #[not_covariant]
    bar: Bar<'this>,
    #[borrows(bar)]
    #[not_covariant]
    baz: &'this Bar<'this>,
}

impl Drop for Bar<'_> {
    fn drop(&mut self) {
        let r1 = self.0.get_mut();
        let string_ref_1 = &mut r1.1;
        let mut r2 = r1.0.unwrap().0.borrow_mut();
        let string_ref_2 = &mut r2.1;

        let s = &string_ref_1[..];
        string_ref_2.clear();
        string_ref_2.shrink_to_fit();
        println!("{}", s); // prints garbage
    }
}

fn main() {
    Foo::new(
        (),
        |_| Bar(RefCell::new((None, "Hello World!".to_owned()))),
        |bar| {
            bar.0.borrow_mut().0 = Some(bar);
            bar
        },
    );
}

[steffahn]

To remove the issue in self_cell, I think one would just need to make the signatures of with_dependent[_mut] more like the ones generated by ouroboros.

I’m not sure how much breakage comes with such a change, though 😅

[Voultapher]

just need to make the signatures of with_dependent[_mut] more like the ones generated by ouroboros.

Could you please elaborate what you mean.

[steffahn]

Ah, sorry, I wanted to elaborate anyways, but I kind-of forgot about this issue over reporting the ouroboros one and even finding another ouroboros issue.

---

Comparing

pub struct Bar<'a>(*mut &'a ());

self_cell! {
    pub struct Foo1 {
        owner: (),
        #[not_covariant]
        dependent: Bar,
    }
}

#[self_referencing]
pub struct Foo2 {
    owner: (),
    #[not_covariant]
    #[borrows(owner)]
    dependent: Bar<'this>,
}

[impl Foo1] (self_cell)

pub fn with_dependent<Ret>(
    &self,
    func: impl for<'_q> FnOnce(&'_q (), &'_q Bar<'_q>) -> Ret
) -> Ret

pub fn with_dependent_mut<Ret>(
    &mut self,
    func: impl for<'_q> FnOnce(&'_q (), &'_q mut Bar<'_q>) -> Ret
) -> Ret

[impl Foo2] (ouroboros)

pub(crate) fn with<'outer_borrow, ReturnType>(
    &'outer_borrow self,
    user: impl for<'this> FnOnce(BorrowedFields<'outer_borrow, 'this>) -> ReturnType
) -> ReturnType

[src]
pub(crate) fn with_mut<'outer_borrow, ReturnType>(
    &'outer_borrow mut self,
    user: impl for<'this> FnOnce(BorrowedMutFields<'outer_borrow, 'this>) -> ReturnType
) -> ReturnType

pub(crate) struct BorrowedFields<'outer_borrow, 'this> 
where
    'static: 'this, 
 {
    pub(crate) dependent: &'outer_borrow Bar<'this>,
    pub(crate) owner: &'this (),
}
pub(crate) struct BorrowedMutFields<'outer_borrow, 'this> 
where
    'static: 'this, 
 {
    pub(crate) dependent: &'outer_borrow mut Bar<'this>,
    pub(crate) owner: &'this (),
}

Hence, suggested change to self_cell impl

[impl Foo1] (self_cell)

pub fn with_dependent<'outer_borrow, Ret>(
    &'outer_borrow self,
    func: impl for<'_q> FnOnce(&'_q (), &'outer_borrow Bar<'_q>) -> Ret
) -> Ret

pub fn with_dependent_mut<'outer_borrow, Ret>(
    &'outer_borrow mut self,
    func: impl for<'_q> FnOnce(&'_q (), &'outer_borrow mut Bar<'_q>) -> Ret
) -> Ret

😉

(feel free to use a different name for the lifetime if you want)

Note that the &'_q lifetime for the owner is fine (and probably useful, since it’s a bit more general this way); the problematic part is really only the lifetime on the borrow of the dependent. It’s also (generally) only really problematic in the specific case that the dependent, e.g. Bar in this example, is not covariant and implements Drop. See more details on what exactly the soundness problem is with the lifetimes like this in the discussion on the ouroboros issue: someguynamedjosh/ouroboros#41

[steffahn]

Small addition to my previous comment

It’s also (generally) only really problematic in the specific case that the dependent, e.g. Bar in this example, is not covariant and implements Drop.

Actually, the second condition regarding Drop is not true; you can also get a mutable reference using with_dependent_mut.