Safety of scheme for downcasting data with lifetime

help
#1

I'm in need of the ability to package semi-arbitrary data that is either 'static or 'a within something with
the properties of Box<dyn Any + 'a>. (Note that I aim to potentially use this technique with a slightly different container, like maybe bumpalo's Box.)

I've written some unsafe code that does this by casting data with lifetime 'a to a version of that type with lifetime 'static. If the type is already 'static, its identity is preserved; otherwise, if it is of form T<'a>, I cast it to T<'static>. This is restricted only to types that fit these categories. Then, I store the casted data in Box<dyn Any>, and on downcast, if downcasting to the 'static variant succeeds, I then cast to the 'a variant.

My question is, is this code sound? Lifetimes and transmutes are both very subtle, so I'm concerned whether I might not be creating UB.

Here's my current implementation (that passes MIRI). Thanks!

#2

I need to reread again later, but after my first two passes, I get the impression it can leak somehow. Or rather the owning resource can be freed or resized. You could also violate AXM. Again, not sure, need to read closely. I'm sure it Can work soundly, but the issue would be if it got into the wild and triggered my fear cases.

#3

Your type DynBox<'a> is currently defined to be covariant over 'a. However, this is unsound: interior-mutability types cannot be soundly converted from longer lifetimes to shorter lifetimes. To demonstrate the issue (Rust Playground):

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

pub fn lifetime_transmute<T: ?Sized + 'static>(src: &T) -> &'static T {
    struct Helper<'a, T: ?Sized>(Rc<Cell<Option<&'a T>>>);
    // SAFETY: Trivial.
    unsafe impl<'a, T: ?Sized + 'static> AnyRef<'a> for Helper<'a, T> {
        type Static = Helper<'static, T>;
    }
    let dest = Rc::new(Cell::new(None));
    let helper = Helper(dest.clone());
    let helper: Helper<T> = DynBox::new(helper).downcast().unwrap();
    helper.0.set(Some(src));
    dest.get().unwrap()
}
3 Likes
#4

Hm ok definitely unsafe. Any suggestions on how to fix the problem?

Edited because I originally misunderstood the problem.

#5

Either strengthen the safety requirements for implementing AnyRef to require covariance in 'a, or make DynBox<'a> invariant in 'a by using something like PhantomData<Cell<&'a ()>> or PhantomData<fn(&'a ()) -> &'a ()> in place of PhantomData<&'a ()>.

The safety comment on AnyRef probably needs some improvement in any case. E.g. it would (usually) be unsound for some type Foo<'a> to have both Foo<'a>: AnyRef<'a> and Foo<'static>: AnyRef<'a>, yet one of the implementations could fall under the

// ... if the type is the result of parameterizing a type constructor
/// with the lifetime 'a, the result of that type constructor parameterized
/// with 'static.

clause while the other can fall under

// the type for which this is implemented
3 Likes
#6

Also, you might want to note in AnyRef's docs that the type cannot be parameterized over any lifetime other than 'a. Otherwise, converting to a DynBox<'a> and back could trivially transmute the other lifetimes. (That is, impl<'a, 'b> AnyRef<'a> for MyType<'a, 'b> must be disallowed, but impl<'a> AnyRef<'a> for MyType<'a, 'a> can be allowed.) Once you do that, I think your DynBox is completely sound.

1 Like
#7

That was really helpful, thanks!

#8

It has some of the concerns as static mut -- i.e. the mere existence of two aliasing &mut is technically UB. If I follow your safety guidelines for &'_ mut i32 for example, your code has that situation for a brief period when transforming into/out-of DynBox.

One may say, well, that's just some transient state that is never observed, so it's okay. And this may be true in practice today. But there is also a history of things like RFC 2582.

Perhaps there's a way to "rephrase" the implementation so that the output is a reborrow of the input. I think this might actually be easier with borrowed data, but don't have the time to sink into it at this moment.

1 Like
#9

If there is a safer way to do the transmute I'd love to do it; I wish std::mem::transmute were usable in a generic context even if it might cause monomorphization errors.

#10

I suppose you're referring to the simultaneous ManuallyDrop<&mut i32> and returned &mut i32 in transmute. But as I understand it, creating the second reference only makes the reference in the ManuallyDrop an invalid value. Since we destruct the ManuallyDrop without touching it again, we never have to assert the first reference's validity; otherwise, methods like ManuallyDrop::drop() would be wildly unsafe. I've depended on this property myself for dropping a ManuallyDrop<Box<_>> within a Drop impl, and Ralf Jung seemed to tentatively agree with the reasoning.

Correctness of "untouched" mutable borrow of a value in post-drop state
#11

On the road thought - perhaps you could wrap the static type in MaybeUninit (at the cost of leaking if dropped instead of reconstructed).

#12

