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

jakobhellermann · 2022-04-14T07:51:04Z

Objective

#4447 adds functions that can fetch resources/components as *const () ptr by providing the ComponentId. This alone is not enough for them to be usable safely with reflection, because there is no general way to go from the raw pointer to a &dyn Reflect which is the pointer + a pointer to the VTable of the Reflect impl.

By adding a ReflectFromPtr type that is included in the type type registration when deriving Reflect, safe functions can be implemented in scripting languages that don't assume a type layout and can access the component data via reflection:

#[derive(Reflect)]
struct StringResource {
    value: String
}

local res_id = world:resource_id_by_name("example::StringResource")
local res = world:resource(res_id)

print(res.value)

Solution

add a ReflectFromPtr type with a FromType<T: Reflect> implementation and the following methods:

pub unsafe fn as_reflect_ptr<'a>(&self, val: Ptr<'a>) -> &'a dyn Reflect
pub unsafe fn as_reflect_ptr_mut<'a>(&self, val: PtrMut<'a>) -> &'a mud dyn Reflect

Safety requirements of the methods are that you need to check that the ReflectFromPtr was constructed for the correct type.

add that type to the TypeRegistration in the GetTypeRegistration impl generated by #[derive(Reflect)].
This is different to other reflected traits because it doesn't need #[reflect(ReflectReflectFromPtr)] which IMO should be there by default.

crates/bevy_reflect/src/type_registry.rs

…f &dyn Reflect

jakobhellermann · 2022-04-18T12:16:10Z

I changed the unsafe functions to return a *const dyn Reflect instead of a &dyn Reflect with an arbitrary caller-chosen lifetime. This is safer because now the caller must explicitly dereference the pointer and think about the lifetime it is valid for instead of rust automatically choosing a lifetime which is big enough for its uses.

-/// Safety: correct type and correct chosen lifetime
-pub unsafe fn to_reflect_ptr<'a>(&self, val: *const ()) -> &'a dyn Reflect
+/// Safety: correct type
+pub unsafe fn to_reflect_ptr(&self, val: *const ()) -> *const dyn Reflect

MrGVSV

LGTM! Unfortunately, I don't feel confident enough in Rust pointers to approve it myself, though :/

crates/bevy_reflect/src/type_registry.rs

jakobhellermann · 2022-04-23T09:42:35Z

I noticed a problem in this PR where a malicious user could

impl GetTypeRegisteration for T {
  fn get_type_registration() {
    let mut reg = TypeRegistration::of::<T>();
    reg.insert::<<ReflectFromPtr as FromType<DefinitelyNotT>>::from_type());
    reg
  }
}

Which would make it impossible to use to_reflect_ptr soundly because as a user you cannot rely that the type you fetched it for is the correct one, and the type check in to_reflect is wrong.

This can only be fixed by tweaking the type registration process so that the TypeRegistry maintains the invariant that and TypeData inserted will be 100% for the correct type.

jakobhellermann · 2022-04-23T10:24:22Z

blocked on #4567

# Objective The pointer types introduced in #3001 are useful not just in `bevy_ecs`, but also in crates like `bevy_reflect` (#4475) or even outside of bevy. ## Solution Extract `Ptr<'a>`, `PtrMut<'a>`, `OwnedPtr<'a>`, `ThinSlicePtr<'a, T>` and `UnsafeCellDeref` from `bevy_ecs::ptr` into `bevy_ptr`. **Note:** `bevy_ecs` still reexports the `bevy_ptr` as `bevy_ecs::ptr` so that crates like `bevy_transform` can use the `Bundle` derive without needing to depend on `bevy_ptr` themselves.

jakobhellermann · 2022-05-05T10:33:21Z

Updated the PR to bevy_ptr. And perhaps "blocked on #4567" is a bit much, the PR is still useful for cases where you know the type of the value, it's just that if you don't you can't really trust that the ReflectFromPtr was created for the correct type.

# Objective The pointer types introduced in bevyengine#3001 are useful not just in `bevy_ecs`, but also in crates like `bevy_reflect` (bevyengine#4475) or even outside of bevy. ## Solution Extract `Ptr<'a>`, `PtrMut<'a>`, `OwnedPtr<'a>`, `ThinSlicePtr<'a, T>` and `UnsafeCellDeref` from `bevy_ecs::ptr` into `bevy_ptr`. **Note:** `bevy_ecs` still reexports the `bevy_ptr` as `bevy_ecs::ptr` so that crates like `bevy_transform` can use the `Bundle` derive without needing to depend on `bevy_ptr` themselves.

