From 6f24116369a7d14858791bf85e1f8ea7590cbf19 Mon Sep 17 00:00:00 2001 From: Zicklag Date: Wed, 23 Aug 2023 19:11:01 +0000 Subject: [PATCH] fix: fix soundess issue in bones_ecs related to `UntypedResource` access. (#185) Previously it was possible to change the underlying `SchemaBox` for an `UntypedResource` through it's cell, which would allow de-syncing the schema ID of the box, which was relied on for the typed resources store. This would be unsound. To fix it we prevent accessing the `SchemaBox` in an `UntypedResource` directly and only give out atomic wrappers around `SchemaRef` and `SchemaRefMut`. This required switching from `atomic_refcell` to `atomicell`, which had the ability to do unsafe maps on the atomic borrows. Both libraries are fundamentally similar in idea and implementation, and I like the way that `atomicell` allows us to create our own borrowed types, so I think it's a benefit either way. --- Cargo.lock | 8 +- framework_crates/bones_ecs/Cargo.toml | 12 +- framework_crates/bones_ecs/src/components.rs | 24 ++-- framework_crates/bones_ecs/src/lib.rs | 4 +- framework_crates/bones_ecs/src/resources.rs | 132 +++++++++++++----- framework_crates/bones_ecs/src/stage.rs | 2 +- framework_crates/bones_ecs/src/system.rs | 20 +-- framework_crates/bones_ecs/src/world.rs | 8 +- .../bones_framework/src/localization.rs | 4 +- .../bones_framework/src/params.rs | 6 +- framework_crates/bones_lib/src/lib.rs | 2 +- 11 files changed, 144 insertions(+), 78 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 081adc58da..b6ae571a20 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -328,6 +328,12 @@ version = "0.1.11" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "112ef6b3f6cb3cb6fc5b6b494ef7a848492cff1ab0ef4de10b0f7d572861c905" +[[package]] +name = "atomicell" +version = "0.1.9" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "157342dd84c64f16899b4b16c1fb2cce54b887990362aac3c590b3d13810890f" + [[package]] name = "autocfg" version = "1.1.0" @@ -1074,7 +1080,7 @@ name = "bones_ecs" version = "0.3.0" dependencies = [ "anyhow", - "atomic_refcell", + "atomicell", "bitset-core", "bones_schema", "bones_utils", diff --git a/framework_crates/bones_ecs/Cargo.toml b/framework_crates/bones_ecs/Cargo.toml index 85f7aa39f3..004a8e06e8 100644 --- a/framework_crates/bones_ecs/Cargo.toml +++ b/framework_crates/bones_ecs/Cargo.toml @@ -23,12 +23,12 @@ keysize32 = [] bones_utils = { version = "0.3", path = "../bones_utils" } bones_schema = { version = "0.3", path = "../bones_schema" } -anyhow = "1.0" -atomic_refcell = "0.1" -bitset-core = "0.1" -thiserror = "1.0" -glam = { version = "0.24", optional = true } -paste = { version = "1.0", optional = true } +anyhow = "1.0" +atomicell = "0.1" +bitset-core = "0.1" +thiserror = "1.0" +glam = { version = "0.24", optional = true } +paste = { version = "1.0", optional = true } [dev-dependencies] glam = "0.24" diff --git a/framework_crates/bones_ecs/src/components.rs b/framework_crates/bones_ecs/src/components.rs index f4d39518f7..e6427cd667 100644 --- a/framework_crates/bones_ecs/src/components.rs +++ b/framework_crates/bones_ecs/src/components.rs @@ -12,7 +12,7 @@ pub use iterator::*; pub use typed::*; pub use untyped::*; -type AtomicComponentStore = Arc>>; +type AtomicComponentStore = Arc>>; /// A collection of [`ComponentStore`]. /// @@ -20,7 +20,7 @@ type AtomicComponentStore = Arc>>; /// initialized for that world. #[derive(Default)] pub struct ComponentStores { - pub(crate) components: HashMap>>, + pub(crate) components: HashMap>>, } // SOUND: all of the functions for ComponentStores requires that the types stored implement Sync + @@ -46,9 +46,9 @@ impl ComponentStores { /// Initialize component storage for type `T`. pub fn init(&mut self) { let schema = T::schema(); - self.components.entry(schema.id()).or_insert_with(|| { - Arc::new(AtomicRefCell::new(UntypedComponentStore::for_type::())) - }); + self.components + .entry(schema.id()) + .or_insert_with(|| Arc::new(AtomicCell::new(UntypedComponentStore::for_type::()))); } /// Get the components of a certain type @@ -59,8 +59,8 @@ impl ComponentStores { // `UntypedComponentStore`. unsafe { Ok(std::mem::transmute::< - Arc>, - Arc>>, + Arc>, + Arc>>, >(untyped)) } } @@ -68,7 +68,7 @@ impl ComponentStores { /// Borrow a component store. /// # Errors /// Errors if the component store has not been initialized yet. - pub fn get(&self) -> Result>, EcsError> { + pub fn get(&self) -> Result>, EcsError> { let id = T::schema().id(); let atomicref = self .components @@ -77,7 +77,7 @@ impl ComponentStores { .borrow(); // SOUND: ComponentStore is repr(transparent) over UntypedComponent store. - let atomicref = AtomicRef::map(atomicref, |x| unsafe { + let atomicref = Ref::map(atomicref, |x| unsafe { std::mem::transmute::<&UntypedComponentStore, &ComponentStore>(x) }); @@ -87,7 +87,7 @@ impl ComponentStores { /// Borrow a component store. /// # Errors /// Errors if the component store has not been initialized yet. - pub fn get_mut(&self) -> Result>, EcsError> { + pub fn get_mut(&self) -> Result>, EcsError> { let id = T::schema().id(); let atomicref = self .components @@ -96,7 +96,7 @@ impl ComponentStores { .borrow_mut(); // SOUND: ComponentStore is repr(transparent) over UntypedComponent store. - let atomicref = AtomicRefMut::map(atomicref, |x| unsafe { + let atomicref = RefMut::map(atomicref, |x| unsafe { std::mem::transmute::<&mut UntypedComponentStore, &mut ComponentStore>(x) }); @@ -107,7 +107,7 @@ impl ComponentStores { pub fn get_cell_by_schema_id( &self, id: SchemaId, - ) -> Result>, EcsError> { + ) -> Result>, EcsError> { self.components .get(&id) .cloned() diff --git a/framework_crates/bones_ecs/src/lib.rs b/framework_crates/bones_ecs/src/lib.rs index 26912c02f5..0e53d3b566 100644 --- a/framework_crates/bones_ecs/src/lib.rs +++ b/framework_crates/bones_ecs/src/lib.rs @@ -9,7 +9,7 @@ pub mod atomic { //! Atomic Refcells are from the [`atomic_refcell`] crate. //! //! [`atomic_refcell`]: https://docs.rs/atomic_refcell - pub use atomic_refcell::*; + pub use atomicell::*; } pub mod bitset; pub mod components; @@ -30,7 +30,7 @@ pub use world::{FromWorld, World}; /// The prelude. pub mod prelude { pub use { - atomic_refcell::*, bitset_core::BitSet, bones_schema::prelude::*, bones_utils::prelude::*, + atomicell::*, bitset_core::BitSet, bones_schema::prelude::*, bones_utils::prelude::*, }; pub use crate::{ diff --git a/framework_crates/bones_ecs/src/resources.rs b/framework_crates/bones_ecs/src/resources.rs index b936234d75..7fc02d368f 100644 --- a/framework_crates/bones_ecs/src/resources.rs +++ b/framework_crates/bones_ecs/src/resources.rs @@ -2,6 +2,8 @@ use std::{fmt::Debug, marker::PhantomData, sync::Arc}; +use atomicell::borrow::{AtomicBorrow, AtomicBorrowMut}; + use crate::prelude::*; /// Storage for un-typed resources. @@ -16,10 +18,8 @@ pub struct UntypedResources { } /// An untyped resource that may be inserted into [`UntypedResources`]. -#[derive(Deref, DerefMut)] pub struct UntypedAtomicResource { - #[deref] - cell: Arc>, + cell: Arc>, schema: &'static Schema, } @@ -28,7 +28,7 @@ impl UntypedAtomicResource { pub fn new(resource: SchemaBox) -> Self { Self { schema: resource.schema(), - cell: Arc::new(AtomicRefCell::new(resource)), + cell: Arc::new(AtomicCell::new(resource)), } } @@ -36,7 +36,7 @@ impl UntypedAtomicResource { /// value for the schema. pub fn from_schema(schema: &'static Schema) -> Self { Self { - cell: Arc::new(AtomicRefCell::new(SchemaBox::default(schema))), + cell: Arc::new(AtomicCell::new(SchemaBox::default(schema))), schema, } } @@ -49,16 +49,64 @@ impl UntypedAtomicResource { } } + /// Borrow the resource. + pub fn borrow(&self) -> AtomicSchemaRef { + // SOUND: we keep the borrow along with the reference. + let (reference, borrow) = unsafe { Ref::into_split(self.cell.borrow()) }; + let schema_ref = reference.as_ref(); + AtomicSchemaRef { schema_ref, borrow } + } + + /// Mutably borrow the resource. + pub fn borrow_mut(&self) -> AtomicSchemaRefMut { + // SOUND: we keep the borrow along with the reference. + let (reference, borrow) = unsafe { RefMut::into_split(self.cell.borrow_mut()) }; + let schema_ref = reference.as_mut(); + AtomicSchemaRefMut { schema_ref, borrow } + } + /// Get the schema of the resource. pub fn schema(&self) -> &'static Schema { self.schema } } +/// An atomic borrow of a [`SchemaRef`]. +#[derive(Deref)] +pub struct AtomicSchemaRef<'a> { + #[deref] + schema_ref: SchemaRef<'a>, + borrow: AtomicBorrow<'a>, +} + +impl<'a> AtomicSchemaRef<'a> { + /// # Safety + /// You must know that T represents the data in the [`SchemaRef`]. + pub unsafe fn deref(self) -> Ref<'a, T> { + Ref::with_borrow(self.schema_ref.deref(), self.borrow) + } +} + +/// An atomic borrow of a [`SchemaRefMut`]. +#[derive(Deref, DerefMut)] +pub struct AtomicSchemaRefMut<'a> { + #[deref] + schema_ref: SchemaRefMut<'a, 'a>, + borrow: AtomicBorrowMut<'a>, +} + +impl<'a> AtomicSchemaRefMut<'a> { + /// # Safety + /// You must know that T represents the data in the [`SchemaRefMut`]. + pub unsafe fn deref_mut(self) -> RefMut<'a, T> { + RefMut::with_borrow(self.schema_ref.deref_mut(), self.borrow) + } +} + impl Clone for UntypedAtomicResource { fn clone(&self) -> Self { Self { - cell: Arc::new(AtomicRefCell::new(self.cell.borrow().clone())), + cell: Arc::new(AtomicCell::new((*self.cell.borrow()).clone())), schema: self.schema, } } @@ -82,7 +130,7 @@ impl UntypedResources { &mut self, resource: UntypedAtomicResource, ) -> Option { - let id = resource.cell.borrow().schema().id(); + let id = resource.schema().id(); self.resources.insert(id, resource) } @@ -92,17 +140,13 @@ impl UntypedResources { } /// Get a reference to an untyped resource. - pub fn get(&self, schema_id: SchemaId) -> Option> { - self.resources - .get(&schema_id) - .map(|x| AtomicRefCell::borrow(&x.cell)) + pub fn get(&self, schema_id: SchemaId) -> Option { + self.resources.get(&schema_id).map(|x| x.borrow()) } /// Get a mutable reference to an untyped resource. - pub fn get_mut(&mut self, schema_id: SchemaId) -> Option> { - self.resources - .get(&schema_id) - .map(|x| AtomicRefCell::borrow_mut(&x.cell)) + pub fn get_mut(&mut self, schema_id: SchemaId) -> Option { + self.resources.get(&schema_id).map(|x| x.borrow_mut()) } /// Remove a resource. @@ -138,24 +182,18 @@ impl Resources { } /// Borrow a resource. - pub fn get(&self) -> Option> { - self.untyped.get(T::schema().id()).map(|sbox| { - AtomicRef::map(sbox, |sbox| { - // SOUND: schema matches as checked by retreiving from the untyped store by the schema - // ID. - unsafe { sbox.as_ref().deref() } - }) + pub fn get(&self) -> Option> { + self.untyped.get(T::schema().id()).map(|x| { + // SOUND: untyped resources returns data matching the schema of T. + unsafe { x.deref() } }) } /// Mutably borrow a resource. - pub fn get_mut(&mut self) -> Option> { - self.untyped.get_mut(T::schema().id()).map(|sbox| { - AtomicRefMut::map(sbox, |sbox| { - // SOUND: schema matches as checked by retreiving from the untyped store by the - // schema ID. - unsafe { sbox.as_mut().deref_mut() } - }) + pub fn get_mut(&mut self) -> Option> { + self.untyped.get_mut(T::schema().id()).map(|x| { + // SOUND: untyped resources returns data matching the schema of T. + unsafe { x.deref_mut() } }) } @@ -249,19 +287,19 @@ impl AtomicResource { /// Lock the resource for reading. /// /// This returns a read guard, very similar to an [`RwLock`][std::sync::RwLock]. - pub fn borrow(&self) -> AtomicRef { - let borrow = AtomicRefCell::borrow(&self.untyped); + pub fn borrow(&self) -> Ref { + let borrow = self.untyped.borrow(); // SOUND: We know that the data pointer is valid for type T. - AtomicRef::map(borrow, |data| unsafe { data.as_ref().deref() }) + unsafe { borrow.deref() } } /// Lock the resource for read-writing. /// /// This returns a write guard, very similar to an [`RwLock`][std::sync::RwLock]. - pub fn borrow_mut(&self) -> AtomicRefMut { - let borrow = AtomicRefCell::borrow_mut(&self.untyped); + pub fn borrow_mut(&self) -> RefMut { + let borrow = self.untyped.borrow_mut(); // SOUND: We know that the data pointer is valid for type T. - AtomicRefMut::map(borrow, |data| unsafe { data.as_mut().deref_mut() }) + unsafe { borrow.deref_mut() } } } @@ -298,4 +336,28 @@ mod test { assert_eq!(resources.get::().unwrap().0, 2); assert_eq!(resources.get::().unwrap().0, "world"); } + + #[test] + fn untyped_resources_no_rewrite() { + let mut store = UntypedResources::default(); + + // Create two datas with different types. + let data1 = SchemaBox::new(0usize); + let data1_schema = data1.schema(); + let mut data2 = SchemaBox::new(String::from("bad_data")); + + { + // Try to "cheat" and overwrite the the data1 resource with data of a different type. + store.insert(data1); + let data1_cell = store.get_cell(data1_schema.id()).unwrap(); + let mut data1_borrow = data1_cell.borrow_mut(); + // This can't actually write to data1, it just changes the reference we have. + *data1_borrow = data2.as_mut(); + } + + // Validate that data1 is unchanged + let data1_cell = store.get_cell(data1_schema.id()).unwrap(); + let data1_borrow = data1_cell.borrow(); + assert_eq!(data1_borrow.cast::(), &0); + } } diff --git a/framework_crates/bones_ecs/src/stage.rs b/framework_crates/bones_ecs/src/stage.rs index 65ca9ec872..2c867e3fd3 100644 --- a/framework_crates/bones_ecs/src/stage.rs +++ b/framework_crates/bones_ecs/src/stage.rs @@ -301,7 +301,7 @@ impl CommandQueue { /// /// This is a shortcut for [`ResMut`]. #[derive(Deref, DerefMut)] -pub struct Commands<'a>(AtomicRefMut<'a, CommandQueue>); +pub struct Commands<'a>(RefMut<'a, CommandQueue>); impl<'a> SystemParam for Commands<'a> { type State = AtomicResource; diff --git a/framework_crates/bones_ecs/src/system.rs b/framework_crates/bones_ecs/src/system.rs index 40d698bbbc..63cd3e1d89 100644 --- a/framework_crates/bones_ecs/src/system.rs +++ b/framework_crates/bones_ecs/src/system.rs @@ -141,7 +141,7 @@ pub trait SystemParam: Sized { /// [`SystemParam`] for getting read access to a resource. /// /// Use [`Res`] if you want to automatically initialize the resource. -pub struct Res<'a, T: HasSchema>(AtomicRef<'a, T>); +pub struct Res<'a, T: HasSchema>(Ref<'a, T>); impl<'a, T: HasSchema> std::ops::Deref for Res<'a, T> { type Target = T; fn deref(&self) -> &Self::Target { @@ -153,7 +153,7 @@ impl<'a, T: HasSchema> std::ops::Deref for Res<'a, T> { /// exist. /// /// Use [`Res`] if you don't want to automatically initialize the resource. -pub struct ResInit<'a, T: HasSchema + FromWorld>(AtomicRef<'a, T>); +pub struct ResInit<'a, T: HasSchema + FromWorld>(Ref<'a, T>); impl<'a, T: HasSchema + FromWorld> std::ops::Deref for ResInit<'a, T> { type Target = T; fn deref(&self) -> &Self::Target { @@ -164,7 +164,7 @@ impl<'a, T: HasSchema + FromWorld> std::ops::Deref for ResInit<'a, T> { /// [`SystemParam`] for getting mutable access to a resource. /// /// Use [`ResMutInit`] if you want to automatically initialize the resource. -pub struct ResMut<'a, T: HasSchema>(AtomicRefMut<'a, T>); +pub struct ResMut<'a, T: HasSchema>(RefMut<'a, T>); impl<'a, T: HasSchema> std::ops::Deref for ResMut<'a, T> { type Target = T; fn deref(&self) -> &Self::Target { @@ -181,7 +181,7 @@ impl<'a, T: HasSchema> std::ops::DerefMut for ResMut<'a, T> { /// already exist. /// /// Use [`ResMut`] if you don't want to automatically initialize the resource. -pub struct ResMutInit<'a, T: HasSchema + FromWorld>(AtomicRefMut<'a, T>); +pub struct ResMutInit<'a, T: HasSchema + FromWorld>(RefMut<'a, T>); impl<'a, T: HasSchema + FromWorld> std::ops::Deref for ResMutInit<'a, T> { type Target = T; fn deref(&self) -> &Self::Target { @@ -279,12 +279,12 @@ impl<'a, T: HasSchema + FromWorld> SystemParam for ResMutInit<'a, T> { } /// [`SystemParam`] for getting read access to a [`ComponentStore`]. -pub type Comp<'a, T> = AtomicRef<'a, ComponentStore>; +pub type Comp<'a, T> = Ref<'a, ComponentStore>; /// [`SystemParam`] for getting mutable access to a [`ComponentStore`]. -pub type CompMut<'a, T> = AtomicRefMut<'a, ComponentStore>; +pub type CompMut<'a, T> = RefMut<'a, ComponentStore>; impl<'a, T: HasSchema> SystemParam for Comp<'a, T> { - type State = Arc>>; + type State = Arc>>; type Param<'p> = Comp<'p, T>; fn initialize(world: &mut World) { @@ -301,7 +301,7 @@ impl<'a, T: HasSchema> SystemParam for Comp<'a, T> { } impl<'a, T: HasSchema> SystemParam for CompMut<'a, T> { - type State = Arc>>; + type State = Arc>>; type Param<'p> = CompMut<'p, T>; fn initialize(world: &mut World) { @@ -436,8 +436,8 @@ mod tests { #[test] fn convert_system() { fn tmp( - _var1: AtomicRef>, - _var2: AtomicRef>, + _var1: Ref>, + _var2: Ref>, _var3: Res, _var4: ResMut, ) -> SystemResult { diff --git a/framework_crates/bones_ecs/src/world.rs b/framework_crates/bones_ecs/src/world.rs index 87dc98a486..7884909219 100644 --- a/framework_crates/bones_ecs/src/world.rs +++ b/framework_crates/bones_ecs/src/world.rs @@ -113,7 +113,7 @@ impl World { /// # Panics /// Panics if the resource does not exist in the store. #[track_caller] - pub fn resource(&self) -> AtomicRef { + pub fn resource(&self) -> Ref { match self.resources.get::() { Some(r) => r, None => panic!( @@ -128,7 +128,7 @@ impl World { /// # Panics /// Panics if the resource does not exist in the store. #[track_caller] - pub fn resource_mut(&mut self) -> AtomicRefMut { + pub fn resource_mut(&mut self) -> RefMut { match self.resources.get_mut::() { Some(r) => r, None => panic!( @@ -140,12 +140,12 @@ impl World { } /// Borrow a resource from the world, if it exists. - pub fn get_resource(&self) -> Option> { + pub fn get_resource(&self) -> Option> { self.resources.get() } /// Borrow a resource from the world, if it exists. - pub fn get_resource_mut(&mut self) -> Option> { + pub fn get_resource_mut(&mut self) -> Option> { self.resources.get_mut() } } diff --git a/framework_crates/bones_framework/src/localization.rs b/framework_crates/bones_framework/src/localization.rs index 14d327d303..b4961105fa 100644 --- a/framework_crates/bones_framework/src/localization.rs +++ b/framework_crates/bones_framework/src/localization.rs @@ -91,7 +91,7 @@ impl LocalizationAsset { #[derive(Deref, DerefMut)] pub struct Localization<'a, T> { #[deref] - asset: AtomicRef<'a, LocalizationAsset>, + asset: Ref<'a, LocalizationAsset>, _phantom: PhantomData, } @@ -147,7 +147,7 @@ impl SystemParam for Localization<'_, T> { idx.expect(ERR) }); - let asset = AtomicRef::map(asset_server, |asset_server| { + let asset = Ref::map(asset_server, |asset_server| { let root = asset_server.root::().as_schema_ref(); let handle = root .get_field(*field_idx) diff --git a/framework_crates/bones_framework/src/params.rs b/framework_crates/bones_framework/src/params.rs index b6678d8018..1cf7d483af 100644 --- a/framework_crates/bones_framework/src/params.rs +++ b/framework_crates/bones_framework/src/params.rs @@ -3,7 +3,7 @@ use crate::prelude::*; /// Get the root asset of the core asset pack and cast it to type `T`. -pub struct Root<'a, T: HasSchema>(AtomicRef<'a, T>); +pub struct Root<'a, T: HasSchema>(Ref<'a, T>); impl<'a, T: HasSchema> std::ops::Deref for Root<'a, T> { type Target = T; fn deref(&self) -> &Self::Target { @@ -19,8 +19,6 @@ impl<'a, T: HasSchema> SystemParam for Root<'a, T> { world.resources.get_cell::().unwrap() } fn borrow(state: &mut Self::State) -> Self::Param<'_> { - Root(AtomicRef::map(state.borrow(), |asset_server| { - asset_server.root() - })) + Root(Ref::map(state.borrow(), |asset_server| asset_server.root())) } } diff --git a/framework_crates/bones_lib/src/lib.rs b/framework_crates/bones_lib/src/lib.rs index a454797828..5c9446fefe 100644 --- a/framework_crates/bones_lib/src/lib.rs +++ b/framework_crates/bones_lib/src/lib.rs @@ -156,7 +156,7 @@ impl Game { } /// Borrow the asset server. - pub fn asset_server(&self) -> AtomicRefMut { + pub fn asset_server(&self) -> RefMut { self.asset_server.borrow_mut() }