fix LinkedList invalidating mutable references by RalfJung · Pull Request #60072 · rust-lang/rust (original) (raw)
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.
By clicking “Sign up for GitHub”, you agree to our terms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation17 Commits1 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
The test test_insert_prev failed in Miri due to what I consider a bug in LinkedList: in various places, NonNull::as_mut got called to modify the prev/next pointers of existing nodes. In particular, the unstable insert_next has to modify the next pointer of the node that was last handed out by the iterator; to this end it creates a mutable reference to the entire node that overlaps with the mutable reference to the node's content that was handed out by the iterator! Thus, the next use if said mutable reference is UB.
In code:
loop {
match it.next() { // mutable reference handed to us
None => break,
Some(elt) => {
it.insert_next(*elt + 1); // this invalidates `elt` because it creates an overlapping mutable reference
match it.peek_next() {
Some(x) => assert_eq!(*x, *elt + 2), // this use of `elt` now is a use of an invalid pointer
None => assert_eq!(8, *elt),
}
}
}
}This PR fixes that by using as_ptr instead of as_mut. This avoids invalidating the mutable reference that was handed to the user. I did this in all methods called by iterators, just to be sure.
Cc @gankro
r? @shepmaster
(rust_highfive has picked a reviewer for you, use r? to override)
| Some(mut head) => head.as_mut().prev = node, |
|---|
| // Not creating new mutable (unique!) references to not invalidate |
| // references we handed out. |
| Some(head) => (*head.as_ptr()).prev = node, |
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?
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.
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.
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?
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
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).
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems fine, r=me, but I'm not actually a reviewer, so
| @@ -143,14 +146,18 @@ impl LinkedList { |
|---|
| /// Adds the given node to the front of the list. |
| #[inline] |
| fn push_front_node(&mut self, mut node: Box<Node>) { |
| // This method takes care not to create mutable references, to maintain |
| // validity of aliasing pointers into existing nodes. |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you clarify that the aliasing pointers are to the elements so it is specifically fine for us to prod at all the next/prev links with wild abandon?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, done.
| None => self.list.push_back(element), // push_back is okay with aliasing nodes |
|---|
| Some(head) => unsafe { |
| let prev = match head.as_ref().prev { |
| // push_back is okay with aliasing nodes |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should that be "push_front is okay with aliasing nodes" ?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, thanks! Fixed.
Thank you for the clarifications! r=me after squashing ❤️
📌 Commit 8b09d04 has been approved by shepmaster
Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
and removed S-waiting-on-review
Status: Awaiting review from the assignee but also interested parties.
labels
bors added a commit that referenced this pull request
fix LinkedList invalidating mutable references
The test test_insert_prev failed in Miri due to what I consider a bug in LinkedList: in various places, NonNull::as_mut got called to modify the prev/next pointers of existing nodes. In particular, the unstable insert_next has to modify the next pointer of the node that was last handed out by the iterator; to this end it creates a mutable reference to the entire node that overlaps with the mutable reference to the node's content that was handed out by the iterator! Thus, the next use if said mutable reference is UB.
In code:
loop {
match it.next() { // mutable reference handed to us
None => break,
Some(elt) => {
it.insert_next(*elt + 1); // this invalidates `elt` because it creates an overlapping mutable reference
match it.peek_next() {
Some(x) => assert_eq!(*x, *elt + 2), // this use of `elt` now is a use of an invalid pointer
None => assert_eq!(8, *elt),
}
}
}
}This PR fixes that by using as_ptr instead of as_mut. This avoids invalidating the mutable reference that was handed to the user. I did this in all methods called by iterators, just to be sure.
Cc @gankro
This was referenced
Aug 16, 2019
Labels
This PR was explicitly merged by bors.
Status: Waiting on bors to run and complete tests. Bors will change the label on completion.