Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Merged by Bors] - bevy_reflect: ReflectFromPtr to create &dyn Reflect from a *const () #4475

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions crates/bevy_reflect/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ bevy = ["glam", "smallvec"]
# bevy
bevy_reflect_derive = { path = "bevy_reflect_derive", version = "0.8.0-dev" }
bevy_utils = { path = "../bevy_utils", version = "0.8.0-dev" }
bevy_ptr = { path = "../bevy_ptr", version = "0.8.0-dev" }

# other
erased-serde = "0.3"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ pub(crate) fn impl_get_type_registration(
impl #impl_generics #bevy_reflect_path::GetTypeRegistration for #type_name #ty_generics #where_clause {
fn get_type_registration() -> #bevy_reflect_path::TypeRegistration {
let mut registration = #bevy_reflect_path::TypeRegistration::of::<#type_name #ty_generics>();
registration.insert::<#bevy_reflect_path::ReflectFromPtr>(#bevy_reflect_path::FromType::<#type_name #ty_generics>::from_type());
#(registration.insert::<#registration_data>(#bevy_reflect_path::FromType::<#type_name #ty_generics>::from_type());)*
registration
}
Expand Down
9 changes: 6 additions & 3 deletions crates/bevy_reflect/src/impls/std.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use crate as bevy_reflect;
use crate::{self as bevy_reflect, ReflectFromPtr};
use crate::{
map_apply, map_partial_eq, Array, ArrayInfo, ArrayIter, DynamicMap, FromReflect, FromType,
GetTypeRegistration, List, ListInfo, Map, MapInfo, MapIter, Reflect, ReflectDeserialize,
Expand Down Expand Up @@ -175,6 +175,7 @@ impl<T: FromReflect + for<'de> Deserialize<'de>> GetTypeRegistration for Vec<T>
fn get_type_registration() -> TypeRegistration {
let mut registration = TypeRegistration::of::<Vec<T>>();
registration.insert::<ReflectDeserialize>(FromType::<Vec<T>>::from_type());
registration.insert::<ReflectFromPtr>(FromType::<Vec<T>>::from_type());
registration
}
}
Expand Down Expand Up @@ -326,8 +327,9 @@ where
V: FromReflect + Clone + for<'de> Deserialize<'de>,
{
fn get_type_registration() -> TypeRegistration {
let mut registration = TypeRegistration::of::<Self>();
registration.insert::<ReflectDeserialize>(FromType::<Self>::from_type());
let mut registration = TypeRegistration::of::<HashMap<K, V>>();
registration.insert::<ReflectDeserialize>(FromType::<HashMap<K, V>>::from_type());
registration.insert::<ReflectFromPtr>(FromType::<HashMap<K, V>>::from_type());
registration
}
}
Expand Down Expand Up @@ -575,6 +577,7 @@ impl GetTypeRegistration for Cow<'static, str> {
fn get_type_registration() -> TypeRegistration {
let mut registration = TypeRegistration::of::<Cow<'static, str>>();
registration.insert::<ReflectDeserialize>(FromType::<Cow<'static, str>>::from_type());
registration.insert::<ReflectFromPtr>(FromType::<Cow<'static, str>>::from_type());
registration.insert::<ReflectSerialize>(FromType::<Cow<'static, str>>::from_type());
registration
}
Expand Down
134 changes: 131 additions & 3 deletions crates/bevy_reflect/src/type_registry.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
use crate::serde::Serializable;
use crate::{Reflect, TypeInfo, Typed};
use crate::{serde::Serializable, Reflect, TypeInfo, Typed};
use bevy_ptr::{Ptr, PtrMut};
use bevy_utils::{HashMap, HashSet};
use downcast_rs::{impl_downcast, Downcast};
use parking_lot::{RwLock, RwLockReadGuard, RwLockWriteGuard};
Expand Down Expand Up @@ -411,11 +411,139 @@ impl<T: for<'a> Deserialize<'a> + Reflect> FromType<T> for ReflectDeserialize {
}
}

/// [`Reflect`] values are commonly used in situations where the actual types of values
/// are not known at runtime. In such situations you might have access to a `*const ()` pointer
/// that you know implements [`Reflect`], but have no way of turning it into a `&dyn Reflect`.
///
/// This is where [`ReflectFromPtr`] comes in, when creating a [`ReflectFromPtr`] for a given type `T: Reflect`.
/// Internally, this saves a concrete function `*const T -> const dyn Reflect` which lets you create a trait object of [`Reflect`]
/// from a pointer.
///
/// # Example
/// ```rust
/// use bevy_reflect::{TypeRegistry, Reflect, ReflectFromPtr};
/// use bevy_ptr::Ptr;
/// use std::ptr::NonNull;
///
/// #[derive(Reflect)]
/// struct Reflected(String);
///
/// let mut type_registry = TypeRegistry::default();
/// type_registry.register::<Reflected>();
///
/// let mut value = Reflected("Hello world!".to_string());
/// let value = unsafe { Ptr::new(NonNull::from(&mut value).cast()) };
///
/// let reflect_data = type_registry.get(std::any::TypeId::of::<Reflected>()).unwrap();
/// let reflect_from_ptr = reflect_data.data::<ReflectFromPtr>().unwrap();
/// // SAFE: `value` is of type `Reflected`, which the `ReflectFromPtr` was created for
/// let value = unsafe { reflect_from_ptr.as_reflect_ptr(value) };
///
/// assert_eq!(value.downcast_ref::<Reflected>().unwrap().0, "Hello world!");
/// ```
#[derive(Clone)]
pub struct ReflectFromPtr {
jakobhellermann marked this conversation as resolved.
Show resolved Hide resolved
type_id: TypeId,
to_reflect: for<'a> unsafe fn(Ptr<'a>) -> &'a dyn Reflect,
to_reflect_mut: for<'a> unsafe fn(PtrMut<'a>) -> &'a mut dyn Reflect,
}

