Implementation is technically unsound

[kotauskas]

I'm talking about lines 167 to 173 of lib.rs:

#[repr(C)]
#[derive(Copy, Clone)]
#[doc(hidden)]
pub struct TraitObject {
    pub data: *mut (),
    pub vtable: *mut (),
}

The macro expansion then goes on to use this declaration like this:

pub unsafe fn downcast_ref_unchecked<T: $trait_>(&self) -> &T {
    let trait_object: $crate::TraitObject = ::$core::mem::transmute(self);
    ::$core::mem::transmute(trait_object.data)
}

This code makes an assumption that the memory layout of &dyn Trait for any trait Trait is the layout of TraitObject, which isn't a real guarantee that the Rust compiler is required to abide. Using the same TraitObject struct from core::raw would require nightly, but break compilation when this assumption would be broken because of a change to how trait object references are stored, averting possible undefined behavior by stopping compilation of the code.

The only sound alternative to this right now is to depend on a nightly compiler, make use of the unstable Pointee trait and await its stabilization, thus removing the nightly requirement.

Keep in mind that this is a breaking change. Code which depends on the 0.2.x line and is expected to be compiled with the stable compiler would break if mopa starts requiring nightly on that release line. This fix must therefore be deployed as 0.3.0 or, preferably, 1.0.0-rc1 (since Pointee is not stable yet and its fate is not absolutely guaranteed). The previous 0.2.x and 0.1.x release lines may then be yanked.

[danielhenrymantilla]

The only sound alternative to this right now is to depend on a nightly compiler, make use of the unstable Pointee trait and await its stabilization, thus removing the nightly requirement.

Not really necessary, since one can get the data pointer out of a &dyn … ("thin the pointer") by casting raw pointers:

pub unsafe fn downcast_ref_unchecked<T : $trait_>(&self) -> &T {
    &*(self as *const _ as *const T)
}

And this has already been done in this repo: it was done 5 years ago! https://github.com/chris-morgan/mopa/commit/2bb823b4da60316868a8adb8be573b6b5dd6f508

But the crate seems to be unmaintained, and that commit never made it to crates.io

[chetaldrich]

https://github.com/advisories/GHSA-2gxj-qrp2-53jv Just linking the relevant CVE here.

[Cypher1]

Any chance this can be fixed? It seems that shred, used by specs depends on this

[kotauskas]

@Cypher1, perhaps shred could specifically pull in the fixed version like this:

[patch.crates-io]
mopa = { git = "https://github.com/chris-morgan/mopa", rev = "de0066e" }

[danielhenrymantilla]

Sadly a patch section only works for the downstream user, so it would require that specs use it when testing, and that further downstream users do the same, etc.

I've decided to go and publish https://crates.io/crates/mopa-maintained to palliate that, using my own otherwise-identical fork: https://github.com/chris-morgan/mopa/compare/master...danielhenrymantilla:mopa:v0.2.3

The fix is then to do:

- mopa = "0.2.2"
+ mopa-maintained = "0.2.3"

[Cypher1]

Thank you Daniel! I'll follow up with shred when I can to make sure all shred users (including specs, amethyst etc.) get the fix.

On Sat, 9 July 2022, 12:25 am Daniel Henry-Mantilla, < @.***> wrote:

Sadly a patch section only works for the downstream user, so it would require that specs use it when testing, and that further downstream users do the same, etc.

I've decided to go and publish https://crates.io/crates/mopa-maintained to palliate that.

The fix is then to do:

• mopa = "0.2.2"+ mopa-maintained = "0.2.3"

— Reply to this email directly, view it on GitHub https://github.com/chris-morgan/mopa/issues/13#issuecomment-1179049634, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAIRUHV5C37UUACAQS57O2TVTA25BANCNFSM45472VXQ . You are receiving this because you were mentioned.Message ID: @.***>

[Cypher1]

