How unsafe is my reference type?


#21

This is really clever. :sunglasses::+1:

I’m almost convinced at this point the API is safe. In other words, I’ve exhausted the ways I could think of to break it and none of them will work. There might be some way to make it cleaner with Pin, but I still don’t understand that stuff well enough.

I agree that you should dereference instead of mem::transmuteing, and would add that Sc::is_none can be written safely and more simply as self.0.get().is_none(). Although they probably compile to the same thing after optimization.

Really interesting work.


#22

Oops, I’m still learning all of this. Thank you for the correction, it is very welcome :slight_smile:. I’ll be updating my code soon.

The code I posted (which you can find in my repository) compiles and the test I posted are passing (meaning that sc.is_none() becomes true as soon as the locker gets out of scope).
I also tested that you cannot call mem::forget on the Locker object after having called lock. Also, I just tested, and you cannot have a Locker instance that has a lifetime greater than the T it refers to, because when you call Locker::lock, it attempts to borrow T for the entire lifetime of Locker, and so it fails to compile (which is what we want, I suppose).

Regarding the drop code, note that the Locker itself does not implement Drop. Attempting to implement Drop directly on Locker was actually my first attempt, but it failed because since Locker is autoborrowed by Locker::lock, it couldn’t be borrowed mutable by the drop function.

Having one of its fields implement Drop works, because the fields are dropped right after the end of the Locker's lifetime, and at this point it is no longer borrowed :slight_smile:. I wish this trick of delegating the drop to a field for perma-borrowing struct was documented somewhere, would have saved me a bit of time!

I guess, a thing I need to do to gain confidence that this is safe is to try and put Locker in a Rc cycle, and then lock it. If this proves impossible, then I’ll be happy I guess :grinning:.

@trentj: Thank you! I wonder if this type would be useful to store callbacks, like was discussed earlier in the thread? I really need to add Trait objects support back soon.

EDIT: I pushed the suggested changes, thank to both of you :slight_smile:


#23

Still broken :expressionless:.

#[test]
fn oopsie() {
    let sc = Sc::new();
    assert!(sc.is_none());
    {
        let s = String::from("foo");
        assert!(sc.is_none());
        {
            let locker = Box::new(Locker::new());
            let locker = Box::leak(locker);
            locker.lock(&s, &sc);
            assert!(!sc.is_none());
        }
    }
    sc.map(|x| println!("{}", x));
    assert!(sc.is_none());
 }

Here we are creating the locker in a Box, then getting a leaked reference to it by calling Box::leak, and finally locking the leaked reference with Locker::lock.
Interestingly, the Wrapper<T> doesn’t suffer from this problem, because since it also owns the T, leaking the locker leaks the T at the same time, meaning calling Sc::map is never unsafe when the Sc is locker to a Wrapper<T>. So, that’s a thing, I guess? :sweat:

Other than that, I really don’t know how I could prevent a user to put a locker in a Box and then Box::leak'ing it. Any idea of how I could do that? A possibility would be to produce an autoborrowed struct when calling Locker::lock, but I don’t know how to return such autoborrowed struct.


#24

Awwh.

Well, it was fun while it lasted.

I don’t think the Wrapper is as useful as the non-Wrapper version, because you have to move the T into it, which offsets the advantage of not allocating. Perhaps it could be useful when allocating isn’t an option, though.

On the bright side you can probably simplify the API now that Locker isn’t useful on its own.


#25

I’d still be curious to know if I can leak a Locker with a cycle of Rc, because that’d still be something regarding the safety of the API.
Naively using the safe_forget method defined in the PPYP article doesn’t work, because it requires moving, so I tried to create cycle by allocating a Locker in a Rc, and then calling leak on it, but it doesn’t seem to work:

  • Locker::lock() requires a &mut Locker, but Rc only gives out an immutable borrow.
  • I tried to put a RefCell<Locker> in the Rc, but then I must access it with RefCell::borrow_mut, which returns me a reference with a lifetime that cannot shorter than that of the Rc itself, so I cannot call Locker::lock on it.

So it looks like there can be no cycles with Rc because it seems impossible to lock a Locker inside a Rc in the first place. Is this assumption correct? If it is not, could you perhaps show me a cycle involving locked Lockers? Thank you! :grinning:


#26

So, I was just reading @withoutboats’ new article on their GC library, shifgrethor, and it occured to me that they are using a macro, together with the Pin API, to achieve a safe API in circumstances similar to these of the Sc type.

So basically, they expose some unsafe functions, plus a macro that lets them safely use these functions to declare their root object and ensure that it was declared on the stack.
The magic happens here. I can definitely do something similar for my Sc library.

I need to try this as soon as I find some time!