Skip to content

Commit

Permalink
Fix soundness hole
Browse files Browse the repository at this point in the history
  • Loading branch information
Cassy343 committed Jul 23, 2022
1 parent ae42534 commit c7c07ab
Show file tree
Hide file tree
Showing 6 changed files with 253 additions and 14 deletions.
2 changes: 1 addition & 1 deletion src/core/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ where
K: Eq + Hash,
S: BuildHasher,
{
pub fn build_map(options: Builder<S>) -> (WriteHandle<K, V, S>, ReadHandle<K, V, S>) {
pub unsafe fn build_map(options: Builder<S>) -> (WriteHandle<K, V, S>, ReadHandle<K, V, S>) {
let BuilderArgs { capacity, h1, h2 } = options.into_args();

let maps = Box::new([
Expand Down
46 changes: 36 additions & 10 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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::*;

Expand All @@ -31,7 +31,7 @@ pub(crate) type Map<K, V, S = RandomState> = hashbrown::HashMap<Alias<K>, Alias<
/// [`with_hasher`](crate::with_hasher), and [`Builder`](crate::Builder).
pub fn new<K, V>() -> (WriteHandle<K, V>, ReadHandle<K, V>)
where
K: Eq + Hash,
K: TrustedHashEq,
{
Builder::new().build()
}
Expand All @@ -42,20 +42,25 @@ where
/// If you wish to specify additional parameters, see [`Builder`](crate::Builder).
pub fn with_capacity<K, V>(capacity: usize) -> (WriteHandle<K, V>, ReadHandle<K, V>)
where
K: Eq + Hash,
K: TrustedHashEq,
{
Builder::new().with_capacity(capacity).build()
}

/// Creates a new map with the specified hasher.
///
/// If you wish to specify additional parameters, see [`Builder`](crate::Builder).
pub fn with_hasher<K, V, S>(hasher: S) -> (WriteHandle<K, V, S>, ReadHandle<K, V, S>)
///
/// # 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<K, V, S>(hasher: S) -> (WriteHandle<K, V, S>, ReadHandle<K, V, S>)
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.
Expand Down Expand Up @@ -108,7 +113,11 @@ impl<S> Builder<S> {

/// 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<H>(self, hasher: H) -> Builder<H>
///
/// # Safety
///
/// See [`crate::with_hasher`](crate::with_hasher).
pub unsafe fn with_hasher<H>(self, hasher: H) -> Builder<H>
where
H: Clone + BuildHasher,
{
Expand All @@ -122,7 +131,11 @@ impl<S> Builder<S> {
/// [`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<H>(self, make: fn() -> H) -> Builder<H>
///
/// # Safety
///
/// See [`crate::with_hasher`](crate::with_hasher).
pub unsafe fn with_hasher_fn<H>(self, make: fn() -> H) -> Builder<H>
where
H: BuildHasher,
{
Expand All @@ -148,10 +161,23 @@ impl<S> Builder<S> {
/// ```
pub fn build<K, V>(self) -> (WriteHandle<K, V, S>, ReadHandle<K, V, S>)
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<K, V>(self) -> (WriteHandle<K, V, S>, ReadHandle<K, V, S>)
where
K: Hash + Eq,
S: BuildHasher,
{
Core::build_map(self)
unsafe { Core::build_map(self) }
}

pub(crate) fn into_args(self) -> BuilderArgs<S> {
Expand Down
198 changes: 198 additions & 0 deletions src/util/deterministic.rs
Original file line number Diff line number Diff line change
@@ -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<T>,
{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>,
{Idx} RangeFrom<Idx>,
{Idx} RangeInclusive<Idx>,
{Idx} RangeTo<Idx>,
{Idx} RangeToInclusive<Idx>,
{K, V} BTreeMap<K, V>,
}

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>,
{T} Option<T>,
{T} Poll<T>,
{T: ?Sized}{} *const T,
{T: ?Sized}{} *mut T,
{T} [T],
{T: ?Sized} Box<T>,
{T} Reverse<T>,
{T} BTreeSet<T>,
{T} LinkedList<T>,
{T: ?Sized}{} PhantomData<T>,
{T}{} Discriminant<T>,
{T} ManuallyDrop<T>,
{T} Wrapping<T>,
{T: ?Sized}{} NonNull<T>,
{T: ?Sized} Rc<T>,
{T: ?Sized} Arc<T>,
{T} Vec<T>,
{T, E} Result<T, E>,
}

unsafe impl<T, const N: usize> TrustedHashEq for [T; N] where T: TrustedHashEq {}
1 change: 1 addition & 0 deletions src/util/mod.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
mod aliasing;
mod cache_padded;
pub mod deterministic;
pub mod loom;

pub use aliasing::*;
Expand Down
11 changes: 8 additions & 3 deletions tests/functionality.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@
mod util;

use flashmap::Evicted;
use util::dderef;

#[test]
Expand Down Expand Up @@ -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);
}
Expand All @@ -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);
}
9 changes: 9 additions & 0 deletions tests/util.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,12 @@ mod track_access {
hash::{Hash, Hasher},
};

use flashmap::TrustedHashEq;

pub struct TrackAccess<T>(Track<Box<UnsafeCell<T>>>);

unsafe impl<T> TrustedHashEq for TrackAccess<T> where Self: Hash + Eq {}

impl<T> TrackAccess<T> {
pub fn new(val: T) -> Self {
Self(Track::new(Box::new(UnsafeCell::new(val))))
Expand Down Expand Up @@ -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<T>(Box<T>);

unsafe impl<T> TrustedHashEq for TrackAccess<T> where Self: Hash + Eq {}

impl<T> TrackAccess<T> {
pub fn new(val: T) -> Self {
Self(Box::new(val))
Expand Down

0 comments on commit c7c07ab

Please sign in to comment.