Surprising case of UB arising from stacked borrow rules

[dfoxfranke]

This ticket arose from some toy programs I wrote while investigating the internals of the ouroboros crate and trying to wrap my head around what AliasableDeref is needed for, but the issue turned out to be more basic. Just to be sure that what I was seeing had nothing really to do with core::ptr::Unique or even with NonNull, I wrote a trivial box implementation that uses neither of these:

use std::{
    alloc::{alloc, dealloc, Layout},
    ops::{Deref, DerefMut},
};

struct MyBox<T>(*mut T);

impl<T> MyBox<T> {
    fn new(val: T) -> MyBox<T> {
        unsafe {
            let p: *mut T = alloc(Layout::new::<T>()).cast();
            assert!(!p.is_null());
            p.write(val);
            MyBox(p)
        }
    }
}

impl<T> Deref for MyBox<T> {
    type Target = T;

    fn deref(&self) -> &T {
        unsafe { self.0.as_ref().unwrap() }
    }
}

impl<T> DerefMut for MyBox<T> {
    fn deref_mut(&mut self) -> &mut T {
        unsafe { self.0.as_mut().unwrap() }
    }
}

impl<T> Drop for MyBox<T> {
    fn drop(&mut self) {
        unsafe {
            dealloc(self.0.cast(), Layout::new::<T>());
        }
    }
}

Now, the following program produces UB in MIRI:

fn main() {
    let mut b = MyBox::new(1);
    let p = std::ptr::addr_of_mut!(*b);
    *b = 2;
    assert_eq!( unsafe { p.read() }, 2);
}

error: Undefined Behavior: attempting a read access using <2415> at alloc1092[0x0], but that tag does not exist in the borrow stack for this location
  --> src/main.rs:45:26
   |
45 |     assert_eq!( unsafe { p.read() }, 2);
   |                          ^^^^^^^^
   |                          |
   |                          attempting a read access using <2415> at alloc1092[0x0], but that tag does not exist in the borrow stack for this location
   |                          this error occurs as part of an access at alloc1092[0x0..0x4]
   |
   = help: this indicates a potential bug in the program: it performed an invalid operation, but the Stacked Borrows rules it violated are still experimental
   = help: see https://github.com/rust-lang/unsafe-code-guidelines/blob/master/wip/stacked-borrows.md for further information
help: <2415> was created by a SharedReadWrite retag at offsets [0x0..0x4]
  --> src/main.rs:43:13
   |
43 |     let p = std::ptr::addr_of_mut!(*b);
   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^
help: <2415> was later invalidated at offsets [0x0..0x4] by a Unique retag
  --> src/main.rs:29:18
   |
29 |         unsafe { self.0.as_mut().unwrap() }

But with a subtle change in how p is constructed, it runs cleanly:

fn main() {
    let mut b = MyBox::new(1);
    let p = b.0;
    *b = 2;
    assert_eq!( unsafe { p.read() }, 2);
}

The difference is that, even despite my use of addr_of_mut!, in the first version I'm temporarily creating a &mut i32 returned by deref_mut and implicitly casting it to a *mut i32, rather than obtaining a *mut i32 directly. This subtle distinction causes my pointer to be infected with stacked-borrow cooties, and become invalidated after assigning to *b. (Edit for clarification: it's not surprising that the reference is being created despite addr_of_mut!; I wasn't expecting that to do anything load-bearing, it's just how I habitually create pointers. What's surprising is that the transient existence of the reference matters at all, since it's valid).

This is quite counterintuitive, especially considering that the following practically line-for-line equivalent code is perfectly good C++:

#include <memory>
#include <cassert>

int main(void) {
    auto b = std::make_unique<int>(1);
    int *p = &*b;
    *b = 2;
    assert(*p == 2);
    return 0;
}

For Rust, even unsafe Rust, to contain a landmine like this seems to go very much against its philosophy, and I see it as indictment of the stacked borrow experiment, or at least the current iteration of its design.

cc @RalfJung

[saethlin]

You are just rediscovering rust-lang/unsafe-code-guidelines#133.

[saethlin]

I don't think there's anything in this issue that is not already captured in the comments on ucg#133. So I'm going to close this and if you think you have something to add that's not already expressed on that issue, it would be much better for you to comment there.

[RalfJung]

Thanks for the report! Yes, this is counter-intuitive. Writing unsafe code that mixes references and raw pointers is tricky business, and the worst case is when you don't even realize you are using any references.

You are just rediscovering rust-lang/unsafe-code-guidelines#133.

I don't think this is an instance of that -- I expect this will still be UB even with Tree Borrows. The OP would likely be just as surprised by this UB:

fn main() {
    let mut b = MyBox::new(1);
    let p = std::ptr::addr_of_mut!(*b);
    unsafe { p.write(13); }
    *b = 2;
    assert_eq!( unsafe { p.read() }, 2);
}

If this corresponds to any UCG issue it is rust-lang/unsafe-code-guidelines#450. It's also closely related to the UB that arises from slice::as_mut_ptr adding an intermediate mutable reference to the derivation of the resulting raw pointer -- though here, the culprit is a deref_mut method that returns a mutable reference.

Maybe std::ptr::addr_of_mut!(*b) should go through a new DerefRawMut trait or something like that, avoiding the intermediate references. But for now, the typical API pattern we use is to add a method like as_raw_ptr that avoids the intermediate reference (see e.g. Vec::as_raw_ptr -- Box doesn't need one since it is a primitive type and does not go through DerefMut, though that is also quite surprising in itself).

[dfoxfranke]

I agree with @RalfJung: this is closely related to rust-lang/unsafe-code-guidelines#450 and not related to rust-lang/unsafe-code-guidelines#133. Though that @saethlin got this wrong reinforces my view that it's going to become impossibly tricky to write sound unsafe code if any of this ever becomes more than a MIRI experiment. Whatever final form these rules take needs to be something that programmers can understand and reason about, not something that empirically covers most real-world code but is only fully understood by a handful of people and everybody else just cargo-cults around.

The DerefRawMut trait idea seems problematic to me. Changing addr_of_mut to interpret any dereference operators as desugaring to .deref_raw_mut() instead of .deref_mut() would be backward-incompatible unless it's guaranteed that everything which implements DerefMut also has some DerefRawMut implementation. But it can't be a blanket impl <T> DerefRawMut for T where T: DerefMut because that would cause overlap conflicts with any type like Vec which has a real, not-a-fallback DerefRawMut in addition to a DerefMut. So DerefRawMut would have to be an auto trait, derived for everything that implements DerefMut, but also can be overridden. Then maybe emit a warning whenever addr_of_mut invokes an auto fallback rather than a real impl, to alert you that you weren't actually avoiding the reference like you probably intended to? This all sounds unsatisfactory and doesn't get to the core issue, which is the surprise to the programmer that the pointer's heredity through a reference that doesn't even exist any more matters at all. (The same complaint could be made of strict provenance, but I find that much easier to mentally model).

Anyway, the example that Ralf gives at the top of the ucg#450 thread is even nastier than mine, and any plausible resolution to that issue will probably address this one too. So I think this issue can remain closed from here and I'll follow up on ucg#450 if I have any insight on it.