ICE when dealloc causes Stacked Borrows error

[RalfJung]

I realized we don't have a test hitting this, and indeed trying to do so currently ICEs:

use std::alloc::{alloc, dealloc, Layout};

fn main() { unsafe {
    let x = alloc(Layout::from_size_align_unchecked(1, 1));
    let ptr1 = (&mut *x) as *mut u8;
    let ptr2 = (&mut *ptr1) as *mut u8;
    // Invalidate ptr2 by writing to ptr1.
    ptr1.write(0);
    // Deallocate through ptr2.
    dealloc(ptr2, Layout::from_size_align_unchecked(1, 1));
} }

produces

thread '<unnamed>' panicked at 'internal error: entered unreachable code: access_error should only be called during an access', src/stacked_borrows/diagnostics.rs:398:13

The trouble is here:

miri/src/stacked_borrows/mod.rs

Lines 477 to 480 in 2f92cf2

	// Step 1: Make a write access.
	// As part of this we do regular protector checking, i.e. even weakly protected items cause UB when popped.
	self.access(AccessKind::Write, tag, global, dcx, exposed_tags)
	.map_err(|_| dcx.dealloc_error())?;

We try to use map_err to make this a deallocation error, but when that happens access has already called dcx.access_error, causing the ICE. (The ICE is a regression introduced by #2684.)

@saethlin any idea how to best fix this?

[RalfJung]

I feel like access_error should just use the information present in self.operation to make a nice error.

[saethlin]

grant_error, access_error, and dealloc_error only work with one kind of Operation each. protector_error however recognized 3 kinds of Operation. So if you can find a way to merge all 3 of grant_error, access_error, and dealloc_error together into a function with a decent name which branches on Operation to figure out what error text to emit, and when presented this program mentions a deallocation that sounds good to me. I'm not sure if that is workable though.

Special-casing access_error for this code pattern doesn't seem right to me, because there is no memory access. But perhaps there another generalization we are missing, because the way you (or SB) use "access" is not necessarily in line with "a read or write to memory". SB does an as-if access inside grant. Maybe I'm just grumpy about the fact that there is no distinction between "a read or write to memory" and "a virtual access that establishes aliasing guarantees" in the nomenclature. So perhaps there should be a branch inside access_error, and we should adjust the names so that it makes more sense.

[RalfJung]

Yeah, some of our 'access' mean 'do all the things we would do for a memory access, as a way to enforce some aliasing property', and some mean 'an actual load/store'. But the codebase was already confused about that before, #2694 (which makes access_error also work fine for retags and dealloc) doesn't really make it any worse.

[saethlin]

So yes I agree that #2694 is good enough with the somewhat-confused status of our naming.