Stream: t-lang/wg-unsafe-code-guidelines

Topic: stacked borrows: "gap" between mutable and shared refs


RalfJ (Nov 21 2018 at 16:09, on Zulip):

I just realized that one of these things that I "accidentally" made work, maybe shouldn't work... namely, there is a guarantee that we might want to have but currently do not have:

fn foo(x: &mut i32) -> i32 {
  *x = 5;
  unknown_code(&*x);
  *x // must return 5
}

Here's a counterexample that is accepted by miri.
The thing is that we see "creating a shared reference" first as sharing, then as freezing. Writing to this location un-freezes, but can then rely on the sharing to write. Not good.
I think this would be easy to fix, but then we would have to patch libcore:

impl<'a, T: ?Sized> From<&'a mut T> for NonNull<T> {
    #[inline]
    fn from(reference: &'a mut T) -> Self {
        NonNull { pointer: NonZero(reference as _) }
    }
}

this desugars, with coercions, to reference as &T as *const T, so if later we do as_ptr on this NonNull, we are not allowed to write. My problem is less with ruling out this sequence of events (it seems that we really should), and more with the fact that looking at the code here, it is very non-obvious what happens. Ideally, we could tweak the coercion mechanism such that it would desugar to reference as *mut T as *const T, but @nikomatsakis said they don't want to touch coercions as they are too fragile.^^

Cc @Gankro @nikomatsakis

Gankro (Nov 21 2018 at 16:24, on Zulip):

thinking out loud: It seems like there should be a difference between creating a shared reference locally and passing a shared reference to a function

Gankro (Nov 21 2018 at 16:29, on Zulip):

or alternatively, transiently constructing a reference to make a raw ptr shouldn't "count"?

RalfJ (Nov 21 2018 at 16:48, on Zulip):

or alternatively, transiently constructing a reference to make a raw ptr shouldn't "count"?

not sure how to formalize that. we cannot predict the future, when the shared ref is created

RalfJ (Nov 21 2018 at 16:49, on Zulip):

thinking out loud: It seems like there should be a difference between creating a shared reference locally and passing a shared reference to a function

we could make this about the fn barrier, but I am not sure if we want to. I sometimes like to imagine unknown_code to be a hard-to-analyze block of code in the same function that does not access x directly (in the above example)

RalfJ (Nov 21 2018 at 22:07, on Zulip):

Ah I remember now, just fixing NonNull::from is not enough... we also have

        Drain {
            deque: NonNull::from(&mut *self),
            after_tail: drain_head,
            after_head: head,
            iter: Iter {
                tail: drain_tail,
                head: drain_head,
                ring: unsafe { self.buffer_as_mut_slice() },
            },
        }

Notice how we first create a raw pointer from self (for deque), but then pass self to another function, which promises that function exclusive access to the given memory and invalidates every reference previously derived from self.

RalfJ (Nov 22 2018 at 13:47, on Zulip):

PR to fix VecDeque (well, the tiny bits of it covered by miri's test suite): https://github.com/rust-lang/rust/pull/56161

gnzlbg (Nov 22 2018 at 13:53, on Zulip):

Creating a NonNull<T> from &mut T goes through &T, then *const T, then NonNull<T>.

Uhhhhh

gnzlbg (Nov 22 2018 at 13:55, on Zulip):

VecDeque::drain creates the Drain struct by first creating a NonNull from self (which is an &mut VecDeque), and then calling self.buffer_as_mut_slice(). The latter reborrows self, asserting that self is currently the unique pointer to access this VecDeque, and hence invalidating the NonNull that was created earlier. This PR fixes that by calling buffer_as_mut_slice on an &mut derived from the NonNull.

So IIUC Drain contains a NonNull to the VecDeque and a &mut [T] to the VecDeque and it currently constructs both from self.

gnzlbg (Nov 22 2018 at 13:56, on Zulip):

So IIUC, a minimal version of this problem is:

let mut x = Foo;
Bar {
   a: &mut x as *mut _ as *const _,
   b: &mut x,
}

?

RalfJ (Nov 22 2018 at 13:57, on Zulip):

So IIUC Drain contains a NonNull to the VecDeque and a &mut [T] to the VecDeque and it currently constructs both from self.

Yes, and it constructors the latter later, by calling a function which reborrows the entire self, which invalidates the first.

RalfJ (Nov 22 2018 at 13:57, on Zulip):

@gnzlbg Yes. That example itself is fine, but using a afterwards is not

RalfJ (Nov 22 2018 at 13:58, on Zulip):

because you created b as a unique reference to x, so there cannot be another one

gnzlbg (Nov 22 2018 at 13:58, on Zulip):

but a is a raw pointer

gnzlbg (Nov 22 2018 at 13:58, on Zulip):

I'd expect that reading through a is ok, but writing through it is not.

gnzlbg (Nov 22 2018 at 13:58, on Zulip):

ah no, wait, _unique_

gnzlbg (Nov 22 2018 at 13:59, on Zulip):

I find the fix of NonNull ok, but I find the fix of VecDeque tricky

gnzlbg (Nov 22 2018 at 13:59, on Zulip):

I would prefer if ring would also just be a raw pointer

gnzlbg (Nov 22 2018 at 14:01, on Zulip):

that is, i find the fact that we have to get the raw pointer for ring from the raw pointer to the deque tricky

RalfJ (Nov 22 2018 at 14:01, on Zulip):

I would prefer if ring would also just be a raw pointer

that would be an option. Though notice that the way the code is written, we are creating new exclusive references already just to call buffer_as_mut_slice. So if that function returned a raw ptr, that would not change anything on its own.

gnzlbg (Nov 22 2018 at 14:03, on Zulip):

or for Iter to just contain a pointer to the deque instead of the slice

RalfJ (Nov 22 2018 at 14:03, on Zulip):

that is, i find the fact that we have to get the raw pointer for ring from the raw pointer to the deque tricky

it's a direct consequence of the idea that &mut is unique when being used in a reborrow-to-&mut

RalfJ (Nov 22 2018 at 14:04, on Zulip):

but yes, mixing raw pointers and mutable references can be tricky in this model, particularily considering that self methods usually take references -- so mixing methods that take &mut self and raw pointers is risky business

gnzlbg (Nov 22 2018 at 14:04, on Zulip):

Yeah the problem is that the &mut used to construct the NonNull is unique, and we invalidate it by taking another &mut to self in ring

rkruppe (Nov 22 2018 at 14:06, on Zulip):

or for Iter to just contain a pointer to the deque instead of the slice

wouldn't that mean double indirection and inability to use the (start ptr, end ptr) representation of iterators (= slowdowns)?

gnzlbg (Nov 22 2018 at 14:06, on Zulip):

yeah, i think the current fix is fine

gnzlbg (Nov 22 2018 at 14:09, on Zulip):

The current Iter has a ring: &[T]

gnzlbg (Nov 22 2018 at 14:09, on Zulip):

why does it have to use "buffer_as_mut_slice()" ?

RalfJ (Nov 22 2018 at 15:05, on Zulip):

Turns out just not using a mutable buffer, as suggested by @gnzlbg , is enough :)

RalfJ (Nov 22 2018 at 15:14, on Zulip):

And here is the second PR do deal with this strengthening of Stacked Borrows: We need to retag in the drop shims. https://github.com/rust-lang/rust/pull/56165

RalfJ (Nov 22 2018 at 15:26, on Zulip):

With these two fixes, the strengthened model passes miri's test suite :D

RalfJ (Nov 22 2018 at 15:27, on Zulip):

(for whatever that is worth^^)

Last update: Nov 19 2019 at 18:05UTC