Skip to content

Commit

Permalink
Don't produce temporary &mut T when retiring
Browse files Browse the repository at this point in the history
Or: Miri saves my butt, exhibit number 19388228.

Miri was complaining that accessing a reference obtained from
`HazardPointer::load` after a call to `retire` for the loaded value was
illegal. This should not be the case, since it's exactly what hazard
pointers are intended to guard against. Following the call-chain for
`Replaced::retire`, on a hunch I hovered over `self.ptr.as_mut` and saw
(to my horror) that its return type was `&mut T` (not `*mut T`). This is
undefined behavior, since we're trying to create a `&mut T`, which
_requires_ exclusivity, while there is an active `&T` to the same
referent. Even though it's immediately turned back into a `*mut T`,
that's enough to trigger UB. The fix is to never create the `&mut` in
the first place, and just directly get the pointer from the `NonNull`.

Normally, this error would have been caught by `NonNull::as_mut` being
`unsafe`. _But_, since we're calling `retire_ptr` in the same statement,
and wrapped that whole call in `unsafe`, I didn't realize that there
were _two_ `unsafe` calls in there, not just the one. And we weren't
meeting the safety requirements of one of those calls (`as_mut`). So,
I've hoisted out the method call on `ptr` to avoid this happening again
in the future. The move from `as_mut` to `as_ptr` also means we no
longer need `&mut` to the `NonNull` (which should have been another
clue something was wrong), so the function doesn't need `mut self`
(though it does still consume ownership).

I'll add that `-Zmiri-track-pointer-tag=<tag from error>` would also
have helped track this down (rust-lang/miri#531), but I didn't find it
until after I'd found the problem. Lesson learned for next time!
  • Loading branch information
jonhoo committed Feb 28, 2022
1 parent 733917d commit 2cd10be
Showing 1 changed file with 3 additions and 2 deletions.
5 changes: 3 additions & 2 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -358,13 +358,14 @@ where
/// Note that requirement #3 is _partially_ enforced by the domain family (`F`), but it's on
/// you to ensure that you don't "cross the streams" between multiple `Domain<F>`, if those can
/// arise in your application.
pub unsafe fn retire_in(mut self, domain: &Domain<F>) -> usize {
pub unsafe fn retire_in(self, domain: &Domain<F>) -> usize {
// Safety:
//
// 1. implied by our #1 and #3: if load won't return it, there's no other way to guard it
// 2. implied by our #2
// 3. implied by AtomicPtr::new's #1 and #3
unsafe { domain.retire_ptr::<T, P>(self.ptr.as_mut()) }
let ptr = self.ptr.as_ptr();
unsafe { domain.retire_ptr::<T, P>(ptr) }
}
}

Expand Down

0 comments on commit 2cd10be

Please sign in to comment.