From 8b5f9c86ca006c3a0dbb8cc91f7ec2a82d5c06f7 Mon Sep 17 00:00:00 2001 From: Thomas Schaller Date: Sun, 31 Mar 2019 10:19:28 +0200 Subject: [PATCH] Audit most uses of unsafe --- src/bitset.rs | 4 +++ src/changeset.rs | 13 ++++++++++ src/join/mod.rs | 40 +++++++++++++++++++++++++++++ src/join/par_join.rs | 18 +++++++++++++ src/storage/drain.rs | 2 ++ src/storage/entry.rs | 12 +++++++++ src/storage/mod.rs | 61 ++++++++++++++++++++++++++++++++++++++++++++ 7 files changed, 150 insertions(+) diff --git a/src/bitset.rs b/src/bitset.rs index b7bfe94f3..27d9cbe02 100644 --- a/src/bitset.rs +++ b/src/bitset.rs @@ -19,9 +19,13 @@ macro_rules! define_bit_join { type Type = Index; type Value = (); type Mask = $bitset; + + // SAFETY: This just moves a `BitSet`; invariants of `Join` are fulfilled, since `Self::Value` cannot be mutated. unsafe fn open(self) -> (Self::Mask, Self::Value) { (self, ()) } + + // SAFETY: No unsafe code and no invariants to meet. unsafe fn get(_: &mut Self::Value, id: Index) -> Self::Type { id } diff --git a/src/changeset.rs b/src/changeset.rs index e0adc3e1a..6f88aecb6 100644 --- a/src/changeset.rs +++ b/src/changeset.rs @@ -59,10 +59,12 @@ impl ChangeSet { T: AddAssign, { if self.mask.contains(entity.id()) { + // SAFETY: we checked the mask, thus it's safe to call unsafe { *self.inner.get_mut(entity.id()) += value; } } else { + // SAFETY: we checked the mask, thus it's safe to call unsafe { self.inner.insert(entity.id(), value); } @@ -73,6 +75,7 @@ impl ChangeSet { /// Clear the changeset pub fn clear(&mut self) { for id in &self.mask { + // SAFETY: we checked the mask, thus it's safe to call unsafe { self.inner.remove(id); } @@ -110,10 +113,13 @@ impl<'a, T> Join for &'a mut ChangeSet { type Value = &'a mut DenseVecStorage; type Mask = &'a BitSet; + // SAFETY: No unsafe code and no invariants to meet. unsafe fn open(self) -> (Self::Mask, Self::Value) { (&self.mask, &mut self.inner) } + // SAFETY: No unsafe code and no invariants to meet. + // `DistinctStorage` invariants are also met, but no `ParJoin` implementation exists yet. unsafe fn get(v: &mut Self::Value, id: Index) -> Self::Type { let value: *mut Self::Value = v as *mut Self::Value; (*value).get_mut(id) @@ -125,24 +131,31 @@ impl<'a, T> Join for &'a ChangeSet { type Value = &'a DenseVecStorage; type Mask = &'a BitSet; + // SAFETY: No unsafe code and no invariants to meet. unsafe fn open(self) -> (Self::Mask, Self::Value) { (&self.mask, &self.inner) } + // SAFETY: No unsafe code and no invariants to meet. + // `DistinctStorage` invariants are also met, but no `ParJoin` implementation exists yet. unsafe fn get(value: &mut Self::Value, id: Index) -> Self::Type { value.get(id) } } +/// A `Join` implementation for `ChangeSet` that simply removes all the entries on a call to `get`. impl Join for ChangeSet { type Type = T; type Value = DenseVecStorage; type Mask = BitSet; + // SAFETY: No unsafe code and no invariants to meet. unsafe fn open(self) -> (Self::Mask, Self::Value) { (self.mask, self.inner) } + // SAFETY: No unsafe code and no invariants to meet. + // `DistinctStorage` invariants are also met, but no `ParJoin` implementation exists yet. unsafe fn get(value: &mut Self::Value, id: Index) -> Self::Type { value.remove(id) } diff --git a/src/join/mod.rs b/src/join/mod.rs index f0e5488cc..635add92e 100644 --- a/src/join/mod.rs +++ b/src/join/mod.rs @@ -223,6 +223,8 @@ pub trait Join { /// Open this join by returning the mask and the storages. /// + /// # Safety + /// /// This is unsafe because implementations of this trait can permit /// the `Value` to be mutated independently of the `Mask`. /// If the `Mask` does not correctly report the status of the `Value` @@ -230,6 +232,11 @@ pub trait Join { unsafe fn open(self) -> (Self::Mask, Self::Value); /// Get a joined component value by a given index. + /// + /// # Safety + /// + /// * A call to `get` must be preceded by a check if `id` is part of `Self::Mask` + /// * The implementation of this method may use unsafe code, but has no invariants to meet unsafe fn get(value: &mut Self::Value, id: Index) -> Self::Type; /// If this `Join` typically returns all indices in the mask, then iterating over only it @@ -261,10 +268,15 @@ where type Type = Option<::Type>; type Value = (::Mask, ::Value); type Mask = BitSetAll; + + // SAFETY: This wraps another implementation of `open`, making it dependent on `J`'s correctness. + // We can safely assume `J` is valid, thus this must be valid, too. No invariants to meet. unsafe fn open(self) -> (Self::Mask, Self::Value) { let (mask, value) = self.0.open(); (BitSetAll, (mask, value)) } + + // SAFETY: No invariants to meet and the unsafe code checks the mask, thus fulfills the requirements for calling `get` unsafe fn get((mask, value): &mut Self::Value, id: Index) -> Self::Type { if mask.contains(id) { Some(::get(value, id)) @@ -293,6 +305,7 @@ impl JoinIter { println!("WARNING: `Join` possibly iterating through all indices, you might've made a join with all `MaybeJoin`s, which is unbounded in length."); } + // SAFETY: We do not swap out the mask or the values, nor do we allow it by exposing them. let (keys, values) = unsafe { j.open() }; JoinIter { keys: keys.iter(), @@ -353,6 +366,7 @@ impl JoinIter { /// ``` pub fn get(&mut self, entity: Entity, entities: &Entities) -> Option { if self.keys.contains(entity.id()) && entities.is_alive(entity) { + // SAFETY: the mask (`keys`) is checked as specified in the docs of `get`. Some(unsafe { J::get(&mut self.values, entity.id()) }) } else { None @@ -367,6 +381,7 @@ impl JoinIter { /// so the caller should ensure it instead. pub fn get_unchecked(&mut self, index: Index) -> Option { if self.keys.contains(index) { + // SAFETY: the mask (`keys`) is checked as specified in the docs of `get`. Some(unsafe { J::get(&mut self.values, index) }) } else { None @@ -378,6 +393,8 @@ impl std::iter::Iterator for JoinIter { type Item = J::Type; fn next(&mut self) -> Option { + // SAFETY: since `idx` is yielded from `keys` (the mask), it is necessarily a part of it. + // Thus, requirements are fulfilled for calling `get`. self.keys .next() .map(|idx| unsafe { J::get(&mut self.values, idx) }) @@ -395,6 +412,9 @@ macro_rules! define_open { type Value = ($($from::Value),*,); type Mask = <($($from::Mask,)*) as BitAnd>::Value; #[allow(non_snake_case)] + + // SAFETY: While we do expose the mask and the values and therefore would allow swapping them, + // this method is `unsafe` and relies on the same invariants. unsafe fn open(self) -> (Self::Mask, Self::Value) { let ($($from,)*) = self; let ($($from,)*) = ($($from.open(),)*); @@ -404,6 +424,8 @@ macro_rules! define_open { ) } + // SAFETY: No invariants to meet and `get` is safe to call as the caller must have checked the mask, + // which only has a key that exists in all of the storages. #[allow(non_snake_case)] unsafe fn get(v: &mut Self::Value, i: Index) -> Self::Type { let &mut ($(ref mut $from,)*) = v; @@ -417,6 +439,10 @@ macro_rules! define_open { unconstrained } } + + // SAFETY: This is safe to implement since all components implement `ParJoin`. + // If the access of every individual `get` leads to disjoint memory access, calling + // all of them after another does in no case lead to access of common memory. #[cfg(feature = "parallel")] unsafe impl<$($from,)*> ParJoin for ($($from),*,) where $($from: ParJoin),*, @@ -463,10 +489,15 @@ macro_rules! immutable_resource_join { type Type = <&'a T as Join>::Type; type Value = <&'a T as Join>::Value; type Mask = <&'a T as Join>::Mask; + + // SAFETY: This only wraps `T` and, while exposing the mask and the values, + // requires the same invariants as the original implementation and is thus safe. unsafe fn open(self) -> (Self::Mask, Self::Value) { self.deref().open() } + // SAFETY: The mask of `Self` and `T` are identical, thus a check to `Self`'s mask (which is required) + // is equal to a check of `T`'s mask, which makes `get` safe to call. unsafe fn get(v: &mut Self::Value, i: Index) -> Self::Type { <&'a T as Join>::get(v, i) } @@ -477,6 +508,8 @@ macro_rules! immutable_resource_join { } } + // SAFETY: This is just a wrapper of `T`'s implementation for `ParJoin` and can + // in no case lead to other memory access patterns. #[cfg(feature = "parallel")] unsafe impl<'a, 'b, T> ParJoin for &'a $ty where @@ -498,10 +531,15 @@ macro_rules! mutable_resource_join { type Type = <&'a mut T as Join>::Type; type Value = <&'a mut T as Join>::Value; type Mask = <&'a mut T as Join>::Mask; + + // SAFETY: This only wraps `T` and, while exposing the mask and the values, + // requires the same invariants as the original implementation and is thus safe. unsafe fn open(self) -> (Self::Mask, Self::Value) { self.deref_mut().open() } + // SAFETY: The mask of `Self` and `T` are identical, thus a check to `Self`'s mask (which is required) + // is equal to a check of `T`'s mask, which makes `get_mut` safe to call. unsafe fn get(v: &mut Self::Value, i: Index) -> Self::Type { <&'a mut T as Join>::get(v, i) } @@ -512,6 +550,8 @@ macro_rules! mutable_resource_join { } } + // SAFETY: This is just a wrapper of `T`'s implementation for `ParJoin` and can + // in no case lead to other memory access patterns. #[cfg(feature = "parallel")] unsafe impl<'a, 'b, T> ParJoin for &'a mut $ty where diff --git a/src/join/par_join.rs b/src/join/par_join.rs index 719fa634b..c9d44083c 100644 --- a/src/join/par_join.rs +++ b/src/join/par_join.rs @@ -11,6 +11,14 @@ use join::Join; /// The purpose of the `ParJoin` trait is to provide a way /// to access multiple storages in parallel at the same time with /// the merged bit set. +/// +/// # Safety +/// +/// The implementation of `ParallelIterator` for `ParJoin` makes multiple assumptions on the structure of `Self`. +/// In particular, `::get` must be callable from multiple threads, simultaneously, without mutating +/// values not exclusively associated with `id`. +// NOTE: This is currently unspecified behavior. It seems very unlikely that it breaks in the future, +// but technically it's not specified as valid Rust code. pub unsafe trait ParJoin: Join { /// Create a joined parallel iterator over the contents. fn par_join(self) -> JoinParIter @@ -45,6 +53,8 @@ where let (keys, values) = unsafe { self.0.open() }; // Create a bit producer which splits on up to three levels let producer = BitProducer((&keys).iter(), 3); + // HACK: use `UnsafeCell` to share `values` between threads; + // this is the unspecified behavior referred to above. let values = UnsafeCell::new(values); bridge_unindexed(JoinProducer::::new(producer, &values), consumer) @@ -74,6 +84,14 @@ where } } +// SAFETY: `Send` is safe to implement if all components of `Self` are logically `Send`. +// `keys` already has `Send` implemented, thus no reasoning is required. +// `values` is a reference to an `UnsafeCell` wrapping `J::Value`; +// `J::Value` is constrained to implement `Send`. +// `UnsafeCell` provides interior mutability, but the specification of it allows sharing +// as long as access does not happen simultaneously; this makes it generally safe to `Send`, +// but we are accessing it simultaneously, which is technically not allowed. +// Also see https://github.com/slide-rs/specs/issues/220 unsafe impl<'a, J> Send for JoinProducer<'a, J> where J: Join + Send, diff --git a/src/storage/drain.rs b/src/storage/drain.rs index 3b388ae1c..06c320733 100644 --- a/src/storage/drain.rs +++ b/src/storage/drain.rs @@ -19,12 +19,14 @@ where type Value = &'a mut MaskedStorage; type Mask = BitSet; + // SAFETY: No invariants to meet and no unsafe code. unsafe fn open(self) -> (Self::Mask, Self::Value) { let mask = self.data.mask.clone(); (mask, self.data) } + // SAFETY: No invariants to meet and no unsafe code. unsafe fn get(value: &mut Self::Value, id: Index) -> T { value.remove(id).expect("Tried to access same index twice") } diff --git a/src/storage/entry.rs b/src/storage/entry.rs index 0d7df8bac..e5d064d54 100644 --- a/src/storage/entry.rs +++ b/src/storage/entry.rs @@ -40,7 +40,9 @@ where if self.entities.is_alive(e) { unsafe { let entries = self.entries(); + // SAFETY: This is safe since we're not swapping out the mask or the values. let (_, mut value): (BitSetAll, _) = entries.open(); + // SAFETY: We did check the mask, because the mask is `BitSetAll` and every index is part of it. Ok(Entries::get(&mut value, e.id())) } } else { @@ -132,10 +134,13 @@ where type Value = &'a mut Storage<'b, T, D>; type Mask = BitSetAll; + // SAFETY: No invariants to meet and no unsafe code. unsafe fn open(self) -> (Self::Mask, Self::Value) { (BitSetAll, self.0) } + // SAFETY: We are lengthening the lifetime of `value` to `'a`; + // TODO: how to prove this is safe? unsafe fn get(value: &mut Self::Value, id: Index) -> Self::Type { // This is HACK. See implementation of Join for &'a mut Storage<'e, T, D> for // details why it is necessary. @@ -172,6 +177,8 @@ where { /// Get a reference to the component associated with the entity. pub fn get(&self) -> &T { + // SAFETY: This is safe since `OccupiedEntry` is only constructed + // after checking the mask. unsafe { self.storage.data.inner.get(self.id) } } } @@ -183,12 +190,16 @@ where { /// Get a mutable reference to the component associated with the entity. pub fn get_mut(&mut self) -> &mut T { + // SAFETY: This is safe since `OccupiedEntry` is only constructed + // after checking the mask. unsafe { self.storage.data.inner.get_mut(self.id) } } /// Converts the `OccupiedEntry` into a mutable reference bounded by /// the storage's lifetime. pub fn into_mut(self) -> &'a mut T { + // SAFETY: This is safe since `OccupiedEntry` is only constructed + // after checking the mask. unsafe { self.storage.data.inner.get_mut(self.id) } } @@ -218,6 +229,7 @@ where /// Inserts a value into the storage. pub fn insert(self, component: T) -> &'a mut T { self.storage.data.mask.add(self.id); + // SAFETY: This is safe since we added `self.id` to the mask. unsafe { self.storage.data.inner.insert(self.id, component); self.storage.data.inner.get_mut(self.id) diff --git a/src/storage/mod.rs b/src/storage/mod.rs index 7b88c7524..6eb6150e0 100644 --- a/src/storage/mod.rs +++ b/src/storage/mod.rs @@ -45,17 +45,21 @@ impl<'a> Join for AntiStorage<'a> { type Value = (); type Mask = BitSetNot<&'a BitSet>; + // SAFETY: No invariants to meet and no unsafe code. unsafe fn open(self) -> (Self::Mask, ()) { (BitSetNot(self.0), ()) } + // SAFETY: No invariants to meet and no unsafe code. unsafe fn get(_: &mut (), _: Index) -> () { () } } +// SAFETY: Since `get` does not do any memory access, this is safe to implement. unsafe impl<'a> DistinctStorage for AntiStorage<'a> {} +// SAFETY: Since `get` does not do any memory access, this is safe to implement. #[cfg(feature = "parallel")] unsafe impl<'a> ParJoin for AntiStorage<'a> {} @@ -141,6 +145,7 @@ impl MaskedStorage { /// Clear the contents of this storage. pub fn clear(&mut self) { + // SAFETY: `self.mask` is the correct mask as specified. unsafe { self.inner.clean(&self.mask); } @@ -150,6 +155,7 @@ impl MaskedStorage { /// Remove an element by a given index. pub fn remove(&mut self, id: Index) -> Option { if self.mask.remove(id) { + // SAFETY: We checked the mask (`remove` returned `true`) Some(unsafe { self.inner.remove(id) }) } else { None @@ -159,6 +165,7 @@ impl MaskedStorage { /// Drop an element by a given index. pub fn drop(&mut self, id: Index) { if self.mask.remove(id) { + // SAFETY: We checked the mask (`remove` returned `true`) unsafe { self.inner.drop(id); } @@ -214,6 +221,7 @@ where /// Tries to read the data associated with an `Entity`. pub fn get(&self, e: Entity) -> Option<&T> { if self.data.mask.contains(e.id()) && self.entities.is_alive(e) { + // SAFETY: We checked the mask, so all invariants are met. Some(unsafe { self.data.inner.get(e.id()) }) } else { None @@ -250,6 +258,8 @@ where { /// Gets mutable access to the wrapped storage. /// + /// # Safety + /// /// This is unsafe because modifying the wrapped storage without also /// updating the mask bitset accordingly can result in illegal memory access. pub unsafe fn unprotected_storage_mut(&mut self) -> &mut T::Storage { @@ -259,6 +269,7 @@ where /// Tries to mutate the data associated with an `Entity`. pub fn get_mut(&mut self, e: Entity) -> Option<&mut T> { if self.data.mask.contains(e.id()) && self.entities.is_alive(e) { + // SAFETY: We checked the mask, so all invariants are met. Some(unsafe { self.data.inner.get_mut(e.id()) }) } else { None @@ -275,10 +286,12 @@ where if self.entities.is_alive(e) { let id = e.id(); if self.data.mask.contains(id) { + // SAFETY: We checked the mask, so all invariants are met. std::mem::swap(&mut v, unsafe { self.data.inner.get_mut(id) }); Ok(Some(v)) } else { self.data.mask.add(id); + // SAFETY: The mask was previously empty, so it is safe to insert. unsafe { self.data.inner.insert(id, v) }; Ok(None) } @@ -314,6 +327,8 @@ where } } +// SAFETY: This is safe, since `T::Storage` is `DistinctStorage` and `Join::get` only +// accesses the storage and nothing else. unsafe impl<'a, T: Component, D> DistinctStorage for Storage<'a, T, D> where T::Storage: DistinctStorage {} @@ -327,10 +342,13 @@ where type Value = &'a T::Storage; type Mask = &'a BitSet; + // SAFETY: No unsafe code and no invariants. unsafe fn open(self) -> (Self::Mask, Self::Value) { (&self.data.mask, &self.data.inner) } + // SAFETY: Since we require that the mask was checked, an element for `i` must have + // been inserted without being removed. unsafe fn get(v: &mut Self::Value, i: Index) -> &'a T { v.get(i) } @@ -348,6 +366,8 @@ where } } +// SAFETY: This is always safe because immutable access can in no case cause memory +// issues, even if access to common memory occurs. #[cfg(feature = "parallel")] unsafe impl<'a, 'e, T, D> ParJoin for &'a Storage<'e, T, D> where @@ -365,10 +385,12 @@ where type Value = &'a mut T::Storage; type Mask = &'a BitSet; + // SAFETY: No unsafe code and no invariants to fulfill. unsafe fn open(self) -> (Self::Mask, Self::Value) { self.data.open_mut() } + // TODO: audit unsafe unsafe fn get(v: &mut Self::Value, i: Index) -> &'a mut T { // This is horribly unsafe. Unfortunately, Rust doesn't provide a way // to abstract mutable/immutable state at the moment, so we have to hack @@ -378,6 +400,7 @@ where } } +// SAFETY: This is safe because of the `DistinctStorage` guarantees. #[cfg(feature = "parallel")] unsafe impl<'a, 'e, T, D> ParJoin for &'a mut Storage<'e, T, D> where @@ -414,6 +437,11 @@ where pub trait UnprotectedStorage: TryDefault { /// Clean the storage given a bitset with bits set for valid indices. /// Allows us to safely drop the storage. + /// + /// # Safety + /// + /// May only be called with the mask which keeps track of the elements + /// existing in this storage. unsafe fn clean(&mut self, has: B) where B: BitSetLike; @@ -421,20 +449,53 @@ pub trait UnprotectedStorage: TryDefault { /// Tries reading the data associated with an `Index`. /// This is unsafe because the external set used /// to protect this storage is absent. + /// + /// # Safety + /// + /// May only be called after a call to `insert` with `id` and + /// no following call to `remove` with `id`. + /// + /// A mask should keep track of those states, and an `id` being contained + /// in the tracking mask is sufficient to call this method. unsafe fn get(&self, id: Index) -> &T; /// Tries mutating the data associated with an `Index`. /// This is unsafe because the external set used /// to protect this storage is absent. + /// + /// # Safety + /// + /// May only be called after a call to `insert` with `id` and + /// no following call to `remove` with `id`. + /// + /// A mask should keep track of those states, and an `id` being contained + /// in the tracking mask is sufficient to call this method. unsafe fn get_mut(&mut self, id: Index) -> &mut T; /// Inserts new data for a given `Index`. + /// + /// # Safety + /// + /// May only be called if `insert` was not called with `id` before, or + /// was reverted by a call to `remove` with `id. + /// + /// A mask should keep track of those states, and an `id` missing from the mask + /// is sufficient to call `insert`. unsafe fn insert(&mut self, id: Index, value: T); /// Removes the data associated with an `Index`. + /// + /// # Safety + /// + /// May only be called if an element with `id` was `insert`ed and not yet removed / dropped. unsafe fn remove(&mut self, id: Index) -> T; /// Drops the data associated with an `Index`. + /// This is simply more efficient than `remove` and can be used if the data is no longer needed. + /// + /// # Safety + /// + /// May only be called if an element with `id` was `insert`ed and not yet removed / dropped. unsafe fn drop(&mut self, id: Index) { self.remove(id); }