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

Fix UB in RawStorageMut::swap_unchecked_linear #1317

Merged
merged 6 commits into from
Mar 28, 2024
Merged

Fix UB in RawStorageMut::swap_unchecked_linear #1317

Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
adc3a81
Fix UB in `RawStorageMut::swap_unchecked_linear`
yotamofek Nov 13, 2023
546d06b
Update src/base/storage.rs
yotamofek Mar 19, 2024
d884a7e
fmt
yotamofek Mar 19, 2024
d1b5df4
Merge branch 'dev' of https://github.com/dimforge/nalgebra into swap-…
yotamofek Mar 20, 2024
095c561
Apply suggestions from code review
yotamofek Mar 20, 2024
3651942
Merge branch 'dev' of https://github.com/dimforge/nalgebra into swap-…
yotamofek Mar 22, 2024
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
22 changes: 20 additions & 2 deletions src/base/storage.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -206,10 +206,28 @@ pub unsafe trait RawStorageMut<T, R: Dim, C: Dim = U1>: RawStorage<T, R, C> {
///
/// # Safety
/// If the indices are out of bounds, the method will cause undefined behavior.
yotamofek marked this conversation as resolved.
Show resolved Hide resolved
///
/// # Validity
/// The default implementation of this trait function is only guaranteed to be
/// sound if invocations of `self.ptr_mut()` and `self.get_address_unchecked_linear_mut()`
/// result in stable references. If any of the data pointed to by these trait methods
/// moves as a consequence of invoking either of these methods then this default
/// trait implementation may be invalid or unsound and should be overridden.
#[inline]
unsafe fn swap_unchecked_linear(&mut self, i1: usize, i2: usize) {
let a = self.get_address_unchecked_linear_mut(i1);
let b = self.get_address_unchecked_linear_mut(i2);
// we can't just use the pointers returned from `get_address_unchecked_linear_mut` because calling a
// method taking self mutably invalidates any existing (mutable) pointers. since `get_address_unchecked_linear_mut` can
// also be overriden by a custom implementation, we can't just use `wrapping_add` assuming that's what the method does.
// instead, we use `offset_from` to compute the re-calculate the pointers from the base pointer.
// this is sound as long as this trait matches the Validity preconditions
// (and it's the caller's responsibility to ensure the indices are in-bounds).
let base = self.ptr_mut();
let offset1 = self.get_address_unchecked_linear_mut(i1).offset_from(base);
let offset2 = self.get_address_unchecked_linear_mut(i2).offset_from(base);
Comment on lines +224 to +226
Copy link
Collaborator

Choose a reason for hiding this comment

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

Come to think of it -- is this technically violating stacked borrows, as well? ptr_mut and get_address_unchecked_linear_mut both accept &mut self and so should also invalidate the previous references, right? I suspect the "correct" way to do this would be to have immutable versions of the trait methods that can be used to compute this validly.

But barring that, we'd need to make sure the borrows don't overlap. Since we need both the base pointer and the element pointer to be alive at the same time, that's a nonstarter. So perhaps integer arithmetic is the escape hatch;

Suggested change
let base = self.ptr_mut();
let offset1 = self.get_address_unchecked_linear_mut(i1).offset_from(base);
let offset2 = self.get_address_unchecked_linear_mut(i2).offset_from(base);
let base = self.ptr_mut() as isize;
let offset1 = self.get_address_unchecked_linear_mut(i1) as isize - base;
let offset2 = self.get_address_unchecked_linear_mut(i2) as isize - base;

I'm not sure if this is equally-invalid though. It might just be tricking MIRI into not realizing we're doing something we're not meant to be doing in the first place. I'm not familiar enough with MIRI to know, myself. Does the current commit satisfy MIRI's checker?

But maybe I'm missing the point, and doing all this with mutable pointers and mutable borrows is fine as long as they're never dereferenced in an invalid state.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You said it yourself - calling methods with a &mut self receiver invalidates any previous references. Pointers are not references, and are in themselves an escape hatch from the stacked borrows model, since pointers don't constitute a borrow. 😊 My code causes the swap tests to pass under MIRI so I'm pretty confident they're okay.

OTOH, ptr->int casts are almost always a bad idea, since that causes the loss of provenance. (it might not matter here - but I'm not much of an expert, I mostly rely on MIRI to tell me if I'm misbehaving)

Copy link
Collaborator

Choose a reason for hiding this comment

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

The original code also operated on pointers, so I suspect MIRI is smart enough to extend its analysis to pointers if it was nevertheless managing to raise an error there. But the original error message says "read access" which leads me to believe that probably the issue was that the pointers were dereferenced after being invalidated, rather than just that they were used, so it's reasonable that MIRI's okay with this code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right you are, my last comment is just completely wrong, was not thinking straight. 😅 Pointers are an escape hatch from the borrow checker, but their usage should still uphold (stacked) borrowing rules.
The reason my PR appeases MIRI is because we get a "fresh" base from self.ptr_mut() just below this comment, and we don't use any of the already-invalidated pointers from before that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FWIW, your version with the ptrtoint casts also appeases MIRI, after a minor correction. It should look like this:

let base = self.ptr_mut() as isize;
let offset1 = (self.get_address_unchecked_linear_mut(i1) as isize - base) / (size_of::<T>() as isize);
let offset2 = (self.get_address_unchecked_linear_mut(i2) as isize - base) / (size_of::<T>() as isize);

(notice the div by size_of::<T>() that you were missing)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Right; or rather, we do "use" an invalidated pointer here:

self.get_address_unchecked_linear_mut(i1).offset_from(base);

Since this uses base after invalidating it with a call to get_address_unchecked_linear_mut, but it doesn't dereference it, which I suspect is why MIRI is fine with it. It just uses it as basically an integer.


let base = self.ptr_mut();
let a = base.offset(offset1);
let b = base.offset(offset2);

ptr::swap(a, b);
}
Expand Down
Loading