Unsafe code review: crate for lifetime erasure

code review
1

After running into somewhat similar lifetime-related issues a few times, I've decided to generalize my solution into a crate. Before publishing it, I'd like to know if there is some fundamental soundness problem I could be overlooking.

The source code (with documentation and examples) is here: GitHub - SReichelt/temp-inst: A Rust crate to convert between types with lifetime parameters and corresponding lifetime-erased variants.. I'm not really asking anyone to thoroughly review it, but I could imagine someone quickly responding with something like: "Oh, this has been tried before but can't be made sound; here's an example that causes UB."

The cases where only shared or pinned references are involved are not so complicated, but I encountered many "gotchas" when dealing with non-pinned mutable references. In fact, when searching for similar crates, I came across dyn-context, but I believe it's probably unsound because of the same "swapping" problem I was facing. (Will check this.) Still hoping that my solution works.

Thanks in advance.

1 Like
2

If you haven't yet, try running code exercising your API under Miri. Particularly interesting is after passing your bundle as a function argument.

And indeed, mutable references are a significant source of issues. When present in a function argument (even as a field of a struct), they're required to be unique again and any derived pointers and/or references become invalid. The other common issue with attempts at lifetime erasure is variance; it's unsound to convert &mut &'long T to &mut &'short T (irregardless of pinning).

Generally, I recommend using the yoke crate. If the same API isn't possible to replicate using yoke, it's probably unsound. Yoke isn't perfectly all-powerful, but it's fairly close and would (as far as anyone can tell) be perfectly sound if it were possible for it to be[1].

  1. There's an RFC accepted for a lang type tentatively called MaybeDangling which is necessary to plug a soundness hole that also still affects the standard library except for an overly pedantic gotcha. (What are you allowed to do with a ManuallyDrop after manually dropping its payload?) ↩︎

2 Likes
3

call_with_mut_async isn’t sound because in-flight futures can be leaked

use std::{
    future::Future,
    pin::Pin,
    sync::Arc,
    task::{Context, Poll, Wake, Waker},
};

use temp_inst::{TempInst, TempRefMut};

fn poll_no_wake(f: Pin<&mut impl Future>) {
    struct W;
    impl Wake for W {
        fn wake(self: std::sync::Arc<Self>) {}
    }
    let w = Waker::from(Arc::new(W));
    let _ = f.poll(&mut Context::from_waker(&w));
}

async fn pending() {
    struct Pending;
    impl Future for Pending {
        type Output = ();

        fn poll(self: Pin<&mut Self>, _: &mut Context<'_>) -> Poll<Self::Output> {
            Poll::Pending
        }
    }
    Pending.await
}

fn main() {
    let r1 = &mut String::from("dummy");
    let fut = TempInst::<TempRefMut<String>>::call_with_mut_async(r1, |r1_inst| {
        let mut s = String::from("Hello World!");
        let r1 = r1_inst.get_mut();

        println!("{r1}"); // "dummy"

        let fut = TempInst::<TempRefMut<String>>::call_with_mut_async(&mut s, |r2_inst| {
            std::mem::swap(r1_inst, r2_inst);
            pending()
        });
        let fut = Box::leak(Box::new(Box::pin(fut)));
        poll_no_wake(fut.as_mut());
        let r1 = r1_inst.get_mut();

        println!("{r1}"); // "Hello World!"

        drop(s);

        println!("{r1}"); // prints garbage

        pending()
    });
    let fut = Box::leak(Box::new(Box::pin(fut)));
    poll_no_wake(fut.as_mut());
}
4 Likes
4

If you haven't yet, try running code exercising your API under Miri.

Thanks, I should have mentioned that the unit tests pass under Miri. I'm a bit worried that Miri could be too lenient in the zero-size case (the last test case), but it seems to me that references to zero-size objects are a bit of a special case in some respects.

The other common issue with attempts at lifetime erasure is variance; it's unsound to convert &mut &'long T to &mut &'short T (irregardless of pinning).

Good point, fortunately I only need to convert &'long mut T to &'short mut T, and (in TempRefPin, which is sort of an optional feature) Pin<&'long mut T> to Pin<&'short mut T> (via &mut T and NonNull<T>). I have to admit the variance of Pin isn't officially documented, but it's covariant as currently defined. Is it safe to rely on it staying the same?

Generally, I recommend using the yoke crate.

It looks interesting, but I think it's solving a slightly different problem because it packages the source and the reference together, whereas I want the source to stay where it is.

call_with_mut_async isn’t sound because in-flight futures can be leaked

Oops, good catch! And thanks a lot for even writing a complete test case!
I've removed the async methods because the others don't provide any advantage over using new_wrapper[...] directly.
Now hoping that was the only blunder (fingers crossed).

5

Your Send implementation of TempInst is not sound:

use std::{
    cell::Cell,
    rc::Rc,
    thread,
};

use temp_inst::{
    mapped::{HasTempRepr, MappedTempRepr},
    SelfRepr, TempInst,
};

struct Has;

impl HasTempRepr for Has {
    type Temp = SelfRepr<Rc<Cell<String>>>;