MrGVSV · 2022-05-12T21:00:33Z

Ping @jakobhellermann this branch likely has merge conflicts from #4712. To resolve, simply move your line of code in bevy_reflect_derive to the new registration.rs file.

MrGVSV

Looks good to me (though, again, not super familiar with pointer manipulation in Rust so take my approval with a grain of salt haha).

crates/bevy_reflect/src/type_registry.rs

PROMETHIA-27

A bit spooky, but I can't see any Undefined Behavior unless you break the type-safety promise. Definitely going to appreciate this for scripting.

crates/bevy_reflect/src/impls/std.rs

alice-i-cecile · 2022-05-17T18:48:10Z

as_reflect and as_reflect_mut seem wholly unnecessary, given &'a T where T: Reflect can be coerced to &'a dyn Reflect without any additional help.

Pulling this comment by @TheRawMeatball up a level; we should block on that feedback being resolved in some form or another.

PROMETHIA-27 · 2022-05-17T18:50:45Z

Hm, you could just replace the body of those functions with their input couldn't you... why are they there?

jakobhellermann · 2022-05-17T18:53:21Z

hm so they were meant as a safer alternative for when you don't need pointers involved but in that case you can just val as &dyn Reflect so the methods are pretty useless. I'll remove them.

Co-authored-by: PROMETHIA-27 <[email protected]>

# Objective The pointer types introduced in bevyengine#3001 are useful not just in `bevy_ecs`, but also in crates like `bevy_reflect` (bevyengine#4475) or even outside of bevy. ## Solution Extract `Ptr<'a>`, `PtrMut<'a>`, `OwnedPtr<'a>`, `ThinSlicePtr<'a, T>` and `UnsafeCellDeref` from `bevy_ecs::ptr` into `bevy_ptr`. **Note:** `bevy_ecs` still reexports the `bevy_ptr` as `bevy_ecs::ptr` so that crates like `bevy_transform` can use the `Bundle` derive without needing to depend on `bevy_ptr` themselves.

cart

Looks good to me! One conversation I'd like to have, but I won't block on it.
(Please reply even after this is merged)

cart · 2022-07-19T22:45:32Z

crates/bevy_reflect/src/type_registry.rs

+        // 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>());


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).

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.

…75-1

cart · 2022-07-19T22:56:14Z

bors r+

…t ()` (#4475) # Objective #4447 adds functions that can fetch resources/components as `*const ()` ptr by providing the `ComponentId`. This alone is not enough for them to be usable safely with reflection, because there is no general way to go from the raw pointer to a `&dyn Reflect` which is the pointer + a pointer to the VTable of the `Reflect` impl. By adding a `ReflectFromPtr` type that is included in the type type registration when deriving `Reflect`, safe functions can be implemented in scripting languages that don't assume a type layout and can access the component data via reflection: ```rust #[derive(Reflect)] struct StringResource { value: String } ``` ```lua local res_id = world:resource_id_by_name("example::StringResource") local res = world:resource(res_id) print(res.value) ``` ## Solution 1. add a `ReflectFromPtr` type with a `FromType<T: Reflect>` implementation and the following methods: - ` pub unsafe fn as_reflect_ptr<'a>(&self, val: Ptr<'a>) -> &'a dyn Reflect` - ` pub unsafe fn as_reflect_ptr_mut<'a>(&self, val: PtrMut<'a>) -> &'a mud dyn Reflect` Safety requirements of the methods are that you need to check that the `ReflectFromPtr` was constructed for the correct type. 2. add that type to the `TypeRegistration` in the `GetTypeRegistration` impl generated by `#[derive(Reflect)]`. This is different to other reflected traits because it doesn't need `#[reflect(ReflectReflectFromPtr)]` which IMO should be there by default. Co-authored-by: Jakob Hellermann <[email protected]> Co-authored-by: Carter Anderson <[email protected]>

bors · 2022-07-19T23:00:13Z

Canceled.

cart · 2022-07-19T23:00:18Z

bors retry

