rust-lang / miri

An interpreter for Rust's mid-level intermediate representation
Apache License 2.0
4.13k stars 318 forks source link

Calling a method on `&mut Box<T>` is not detected as a Unique retag #3673

Closed shdnx closed 5 days ago

shdnx commented 2 weeks ago

Given:

struct Counter {
    value: i32,
}

impl Counter {
    fn new() -> Self {
        Self { value: 0 }
    }

    fn increment(&mut self) {
        self.value += 1;
    }
}

This is correctly detected as UB:

fn without_box_fail() {
    let mut counter = Counter::new();
    let ptr: *mut Counter = &mut counter;

    // Miri correctly detects that this invalidates `ptr` ("Unique function-entry retag"):
    counter.increment();

    // Therefore this is reported as UB ("trying to retag from <2456> for SharedReadWrite permission at alloc1257[0x0], but that tag does not exist in the borrow stack for this location"):
    unsafe {
        (*ptr).increment();
    }
    println!("{}", counter.value);
}

But if counter is a Box<Counter> instead, then no UB is reported because the increment() call seemingly is not detected as creating a &mut Counter:

// Same as `without_box_fail()`, but with `Box<Counter>`:
fn with_box_pass() {
    let mut counter = Box::new(Counter::new());
    let ptr: *mut Box<Counter> = &mut counter;

    // Miri doesn't detect that this invalidates `ptr`:
    counter.increment();

    // UB should be reported, but is not!
    unsafe {
        (*ptr).increment();
    }
    println!("{}", counter.value);
}

However, if ptr is instead *mut Counter, then UB is again correctly detected:

// Same as `with_box_pass()` but with taking a pointer to `Counter` and not `Box<Counter>`
fn with_box_fail_deref() {
    let mut counter = Box::new(Counter::new());
    let ptr: *mut Counter = &mut *counter;

    // Miri does correctly detect that this invalidates `ptr`:
    counter.increment();

    // UB is reported:
    unsafe {
        (*ptr).increment();
    }
    println!("{}", counter.value);
}

Also correctly detected if I make the formation of &mut Counter before the method call explicit:

// Same as `with_box_pass()` but with the formation of `&mut` explicit:
fn with_box_fail_explicit() {
    let mut counter = Box::new(Counter::new());
    let ptr: *mut Box<Counter> = &mut counter;

    // Miri does correctly invalidate `ptr` if you make the formation of `&mut` explicit:
    let mref = &mut counter;
    mref.increment();

    // UB is reported:
    unsafe {
        (*ptr).increment();
    }
    println!("{}", counter.value);
}
RalfJung commented 2 weeks ago

Thanks for the report!

The aliasing model is not recursive, so this seems expected to me -- a &mut Box<T> is a unique pointer to a Box, but the inner T only becomes unique once you actually manifest a Box<T>.

Also correctly detected if I make the formation of &mut Counter before the method call explicit:

This is because with an explicit &mut counter, you are doing the equivalent of a write to counter (i.e. the Box itself, not the integer inside). An implicit &mut is a two-phase borrow and does not make such uniqueness assertions.

RalfJung commented 1 week ago

Also, note that if you want to make the implicit &mut explicit, the result will look a bit different: counter.increment() where counter: Box<Counter> becomes (&mut *counter).increment(), not (&mut counter).increment().

shdnx commented 6 days ago

Thank you! I think I see what you mean: having a pointer to Box<T> and having a pointer to the underlying T are two different things, and do not alias. Which makes sense because they are two different memory locations.

It then indeed makes sense that a pointer to the Box<T> is not invalidated by forming a mutable reference to the underlying T. Except that I imagine that the only way to take a mutable reference to the underlying T is by first taking a mutable reference to its containing Box: deref_mut() takes a &mut self, and I expect that the * operator is just syntactic sugar for calling deref_mut() (or deref()).

In other words, I'd expect that all of these are equivalent:

let mut counter = Box::new(Counter::new());
let ptr: *mut Box<Counter> = &mut counter;

counter.increment(); // 1
(*counter).increment(); // 2
(&mut *counter).increment(); // 3
counter.deref_mut().increment(); // 4
(&mut counter).deref_mut().increment(); // 5

But according to Miri, 1, 2, and 3 don't invalidate ptr, but 4 and 5 do.

shdnx commented 6 days ago

This is how Deref and DerefMut seem to be implemented for Box:

#[stable(feature = "rust1", since = "1.0.0")]
impl<T: ?Sized, A: Allocator> Deref for Box<T, A> {
    type Target = T;

    fn deref(&self) -> &T {
        &**self
    }
}

#[stable(feature = "rust1", since = "1.0.0")]
impl<T: ?Sized, A: Allocator> DerefMut for Box<T, A> {
    fn deref_mut(&mut self) -> &mut T {
        &mut **self
    }
}

Which either shoots a hole in my expectation that the * operator is just sugar for deref() / deref_mut() (since then this would be unbounded recursion)... or Box is just magic?

RalfJung commented 5 days ago

Yeah Box deref is special, it does not go through DerefMut (similar to how 1 + 2 does not go through Add).

shdnx commented 5 days ago

Thank you for the information and help! :) I will close this then, since it is not an issue in Miri.