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

Jump to bottom

fix LinkedList invalidating mutable references #60072

Merged
merged 1 commit into from
Apr 19, 2019
Merged

fix LinkedList invalidating mutable references #60072

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
48 changes: 37 additions & 11 deletions src/liballoc/collections/linked_list.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,9 @@ impl<T> Clone for Iter<'_, T> {
/// [`LinkedList`]: struct.LinkedList.html
#[stable(feature = "rust1", since = "1.0.0")]
pub struct IterMut<'a, T: 'a> {
// We do *not* exclusively own the entire list here, references to node's `element`
// have been handed out by the iterator! So be careful when using this; the methods
// called must be aware that there can be aliasing pointers to `element`.
list: &'a mut LinkedList<T>,
head: Option<NonNull<Node<T>>>,
tail: Option<NonNull<Node<T>>>,
Expand Down Expand Up @@ -143,14 +146,17 @@ impl<T> LinkedList<T> {
/// Adds the given node to the front of the list.
#[inline]
fn push_front_node(&mut self, mut node: Box<Node<T>>) {
// This method takes care not to create mutable references to whole nodes,
// to maintain validity of aliasing pointers into `element`.
unsafe {
node.next = self.head;
node.prev = None;
let node = Some(Box::into_raw_non_null(node));

match self.head {
None => self.tail = node,
Some(mut head) => head.as_mut().prev = node,
// Not creating new mutable (unique!) references overlapping `element`.
Some(head) => (*head.as_ptr()).prev = node,
Copy link
Member

@shepmaster shepmaster Apr 18, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not sure I even understand what is going on here...

Previously, as_mut returned a &mut Node<T>, which we then modified the prev field of. That much makes sense.

Now, we get a *mut Node<T>, dereference it to get a Node<T> then set the prev field of.

I've been under the impression that setting the field of a struct implicitly created a mutable reference — where am I wrong? If I'm not wrong, why aren't we creating a second mutable reference here?

Copy link
Contributor

@Gankra Gankra Apr 19, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I believe a mutable borrow is established only if you create an &mut that isn't immediately explicitly cast to a *mut. It's very janky immediate-action rules, with similar logic to why you can take a reference to the output of arr[x] and it's not a reference to a temporary.

Regardless for a lot of these the issue isn't a mutable borrow, but rather a mutable borrow that overlaps with node.elem. By writing the code this way we never claim unique ownership of the whole type, just that we can mutate the next/prev links.

Copy link
Member Author

Choose a reason for hiding this comment

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

I believe a mutable borrow is established only if you create an &mut that isn't immediately explicitly cast to a *mut.

No, that would be rust-lang/rfcs#2582 which is not yet in.

I've been under the impression that setting the field of a struct implicitly created a mutable reference

Writing to a field has all the same effects (in terms of invalidating other references) as creating a mutable reference to it, yes.

The key point is that we only modify the prev field, and the aliasing reference we are worried about points to the data field (element or whatever it is called). I will expand the comments to clarify this.

By writing the code this way we never claim unique ownership of the whole type, just that we can mutate the next/prev links.

s/type/node/. But yes.

Copy link
Member Author

Choose a reason for hiding this comment

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

@shepmaster I tried to improve the comments explaining why these changes are needed; does this make more sense now?

Copy link
Member

Choose a reason for hiding this comment

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

Writing to a field has all the same effects (in terms of invalidating other references) as creating a mutable reference to it.

Can you remove the pronoun here? Do you mean:

  • Writing to a struct's field has the same effects as creating a mutable reference to
    • the struct
    • the field

Copy link
Member Author

Choose a reason for hiding this comment

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

I mean "to the field".

It's really the other way around -- creating a mutable reference has the effects of a write to the memory that is "covered" by the reference (size_of::<T> bytes starting at where the pointer points).

}

self.head = node;
Expand All @@ -161,13 +167,16 @@ impl<T> LinkedList<T> {
/// Removes and returns the node at the front of the list.
#[inline]
fn pop_front_node(&mut self) -> Option<Box<Node<T>>> {
// This method takes care not to create mutable references to whole nodes,
// to maintain validity of aliasing pointers into `element`.
self.head.map(|node| unsafe {
let node = Box::from_raw(node.as_ptr());
self.head = node.next;

match self.head {
None => self.tail = None,
Some(mut head) => head.as_mut().prev = None,
// Not creating new mutable (unique!) references overlapping `element`.
Some(head) => (*head.as_ptr()).prev = None,
}

self.len -= 1;
Expand All @@ -178,14 +187,17 @@ impl<T> LinkedList<T> {
/// Adds the given node to the back of the list.
#[inline]
fn push_back_node(&mut self, mut node: Box<Node<T>>) {
// This method takes care not to create mutable references to whole nodes,
// to maintain validity of aliasing pointers into `element`.
unsafe {
node.next = None;
node.prev = self.tail;
let node = Some(Box::into_raw_non_null(node));

match self.tail {
None => self.head = node,
Some(mut tail) => tail.as_mut().next = node,
// Not creating new mutable (unique!) references overlapping `element`.
Some(tail) => (*tail.as_ptr()).next = node,
}

self.tail = node;
Expand All @@ -196,13 +208,16 @@ impl<T> LinkedList<T> {
/// Removes and returns the node at the back of the list.
#[inline]
fn pop_back_node(&mut self) -> Option<Box<Node<T>>> {
// This method takes care not to create mutable references to whole nodes,
// to maintain validity of aliasing pointers into `element`.
self.tail.map(|node| unsafe {
let node = Box::from_raw(node.as_ptr());
self.tail = node.prev;

match self.tail {
None => self.head = None,
Some(mut tail) => tail.as_mut().next = None,
// Not creating new mutable (unique!) references overlapping `element`.
Some(tail) => (*tail.as_ptr()).next = None,
}

self.len -= 1;
Expand All @@ -213,18 +228,22 @@ impl<T> LinkedList<T> {
/// Unlinks the specified node from the current list.
///
/// Warning: this will not check that the provided node belongs to the current list.
///
/// This method takes care not to create mutable references to `element`, to
/// maintain validity of aliasing pointers.
#[inline]
unsafe fn unlink_node(&mut self, mut node: NonNull<Node<T>>) {
let node = node.as_mut();
let node = node.as_mut(); // this one is ours now, we can create an &mut.

// Not creating new mutable (unique!) references overlapping `element`.
match node.prev {
Some(mut prev) => prev.as_mut().next = node.next.clone(),
Some(prev) => (*prev.as_ptr()).next = node.next.clone(),
// this node is the head node
None => self.head = node.next.clone(),
};

match node.next {
Some(mut next) => next.as_mut().prev = node.prev.clone(),
Some(next) => (*next.as_ptr()).prev = node.prev.clone(),
// this node is the tail node
None => self.tail = node.prev.clone(),
};
Expand Down Expand Up @@ -297,6 +316,8 @@ impl<T> LinkedList<T> {
match self.tail {
None => mem::swap(self, other),
Some(mut tail) => {
// `as_mut` is okay here because we have exclusive access to the entirety
// of both lists.
if let Some(mut other_head) = other.head.take() {
unsafe {
tail.as_mut().next = Some(other_head);
Expand Down Expand Up @@ -916,9 +937,11 @@ impl<T> IterMut<'_, T> {
issue = "27794")]
pub fn insert_next(&mut self, element: T) {
match self.head {
// `push_back` is okay with aliasing `element` references
None => self.list.push_back(element),
Some(mut head) => unsafe {
let mut prev = match head.as_ref().prev {
Some(head) => unsafe {
let prev = match head.as_ref().prev {
// `push_front` is okay with aliasing nodes
None => return self.list.push_front(element),
Some(prev) => prev,
};
Expand All @@ -929,8 +952,10 @@ impl<T> IterMut<'_, T> {
element,
}));

prev.as_mut().next = node;
head.as_mut().prev = node;
// Not creating references to entire nodes to not invalidate the
// reference to `element` we handed to the user.
(*prev.as_ptr()).next = node;
(*head.as_ptr()).prev = node;

self.list.len += 1;
},
Expand Down Expand Up @@ -994,6 +1019,7 @@ impl<T, F> Iterator for DrainFilter<'_, T, F>
self.idx += 1;

if (self.pred)(&mut node.as_mut().element) {
// `unlink_node` is okay with aliasing `element` references.
self.list.unlink_node(node);
return Some(Box::from_raw(node.as_ptr()).element);
}
Expand Down