Undefined Behavior in ARCacheReadTxn::get

[y21]

https://github.com/kanidm/concread/blob/020e8c2ad2fba94cd5efff759fdae20c332742d1/src/arcache/mod.rs#L2041-L2043

The transmute is UB according to miri, the language reference and the Rustonomicon:

Transmuting an & to &mut is Undefined Behavior. While certain usages may appear safe, note that the Rust optimizer is free to assume that a shared reference won't change through its lifetime and thus such transmutation will run afoul of those assumptions. So:

• Transmuting an & to &mut is always Undefined Behavior.

Miri output

``` Undefined Behavior: trying to reborrow <914429> for Unique permission at alloc355143[0x0], but that tag only grants SharedReadOnly permission for this location --> src/arcache/mod.rs:2061:36 | 2061 | let hits = std::mem::transmute::<&u32, &mut u32>(&self.hits); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | | | trying to reborrow <914429> for Unique permission at alloc355143[0x0], but that tag only grants SharedReadOnly permission for this location | this error occurs as part of a reborrow at alloc355143[0x0..0x4] ```

Thought I'd let you know as the README says:

This library has extensive testing, and passes it's test suite under miri, a rust undefined behaviour checker. If you find an issue however, please let us know so we can fix it!

[Firstyear]

Thanks! Yeah, this miri rule looks new - we need to also update to use sptr to allow pointer tracking given the amount of raw pointer handling we do.

In this situation, this is safe, because we are mutating a value from within the mut of self, so we are the only holder, the reason we need that undefined transmute is because the rust borrow checker is too coarse. When we borrow for the call to .get() it makes the whole instance of self immutable as a borrow meaning we also can't mutate the internal set of counters. We could refactor a bit to get around this though and to "appease the powers that be".

[y21]

In this situation, this is safe, because we are mutating a value from within the mut of self, so we are the only holder

I don't think that's quite true, to my knowledge transmute<&T, &mut T>() is always UB, no matter what the T is or where it's from, even if it's derived from a mutable location and no matter if aliasing is taking place or not, no exceptions. Not even if T is an UnsafeCell or behind an UnsafeCell.

For example, this fails under miri

#![allow(mutable_transmutes)]
use core::cell::UnsafeCell;
struct X(u32);

fn main() {
    let mut x = UnsafeCell::new(X(5)); // <- inner u32 can be mutated with just &UnsafeCell<X>

    // this fails, even though x is clearly marked a mutable memory location,
    // because of the intermediate shared reference
    // this effectively goes `&UnsafeCell<X> -> &u32 -> &mut u32`
    let y = unsafe { std::mem::transmute::<&u32, &mut u32>(&(*x.get()).0) };
}

I might be mistaken; feel free to correct me if I'm wrong. 🙂

[Firstyear]

It's all good. I think in this case I mean it's safe WRT to thread safety, but it's UB by Rust's ideals. I'm going to clean it up and fix it anyway, since appeasing miri is worth it :)