Bevy_reflect: Use into_remote to replace transmute in ReflectRemote impl

[yilin0518]

Objective

Make ReflectRemote an unsafe trait to encode the representation invariants required by Bevy's generated remote-type conversions.

ReflectRemote implementations are relied upon by generated code that reinterprets a wrapper as its remote type. Previously, downstream crates could implement this trait by providing a representation-incompatible implementation entirely through safe Rust, allowing generated FromReflect code to reach undefined behavior.

This change is possible problematical, as it change the trait into unsafe. The reason I do this is this trait has safety requirements, so I think this trait should be marked unsafe.

Solution

• Change ReflectRemote from a safe trait to an unsafe trait.
• Document the representation, reference conversion, and ownership-transfer invariants required of implementors.
• Update #[reflect_remote] to generate an unsafe impl ReflectRemote, with a safety justification based on its generated #[repr(transparent)] wrapper.
• Add a compile-fail test ensuring handwritten implementations must explicitly use unsafe impl.

Here is the reason that adding these safety requirements:

• A single-field #[repr(transparent)] newtype guarantees that Self and Self::Remote have the required compatible representation. Since the only field is a Self::Remote, a valid Self contains a valid remote value.
• The reference conversion methods must refer to the same underlying object and other requirements. Layout compatibility alone does not guarantee that an implementation returns the corresponding reference.
• The by-value conversion methods must transfer ownership exactly once.

The current trait and implmentation will cuase UB when manually implment this trait in the following code snippet:

#[cfg(test)]
mod tests {
    use bevy_reflect::{structs::DynamicStruct, FromReflect, Reflect, ReflectRemote};

    #[derive(Reflect)]
    struct Bad(u8);

    // This is intentionally a safe implementation of a trait whose contract
    // requires unsafe layout and validity guarantees.
    impl ReflectRemote for Bad {
        type Remote = bool;
       // All the implmentations are ignored
    }

    #[derive(Reflect)]
    struct Holder {
        #[reflect(remote = Bad)]
        value: bool,
    }

    #[test]
    fn safe_reflect_remote_impl_reaches_ub() {
        let mut patch = DynamicStruct::default();
        patch.insert("value", Bad(2));

        // The derive expansion transmutes `Bad(2)` into `bool`.
        // Under Miri this reports an invalid `bool` value (0x02).
        let _ = Holder::from_reflect(&patch).unwrap();
    }
}

The UB miri reported like this:

running 1 test
test tests::safe_reflect_remote_impl_reaches_ub ... error: Undefined Behavior: constructing invalid value of type bool: encountered 0x02, but expected a boolean
  --> src/lib.rs:43:14
   |