…t ()` (#4475) # Objective #4447 adds functions that can fetch resources/components as `*const ()` ptr by providing the `ComponentId`. This alone is not enough for them to be usable safely with reflection, because there is no general way to go from the raw pointer to a `&dyn Reflect` which is the pointer + a pointer to the VTable of the `Reflect` impl. By adding a `ReflectFromPtr` type that is included in the type type registration when deriving `Reflect`, safe functions can be implemented in scripting languages that don't assume a type layout and can access the component data via reflection: ```rust #[derive(Reflect)] struct StringResource { value: String } ``` ```lua local res_id = world:resource_id_by_name("example::StringResource") local res = world:resource(res_id) print(res.value) ``` ## Solution 1. add a `ReflectFromPtr` type with a `FromType<T: Reflect>` implementation and the following methods: - ` pub unsafe fn as_reflect_ptr<'a>(&self, val: Ptr<'a>) -> &'a dyn Reflect` - ` pub unsafe fn as_reflect_ptr_mut<'a>(&self, val: PtrMut<'a>) -> &'a mud dyn Reflect` Safety requirements of the methods are that you need to check that the `ReflectFromPtr` was constructed for the correct type. 2. add that type to the `TypeRegistration` in the `GetTypeRegistration` impl generated by `#[derive(Reflect)]`. This is different to other reflected traits because it doesn't need `#[reflect(ReflectReflectFromPtr)]` which IMO should be there by default. Co-authored-by: Jakob Hellermann <[email protected]> Co-authored-by: Carter Anderson <[email protected]>

bors · 2022-07-19T23:34:15Z

Pull request successfully merged into main.

Build succeeded:

…t ()` (bevyengine#4475) # Objective bevyengine#4447 adds functions that can fetch resources/components as `*const ()` ptr by providing the `ComponentId`. This alone is not enough for them to be usable safely with reflection, because there is no general way to go from the raw pointer to a `&dyn Reflect` which is the pointer + a pointer to the VTable of the `Reflect` impl. By adding a `ReflectFromPtr` type that is included in the type type registration when deriving `Reflect`, safe functions can be implemented in scripting languages that don't assume a type layout and can access the component data via reflection: ```rust #[derive(Reflect)] struct StringResource { value: String } ``` ```lua local res_id = world:resource_id_by_name("example::StringResource") local res = world:resource(res_id) print(res.value) ``` ## Solution 1. add a `ReflectFromPtr` type with a `FromType<T: Reflect>` implementation and the following methods: - ` pub unsafe fn as_reflect_ptr<'a>(&self, val: Ptr<'a>) -> &'a dyn Reflect` - ` pub unsafe fn as_reflect_ptr_mut<'a>(&self, val: PtrMut<'a>) -> &'a mud dyn Reflect` Safety requirements of the methods are that you need to check that the `ReflectFromPtr` was constructed for the correct type. 2. add that type to the `TypeRegistration` in the `GetTypeRegistration` impl generated by `#[derive(Reflect)]`. This is different to other reflected traits because it doesn't need `#[reflect(ReflectReflectFromPtr)]` which IMO should be there by default. Co-authored-by: Jakob Hellermann <[email protected]> Co-authored-by: Carter Anderson <[email protected]>

# Objective The pointer types introduced in bevyengine#3001 are useful not just in `bevy_ecs`, but also in crates like `bevy_reflect` (bevyengine#4475) or even outside of bevy. ## Solution Extract `Ptr<'a>`, `PtrMut<'a>`, `OwnedPtr<'a>`, `ThinSlicePtr<'a, T>` and `UnsafeCellDeref` from `bevy_ecs::ptr` into `bevy_ptr`. **Note:** `bevy_ecs` still reexports the `bevy_ptr` as `bevy_ecs::ptr` so that crates like `bevy_transform` can use the `Bundle` derive without needing to depend on `bevy_ptr` themselves.

github-actions bot added the S-Needs-Triage This issue needs to be labelled label Apr 14, 2022

jakobhellermann mentioned this pull request Apr 14, 2022

Tracking issue: dynamic APIs and reflection improvements for scripting #4474

Closed

5 tasks

jakobhellermann added C-Feature A new feature, making something new possible A-Reflection Runtime information about types and removed S-Needs-Triage This issue needs to be labelled labels Apr 14, 2022

bevy_reflect: ReflectFromPtr to create &dyn Reflect

cddb930

jakobhellermann force-pushed the bevy-reflect-from-ptr branch from be21c0e to cddb930 Compare April 14, 2022 07:59

MrGVSV reviewed Apr 16, 2022

View reviewed changes

crates/bevy_reflect/src/type_registry.rs Show resolved Hide resolved

crates/bevy_reflect/src/type_registry.rs Outdated Show resolved Hide resolved

crates/bevy_reflect/src/type_registry.rs Outdated Show resolved Hide resolved

jakobhellermann added 3 commits April 18, 2022 13:25

fix '# Safety' comment to be recognized by clippy

3038e3f

change names from to_ to as_ and get use *const dyn Reflect instead o…

6438f55

…f &dyn Reflect

add more docs

4665195

jakobhellermann added 2 commits April 18, 2022 14:16

fix doctest

