Clone for UnionN is unsound with malicious P: ErasablePtr + Clone

[RustyYato]

Currently ErasablePtr doesn't require that Clone preserves that the pointer is aligned. So it's possible to construct a type like this

https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=292dca0467b3b0e1e8514b98087dc0c7

struct FancyBox {
    ptr: *mut u32,
}

impl FancyBox {
    fn new(value: u32) -> Self {
        FancyBox {
            ptr: Box::into_raw(Box::new(value))
        } 
    }
}

impl Clone for FancyBox {
    fn clone(&self) -> Self {
        let mut x = FancyBox::new(**self);
        
        x.ptr = x.ptr.map_addr(|ptr| ptr | 1);
        
        x
    }
}

impl core::ops::Deref for FancyBox {
    type Target = u32;

    fn deref(&self) -> &u32 {
        let ptr = self.ptr.map_addr(|ptr| ptr & !3usize);
        unsafe { &*ptr }
    }
}


fn main() {
    let x = FancyBox::new(10);
    
    assert_eq!(*x, 10);

    let y = x.clone();
    
    assert_eq!(*y, 10);
}

In particular in affects impl Clone for UnionN

pointer-utils/crates/ptr-union/src/lib.rs

Line 561 in 26c260e

let builder = unsafe { <$Builder<$($A,)*>>::new_unchecked() };

since here it's assumed that Clone will always return an aligned pointer.

So either ErasablePtr needs to be updated to include this, or UnionN needs to check that the cloned pointers are aligned.

[CAD97]

Yet another good find, thank you!

It's a little unfortunate to add overhead and a panic opportunity to Clone, but checking alignment is the only correct choice here… it doesn't even take a malicious impl, just some "lucky" enough execution. The reason is that we allow construction of UnionN with a dynamic check that allows getting "lucky" on alignment, so if the clone then gets "unlucky"… 💣💥

(The only saving grace is that allocations tend to be aligned to alignof(max_align_t)_C in practice, at least heap allocated ones.)

That's also the failing that incorrectly tried to justify the current implementation — in the first draft, having UnionN meant that the Builder for the variant must have existed first, thus it's a known guarantee that the pointer must always be aligned as a type level guarantee. But since we allow construction of UnionN via dynamic alignment checks, that chain of logic to create the builder is invalid/unsound.

If you write a patch I should hopefully be able to push an update the following day (US Eastern). If you don't, hopefully I can do it myself on this weekend.

[CAD97]

If you don't mind me asking, how'd you come across this? I honestly thought I hardened all these crates pretty well, but it's been a pretty while.

As a side note, my bar for quality has gone up since I first wrote these crates, and I really should write a successor crate (tentatively named pointrs; ptrs got taken) that's more up to my modern standards. I've just justified waiting so far on every attempt¹ to improve the core design of the (erasable) pointer abstraction wanting for the lack of feature(ptr_metadata), or at least ptr::with_metadata_of, or at least specialization (a for<T: Sized> blanket impl is very desirable but conflicts with a for<Tail: ?Sized + Erasable> impl, unfortunately).

Footnotes

1. Actually, a couple stalled on a tangent with wanting to revolutionize the proc macro / derive ecosystem. I am not great at keeping my scope in check. 😅 ↩

[RustyYato]

Sure, I can make a PR to fix this.

If you don't mind me asking, how'd you come across this? I honestly thought I hardened all these crates pretty well, but it's been a pretty while.

There was some functionality that I wanted (a Visitor api) but I wasn't sure if it was sound. I also wanted to see if I could support an arbitrary number of types (if you had sufficient alignment on the pointers). So I made my own implementation, and while comparing implementations to make sure I didn't screw up somewhere I noticed this Clone impl where I had taken a more conservative approach (panicking), and checked if I could avoid the panic based on your Clone impl, and that's how I discovered the bug.

to improve the core design of the (erasable) pointer abstraction wanting for the lack of feature(ptr_metadata), or at least ptr::with_metadata_of

Yeah, I'm also eagerly awaiting ptr_metadata. It will unlock so many nice pointer tricks on (potentially) wide pointers.

I am not great at keeping my scope in check

Me too! (as you may have guessed from my story above)

[CAD97]

Fix published in v2.3.0