impl ReflectFromPtr {
/// Returns the [`TypeId`] that the [`ReflectFromPtr`] was constructed for
pub fn type_id(&self) -> TypeId {
self.type_id
}

/// # Safety
///
/// `val` must be a pointer to value of the type that the [`ReflectFromPtr`] was constructed for.
/// This can be verified by checking that the type id returned by [`ReflectFromPtr::type_id`] is the expected one.
pub unsafe fn as_reflect_ptr<'a>(&self, val: Ptr<'a>) -> &'a dyn Reflect {
(self.to_reflect)(val)
}

/// # Safety
///
/// `val` must be a pointer to a value of the type that the [`ReflectFromPtr`] was constructed for
/// This can be verified by checking that the type id returned by [`ReflectFromPtr::type_id`] is the expected one.
pub unsafe fn as_reflect_ptr_mut<'a>(&self, val: PtrMut<'a>) -> &'a mut dyn Reflect {
(self.to_reflect_mut)(val)
}
}

impl<T: Reflect> FromType<T> for ReflectFromPtr {
fn from_type() -> Self {
ReflectFromPtr {
type_id: std::any::TypeId::of::<T>(),
to_reflect: |ptr| {
// SAFE: only called from `as_reflect`, where the `ptr` is guaranteed to be of type `T`,
// and `as_reflect_ptr`, where the caller promises to call it with type `T`
unsafe { ptr.deref::<T>() as &dyn Reflect }
},
to_reflect_mut: |ptr| {
// SAFE: only called from `as_reflect_mut`, where the `ptr` is guaranteed to be of type `T`,
// and `as_reflect_ptr_mut`, where the caller promises to call it with type `T`
unsafe { ptr.deref_mut::<T>() as &mut dyn Reflect }
},
}
}
}

#[cfg(test)]
mod test {
use crate::TypeRegistration;
use std::ptr::NonNull;

use crate::{GetTypeRegistration, ReflectFromPtr, TypeRegistration};
use bevy_ptr::{Ptr, PtrMut};
use bevy_utils::HashMap;

use crate as bevy_reflect;
use crate::Reflect;

#[test]
fn test_reflect_from_ptr() {
#[derive(Reflect)]
struct Foo {
a: f32,
}

let foo_registration = <Foo as GetTypeRegistration>::get_type_registration();
let reflect_from_ptr = foo_registration.data::<ReflectFromPtr>().unwrap();

// not required in this situation because we no nobody messed with the TypeRegistry,
// but in the general case somebody could have replaced the ReflectFromPtr with an
// instance for another type, so then we'd need to check that the type is the expected one
assert_eq!(reflect_from_ptr.type_id(), std::any::TypeId::of::<Foo>());
Copy link
Member

@cart cart Jul 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we're in a place where we can treat "arbitrary rust code running in the users' program" as untrusted. Given that rust can do "anything", if there is arbitrary malicious code in a users' program, it is already compromised no matter what we do. Things like public/private visibility don't mean anything here when you can create a mirror struct that provides access and then unsafely cast a pointer to that struct.

We can only provide these guarantees in a sandbox with tight controls over data that passes the boundary. This api won't provide that, so idk if I see the point here (feel free to tell me if/how I'm wrong).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I thought we were trying to be sound? In that case, we'd treat all safe code as untrusted, and make sure that safe code cannot add requirements to an unsafe function.

Basically, this comment really really confuses me.


let mut value = Foo { a: 1.0 };
{
// SAFETY: lifetime doesn't outlive original value, access is unique
let value = unsafe { PtrMut::new(NonNull::from(&mut value).cast()) };
// SAFETY: reflect_from_ptr was constructed for the correct type
let dyn_reflect = unsafe { reflect_from_ptr.as_reflect_ptr_mut(value) };
match dyn_reflect.reflect_mut() {
bevy_reflect::ReflectMut::Struct(strukt) => {
strukt.field_mut("a").unwrap().apply(&2.0f32);
}
_ => panic!("invalid reflection"),
}
}

{
// SAFETY: lifetime doesn't outlive original value
let value = unsafe { Ptr::new(NonNull::from(&mut value).cast()) };
// SAFETY: reflect_from_ptr was constructed for the correct type
let dyn_reflect = unsafe { reflect_from_ptr.as_reflect_ptr(value) };
match dyn_reflect.reflect_ref() {
bevy_reflect::ReflectRef::Struct(strukt) => {
let a = strukt.field("a").unwrap().downcast_ref::<f32>().unwrap();
assert_eq!(*a, 2.0);
}
_ => panic!("invalid reflection"),
}
}
}

#[test]
fn test_property_type_registration() {
assert_eq!(
Expand Down