I mean between transmutation and the unsizing plus pairing with the lifetime carrier, you hold a mut reference with static lifetime (which can't be a subborrow of the original borrow). It's not inside ManuallyDrop. Similarly when downcasting.

(Sorry for the lack of formatting or detail; I'll check out your links later.)

#13

I don't quite understand what you mean, then. Are you referring to transmute not preserving the tag for Stacked Borrows? Could you give an example?

1 Like
#14

Actually, given that the point of SyncUnsafeCell is to supply an alternative to static mut wherein "you only have to prove data accesses correct instead of the existence of the references themselves" (to quote eddyb), I think using UnsafeCell<T::Static> instead of T::Static would address the (perhaps overcautious?) concern. (Or a SyncUnsafeCell-like wrapper if you need Sync too.)

I wasn't thinking in SB terms. I meant that there is an outstanding mutable borrow active, but you create an independent &'static mut to the same location before the original mutable borrow expires. I wrote up some notes in //# comments here. Probably the important bits are about the new case within the body of the local transmute function.

Here's another playground just to illustrate that Miri doesn't currently catch the "two existing &mut to the same memory" case (or this particular version of it anyway).

I think it's different from your citation in that I have a &mut inside a ManuallyDrop, not a &mut to one or about a Box in one. But I guess an argument could go, it's okay to have a &mut inside a ManuallyDrop and do something to make it invalid in some sense, so long as you never take it back out afterwards. That doesn't really jive with UCG 245 I think, though.

And finally, in my experience "do things to the &mut itself" doesn't mean you somehow get new abilities beyond your original lifetime. That is, is it really okay to create the &'static mut during the original borrow, no matter what happens to the reference itself? Usually in my experience, it's really the borrow that matters.

1 Like
#15

If we modify that to impl<'a> AnyRef<'a> for &'a mut u64 directly, Miri indeed emits an error (Rust Playground):

error: Undefined Behavior: trying to reborrow <3011> for Unique permission at alloc1620[0x0], but that tag does not exist in the borrow stack for this location
  --> src/main.rs:33:5
   |
33 |     ret
   |     ^^^
   |     |
   |     trying to reborrow <3011> for Unique permission at alloc1620[0x0], but that tag does not exist in the borrow stack for this location
   |     this error occurs as part of a reborrow at alloc1620[0x0..0x8]
   |
   = help: this indicates a potential bug in the program: it performed an invalid operation, but the rules it violated are still experimental
   = help: see https://github.com/rust-lang/unsafe-code-guidelines/blob/master/wip/stacked-borrows.md for further information
help: <3011> was created by a retag at offsets [0x0..0x8]
  --> src/main.rs:28:15
   |
28 |     let ret = std::mem::transmute_copy::<T, U>(&manually_drop);
   |               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
help: <3011> was later invalidated at offsets [0x0..0x8]
  --> src/main.rs:30:17
   |
30 |     let inner = ManuallyDrop::into_inner(manually_drop);
   |                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   = note: inside `transmute::<&mut u64, &mut u64>` at src/main.rs:33:5

But it emits no error (Rust Playground) if manually_drop isn't touched again. This makes sense given my mental model of transmute, assuming that T has a unique reference:

  1. When the function is entered, val is a valid object of type T.
  2. val is moved into ManuallyDrop::new(). Now, manually_drop is a valid object of type ManuallyDrop<T>.
  3. mem::transmute_copy(&manually_drop) is called, producing the return value. The return value is retagged to become a valid object of type U, and manually_drop simultaneously becomes an invalid object of type ManuallyDrop<T>.
  4. manually_drop falls out of scope. It should be well-formed for an invalid ManuallyDrop<T> to fall out of scope, since otherwise methods like ManuallyDrop::drop() would be impossible to use. Indeed, manually_drop is never even touched in the MIR after the transmute_copy() returns.
  5. The valid U is returned.

Therefore, at no point do we have two valid mutable references: the first is invalidated the moment the second is retagged. That's why Miri emits an error on your modified example, since you pull an invalid object out of manually_drop. Just letting manually_drop fall out of scope does not assert its validity in the same way.

Meanwhile, I was somewhat worried about the transmute_copy w.r.t. retagging. But looking at Stacked Borrows, it handles this scenario without issue: it sees that we have Unique access from the original val's tag, and assigns that to the return value's tag.

Also, I had another worry, that a Drop impl could potentially observe the transient <T as AnyRef>::Static. Luckily, it seems like this isn't the case: not even Drop impls can be specialized on the 'static lifetime. Thus, Drop::drop() cannot assume that the fields live longer than the original 'a. (This reinforces my belief that 'static specialization would make many unsafe interfaces unsound.)

2 Likes
#16

After sleeping on it and re-reading the documented ManuallyDrop behavior and safety notes, I think I'm pretty much in agreement. The thing I needed to solidify it in my mental model was really that transmute{,_copy}, or perhaps we should say unsafe context generally, can result in accessible objects with valid bit-patterns lying around which nonetheless can no longer be used without invoking UB. That is, I needed a nudge with regards to transmute, not ManuallyDrop -- the latter has no special behavior around &mut specifically.

The transmute_copy is similar to a ManuallyDrop::take here; being able to use a normal transmute would be nicer. I think SB moves the tag correctly because it acts on locations, but didn't look into it. I convinced myself the lifetime being extended in transient state is fine too.

There may be a way to trigger unsoundness due to 'static anyway, but I only tried a surface level impl Drop on the 'static variant as well; a way around it would probably be considered a soundness bug at a higher level.