07b60d6

fix doctest again

e35f498

MrGVSV reviewed Apr 23, 2022

View reviewed changes

crates/bevy_reflect/src/type_registry.rs Outdated Show resolved Hide resolved

crates/bevy_reflect/src/type_registry.rs Show resolved Hide resolved

jakobhellermann mentioned this pull request Apr 23, 2022

bevy_reflect: enforce that type data is only inserted for the correct type #4567

Closed

alice-i-cecile added this to the Bevy 0.8 milestone Apr 25, 2022

jakobhellermann mentioned this pull request May 3, 2022

Reflect and (de)serialize resources in scenes #3580

Closed

3 tasks

Merge branch 'main' into bevy-reflect-from-ptr

9553bfe

jakobhellermann mentioned this pull request May 4, 2022

[Merged by Bors] - bevy_ptr standalone crate #4653

Closed

jakobhellermann added 2 commits May 5, 2022 11:27

Merge branch 'main' into bevy-reflect-from-ptr

b42b30f

use bevy_ptr

75bfd0a

jakobhellermann added 2 commits May 13, 2022 15:24

Merge branch 'main' into bevy-reflect-from-ptr

0016eb1

expose TypeId of ReflectFromPtr

156f9f9

MrGVSV approved these changes May 17, 2022

View reviewed changes

crates/bevy_reflect/src/type_registry.rs Outdated Show resolved Hide resolved

crates/bevy_reflect/src/type_registry.rs Outdated Show resolved Hide resolved

PROMETHIA-27 approved these changes May 17, 2022

View reviewed changes

crates/bevy_reflect/src/impls/std.rs Outdated Show resolved Hide resolved

remove unnecessary methods from ReflectFromPtr

e51c5af

alice-i-cecile approved these changes May 17, 2022

View reviewed changes

alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label May 17, 2022

Update crates/bevy_reflect/src/impls/std.rs

a9ce062

Co-authored-by: PROMETHIA-27 <[email protected]>

TheRawMeatball approved these changes May 17, 2022

View reviewed changes

cart approved these changes Jul 19, 2022

View reviewed changes

Merge remote-tracking branch 'origin/main' into pr/jakobhellermann/44…

bd91c13

…75-1

clippy

4b9e974

bors bot changed the title ~~bevy_reflect: ReflectFromPtr to create &dyn Reflect from a *const ()~~ [Merged by Bors] - bevy_reflect: ReflectFromPtr to create &dyn Reflect from a *const () Jul 19, 2022

bors bot closed this Jul 19, 2022

jakobhellermann deleted the bevy-reflect-from-ptr branch July 20, 2022 07:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

jakobhellermann commented Apr 14, 2022 •

edited

Loading

jakobhellermann commented Apr 18, 2022

MrGVSV left a comment

jakobhellermann commented Apr 23, 2022

jakobhellermann commented Apr 23, 2022

jakobhellermann commented May 5, 2022

MrGVSV commented May 12, 2022

MrGVSV left a comment

PROMETHIA-27 left a comment

alice-i-cecile commented May 17, 2022

PROMETHIA-27 commented May 17, 2022

jakobhellermann commented May 17, 2022

cart left a comment

cart Jul 19, 2022 •

edited

Loading

DJMcNab Jul 20, 2022

cart commented Jul 19, 2022

bors bot commented Jul 19, 2022

cart commented Jul 19, 2022

bors bot commented Jul 19, 2022

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

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

Conversation

jakobhellermann commented Apr 14, 2022 • edited Loading

Objective

Solution

jakobhellermann commented Apr 18, 2022

MrGVSV left a comment

Choose a reason for hiding this comment

jakobhellermann commented Apr 23, 2022

jakobhellermann commented Apr 23, 2022

jakobhellermann commented May 5, 2022

MrGVSV commented May 12, 2022

MrGVSV left a comment

Choose a reason for hiding this comment

PROMETHIA-27 left a comment

Choose a reason for hiding this comment

alice-i-cecile commented May 17, 2022

PROMETHIA-27 commented May 17, 2022

jakobhellermann commented May 17, 2022

cart left a comment

Choose a reason for hiding this comment

cart Jul 19, 2022 • edited Loading

Choose a reason for hiding this comment

DJMcNab Jul 20, 2022

Choose a reason for hiding this comment

cart commented Jul 19, 2022

bors bot commented Jul 19, 2022

cart commented Jul 19, 2022

bors bot commented Jul 19, 2022

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

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

jakobhellermann commented Apr 14, 2022 •

edited

Loading

cart Jul 19, 2022 •

edited

Loading