Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

#![deny(unsafe_op_in_unsafe_fn)] #107

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

#![deny(unsafe_op_in_unsafe_fn)] #107

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)]
Expand Down
6 changes: 3 additions & 3 deletions src/map.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -394,7 +394,7 @@ impl<K, V, S> HashMap<K, V, S> {
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()
}
Expand Down Expand Up @@ -1462,15 +1462,15 @@ 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);
if raw_node.is_null() {
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 => {
Expand Down
179 changes: 107 additions & 72 deletions src/node.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -521,7 +521,7 @@ impl<K, V> TreeBin<K, V> {
// 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);

Expand All @@ -530,13 +530,15 @@ impl<K, V> TreeBin<K, V> {
// 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);
}
Expand All @@ -554,30 +556,44 @@ impl<K, V> TreeBin<K, V> {
// 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
Expand All @@ -597,11 +613,15 @@ impl<K, V> TreeBin<K, V> {
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
Expand All @@ -622,23 +642,20 @@ impl<K, V> TreeBin<K, V> {
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);
}
Expand All @@ -647,32 +664,33 @@ impl<K, V> TreeBin<K, V> {
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);
}
successor_deref.parent.store(p_parent, Ordering::Relaxed);
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
Expand Down Expand Up @@ -702,13 +720,16 @@ impl<K, V> TreeBin<K, V> {
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 {
Expand All @@ -733,11 +754,11 @@ impl<K, V> TreeBin<K, V> {
// `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);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder why were previously re-computed get_tree_node here 🤔 There are a couple of other places too where we re-do TreeNode:;get_tree_node. But maybe those were just unnecessary.

} else if p == p_parent_deref.right.load(Ordering::Relaxed, guard) {
p_parent_deref
.right
Expand Down Expand Up @@ -930,33 +951,39 @@ impl<K, V> TreeBin<K, V> {
/// 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.
pub(crate) unsafe fn defer_drop_without_values<'g>(
bin: Shared<'g, BinEntry<K, V>>,
guard: &'g Guard<'_>,
) {
guard.retire(bin.as_ptr(), |mut link| {
let bin = unsafe {
// SAFETY: `bin` is a `BinEntry<K, V>`
let ptr = link.cast::<BinEntry<K, V>>();
// 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<K, V>`
let ptr = link.cast::<BinEntry<K, V>>();
// 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.
Expand All @@ -967,14 +994,16 @@ impl<K, V> TreeBin<K, V> {

// 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) }
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is missing a safety comment.

}

/// 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.
Expand All @@ -985,10 +1014,14 @@ impl<K, V> TreeBin<K, V> {
) {
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);
Expand All @@ -1005,13 +1038,15 @@ impl<K, V> TreeNode<K, V> {
/// 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<K, V>>) -> &'_ TreeNode<K, V> {
bin.deref().as_tree_node().unwrap()
// safety: guaranteed by caller
unsafe { bin.deref().as_tree_node().unwrap() }
}
}

Expand Down
Loading