Just closing the loop: @torkleyy removed shred/specs/amethyst dependency on this crate as a fix for this issue and the crate not being maintained. This fixes my usage as well :)

Thanks to @Torkleyy

[shinmao]

Not really necessary, since one can get the data pointer out of a &dyn … ("thin the pointer") by casting raw pointers:

pub unsafe fn downcast_ref_unchecked<T : $trait_>(&self) -> &T {
    &*(self as *const _ as *const T)
}

@danielhenrymantilla sorry to ping this response after 2 years :) I just felt curious why casting can solve the problem here? As I know, the raw pointer to trait object is still fat pointer. Isn't it still unsound to cast a fat pointer?

[danielhenrymantilla]

@shinmao if using as then by design it is not unsound: a common misconception is that as is just a non-unsafe transmute for certain cases, but as can actually perform conversion operations (in the pointer to usize case, it can "expose the provenance", in the unsizing case, it can embed the necessary metadata so as to get a wide pointer, and when the input is a wide pointer and the output is a thin (data) pointer), the as cast then becomes a legal "deünsizing"/wide-pointer-truncating operation:

fn as_<T : ?Sized>(p: *const T) -> *const () {
    p as _
}

which is guaranteed to extract the data pointer off the *const (impl !Sized) wide pointer, thanks to language semantics. That is, even if, say, the data pointer were laid out after the vtable reference, then this as cast would be guaranteed to extract the data pointer at that offset.

• (the operation which cannot be done in current stable Rust (1.72.0)) is the metadata-extracting one: ::core::ptr::metadata())

[shinmao]

@danielhenrymantilla thanks for the answer. So the difference with transmute is that transmute will just copy the value at the memory address without metadata?

[danielhenrymantilla]

transmute (or transmute_copy() or ptr.cast().read()) is a very precise operation which takes a value (and a size), and reinterprets that span of bytes.

We could imagine having:

//! Since the layout of wide pointers is unspecified, neither
//! `option_a` nor `option_b` are guaranteed to be sound!

fn option_a(p: *const dyn Trait) -> *const () {
    let at_p = <*const _>::cast::<u8>(&p);
    const OFFSET_TO_DATA_PTR: usize = 0; // ???
    unsafe { at_p.add(OFFSET_TO_DATA_PTR).cast().read() }
}

fn option_b(p: *const dyn Trait) -> *const () {
    let at_p = <*const _>::cast::<u8>(&p);
    const OFFSET_TO_DATA_PTR: usize = mem::size_of::<&'static VTable>(); // ???
    unsafe { at_p.add(OFFSET_TO_DATA_PTR).cast().read() }
}

with the option_a also being expressible as:

fn option_a(p: *const dyn Trait) -> *const () {
    unsafe { transmute_copy(&p) }
}

And since 0x142_u32 as u8 == 0x42_u8 == transmute_copy::<_, u8>(&0x142_u32), I was just hinting at the ideä/notion that:

1. we could be tempted to see x as type as being kind of analogous to transmute_copy::<_, type>(&x) (papering over non-Copy cases),
2. which in turn is similar to <*const _>::cast::<u8>(&x).add(0).cast().read()
3. which in turn is assuming that the data pointer of a wide pointer would necessarily be at offset 0

And since this whole issue #13 is about the precise layout of wide pointers being unspecified, that would mean usage of as would not be fine. That's how I have interpreted your question anyhow.

And my point was basically that this first 1. bullet is wrong: as is truly a special (overly-)multipurpose operation, which in the case of a wide pointer magically "knows" how to extract its data pointer in a sound and robust fashion, much like, on nightly, the dedicated ::core::ptr::metadata() function is capable of doing for extracting the non-data pointer (metadata) component of a wide pointer.

---

[META] If you want to keep talking about this, this Github issue may be causing too much "notification noise" to people already subscribed to it; so I'd advise to go elsewhere for this kind of discussions, such as the Rust Community Discord server (https://discord.gg/rust-lang-community), in the #dark-arts channel, for instance 🙂