From fb923e45447ab8e9eba4cee0403100f9a8f93424 Mon Sep 17 00:00:00 2001 From: Imbris Date: Sat, 25 Feb 2023 14:35:08 -0500 Subject: [PATCH 1/6] Address miri's complaints about vtable shenanigans in MetaTable implementation. * Before pointer casts and reads/deferences were used to extract/attach a vtable pointer as a `usize` (which includes operations that miri reports as UB). Now a function pointer is stored for each concrete type, this points to a function that knows the types involved and can thus leverage safe code to upcast a pointer from the concrete type to a trait object. * `CastFrom` trait now uses a single method that operates on pointers rather than two separate methods for `&T` and `&mut T` (this is so that we don't have to store a separate function pointer for each case). Safety requirements for implementing `CastFrom` having been modified (but can be trivially met by just returning the provided pointer (letting it be automatically coerced to a pointer to the trait object)). * `MetaTable::register` no longer requires an instance of the type being registered. * `MetaTable::get_mut` no longer converts a shared reference to an exclusive one (this was most likely UB). * `MetaIter`/`MetaIterMut` no longer cast aside the `Ref`/`RefMut` guard on each element (which would allow safe code to create aliasing references to items being yielded from these iterators). Instead, `Ref::map` and `RefMut::map` are used. * Converted meta iterators to use a loop rather than recursion for advancing past missing values from try_fetch_internal (not sure if this avoids impacting performance though...). Misc changes: * Added crate-level deny for `unsafe_op_in_unsafe_fn` and added unsafe blocks where needed. This makes it easier to identify where unsafe operations are occuring and to document them. * Bump MSRV to 1.65.0, since we are already bumping it in specs to use GATs and it gives some convenient functions for working with pointers. * Remove unnecessary `extern crate`s in benches/bench.rs (not needed in newer rust editions). --- Cargo.toml | 2 +- benches/bench.rs | 4 - examples/basic_dispatch.rs | 2 +- examples/dyn_sys_data.rs | 10 +- src/lib.rs | 1 + src/meta.rs | 275 +++++++++++++++++---------------- src/world/res_downcast/mod.rs | 12 +- tests/meta_table_safety_113.rs | 13 +- 8 files changed, 165 insertions(+), 154 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 5e7a3925..2514aac8 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -14,7 +14,7 @@ categories = ["concurrency"] license = "MIT OR Apache-2.0" exclude = ["bors.toml", ".travis.yml"] edition = "2021" -rust-version = "1.59.0" +rust-version = "1.65.0" [dependencies] ahash = "0.7.6" diff --git a/benches/bench.rs b/benches/bench.rs index 4d3a20ec..ccb4508b 100644 --- a/benches/bench.rs +++ b/benches/bench.rs @@ -1,9 +1,5 @@ #![feature(test)] -extern crate cgmath; -extern crate shred; -#[macro_use] -extern crate shred_derive; extern crate test; use std::ops::{Index, IndexMut}; diff --git a/examples/basic_dispatch.rs b/examples/basic_dispatch.rs index 9dccf243..d6e04b64 100644 --- a/examples/basic_dispatch.rs +++ b/examples/basic_dispatch.rs @@ -22,7 +22,7 @@ impl<'a> System<'a> for PrintSystem { println!("{:?}", &*b); *b = ResB; // We can mutate ResB here - // because it's `Write`. + // because it's `Write`. } } diff --git a/examples/dyn_sys_data.rs b/examples/dyn_sys_data.rs index 93e5fc84..a5bacea0 100644 --- a/examples/dyn_sys_data.rs +++ b/examples/dyn_sys_data.rs @@ -99,11 +99,7 @@ unsafe impl CastFrom for dyn Reflection where T: Reflection + 'static, { - fn cast(t: &T) -> &Self { - t - } - - fn cast_mut(t: &mut T) -> &mut Self { + fn cast(t: *mut T) -> *mut Self { t } } @@ -253,8 +249,8 @@ fn main() { { let mut table = res.entry().or_insert_with(ReflectionTable::new); - table.register(&Foo); - table.register(&Bar); + table.register::(); + table.register::(); } { diff --git a/src/lib.rs b/src/lib.rs index 8bb91848..f13ada59 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -82,6 +82,7 @@ //! virtual function calls. #![deny(unused_must_use, clippy::disallowed_types)] +#![deny(unsafe_op_in_unsafe_fn)] #![warn(missing_docs)] /// Re-exports from [`atomic_refcell`] diff --git a/src/meta.rs b/src/meta.rs index 71898003..a0a774ac 100644 --- a/src/meta.rs +++ b/src/meta.rs @@ -2,6 +2,7 @@ use std::{any::TypeId, collections::hash_map::Entry, marker::PhantomData}; use ahash::AHashMap as HashMap; +use crate::cell::{AtomicRef, AtomicRefMut}; use crate::{Resource, ResourceId, World}; /// This implements `Send` and `Sync` unconditionally. @@ -14,13 +15,14 @@ unsafe impl Send for Invariant where T: ?Sized {} unsafe impl Sync for Invariant where T: ?Sized {} /// Helper trait for the `MetaTable`. +/// /// This trait is required to be implemented for a trait to be compatible with /// the meta table. /// /// # Safety /// -/// Not casting `self` but e.g. a field to the trait object will result in -/// Undefined Bahavior. +/// The produced pointer must have the same provenance and address as the +/// provided pointer and a vtable that is valid for the type `T`. /// /// # Examples /// @@ -36,26 +38,19 @@ unsafe impl Sync for Invariant where T: ?Sized {} /// where /// T: Foo + 'static, /// { -/// fn cast(t: &T) -> &(dyn Foo + 'static) { -/// t -/// } -/// -/// fn cast_mut(t: &mut T) -> &mut (dyn Foo + 'static) { +/// fn cast(t: *mut T) -> *mut (dyn Foo + 'static) { /// t /// } /// } /// ``` pub unsafe trait CastFrom { - /// Casts an immutable `T` reference to a trait object. - fn cast(t: &T) -> &Self; - - /// Casts a mutable `T` reference to a trait object. - fn cast_mut(t: &mut T) -> &mut Self; + /// Casts a concrete pointer to `T` to a trait object pointer. + fn cast(t: *mut T) -> *mut Self; } /// An iterator for the `MetaTable`. pub struct MetaIter<'a, T: ?Sized + 'a> { - fat: &'a [Fat], + vtable_fns: &'a [fn(*mut ()) -> *mut T], index: usize, tys: &'a [TypeId], // `MetaIter` is invariant over `T` @@ -67,61 +62,43 @@ impl<'a, T> Iterator for MetaIter<'a, T> where T: ?Sized + 'a, { - type Item = &'a T; + type Item = AtomicRef<'a, T>; fn next(&mut self) -> Option<::Item> { - let index = self.index; - self.index += 1; - - // Ugly hack that works due to `UnsafeCell` and distinct resources. - unsafe { - self.world - // SAFETY: We just read the value and don't replace it. - .try_fetch_internal(match self.tys.get(index) { - Some(&x) => ResourceId::from_type_id(x), - None => return None, - }) - .map(|res| { - self.fat[index] - .create_ptr::(Box::as_ref(&res.borrow()) as *const dyn Resource as *const - ()) - }) - // we lengthen the lifetime from `'_` to `'a` here, see above - .map(|ptr| &*ptr) - .or_else(|| self.next()) + loop { + let resource_id = match self.tys.get(self.index) { + Some(&x) => ResourceId::from_type_id(x), + None => return None, + }; + + let index = self.index; + self.index += 1; + + // SAFETY: We just read the value and don't replace it. + if let Some(res) = unsafe { self.world.try_fetch_internal(resource_id) } { + let vtable_fn = self.vtable_fns[index]; + let trait_object = AtomicRef::map(res.borrow(), |res: &Box| { + let ptr: *const dyn Resource = Box::as_ref(res); + let trait_ptr = (vtable_fn)(ptr.cast::<()>().cast_mut()); + // SAFETY: For a particular index we store a corresponding + // TypeId and vtable_fn in tys and vtable_fns respectively. + // We rely on `try_fetch_interal` returning a trait object + // with a concrete type that has the provided TypeId. The + // signature of the closure parameter of `AtomicRef::map` + // should ensure we aren't accidentally extending the + // lifetime here. Also see safety note in `MetaTable::get`. + unsafe { &*trait_ptr } + }); + + return Some(trait_object); + } } } } -struct Fat(usize); - -impl Fat { - pub unsafe fn from_ptr(t: &T) -> Self { - use std::ptr::read; - - assert_unsized::(); - - let fat_ptr = &t as *const &T as *const usize; - // Memory layout: - // [object pointer, vtable pointer] - // ^^^^^^^^^^^^^^ ^^^^^^^^^^^^^^ - // 8 bytes | 8 bytes - // (on 32-bit both have 4 bytes) - let vtable = read::(fat_ptr.offset(1)); - - Fat(vtable) - } - - pub unsafe fn create_ptr(&self, ptr: *const ()) -> *const T { - let fat_ptr: (*const (), usize) = (ptr, self.0); - - *(&fat_ptr as *const (*const (), usize) as *const *const T) - } -} - /// A mutable iterator for the `MetaTable`. pub struct MetaIterMut<'a, T: ?Sized + 'a> { - fat: &'a [Fat], + vtable_fns: &'a [fn(*mut ()) -> *mut T], index: usize, tys: &'a [TypeId], // `MetaIterMut` is invariant over `T` @@ -133,32 +110,72 @@ impl<'a, T> Iterator for MetaIterMut<'a, T> where T: ?Sized + 'a, { - type Item = &'a mut T; + type Item = AtomicRefMut<'a, T>; fn next(&mut self) -> Option<::Item> { - let index = self.index; - self.index += 1; - - // Ugly hack that works due to `UnsafeCell` and distinct resources. - unsafe { - self.world - // SAFETY: We don't swap out the Box or expose a mutable reference to it. - .try_fetch_internal(match self.tys.get(index) { - Some(&x) => ResourceId::from_type_id(x), - None => return None, - }) - .map(|res| { - self.fat[index].create_ptr::(Box::as_mut(&mut res.borrow_mut()) - as *mut dyn Resource - as *const ()) as *mut T - }) - // we lengthen the lifetime from `'_` to `'a` here, see above - .map(|ptr| &mut *ptr) - .or_else(|| self.next()) + loop { + let resource_id = match self.tys.get(self.index) { + Some(&x) => ResourceId::from_type_id(x), + None => return None, + }; + + let index = self.index; + self.index += 1; + + // Note: this relies on implementation details of + // try_fetch_internal! + // SAFETY: We don't swap out the Box or expose a mutable reference to it. + if let Some(res) = unsafe { self.world.try_fetch_internal(resource_id) } { + let vtable_fn = self.vtable_fns[index]; + let trait_object = + AtomicRefMut::map(res.borrow_mut(), |res: &mut Box| { + let ptr: *mut dyn Resource = Box::as_mut(res); + let trait_ptr = (vtable_fn)(ptr.cast::<()>()); + // SAFETY: For a particular index we store a corresponding + // TypeId and vtable_fn in tys and vtable_fns respectively. + // We rely on `try_fetch_interal` returning a trait object + // with a concrete type that has the provided TypeId. The + // signature of the closure parameter of `AtomicRefMut::map` + // should ensure we aren't accidentally extending the + // lifetime here. Also see safety note in + // `MetaTable::get_mut`. + unsafe { &mut *trait_ptr } + }); + + return Some(trait_object); + } } } } +/// Given an address and provenance, produces a pointer to a trait object for +/// which `CastFrom` is implemented. +/// +/// Returned pointer has: +/// * the provenance of the provided pointer +/// * the address of the provided pointer +/// * a vtable that is valid for the concrete type `T` +/// +/// We exclusively operate on pointers here so we only need a single function +/// pointer in the meta-table for both `&T` and `&mut T` cases. +fn attach_vtable(value: *mut ()) -> *mut TraitObject +where + TraitObject: CastFrom + 'static, + T: core::any::Any, +{ + // NOTE: This should be equivalent to `Any::downcast_ref_unchecked` except + // with pointers and we don't require `Any` trait but still require that the + // types are 'static. + let trait_ptr = >::cast(value.cast::()); + // TODO: use `.addr()` when stabilized + // assert that address not changed (to catch some mistakes in CastFrom impl) + assert!( + core::ptr::eq(value, trait_ptr.cast::<()>()), + "Bug: `CastFrom` did not cast `self`" + ); + trait_ptr +} + /// The `MetaTable` which allows to store object-safe trait implementations for /// resources. /// @@ -182,11 +199,7 @@ where /// where /// T: Object + 'static, /// { -/// fn cast(t: &T) -> &Self { -/// t -/// } -/// -/// fn cast_mut(t: &mut T) -> &mut Self { +/// fn cast(t: *mut T) -> *mut Self { /// t /// } /// } @@ -221,8 +234,8 @@ where /// world.insert(ImplementorB(1)); /// /// let mut table = MetaTable::::new(); -/// table.register(&ImplementorA(31415)); // Can just be some instance of type `&ImplementorA`. -/// table.register(&ImplementorB(27182)); +/// table.register::(); +/// table.register::(); /// /// { /// let mut iter = table.iter(&mut world); @@ -231,7 +244,10 @@ where /// } /// ``` pub struct MetaTable { - fat: Vec, + // TODO: When `ptr_metadata` is stabilized we can use that to implement this + // without a function call (and without trying to make assumptions about the + // layout of trait object pointers). https://github.com/rust-lang/rust/issues/81513 + vtable_fns: Vec *mut T>, indices: HashMap, tys: Vec, // `MetaTable` is invariant over `T` @@ -247,25 +263,13 @@ impl MetaTable { } /// Registers a resource `R` that implements the trait `T`. - /// This just needs some instance of type `R` to retrieve the vtable. - /// It doesn't have to be the same object you're calling `get` with later. - pub fn register(&mut self, r: &R) + pub fn register(&mut self) where R: Resource, T: CastFrom + 'static, { - let thin_ptr = r as *const R as usize; - let casted_ptr = >::cast(r); - let thin_casted_ptr = casted_ptr as *const T as *const () as usize; - - assert_eq!( - thin_ptr, thin_casted_ptr, - "Bug: `CastFrom` did not cast `self`" - ); - - let fat = unsafe { Fat::from_ptr(casted_ptr) }; - let ty_id = TypeId::of::(); + let vtable_fn = attach_vtable::; // Important: ensure no entry exists twice! let len = self.indices.len(); @@ -273,12 +277,12 @@ impl MetaTable { Entry::Occupied(occ) => { let ind = *occ.get(); - self.fat[ind] = fat; + self.vtable_fns[ind] = vtable_fn; } Entry::Vacant(vac) => { vac.insert(len); - self.fat.push(fat); + self.vtable_fns.push(vtable_fn); self.tys.push(ty_id); } } @@ -288,28 +292,41 @@ impl MetaTable { /// If `world` doesn't have an implementation for `T` (or it wasn't /// registered), this will return `None`. pub fn get<'a>(&self, res: &'a dyn Resource) -> Option<&'a T> { - unsafe { - self.indices - .get(&res.type_id()) - .map(move |&ind| &*self.fat[ind].create_ptr(res as *const _ as *const ())) - } + self.indices.get(&res.type_id()).map(|&ind| { + let vtable_fn = self.vtable_fns[ind]; + + let ptr = <*const dyn Resource>::cast::<()>(res).cast_mut(); + let trait_ptr = (vtable_fn)(ptr); + // SAFETY: We retrieved the `vtable_fn` via TypeId so it will attach + // a vtable that corresponds with the erased type that the TypeId + // refers to. `vtable_fn` will also preserve the provenance and + // address (so we can safely produce a shared reference since we + // started with one). + unsafe { &*trait_ptr } + }) } /// Tries to convert `world` to a trait object of type `&mut T`. /// If `world` doesn't have an implementation for `T` (or it wasn't /// registered), this will return `None`. - pub fn get_mut<'a>(&self, res: &'a dyn Resource) -> Option<&'a mut T> { - unsafe { - self.indices.get(&res.type_id()).map(move |&ind| { - &mut *(self.fat[ind].create_ptr::(res as *const _ as *const ()) as *mut T) - }) - } + pub fn get_mut<'a>(&self, res: &'a mut dyn Resource) -> Option<&'a mut T> { + self.indices.get(&res.type_id()).map(|&ind| { + let vtable_fn = self.vtable_fns[ind]; + let ptr = <*mut dyn Resource>::cast::<()>(res); + let trait_ptr = (vtable_fn)(ptr); + // SAFETY: We retrieved the `vtable_fn` via TypeId so it will attach + // a vtable that corresponds with the erased type that the TypeId + // refers to. `vtable_fn` will also preserve the provenance and + // address (so we can safely produce a mutable reference since we + // started with one). + unsafe { &mut *trait_ptr } + }) } /// Iterates all resources that implement `T` and were registered. pub fn iter<'a>(&'a self, res: &'a World) -> MetaIter<'a, T> { MetaIter { - fat: &self.fat, + vtable_fns: &self.vtable_fns, index: 0, world: res, tys: &self.tys, @@ -320,7 +337,7 @@ impl MetaTable { /// Iterates all resources that implement `T` and were registered mutably. pub fn iter_mut<'a>(&'a self, res: &'a World) -> MetaIterMut<'a, T> { MetaIterMut { - fat: &self.fat, + vtable_fns: &self.vtable_fns, index: 0, world: res, tys: &self.tys, @@ -335,7 +352,7 @@ where { fn default() -> Self { MetaTable { - fat: Default::default(), + vtable_fns: Default::default(), indices: Default::default(), tys: Default::default(), marker: Default::default(), @@ -363,11 +380,7 @@ mod tests { where T: Object + 'static, { - fn cast(t: &T) -> &Self { - t - } - - fn cast_mut(t: &mut T) -> &mut Self { + fn cast(t: *mut T) -> *mut Self { t } } @@ -404,8 +417,8 @@ mod tests { world.insert(ImplementorB(1)); let mut table = MetaTable::::new(); - table.register(&ImplementorA(125)); - table.register(&ImplementorB(111_111)); + table.register::(); + table.register::(); { let mut iter = table.iter(&world); @@ -415,10 +428,10 @@ mod tests { { let mut iter_mut = table.iter_mut(&world); - let obj = iter_mut.next().unwrap(); + let mut obj = iter_mut.next().unwrap(); obj.method2(3); assert_eq!(obj.method1(), 6); - let obj = iter_mut.next().unwrap(); + let mut obj = iter_mut.next().unwrap(); obj.method2(4); assert_eq!(obj.method1(), 4); } @@ -432,8 +445,8 @@ mod tests { world.insert(ImplementorB(1)); let mut table = MetaTable::::new(); - table.register(&ImplementorA(125)); - table.register(&ImplementorB(111_111)); + table.register::(); + table.register::(); { let mut iter = table.iter(&world); @@ -483,8 +496,8 @@ mod tests { world.insert(ImplementorD); let mut table = MetaTable::::new(); - table.register(&ImplementorC); - table.register(&ImplementorD); + table.register::(); + table.register::(); assert_eq!( table diff --git a/src/world/res_downcast/mod.rs b/src/world/res_downcast/mod.rs index 08f780de..c0dbb7e0 100644 --- a/src/world/res_downcast/mod.rs +++ b/src/world/res_downcast/mod.rs @@ -16,6 +16,7 @@ impl dyn Resource { #[inline] pub fn downcast(self: Box) -> Result, Box> { if self.is::() { + // SAFETY: We just checked that the type is `T`. unsafe { Ok(self.downcast_unchecked()) } } else { Err(self) @@ -31,7 +32,8 @@ impl dyn Resource { /// will result in UB. #[inline] pub unsafe fn downcast_unchecked(self: Box) -> Box { - Box::from_raw(Box::into_raw(self) as *mut T) + // SAFETY: Caller promises the concrete type is `T`. + unsafe { Box::from_raw(Box::into_raw(self) as *mut T) } } /// Returns true if the boxed type is the same as `T` @@ -45,6 +47,7 @@ impl dyn Resource { #[inline] pub fn downcast_ref(&self) -> Option<&T> { if self.is::() { + // SAFETY: We just checked that the type is `T`. unsafe { Some(self.downcast_ref_unchecked()) } } else { Option::None @@ -61,7 +64,8 @@ impl dyn Resource { /// will result in UB. #[inline] pub unsafe fn downcast_ref_unchecked(&self) -> &T { - &*(self as *const Self as *const T) + // SAFETY: Caller promises the concrete type is `T`. + unsafe { &*(self as *const Self as *const T) } } /// Returns some mutable reference to the boxed value if it is of type `T`, @@ -69,6 +73,7 @@ impl dyn Resource { #[inline] pub fn downcast_mut(&mut self) -> Option<&mut T> { if self.is::() { + // SAFETY: We just checked that the type is `T`. unsafe { Some(self.downcast_mut_unchecked()) } } else { Option::None @@ -85,6 +90,7 @@ impl dyn Resource { /// will result in UB. #[inline] pub unsafe fn downcast_mut_unchecked(&mut self) -> &mut T { - &mut *(self as *mut Self as *mut T) + // SAFETY: Caller promises the concrete type is `T`. + unsafe { &mut *(self as *mut Self as *mut T) } } } diff --git a/tests/meta_table_safety_113.rs b/tests/meta_table_safety_113.rs index 990ab8d4..bcb7cff1 100644 --- a/tests/meta_table_safety_113.rs +++ b/tests/meta_table_safety_113.rs @@ -18,13 +18,12 @@ struct MultipleData { } unsafe impl CastFrom for dyn PointsToU64 { - fn cast(t: &MultipleData) -> &Self { - // this is wrong and will cause a panic - &t.pointer - } + fn cast(t: *mut MultipleData) -> *mut Self { + // note, this also assumes the pointer is non-null and probably other things which we can't + // assume in an implementation of `CastFrom`. - fn cast_mut(t: &mut MultipleData) -> &mut Self { - &mut t.pointer + // this is wrong and will cause a panic + unsafe { core::ptr::addr_of_mut!((*t).pointer) } } } @@ -36,7 +35,7 @@ fn test_panics() { _number: 0x0, // this will be casted to a pointer, then dereferenced pointer: Box::new(42), }; - table.register(&md); + table.register::(); if let Some(t) = table.get(&md) { println!("{}", t.get_u64()); } From 8b39ae27568144169ddba4408a29ab5a5dcb7e35 Mon Sep 17 00:00:00 2001 From: Imbris Date: Sat, 25 Feb 2023 16:12:36 -0500 Subject: [PATCH 2/6] Disable Miri leak checking for now --- .github/workflows/ci.yml | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 666ae9ab..13eb6903 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -71,4 +71,7 @@ jobs: rustup override set nightly cargo miri setup - name: Test with Miri - run: cargo miri test + # Miri currently reports leaks in some tests so we disable that check + # here (might be due to ptr-int-ptr in crossbeam-epoch so might be + # resolved in future versions of that crate). + run: MIRIFLAGS="-Zmiri-ignore-leaks" cargo miri test From 56d9c21b4196589524d5bdea042358d4141177c4 Mon Sep 17 00:00:00 2001 From: Imbris Date: Sat, 25 Feb 2023 16:50:04 -0500 Subject: [PATCH 3/6] Update MSRV in CI --- .github/workflows/ci.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 13eb6903..8e3fcb91 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -20,7 +20,7 @@ jobs: - nightly - beta - stable - - 1.59.0 # MSRV + - 1.65.0 # MSRV timeout-minutes: 10 From 811f9e5d21e8daa80ac87f5b38fb3282f7c35841 Mon Sep 17 00:00:00 2001 From: Imbris Date: Sat, 25 Feb 2023 17:01:54 -0500 Subject: [PATCH 4/6] Appease clippy --- clippy.toml | 2 +- src/meta.rs | 1 + tests/meta_table_safety_113.rs | 17 +++++++++-------- 3 files changed, 11 insertions(+), 9 deletions(-) diff --git a/clippy.toml b/clippy.toml index 8622df3f..a0f7d24a 100644 --- a/clippy.toml +++ b/clippy.toml @@ -1,2 +1,2 @@ -msrv = "1.56.1" +msrv = "1.65.0" disallowed-types = ["std::collections::HashMap"] diff --git a/src/meta.rs b/src/meta.rs index a0a774ac..a7059c98 100644 --- a/src/meta.rs +++ b/src/meta.rs @@ -64,6 +64,7 @@ where { type Item = AtomicRef<'a, T>; + #[allow(clippy::borrowed_box)] // variant of https://github.com/rust-lang/rust-clippy/issues/5770 fn next(&mut self) -> Option<::Item> { loop { let resource_id = match self.tys.get(self.index) { diff --git a/tests/meta_table_safety_113.rs b/tests/meta_table_safety_113.rs index bcb7cff1..7be0b97f 100644 --- a/tests/meta_table_safety_113.rs +++ b/tests/meta_table_safety_113.rs @@ -14,16 +14,17 @@ impl PointsToU64 for Box { struct MultipleData { _number: u64, - pointer: Box, + _pointer: Box, } unsafe impl CastFrom for dyn PointsToU64 { - fn cast(t: *mut MultipleData) -> *mut Self { - // note, this also assumes the pointer is non-null and probably other things which we can't - // assume in an implementation of `CastFrom`. - - // this is wrong and will cause a panic - unsafe { core::ptr::addr_of_mut!((*t).pointer) } + fn cast(_t: *mut MultipleData) -> *mut Self { + // This is wrong and will cause a panic + // + // NOTE: we use this instead of constructing a pointer to the field since + // there is no way to easily and safely do that currently! (this can be + // changed if offset_of macro is added to std). + core::ptr::NonNull::>::dangling().as_ptr() } } @@ -33,7 +34,7 @@ fn test_panics() { let mut table: MetaTable = MetaTable::new(); let md = MultipleData { _number: 0x0, // this will be casted to a pointer, then dereferenced - pointer: Box::new(42), + _pointer: Box::new(42), }; table.register::(); if let Some(t) = table.get(&md) { From 146b35d212befd21f0436856756690d1fcf80791 Mon Sep 17 00:00:00 2001 From: Imbris Date: Tue, 28 Feb 2023 20:02:42 -0500 Subject: [PATCH 5/6] Remove contine-on-error since miri tests now pass --- .github/workflows/ci.yml | 1 - 1 file changed, 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 8e3fcb91..220f59f9 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -62,7 +62,6 @@ jobs: miri: name: "Miri" runs-on: ubuntu-latest - continue-on-error: true # Needed until all Miri errors are fixed steps: - uses: actions/checkout@v3 - name: Install Miri From de124d934c1b207caba32311ec191875c588d83f Mon Sep 17 00:00:00 2001 From: Imbris Date: Sun, 23 Jul 2023 16:41:24 -0400 Subject: [PATCH 6/6] Add nightly feature to enable MetaTable using ptr_metadata feature, which is potentially more efficient --- Cargo.toml | 1 + src/lib.rs | 1 + src/meta.rs | 198 ++++++++++++++++++++++++++++++++++++++++++++++++++-- 3 files changed, 196 insertions(+), 4 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 2514aac8..ee77eaf5 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -35,6 +35,7 @@ shred-derive = { path = "shred-derive", version = "0.6.3" } [features] default = ["parallel", "shred-derive"] parallel = ["rayon"] +nightly = [] [[example]] name = "async" diff --git a/src/lib.rs b/src/lib.rs index f13ada59..7857e6dc 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -1,3 +1,4 @@ +#![cfg_attr(feature = "nightly", feature(ptr_metadata, strict_provenance))] //! **Sh**ared **re**source **d**ispatcher //! //! This library allows to dispatch diff --git a/src/meta.rs b/src/meta.rs index a7059c98..5982eccb 100644 --- a/src/meta.rs +++ b/src/meta.rs @@ -5,6 +5,9 @@ use ahash::AHashMap as HashMap; use crate::cell::{AtomicRef, AtomicRefMut}; use crate::{Resource, ResourceId, World}; +#[cfg(feature = "nightly")] +use core::ptr::{DynMetadata, Pointee}; + /// This implements `Send` and `Sync` unconditionally. /// (the trait itself doesn't need to have these bounds and the /// resources are already guaranteed to fulfill it). @@ -50,7 +53,10 @@ pub unsafe trait CastFrom { /// An iterator for the `MetaTable`. pub struct MetaIter<'a, T: ?Sized + 'a> { + #[cfg(not(feature = "nightly"))] vtable_fns: &'a [fn(*mut ()) -> *mut T], + #[cfg(feature = "nightly")] + vtables: &'a [DynMetadata], index: usize, tys: &'a [TypeId], // `MetaIter` is invariant over `T` @@ -58,6 +64,7 @@ pub struct MetaIter<'a, T: ?Sized + 'a> { world: &'a World, } +#[cfg(not(feature = "nightly"))] impl<'a, T> Iterator for MetaIter<'a, T> where T: ?Sized + 'a, @@ -97,9 +104,53 @@ where } } +#[cfg(feature = "nightly")] +impl<'a, T> Iterator for MetaIter<'a, T> +where + T: ?Sized + 'a, + T: Pointee>, +{ + type Item = AtomicRef<'a, T>; + + #[allow(clippy::borrowed_box)] // variant of https://github.com/rust-lang/rust-clippy/issues/5770 + fn next(&mut self) -> Option<::Item> { + loop { + let resource_id = match self.tys.get(self.index) { + Some(&x) => ResourceId::from_type_id(x), + None => return None, + }; + + let index = self.index; + self.index += 1; + + // SAFETY: We just read the value and don't replace it. + if let Some(res) = unsafe { self.world.try_fetch_internal(resource_id) } { + let vtable = self.vtables[index]; + let trait_object = AtomicRef::map(res.borrow(), |res: &Box| { + let ptr: *const dyn Resource = Box::as_ref(res); + let trait_ptr = core::ptr::from_raw_parts(ptr.cast::<()>(), vtable); + // SAFETY: For a particular index we store a corresponding + // TypeId and vtable in tys and vtables respectively. + // We rely on `try_fetch_interal` returning a trait object + // with a concrete type that has the provided TypeId. The + // signature of the closure parameter of `AtomicRef::map` + // should ensure we aren't accidentally extending the + // lifetime here. Also see safety note in `MetaTable::get`. + unsafe { &*trait_ptr } + }); + + return Some(trait_object); + } + } + } +} + /// A mutable iterator for the `MetaTable`. pub struct MetaIterMut<'a, T: ?Sized + 'a> { + #[cfg(not(feature = "nightly"))] vtable_fns: &'a [fn(*mut ()) -> *mut T], + #[cfg(feature = "nightly")] + vtables: &'a [DynMetadata], index: usize, tys: &'a [TypeId], // `MetaIterMut` is invariant over `T` @@ -107,6 +158,7 @@ pub struct MetaIterMut<'a, T: ?Sized + 'a> { world: &'a World, } +#[cfg(not(feature = "nightly"))] impl<'a, T> Iterator for MetaIterMut<'a, T> where T: ?Sized + 'a, @@ -149,6 +201,50 @@ where } } +#[cfg(feature = "nightly")] +impl<'a, T> Iterator for MetaIterMut<'a, T> +where + T: ?Sized + 'a, + T: Pointee>, +{ + type Item = AtomicRefMut<'a, T>; + + fn next(&mut self) -> Option<::Item> { + loop { + let resource_id = match self.tys.get(self.index) { + Some(&x) => ResourceId::from_type_id(x), + None => return None, + }; + + let index = self.index; + self.index += 1; + + // Note: this relies on implementation details of + // try_fetch_internal! + // SAFETY: We don't swap out the Box or expose a mutable reference to it. + if let Some(res) = unsafe { self.world.try_fetch_internal(resource_id) } { + let vtable = self.vtables[index]; + let trait_object = + AtomicRefMut::map(res.borrow_mut(), |res: &mut Box| { + let ptr: *mut dyn Resource = Box::as_mut(res); + let trait_ptr = core::ptr::from_raw_parts_mut(ptr.cast::<()>(), vtable); + // SAFETY: For a particular index we store a corresponding + // TypeId and vtable in tys and vtables respectively. + // We rely on `try_fetch_interal` returning a trait object + // with a concrete type that has the provided TypeId. The + // signature of the closure parameter of `AtomicRefMut::map` + // should ensure we aren't accidentally extending the + // lifetime here. Also see safety note in + // `MetaTable::get_mut`. + unsafe { &mut *trait_ptr } + }); + + return Some(trait_object); + } + } + } +} + /// Given an address and provenance, produces a pointer to a trait object for /// which `CastFrom` is implemented. /// @@ -159,6 +255,7 @@ where /// /// We exclusively operate on pointers here so we only need a single function /// pointer in the meta-table for both `&T` and `&mut T` cases. +#[cfg(not(feature = "nightly"))] fn attach_vtable(value: *mut ()) -> *mut TraitObject where TraitObject: CastFrom + 'static, @@ -245,10 +342,10 @@ where /// } /// ``` pub struct MetaTable { - // TODO: When `ptr_metadata` is stabilized we can use that to implement this - // without a function call (and without trying to make assumptions about the - // layout of trait object pointers). https://github.com/rust-lang/rust/issues/81513 + #[cfg(not(feature = "nightly"))] vtable_fns: Vec *mut T>, + #[cfg(feature = "nightly")] + vtables: Vec>, indices: HashMap, tys: Vec, // `MetaTable` is invariant over `T` @@ -258,12 +355,15 @@ pub struct MetaTable { impl MetaTable { /// Creates a new `MetaTable`. pub fn new() -> Self { + // TODO: when ptr_metadata is stablilized this can just be a trait bound: Pointee> assert_unsized::(); Default::default() } /// Registers a resource `R` that implements the trait `T`. + #[cfg(not(feature = "nightly"))] pub fn register(&mut self) where R: Resource, @@ -289,9 +389,47 @@ impl MetaTable { } } + /// Registers a resource `R` that implements the trait `T`. + #[cfg(feature = "nightly")] + pub fn register(&mut self) + where + R: Resource, + T: CastFrom + 'static, + T: Pointee>, + { + let ty_id = TypeId::of::(); + // use self.addr() for unpredictable address to use for checking consistency below + let invalid_ptr = core::ptr::invalid_mut::((self as *mut Self).addr()); + let trait_ptr = >::cast(invalid_ptr); + // assert that address not changed (to catch some mistakes in CastFrom impl) + assert_eq!( + invalid_ptr.addr(), + trait_ptr.addr(), + "Bug: `CastFrom` did not cast `self`" + ); + let vtable = core::ptr::metadata(trait_ptr); + + // Important: ensure no entry exists twice! + let len = self.indices.len(); + match self.indices.entry(ty_id) { + Entry::Occupied(occ) => { + let ind = *occ.get(); + + self.vtables[ind] = vtable; + } + Entry::Vacant(vac) => { + vac.insert(len); + + self.vtables.push(vtable); + self.tys.push(ty_id); + } + } + } + /// Tries to convert `world` to a trait object of type `&T`. /// If `world` doesn't have an implementation for `T` (or it wasn't /// registered), this will return `None`. + #[cfg(not(feature = "nightly"))] pub fn get<'a>(&self, res: &'a dyn Resource) -> Option<&'a T> { self.indices.get(&res.type_id()).map(|&ind| { let vtable_fn = self.vtable_fns[ind]; @@ -307,9 +445,31 @@ impl MetaTable { }) } + /// Tries to convert `world` to a trait object of type `&T`. + /// If `world` doesn't have an implementation for `T` (or it wasn't + /// registered), this will return `None`. + #[cfg(feature = "nightly")] + pub fn get<'a>(&self, res: &'a dyn Resource) -> Option<&'a T> + where + T: Pointee>, + { + self.indices.get(&res.type_id()).map(|&ind| { + let vtable = self.vtables[ind]; + let ptr = <*const dyn Resource>::cast::<()>(res); + let trait_ptr = core::ptr::from_raw_parts(ptr, vtable); + // SAFETY: We retrieved the `vtable` via TypeId so it will be a + // vtable that corresponds with the erased type that the TypeId + // refers to. `from_raw_parts` will also preserve the provenance and + // address (so we can safely produce a shared reference since we + // started with one). + unsafe { &*trait_ptr } + }) + } + /// Tries to convert `world` to a trait object of type `&mut T`. /// If `world` doesn't have an implementation for `T` (or it wasn't /// registered), this will return `None`. + #[cfg(not(feature = "nightly"))] pub fn get_mut<'a>(&self, res: &'a mut dyn Resource) -> Option<&'a mut T> { self.indices.get(&res.type_id()).map(|&ind| { let vtable_fn = self.vtable_fns[ind]; @@ -324,10 +484,34 @@ impl MetaTable { }) } + /// Tries to convert `world` to a trait object of type `&mut T`. + /// If `world` doesn't have an implementation for `T` (or it wasn't + /// registered), this will return `None`. + #[cfg(feature = "nightly")] + pub fn get_mut<'a>(&self, res: &'a mut dyn Resource) -> Option<&'a mut T> + where + T: Pointee>, + { + self.indices.get(&res.type_id()).map(|&ind| { + let vtable = self.vtables[ind]; + let ptr = <*mut dyn Resource>::cast::<()>(res); + let trait_ptr = core::ptr::from_raw_parts_mut(ptr, vtable); + // SAFETY: We retrieved the `vtable` via TypeId so it will be a + // vtable that corresponds with the erased type that the TypeId + // refers to. `from_raw_parts_mut` will also preserve the provenance + // and address (so we can safely produce a mutable reference since + // we started with one). + unsafe { &mut *trait_ptr } + }) + } + /// Iterates all resources that implement `T` and were registered. pub fn iter<'a>(&'a self, res: &'a World) -> MetaIter<'a, T> { MetaIter { + #[cfg(not(feature = "nightly"))] vtable_fns: &self.vtable_fns, + #[cfg(feature = "nightly")] + vtables: &self.vtables, index: 0, world: res, tys: &self.tys, @@ -338,7 +522,10 @@ impl MetaTable { /// Iterates all resources that implement `T` and were registered mutably. pub fn iter_mut<'a>(&'a self, res: &'a World) -> MetaIterMut<'a, T> { MetaIterMut { + #[cfg(not(feature = "nightly"))] vtable_fns: &self.vtable_fns, + #[cfg(feature = "nightly")] + vtables: &self.vtables, index: 0, world: res, tys: &self.tys, @@ -353,7 +540,10 @@ where { fn default() -> Self { MetaTable { + #[cfg(not(feature = "nightly"))] vtable_fns: Default::default(), + #[cfg(feature = "nightly")] + vtables: Default::default(), indices: Default::default(), tys: Default::default(), marker: Default::default(), @@ -362,7 +552,7 @@ where } fn assert_unsized() { - use std::mem::size_of; + use core::mem::size_of; assert_eq!(size_of::<&T>(), 2 * size_of::()); }