43 |     #[derive(Reflect)]
   |              ^^^^^^^ Undefined Behavior occurred here
   |
   = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
   = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information
   = note: this is on thread `tests::safe_ref`
   = note: stack backtrace:
           0: tests::_::<impl bevy_reflect::FromReflect for tests::Holder>::from_reflect
               at src/lib.rs:43:14: 43:21
           1: tests::safe_reflect_remote_impl_reaches_ub
               at src/lib.rs:56:17: 56:45
           2: tests::safe_reflect_remote_impl_reaches_ub::{closure#0}
               at src/lib.rs:50:45: 50:45
   = note: this error originates in the derive macro `Reflect` (in Nightly builds, run with -Z macro-backtrace for more info)

note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace

error: aborting due to 1 previous error

error: test failed, to rerun pass `--lib`

I'm looking forward to anyone's suggestion about this PR. Thank you for your feedback!

[SkiFire13]

ReflectRemote implementations are relied upon by generated code that reinterprets a wrapper as its remote type.

It would be useful to specify where this code is. My understanding is that this happens here, but please correct me if I'm wrong.

From what I see you should be able to use <#ty as ReflectRemote>::into_remote(#value) there, avoiding the use of unsafe and relying on the invalid assumptions for ReflectRemote.

[yilin0518]

This trait is a public trait for now, so other developers can implement this trait manually, not rely on the reflect_remote macro. Users may cause UB if implementing this trait wrongly. Besides, this trait has safety requirements, so is it appropriate to mark this as unsafe? I'm not sure so I open this PR.

The test case is to check that users must unsafely implement this trait.

Also, I think your suggestion is right. I can fix this by calling into_remote instead of transmute, and the safety is ensured by trait implementors. But I think if this trait is marked safe, it's better to limit the visibility of this trait by using sealed trait or trait implementation visibility(introduced by RFC 3323).

Thank you for your advice. Do you think this PR need to be modified? I can fix my PR.

[SkiFire13]

Users may cause UB if implementing this trait wrongly. Besides, this trait has safety requirements

My point was, how can users cause UB by implementing this trait wrongly? Can we change that instead, so that implementing this trait wrongly does not cause UB, and then remove the safety requirement from the trait? That way we don't need unsafe at all around the trait and everything is safer!

But I think if this trait is marked safe, it's better to limit the visibility of this trait by using sealed trait or trait implementation visibility

This trait must be implementable by other crates, otherwise the remote_reflect macro will not work.

Do you think this PR need to be modified?

Maybe you can double check, but I would fix any call site that is relying on the safety requirement of ReflectRemote and have them no longer rely on that, then remove the safety requirement from the documentation of ReflectRemote. Maybe @MrGVSV has more context on this since they implemented the original remote reflection PR in #6042

[yilin0518]

My point was, how can users cause UB by implementing this trait wrongly? Can we change that instead, so that implementing this trait wrongly does not cause UB, and then remove the safety requirement from the trait? That way we don't need unsafe at all around the trait and everything is safer!

A possible wrong implementation is that this trait is implemented on a wrong struct, which doesn't reflect a corresponding remote type.

#[cfg(test)]
mod tests {
    use bevy_reflect::{structs::DynamicStruct, FromReflect, Reflect, ReflectRemote};

    #[derive(Reflect)]
    struct Bad(u8);

    // This is intentionally a safe implementation of a trait whose contract
    // requires unsafe layout and validity guarantees.
    impl ReflectRemote for Bad {
        type Remote = bool;
       // All the implmentations are ignored
    }

    #[derive(Reflect)]
    struct Holder {
        #[reflect(remote = Bad)]
        value: bool,
    }

    #[test]
    fn safe_reflect_remote_impl_reaches_ub() {
        let mut patch = DynamicStruct::default();
        patch.insert("value", Bad(2));

        // The derive expansion transmutes `Bad(2)` into `bool`.
        // Under Miri this reports an invalid `bool` value (0x02).
        let _ = Holder::from_reflect(&patch).unwrap();
    }
}

The above code can cause UB:

running 1 test
test tests::safe_reflect_remote_impl_reaches_ub ... error: Undefined Behavior: constructing invalid value of type bool: encountered 0x02, but expected a boolean
  --> src/lib.rs:43:14
   |
43 |     #[derive(Reflect)]
   |              ^^^^^^^ Undefined Behavior occurred here
   |
   = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
   = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information
   = note: this is on thread `tests::safe_ref`
   = note: stack backtrace:
           0: tests::_::<impl bevy_reflect::FromReflect for tests::Holder>::from_reflect
               at src/lib.rs:43:14: 43:21
           1: tests::safe_reflect_remote_impl_reaches_ub
               at src/lib.rs:56:17: 56:45
           2: tests::safe_reflect_remote_impl_reaches_ub::{closure#0}
               at src/lib.rs:50:45: 50:45
   = note: this error originates in the derive macro `Reflect` (in Nightly builds, run with -Z macro-backtrace for more info)

note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace

error: aborting due to 1 previous error

error: test failed, to rerun pass `--lib`

I approve the way that remove unsafe operation(the usage of transmute) and use into_remote instead, but maybe the safety requirement that as follows still need to be ensured by users? So it seems some paradoxical: The implementation of this trait need to satisfy some requirement, although we can use the into_remote macro the ensure the correct implementation.

/// The macro will ensure that the following safety requirements are met:
/// - `Self` is a single-field tuple struct (i.e. a newtype) containing the remote type.
/// - `Self` is `#[repr(transparent)]` over the remote type.

This trait must be implementable by other crates, otherwise the remote_reflect macro will not work.

You are right.

Maybe you can double check, but I would fix any call site that is relying on the safety requirement of ReflectRemote and have them no longer rely on that, then remove the safety requirement from the documentation of ReflectRemote. Maybe @MrGVSV has more context on this since they implemented the original remote reflection PR in #6042

I can make some changes: withdraw the unsafe mark, and remove the unsafe transmute in into_remote.

[MrGVSV]

Actually, I originally had it as unsafe. I don't exactly remember why I removed it, but I wanna say there was debate as to whether the trait itself should be marked unsafe, since it's really our generated impl that uses unsafe code. Consumers generally don't need to worry about safety there.

However, you're right about the FromReflect logic. We can't just assume every remote reflection impl was generated by our macro and should avoid doing transmutes under such assumptions.

If we can fix that, then I think we should be okay keeping the trait as safe, right?

[MrGVSV]

Also, @yilin0518, could you please add your example snippet here to the PR description? I don't want it to get lost in the review comments, and it really helps showcase the problem. :)

[yilin0518]

Also, @yilin0518, could you please add your example snippet here to the PR description? I don't want it to get lost in the review comments, and it really helps showcase the problem. :)

Thank you for your review! I'll re-design my PR: remove the unsafe and mark this trait as safe, and provide the poc in this PR.

[SkiFire13]

If we end up going for making this unsafe, this is a breaking change that should be documented.

[yilin0518]

After reviewing the current code, I withdraw the solution that mark ReflectRemote and take the way that use into_remote to replace transmute. The related usage and documentation of transmute are removed or modified. Besides, I add another test case to verify the current implmentation.
@MrGVSV @SkiFire13 Can you give some suggestion about the new files changed?

[SkiFire13]

The changes overall look ok to me.

Could you also update the PR title and description to reflect the new approach?

[SkiFire13]

This new error related to ? seems a bit unfortunate, do you know where this comes from? Maybe we could avoid it by adding some type hints in the expanded code?

[yilin0518]

Good point. The diagnostic came from the ? nested inside the generated into_remote call. I changed the expansion to bind the reflected wrapper and converted remote value to explicit types before returning the field.