3

Suppose I have a polymorphic type T<A>:

#[repr(C)]
pub struct T<A> {
    x: u32,
    y: Box<A>,
}

Below are my reasonings:

  • According to the Memory Layout Section of std::boxed:

    So long as T: Sized, a Box<T> is guaranteed to be represented as a single pointer and is also ABI-compatible with C pointers (i.e. the C type T*).

    So no matter what A is, y should have the same layout;

  • Considering additionally the #[repr(C)] attribute on T, I expect that for all As, T<A> would share the same layout;

  • Thus I should be able to modify y in-place, even to values of a different type Box<B>.

My question is whether the following code is well-formed or has undefined behaviours? (The code below has been edited.)

fn update<A, B>(t: Box<T<A>>, f: impl FnOnce(A) -> B) -> Box<T<B>> {
    unsafe {
        let p = Box::into_raw(t);
        let a = std::ptr::read(&(*p).y);
        let q = p as *mut T<B>;
        std::ptr::write(&mut (*q).y, Box::new(f(*a)));
        Box::from_raw(q)
    }
}

Note:

The above code aims to perform a polymorphic update in place, so that the x field is left as-is. Imagine x were not simply a u32, but some very large chunk of data. The whole idea is to change the type of y (along with its value) without touching field x.


As is pointed out by Frxstrem, the code below indeed causes undefined behaviour. I made a silly mistake: I forgot to re-allocate memory for the B produced by f. It seems the new code above passes Miri check.

fn update<A, B>(t: Box<T<A>>, f: impl FnOnce(A) -> B) -> Box<T<B>> {
    unsafe {
        let mut u: Box<T<std::mem::MaybeUninit<B>>> = std::mem::transmute(t);
        let a = std::ptr::read::<A>(u.y.as_ptr() as *const _);
        u.y.as_mut_ptr().write(f(a));
        std::mem::transmute(u)
    }
}
Ruifeng Xie
  • 813
  • 4
  • 16
  • 2
    [Miri says this is undefined behavior](https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=ec5d11fa4192bccdbd27ce68c73b9781) – Frxstrem Mar 03 '21 at 14:28
  • [yep- I caused a segfault](https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=13e61ba7007345257579b697d8fc0b12) – Aiden4 Mar 03 '21 at 14:44
  • @Frxstrem I've updated the code. The previous code is incorrect in that it reuses the heap memory of `A` for the new object `B`. The new code passes Miri check. But this doesn't mean it's 100% correct, does it? – Ruifeng Xie Mar 03 '21 at 14:56
  • @RuifengXie That's right. Miri can find a lot of errors but doesn't guarantee that it will find all of them. – Peter Hall Mar 03 '21 at 15:19
  • Seems like another variation on the `replace` theme. See https://stackoverflow.com/questions/29570781/temporarily-move-out-of-borrowed-content/60382120#60382120 for example. Your code probably misbehaves if `f` panics. – Sebastian Redl Mar 03 '21 at 15:21
  • @SebastianRedl Got it. And what about other parts? Like where field `y` changes its type? – Ruifeng Xie Mar 03 '21 at 15:32
  • 1
    Adding on to what SebastianRedl said, [It leaks the allocation of `T` if `f` panics.](https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=abec2e4c1caa7a476580bf27e5b9865a) You also seem to be doing it right elsewhere with your revised `unsafe` code. – Aiden4 Mar 03 '21 at 15:47
  • Please see my updated answer, I fixed the potential memory leak. – orlp Mar 03 '21 at 16:16

1 Answers1

1

You're confusing yourself with the whole T stuff. This should be much easier to analyze. Specifically, read here.

use core::mem::ManuallyDrop;
use core::alloc::Layout;

unsafe trait SharesLayout<T: Sized>: Sized {
    fn assert_same_layout() {
        assert!(Layout::new::<Self>() == Layout::new::<T>());
    }
}

/// Replaces the contents of a box, without reallocating.
fn box_map<A, B: SharesLayout<A>>(b: Box<A>, f: impl FnOnce(A) -> B) -> Box<B> {
    unsafe {
        B::assert_same_layout();
        let p = Box::into_raw(b);
        let mut dealloc_on_panic = Box::from_raw(p as *mut ManuallyDrop<A>);
        let new_content = f(ManuallyDrop::take(&mut *dealloc_on_panic));
        std::mem::forget(dealloc_on_panic);
        std::ptr::write(p as *mut B, new_content);
        Box::from_raw(p as *mut B)
    }
}

Then simply:

unsafe impl<A, B> SharesLayout<T<A>> for T<B> {}

fn update<A, B>(bt: Box<T<A>>, f: impl FnOnce(A) -> B) -> Box<T<B>> {
    box_map(bt, |t| {
        T { x: t.x, y: Box::new(f(*t.y))}
    })
}
orlp
  • 112,504
  • 36
  • 218
  • 315
  • I get the point for the `SharesLayout` trait, and how `DeallocOnDrop` prevents leaking. But if `x` is large in size, does the new `update` guarantee to avoid copying field `x`? – Ruifeng Xie Mar 03 '21 at 16:29
  • @RuifengXie Well, it gets moved, which *may* get optimized by the compiler, I don't know. Note that there's no `Copy` trait bounds anywhere. – orlp Mar 03 '21 at 16:31
  • Yes, certainly no copying in Rust's sense. By copying I meant `memcpy`. I believe in most circumstances it would be optimized out, but if the function to `box_map` is not inlined, the optimization would become impossible. – Ruifeng Xie Mar 03 '21 at 16:36
  • 1
    @RuifengXie I updated the implementation once more, eliminating the need for `DeallocOnDrop`. – orlp Mar 03 '21 at 16:48