    type Shared<'a> = Rc<Cell<String>>
    where
        Self: 'a;

    type Mutable<'a> = ()
    where
        Self: 'a;

    fn shared_to_mapped(obj: Self::Shared<'_>) -> <Self::Temp as temp_inst::TempRepr>::Shared<'_> {
        obj
    }

    fn mapped_to_shared(
        mapped: <Self::Temp as temp_inst::TempRepr>::Shared<'_>,
    ) -> Self::Shared<'_> {
        mapped
    }

    fn mut_to_mapped((): Self::Mutable<'_>) -> <Self::Temp as temp_inst::TempReprMut>::Mutable<'_> {
        Rc::new(Cell::new(String::new()))
    }

    fn mapped_to_mut(
        _mapped: <Self::Temp as temp_inst::TempReprMut>::Mutable<'_>,
    ) -> Self::Mutable<'_> {
    }
}

fn main() {
    let i = TempInst::<MappedTempRepr<Has>>::new(());
    let r: Rc<Cell<String>> = i.get();
    println!("{r:p}"); // same Rc<Cell<_>> on 2 threads
    let h = thread::spawn(move || {
        let r: Rc<Cell<String>> = i.get();
        println!("{r:p}"); // same Rc<Cell<_>> on 2 threads
    });
    drop(r); // dropping here makes miri notice a data race
    h.join().unwrap();
}

Edit: Or, actually, put differently, the implementation of TempReprMut for MappedTempRepr is unsound w.r.t. the documented safety condition about Send

1 Like
6

I guess I just don't understand the point of having &mut TempInst<TempRefMut<T>> instead of just &mut T. I guess it's "shove it through an API with the shape fn<T: 'static>(&mut T)" but I don't really see that bound happening legitimately, requiring something to be 'static while only borrowing for '_; I suppose perhaps dyn Any tricks maybe?

Yes; the variance of a type isn't directly documented, but it is considered a part of that type's API stability.

Sort of; the runtime borrow semantics as currently (experimentally) implemented are defined in terms of the referenced bytes. Thus the effective rules for references to zero bytes are comparatively weaker, since they borrow zero bytes with which they could conflict with other references.

TempRefMut should to be invariant in T (add a field of type PhantomData<fn(T) -> T>). I don't think it's directly exploitable with the current API but it's still a good idea to prevent coercing TempRefMut<&'long T> into TempRefMut<&'short T>.

7

Oh wow, now I'm even more impressed -- and of course really glad I asked here before publishing.
I'll have to think about the best solution here (besides just not implementing Send and Sync, which would also be fine -- I was actually just trying to avoid seemingly unnecessary restrictions). Maybe adding a Send bound to TempRepr would make the most sense. I'll try to be more careful about it.

8

Another thing: Given AlwaysShared is safe to implement, and TempRepr makes no safety demands towards behavior of PartialEq/Clone to safeguard against swapping, I think that the impl<T: AlwaysShared> TempReprMut for T might currently be unsound.

2 Likes
9

I should try to construct a different example; the 'static bound is really misleading because I only slapped it on there to make the trivial solution impossible.

The problems I were facing looked more like this (and I should have probably seeked help here, but I'm actually having a lot of trouble minimizing them):

trait Foo: Sized {
    fn foo(&mut self, iter: &mut <Self as HasIter>::Iter) -> Option<i32>;
}

// This has two references because we need to construct it from the
// arguments to `Foo::foo`.
struct FooIter<'a, F: Foo>(&'a mut F, &'a mut <F as HasIter>::Iter);

impl<'a, F: Foo> Iterator for FooIter<'a, F> {
    type Item = i32;

    fn next(&mut self) -> Option<Self::Item> {
        self.0.foo(self.1)
    }
}

trait HasIter {
    type Iter: Iterator;
}

impl<F: Foo> HasIter for F {
    // We would need `Iter` to have a lifetime parameter, but can't just add one because then
    // `FooIter` would need it as well because it contains a reference to a `HasIter::Iter`, and
    // then `Iter` would also need a second lifetime parameter, and so on.
    type Iter = FooIter<'_, F>;
}

I'm realizing now that the situations always deal with some sort of "chaining" that's making it difficult to assign lifetimes - though it's not unlikely that I'm just overlooking some safe solutions.

Ah, now I understand. That does seem exploitable, actually!

(FYI, I have reported the swap-related soundness issue of the dyn-context crate: Unsoundness due to mem::swap possibility · Issue #1 · A1-Triard/dyn-context · GitHub)

10

Oh indeed. I wasn't paying much attention when I introduced it. Will fix it.

11

Hopefully the issues both of you have found are fixed now. I've also changed the example to something a bit more realistic.

It's interesting that I was violating my own safety conditions so often. I think one of the causes is that my attempt to generalize everything also vastly increased the public API in ways I hadn't necessarily intended from the start. Definitely a lesson learned. Thanks a lot to both of you!

I've noticed that TempInst may be unnecessary if I make TempInstWrapper more of a first-class citizen. That should reduce the complexity a bit.