From c7c07ab5a45589f706fc3c0d8e8879d80a91ff9d Mon Sep 17 00:00:00 2001 From: Cassy343 Date: Sat, 23 Jul 2022 14:45:09 -0400 Subject: [PATCH] Fix soundness hole --- src/core/mod.rs | 2 +- src/lib.rs | 46 +++++++-- src/util/deterministic.rs | 198 ++++++++++++++++++++++++++++++++++++++ src/util/mod.rs | 1 + tests/functionality.rs | 11 ++- tests/util.rs | 9 ++ 6 files changed, 253 insertions(+), 14 deletions(-) create mode 100644 src/util/deterministic.rs diff --git a/src/core/mod.rs b/src/core/mod.rs index 0ab1526..8267a54 100644 --- a/src/core/mod.rs +++ b/src/core/mod.rs @@ -42,7 +42,7 @@ where K: Eq + Hash, S: BuildHasher, { - pub fn build_map(options: Builder) -> (WriteHandle, ReadHandle) { + pub unsafe fn build_map(options: Builder) -> (WriteHandle, ReadHandle) { let BuilderArgs { capacity, h1, h2 } = options.into_args(); let maps = Box::new([ diff --git a/src/lib.rs b/src/lib.rs index bad5902..a5ac16b 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -12,7 +12,7 @@ mod write; pub use read::*; pub(crate) use util::loom; -pub use util::Alias; +pub use util::{deterministic::*, Alias}; pub use view::View; pub use write::*; @@ -31,7 +31,7 @@ pub(crate) type Map = hashbrown::HashMap, Alias< /// [`with_hasher`](crate::with_hasher), and [`Builder`](crate::Builder). pub fn new() -> (WriteHandle, ReadHandle) where - K: Eq + Hash, + K: TrustedHashEq, { Builder::new().build() } @@ -42,7 +42,7 @@ where /// If you wish to specify additional parameters, see [`Builder`](crate::Builder). pub fn with_capacity(capacity: usize) -> (WriteHandle, ReadHandle) where - K: Eq + Hash, + K: TrustedHashEq, { Builder::new().with_capacity(capacity).build() } @@ -50,12 +50,17 @@ where /// Creates a new map with the specified hasher. /// /// If you wish to specify additional parameters, see [`Builder`](crate::Builder). -pub fn with_hasher(hasher: S) -> (WriteHandle, ReadHandle) +/// +/// # Safety +/// +/// The given hasher builder must produce a deterministic hasher. In other words, the built hasher +/// must always produce the same hash given the same input and initial state. +pub unsafe fn with_hasher(hasher: S) -> (WriteHandle, ReadHandle) where - K: Eq + Hash, + K: TrustedHashEq, S: Clone + BuildHasher, { - Builder::new().with_hasher(hasher).build() + unsafe { Builder::new().with_hasher(hasher).build() } } /// A builder for a map. @@ -108,7 +113,11 @@ impl Builder { /// Sets the hasher for the underlying map. The provided hasher must implement `Clone` due to /// the implementation details of this crate. - pub fn with_hasher(self, hasher: H) -> Builder + /// + /// # Safety + /// + /// See [`crate::with_hasher`](crate::with_hasher). + pub unsafe fn with_hasher(self, hasher: H) -> Builder where H: Clone + BuildHasher, { @@ -122,7 +131,11 @@ impl Builder { /// [`with_hasher`](crate::Builder::with_hasher), but instead of using a concrete hasher /// builder, the provided function will be called as many times as necessary to initialize /// the underlying map. - pub fn with_hasher_fn(self, make: fn() -> H) -> Builder + /// + /// # Safety + /// + /// See [`crate::with_hasher`](crate::with_hasher). + pub unsafe fn with_hasher_fn(self, make: fn() -> H) -> Builder where H: BuildHasher, { @@ -148,10 +161,23 @@ impl Builder { /// ``` pub fn build(self) -> (WriteHandle, ReadHandle) where - K: Eq + Hash, + K: TrustedHashEq, + S: BuildHasher, + { + unsafe { self.build_assert_trusted() } + } + + /// Consumes the builder and returns a write handle and read handle to the map. + /// + /// # Safety + /// + /// The implementations of `Hash` and `Eq` for the key type **must** be deterministic. + pub unsafe fn build_assert_trusted(self) -> (WriteHandle, ReadHandle) + where + K: Hash + Eq, S: BuildHasher, { - Core::build_map(self) + unsafe { Core::build_map(self) } } pub(crate) fn into_args(self) -> BuilderArgs { diff --git a/src/util/deterministic.rs b/src/util/deterministic.rs new file mode 100644 index 0000000..004eb44 --- /dev/null +++ b/src/util/deterministic.rs @@ -0,0 +1,198 @@ +use std::hash::Hash; + +/// A marker trait asserting that a type has a deterministic [`Hash`](std::hash::Hash) and +/// [`Eq`](std::cmp::Eq) implementation. +/// +/// This trait is implemented for all standard library types which have deterministic `Hash` and +/// `Eq` implementations. +/// +/// # Safety +/// +/// A deterministic `Hash` implementation guarantees that if a value is held constant, then the +/// hash of the value will also remain constant if the initial state of the provided hasher is also +/// held constant, and the hasher is itself deterministic. +/// +/// A deterministic `Eq` implementation guarantees that if a value is held constant, then the +/// result of comparing it to another constant will not change. +pub unsafe trait TrustedHashEq: Hash + Eq {} + +// This massive glut of impls was lifted from `evmap`: +// https://github.com/jonhoo/evmap/blob/0daf488a76f9a2f271e0aab75e84cc65661df195/src/stable_hash_eq.rs + +macro_rules! trusted_hash_eq { + ($( + $({$($a:lifetime),*$(,)?$($T:ident$(:?$Sized:ident)?),*$(,)?} + $({$($manual_bounds:tt)*})?)? $Type:ty, + )*) => { + trusted_hash_eq!{# + $( + $({$($a)*$($T$(:?$Sized$Sized)?)*})? $($({where $($manual_bounds)*})? + { + where $( + $T: TrustedHashEq, + )* + })? + $Type, + )* + } + }; + (#$( + $({$($a:lifetime)*$($T:ident$(:?Sized$Sized:ident)?)*} + {$($where_bounds:tt)*}$({$($_t:tt)*})?)? $Type:ty, + )*) => { + $( + unsafe impl$(<$($a,)*$($T$(:?$Sized)?,)*>)? TrustedHashEq for $Type + $($($where_bounds)*)? {} + )* + }; +} + +use std::{ + any::TypeId, + borrow::Cow, + cmp::{self, Reverse}, + collections::{BTreeMap, BTreeSet, LinkedList, VecDeque}, + convert::Infallible, + ffi::{CStr, CString, OsStr, OsString}, + fmt, + fs::FileType, + io::ErrorKind, + marker::{PhantomData, PhantomPinned}, + mem::{Discriminant, ManuallyDrop}, + net::{IpAddr, Ipv4Addr, Ipv6Addr, SocketAddr, SocketAddrV4, SocketAddrV6}, + num::{ + NonZeroI128, NonZeroI16, NonZeroI32, NonZeroI64, NonZeroI8, NonZeroIsize, NonZeroU128, + NonZeroU16, NonZeroU32, NonZeroU64, NonZeroU8, NonZeroUsize, Wrapping, + }, + ops::{Bound, Range, RangeFrom, RangeFull, RangeInclusive, RangeTo, RangeToInclusive}, + path::{Component, Path, PathBuf, Prefix, PrefixComponent}, + ptr::NonNull, + rc::Rc, + sync::{atomic, Arc}, + task::Poll, + thread::ThreadId, + time::{Duration, Instant, SystemTime}, +}; + +trusted_hash_eq! { + cmp::Ordering, + Infallible, + ErrorKind, + IpAddr, + SocketAddr, + atomic::Ordering, + bool, char, + i8, i16, i32, i64, i128, + isize, + str, + u8, u16, u32, u64, u128, + (), + usize, + TypeId, + CStr, + CString, + OsStr, + OsString, + fmt::Error, + FileType, + PhantomPinned, + Ipv4Addr, + Ipv6Addr, + SocketAddrV4, + SocketAddrV6, + NonZeroI8, NonZeroI16, NonZeroI32, NonZeroI64, NonZeroI128, NonZeroIsize, + NonZeroU8, NonZeroU16, NonZeroU32, NonZeroU64, NonZeroU128, NonZeroUsize, + RangeFull, + Path, + PathBuf, + String, + ThreadId, + Duration, + Instant, + SystemTime, + {'a} PrefixComponent<'a>, + {'a} Cow<'a, str>, + {'a} Cow<'a, CStr>, + {'a} Cow<'a, OsStr>, + {'a} Cow<'a, Path>, + {'a, T}{T: Clone + TrustedHashEq} Cow<'a, [T]>, + {'a, T}{T: Clone + TrustedHashEq} Cow<'a, T>, + {'a, T: ?Sized} &'a T, + {'a, T: ?Sized} &'a mut T, + {'a} Component<'a>, + {'a} Prefix<'a>, + {T} VecDeque, + {A: ?Sized} (A,), + {A, B: ?Sized} (A, B), + {A, B, C: ?Sized} (A, B, C), + {A, B, C, D: ?Sized} (A, B, C, D), + {A, B, C, D, E: ?Sized} (A, B, C, D, E), + {A, B, C, D, E, F: ?Sized} (A, B, C, D, E, F), + {A, B, C, D, E, F, G: ?Sized} (A, B, C, D, E, F, G), + {A, B, C, D, E, F, G, H: ?Sized} (A, B, C, D, E, F, G, H), + {A, B, C, D, E, F, G, H, I: ?Sized} (A, B, C, D, E, F, G, H, I), + {A, B, C, D, E, F, G, H, I, J: ?Sized} (A, B, C, D, E, F, G, H, I, J), + {A, B, C, D, E, F, G, H, I, J, K: ?Sized} (A, B, C, D, E, F, G, H, I, J, K), + {A, B, C, D, E, F, G, H, I, J, K, L: ?Sized} (A, B, C, D, E, F, G, H, I, J, K, L), + {Idx} Range, + {Idx} RangeFrom, + {Idx} RangeInclusive, + {Idx} RangeTo, + {Idx} RangeToInclusive, + {K, V} BTreeMap, +} + +macro_rules! trusted_hash_eq_fn { + ($({$($($A:ident),+)?})*) => { + trusted_hash_eq!{ + $( + {Ret$(, $($A),+)?}{} fn($($($A),+)?) -> Ret, + {Ret$(, $($A),+)?}{} extern "C" fn($($($A),+)?) -> Ret, + $({Ret, $($A),+}{} extern "C" fn($($A),+, ...) -> Ret,)? + {Ret$(, $($A),+)?}{} unsafe fn($($($A),+)?) -> Ret, + {Ret$(, $($A),+)?}{} unsafe extern "C" fn($($($A),+)?) -> Ret, + $({Ret, $($A),+}{} unsafe extern "C" fn($($A),+, ...) -> Ret,)? + )* + } + }; +} + +trusted_hash_eq_fn! { + {} + {A} + {A, B} + {A, B, C} + {A, B, C, D} + {A, B, C, D, E} + {A, B, C, D, E, F} + {A, B, C, D, E, F, G} + {A, B, C, D, E, F, G, H} + {A, B, C, D, E, F, G, H, I} + {A, B, C, D, E, F, G, H, I, J} + {A, B, C, D, E, F, G, H, I, J, K} + {A, B, C, D, E, F, G, H, I, J, K, L} +} + +trusted_hash_eq! { + {T} Bound, + {T} Option, + {T} Poll, + {T: ?Sized}{} *const T, + {T: ?Sized}{} *mut T, + {T} [T], + {T: ?Sized} Box, + {T} Reverse, + {T} BTreeSet, + {T} LinkedList, + {T: ?Sized}{} PhantomData, + {T}{} Discriminant, + {T} ManuallyDrop, + {T} Wrapping, + {T: ?Sized}{} NonNull, + {T: ?Sized} Rc, + {T: ?Sized} Arc, + {T} Vec, + {T, E} Result, +} + +unsafe impl TrustedHashEq for [T; N] where T: TrustedHashEq {} diff --git a/src/util/mod.rs b/src/util/mod.rs index 311e4d3..5d75aae 100644 --- a/src/util/mod.rs +++ b/src/util/mod.rs @@ -1,5 +1,6 @@ mod aliasing; mod cache_padded; +pub mod deterministic; pub mod loom; pub use aliasing::*; diff --git a/tests/functionality.rs b/tests/functionality.rs index e667273..2a177f1 100644 --- a/tests/functionality.rs +++ b/tests/functionality.rs @@ -6,7 +6,6 @@ mod util; -use flashmap::Evicted; use util::dderef; #[test] @@ -259,7 +258,10 @@ fn invalid_reclamation() { let mut guard = w1.guard(); guard.insert(Box::new(1), Box::new(1)); - let leaked = guard.remove(Box::new(1)).map(Evicted::leak).unwrap(); + let leaked = guard + .remove(Box::new(1)) + .map(flashmap::Evicted::leak) + .unwrap(); drop(guard); w2.reclaim_one(leaked); } @@ -273,7 +275,10 @@ fn invalid_lazy_drop() { let mut guard = w1.guard(); guard.insert(Box::new(1), Box::new(1)); - let leaked = guard.remove(Box::new(1)).map(Evicted::leak).unwrap(); + let leaked = guard + .remove(Box::new(1)) + .map(flashmap::Evicted::leak) + .unwrap(); w2.guard().drop_lazily(leaked); drop(guard); } diff --git a/tests/util.rs b/tests/util.rs index 34c761b..45846a6 100644 --- a/tests/util.rs +++ b/tests/util.rs @@ -30,8 +30,12 @@ mod track_access { hash::{Hash, Hasher}, }; + use flashmap::TrustedHashEq; + pub struct TrackAccess(Track>>); + unsafe impl TrustedHashEq for TrackAccess where Self: Hash + Eq {} + impl TrackAccess { pub fn new(val: T) -> Self { Self(Track::new(Box::new(UnsafeCell::new(val)))) @@ -72,11 +76,16 @@ mod track_access { #[cfg(not(loom))] mod track_access { use std::borrow::Borrow; + use std::hash::Hash; + + use flashmap::TrustedHashEq; // The intent is that the tests are run with miri which will do the tracking through the box #[derive(PartialEq, Eq, Hash)] pub struct TrackAccess(Box); + unsafe impl TrustedHashEq for TrackAccess where Self: Hash + Eq {} + impl TrackAccess { pub fn new(val: T) -> Self { Self(Box::new(val))