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.

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

fix LinkedList invalidating mutable references #60072

Merged
merged 1 commit into from
Apr 19, 2019

Conversation

RalfJung
Copy link
Member

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

@rust-highfive
Copy link
Collaborator

r? @shepmaster

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 18, 2019
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,
Copy link
Member

@shepmaster shepmaster Apr 18, 2019

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

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).

Copy link
Contributor

@Gankra Gankra left a comment

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<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 maintain
// validity of aliasing pointers into existing nodes.
Copy link
Contributor

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?

Copy link
Member Author

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
Copy link
Member

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" ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Indeed, thanks! Fixed.

@shepmaster
Copy link
Member

Thank you for the clarifications! r=me after squashing ❤️

@shepmaster
Copy link
Member

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Apr 19, 2019

📌 Commit 8b09d04 has been approved by shepmaster

@bors bors added S-waiting-on-bors 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 Apr 19, 2019
@bors
Copy link
Contributor

bors commented Apr 19, 2019

⌛ Testing commit 8b09d04 with merge 130dc3e...

bors added a commit that referenced this pull request Apr 19, 2019
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:
```rust
            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
@bors
Copy link
Contributor

bors commented Apr 19, 2019

☀️ Test successful - checks-travis, status-appveyor
Approved by: shepmaster
Pushing 130dc3e to master...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants