diff --git a/src/lib.rs b/src/lib.rs index 17b20bc..e28c070 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -236,7 +236,8 @@ missing_docs, missing_debug_implementations, unreachable_pub, - rustdoc::broken_intra_doc_links + rustdoc::broken_intra_doc_links, + unsafe_op_in_unsafe_fn )] #![warn(rust_2018_idioms)] #![allow(clippy::cognitive_complexity)] diff --git a/src/map.rs b/src/map.rs index 0fb1ffc..7a7af54 100644 --- a/src/map.rs +++ b/src/map.rs @@ -394,7 +394,7 @@ impl HashMap { if table.is_null() { 0 } else { - // Safety: we loaded `table` under the `guard`, + // safety: we loaded `table` under the `guard`, // so it must still be valid here unsafe { table.deref() }.len() } @@ -1462,7 +1462,7 @@ where let mut idx = 0usize; let mut table = self.table.load(Ordering::SeqCst, guard); - // Safety: self.table is a valid pointer because we checked it above. + // safety: self.table is a valid pointer because we checked it above. while !table.is_null() && idx < unsafe { table.deref() }.len() { let tab = unsafe { table.deref() }; let raw_node = tab.bin(idx, guard); @@ -1470,7 +1470,7 @@ where idx += 1; continue; } - // Safety: node is a valid pointer because we checked + // safety: node is a valid pointer because we checked // it in the above if stmt. match **unsafe { raw_node.deref() } { BinEntry::Moved => { diff --git a/src/node.rs b/src/node.rs index 36ed822..80f634e 100644 --- a/src/node.rs +++ b/src/node.rs @@ -521,7 +521,7 @@ impl TreeBin { // the TreeNodes remain valid for at least as long as we hold onto the // guard. Additionally, this method assumes `p` to be non-null. // Structurally, TreeNodes always point to TreeNodes, so this is sound. - let p_deref = TreeNode::get_tree_node(p); + let p_deref = unsafe { TreeNode::get_tree_node(p) }; let next = p_deref.node.next.load(Ordering::SeqCst, guard); let prev = p_deref.prev.load(Ordering::SeqCst, guard); @@ -530,13 +530,15 @@ impl TreeBin { // the node to delete is the first node self.first.store(next, Ordering::SeqCst); } else { - TreeNode::get_tree_node(prev) + // safety: structurally, TreeNodes always point to TreeNodes + unsafe { TreeNode::get_tree_node(prev) } .node .next .store(next, Ordering::SeqCst); } if !next.is_null() { - TreeNode::get_tree_node(next) + // safety: structurally, TreeNodes always point to TreeNodes + unsafe { TreeNode::get_tree_node(next) } .prev .store(prev, Ordering::SeqCst); } @@ -554,30 +556,44 @@ impl TreeBin { // about restructuring the tree since the bin will be untreeified // anyway, so we check that let mut root = self.root.load(Ordering::SeqCst, guard); + // TODO: Can `root` even be `null`? // The Java code has `NULL` checks for this, but in theory it should not be possible to // encounter a tree that has no root when we have its lock. It should always have at // least `UNTREEIFY_THRESHOLD` nodes. If it is indeed impossible we should replace // this with an assertion instead. - if root.is_null() - || TreeNode::get_tree_node(root) - .right - .load(Ordering::SeqCst, guard) - .is_null() + if root.is_null() { + return true; + } + + // safety: structurally, TreeNodes always point to TreeNodes, + // and we just verified that `root` is not null + if unsafe { TreeNode::get_tree_node(root) } + .right + .load(Ordering::SeqCst, guard) + .is_null() + { + return true; + } + + // safety: structurally, TreeNodes always point to TreeNodes + // and we just verified that `root` is not null + let root_left = unsafe { TreeNode::get_tree_node(root) } + .left + .load(Ordering::SeqCst, guard); + + if root_left.is_null() { + return true; + } + + // safety: structurally, TreeNodes always point to TreeNodes, + // and we just verified that `root_left` is not null + if unsafe { TreeNode::get_tree_node(root_left) } + .left + .load(Ordering::SeqCst, guard) + .is_null() { return true; - } else { - let root_left = TreeNode::get_tree_node(root) - .left - .load(Ordering::SeqCst, guard); - if root_left.is_null() - || TreeNode::get_tree_node(root_left) - .left - .load(Ordering::SeqCst, guard) - .is_null() - { - return true; - } } // if we get here, we know that we will still be a tree and have @@ -597,11 +613,15 @@ impl TreeBin { if !p_left.is_null() && !p_right.is_null() { // find the smalles successor of `p` let mut successor = p_right; - let mut successor_deref = TreeNode::get_tree_node(successor); + // safety: structurally, TreeNodes always point to TreeNodes, + // and we just verified that `successor` is not null + let mut successor_deref = unsafe { TreeNode::get_tree_node(successor) }; let mut successor_left = successor_deref.left.load(Ordering::Relaxed, guard); while !successor_left.is_null() { successor = successor_left; - successor_deref = TreeNode::get_tree_node(successor); + // safety: structurally, TreeNodes always point to TreeNodes, + // and we just verified that `successor` is not null + successor_deref = unsafe { TreeNode::get_tree_node(successor) }; successor_left = successor_deref.left.load(Ordering::Relaxed, guard); } // swap colors @@ -622,23 +642,20 @@ impl TreeBin { let successor_parent = successor_deref.parent.load(Ordering::Relaxed, guard); p_deref.parent.store(successor_parent, Ordering::Relaxed); if !successor_parent.is_null() { - if successor - == TreeNode::get_tree_node(successor_parent) - .left - .load(Ordering::Relaxed, guard) - { - TreeNode::get_tree_node(successor_parent) - .left - .store(p, Ordering::Relaxed); + // safety: the parent of a TreeNode must be a TreeNode, + // and we just verified that `successor_parent` is not null + let successor_parent = unsafe { TreeNode::get_tree_node(successor_parent) }; + if successor == successor_parent.left.load(Ordering::Relaxed, guard) { + successor_parent.left.store(p, Ordering::Relaxed); } else { - TreeNode::get_tree_node(successor_parent) - .right - .store(p, Ordering::Relaxed); + successor_parent.right.store(p, Ordering::Relaxed); } } successor_deref.right.store(p_right, Ordering::Relaxed); if !p_right.is_null() { - TreeNode::get_tree_node(p_right) + // safety: structurally, TreeNodes always point to TreeNodes, + // and we just verified that `p_right` is not null + unsafe { TreeNode::get_tree_node(p_right) } .parent .store(successor, Ordering::Relaxed); } @@ -647,13 +664,17 @@ impl TreeBin { p_deref.left.store(Shared::null(), Ordering::Relaxed); p_deref.right.store(successor_right, Ordering::Relaxed); if !successor_right.is_null() { - TreeNode::get_tree_node(successor_right) + // safety: structurally, TreeNodes always point to TreeNodes, + // and we just verified that `successor_right` is not null + unsafe { TreeNode::get_tree_node(successor_right) } .parent .store(p, Ordering::Relaxed); } successor_deref.left.store(p_left, Ordering::Relaxed); if !p_left.is_null() { - TreeNode::get_tree_node(p_left) + // safety: structurally, TreeNodes always point to TreeNodes, + // and we just verified that `p_left` is not null + unsafe { TreeNode::get_tree_node(p_left) } .parent .store(successor, Ordering::Relaxed); } @@ -661,18 +682,15 @@ impl TreeBin { if p_parent.is_null() { // the successor was swapped to the root as `p` was previously the root root = successor; - } else if p - == TreeNode::get_tree_node(p_parent) - .left - .load(Ordering::Relaxed, guard) - { - TreeNode::get_tree_node(p_parent) - .left - .store(successor, Ordering::Relaxed); } else { - TreeNode::get_tree_node(p_parent) - .right - .store(successor, Ordering::Relaxed); + // safety: the parent of a TreeNode must be a TreeNode, + // and we just verified that `successor_parent` is not null + let p_parent = unsafe { TreeNode::get_tree_node(p_parent) }; + if p == p_parent.left.load(Ordering::Relaxed, guard) { + p_parent.left.store(successor, Ordering::Relaxed); + } else { + p_parent.right.store(successor, Ordering::Relaxed); + } } // We have swapped `p` with `successor`, which is the next element @@ -702,13 +720,16 @@ impl TreeBin { if replacement != p { // `p` (at its potentially new position) has a child, so we need to do a replacement. let p_parent = p_deref.parent.load(Ordering::Relaxed, guard); - TreeNode::get_tree_node(replacement) + // safety: we just assigned `replacement` to a non-null TreeNode + unsafe { TreeNode::get_tree_node(replacement) } .parent .store(p_parent, Ordering::Relaxed); if p_parent.is_null() { root = replacement; } else { - let p_parent_deref = TreeNode::get_tree_node(p_parent); + // safety: the parent of a TreeNode must be a TreeNode, + // and we just verified that `p_parent` is not null + let p_parent_deref = unsafe { TreeNode::get_tree_node(p_parent) }; if p == p_parent_deref.left.load(Ordering::Relaxed, guard) { p_parent_deref.left.store(replacement, Ordering::Relaxed); } else { @@ -733,11 +754,11 @@ impl TreeBin { // `p` does _not_ have children, so we unlink it as a leaf. let p_parent = p_deref.parent.load(Ordering::Relaxed, guard); if !p_parent.is_null() { - let p_parent_deref = TreeNode::get_tree_node(p_parent); + // safety: the parent of a TreeNode must be a TreeNode, + // and we just verified that `p_parent` is not null + let p_parent_deref = unsafe { TreeNode::get_tree_node(p_parent) }; if p == p_parent_deref.left.load(Ordering::Relaxed, guard) { - TreeNode::get_tree_node(p_parent) - .left - .store(Shared::null(), Ordering::Relaxed); + p_parent_deref.left.store(Shared::null(), Ordering::Relaxed); } else if p == p_parent_deref.right.load(Ordering::Relaxed, guard) { p_parent_deref .right @@ -930,6 +951,7 @@ impl TreeBin { /// Defers dropping the given tree bin without its nodes' values. /// /// # Safety + /// /// The given bin must be a valid, non-null BinEntry::TreeBin and the caller must ensure /// that no references to the bin can be obtained by other threads after the call to this /// method. @@ -937,26 +959,31 @@ impl TreeBin { bin: Shared<'g, BinEntry>, guard: &'g Guard<'_>, ) { - guard.retire(bin.as_ptr(), |mut link| { - let bin = unsafe { - // SAFETY: `bin` is a `BinEntry` - let ptr = link.cast::>(); - // SAFETY: `retire` guarantees that we - // have unique access to `bin` at this point - *Box::from_raw(ptr) - }; + // safety: the caller ensures `bin` is non-null, and no + // references to the bin can be obtained by other threads + unsafe { + guard.retire(bin.as_ptr(), |mut link| { + let bin = unsafe { + // safety: `bin` is a `BinEntry` + let ptr = link.cast::>(); + // safety: `retire` guarantees that we + // have unique access to `bin` at this point + *Box::from_raw(ptr) + }; - if let BinEntry::Tree(mut tree_bin) = Linked::into_inner(bin) { - tree_bin.drop_fields(false); - } else { - unreachable!("bin is a tree bin"); - } - }); + if let BinEntry::Tree(mut tree_bin) = Linked::into_inner(bin) { + tree_bin.drop_fields(false); + } else { + unreachable!("bin is a tree bin"); + } + }); + } } /// Drops the given tree bin, but only drops its nodes' values when specified. /// /// # Safety + /// /// The pointer to the tree bin must be valid and the caller must be the single owner /// of the tree bin. If the nodes' values are to be dropped, there must be no outstanding /// references to these values in other threads and it must be impossible to obtain them. @@ -967,14 +994,16 @@ impl TreeBin { // swap out first pointer so nodes will not get dropped again when // `tree_bin` is dropped - let guard = Guard::unprotected(); + // safety: we have exclusive accesss to self + let guard = unsafe { Guard::unprotected() }; let p = self.first.swap(Shared::null(), Ordering::Relaxed, &guard); - Self::drop_tree_nodes(p, drop_values, &guard); + unsafe { Self::drop_tree_nodes(p, drop_values, &guard) } } /// Drops the given list of tree nodes, but only drops their values when specified. /// /// # Safety + /// /// The pointers to the tree nodes must be valid and the caller must be the single owner /// of the tree nodes. If the nodes' values are to be dropped, there must be no outstanding /// references to these values in other threads and it must be impossible to obtain them. @@ -985,10 +1014,14 @@ impl TreeBin { ) { let mut p = from; while !p.is_null() { - if let BinEntry::TreeNode(tree_node) = Linked::into_inner(*p.into_box()) { + // safety: we just verified that `p` is non-null, and the caller + // guarantees exclusive access + let p_owned = unsafe { *p.into_box() }; + if let BinEntry::TreeNode(tree_node) = Linked::into_inner(p_owned) { // if specified, drop the value in this node if drop_values { - let _ = tree_node.node.value.into_box(); + // safety: same as above + let _ = unsafe { tree_node.node.value.into_box() }; } // then we move to the next node p = tree_node.node.next.load(Ordering::SeqCst, guard); @@ -1005,13 +1038,15 @@ impl TreeNode { /// returns its `tree_node`. /// /// # Safety + /// /// All safety concerns of [`deref`](Shared::deref) apply. In particular, the /// supplied pointer must be non-null and must point to valid memory. /// Additionally, it must point to an instance of BinEntry that is actually a /// TreeNode. #[inline] pub(crate) unsafe fn get_tree_node(bin: Shared<'_, BinEntry>) -> &'_ TreeNode { - bin.deref().as_tree_node().unwrap() + // safety: guaranteed by caller + unsafe { bin.deref().as_tree_node().unwrap() } } } diff --git a/src/reclaim.rs b/src/reclaim.rs index 6334c0a..1106a6b 100644 --- a/src/reclaim.rs +++ b/src/reclaim.rs @@ -20,8 +20,13 @@ impl Atomic { self.0.store(new.ptr, ordering); } + /// Converts the pointer to a `Box`. + /// + /// # Safety + /// + /// This method may be called only if the pointer is valid. pub(crate) unsafe fn into_box(self) -> Box> { - Box::from_raw(self.0.into_inner()) + unsafe { Box::from_raw(self.0.into_inner()) } } pub(crate) fn swap<'g>( @@ -97,20 +102,36 @@ impl<'g, T> Shared<'g, T> { Shared::from(collector.link_boxed(value)) } + /// Converts the pointer to a `Box`. + /// + /// # Safety + /// + /// This method may be called only if the pointer is valid + /// and nobody else is holding a reference to the same object. pub(crate) unsafe fn into_box(self) -> Box> { - Box::from_raw(self.ptr) + unsafe { Box::from_raw(self.ptr) } } - pub(crate) unsafe fn as_ptr(&self) -> *mut Linked { + pub(crate) fn as_ptr(&self) -> *mut Linked { self.ptr } + /// Dereference the shared pointer if it is not null. + /// + /// # Safety + /// + /// All concerns of calling `as_ref` on a shared, raw pointer apply. pub(crate) unsafe fn as_ref(&self) -> Option<&'g Linked> { - self.ptr.as_ref() + unsafe { self.ptr.as_ref() } } + /// Dereference the shared pointer. + /// + /// # Safety + /// + /// All concerns of dereferencing a shared, raw pointer apply. pub(crate) unsafe fn deref(&self) -> &'g Linked { - &*self.ptr + unsafe { &*self.ptr } } pub(crate) fn is_null(&self) -> bool { @@ -148,8 +169,15 @@ pub(crate) trait RetireShared { } impl RetireShared for Guard<'_> { + /// Retire the value, reclaiming it when all outstanding references are dropped. + /// + /// # Safety + /// + /// An object can only be retired if it is non-null, and is no longer accessible + /// to any thread. The value also may not be accessed by the current thread after + /// this guard is dropped. unsafe fn retire_shared(&self, shared: Shared<'_, T>) { - self.retire(shared.ptr, seize::reclaim::boxed::); + unsafe { self.retire(shared.ptr, seize::reclaim::boxed::) } } }