Mutable reference protected by a mutex in static context is considered UB

[hieronymusma]

I tried this code:

#![feature(const_mut_refs)]

use std::sync::Mutex;

static MUTABLE_REFERENCE_HOLDER: Mutex<&mut [u8]> = Mutex::new(&mut []);

fn main() {}

I expected to see this happen: This code should compile because the mutable reference is protected by a Mutex and therefore, it is safe to access.

Instead, this happened: I receive the following compiler error:

error[E0080]: it is undefined behavior to use this value
 --> src/main.rs:5:1
  |
5 | static MUTABLE_REFERENCE_HOLDER: Mutex<&mut [u8]> = Mutex::new(&mut []);
  | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ constructing invalid value at .data.value: encountered mutable reference in a `const` or `static`
  |
  = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rustc repository if you believe it should not be considered undefined behavior.
  = note: the raw bytes of the constant (size: 24, align: 8) {
              0x00 │ 00 00 00 00 00 __ __ __ ╾───────alloc3────────╼ │ .....░░░╾──────╼
              0x10 │ 00 00 00 00 00 00 00 00                         │ ........
          }

I am not entirely sure if I'm missing something here. Is this UB or not?
Since a mutable reference to a slice of u8 is Send the mutex is Send + Sync. Therefore, I assume this should be allowed?

If this is in fact a mistake in the compiler, I'd be glad to try to provide a pull request with a fix. In my head the compiler need to look at the Sync trait and if it is implement by a type, this is not UB.

Meta

rustc --version --verbose:

rustc 1.77.0-nightly (6b4f1c5e7 2024-01-27)
binary: rustc
commit-hash: 6b4f1c5e782c72a047a23e922decd33e7d462345
commit-date: 2024-01-27
host: x86_64-unknown-linux-gnu
release: 1.77.0-nightly
LLVM version: 17.0.6

[saethlin]

In my head the compiler need to look at the Sync trait and if it is implement by a type, this is not UB.

I think you're assuming the problem is in type-checking, but if that were the case this would be an entirely different error message. This error comes out of const validation:

rust/compiler/rustc_const_eval/src/interpret/validity.rs

Line 606 in 6351247

throw_validation_failure!(self.path, MutableRefInConst);

[hieronymusma]

I'm new to the rust compiler, so please bear with me.

Is it even possible to fix the behavior I describend in the const validation? Do I have access to the information of a certain type implements Sync or not?
I tried to figure it out myself but didn't come that far...

Thanks!

[saethlin]

Trait bounds are not related. Looking through the tracking issue for const_mut_refs, this seems relevant: #57349 (comment)

[RalfJung]

Oh interesting, is this a regression introduced by #119044?
Though we had MutableRefInConst already before that so probably, no. Still, we should figure out what to do with this before stabilizing const_mut_refs.
Cc @oli-obk

[RalfJung]

Urgh, this is a nasty interaction. This gets rejected the way it should:

static MUTABLE_REFERENCE_HOLDER: Mutex<&mut [u8]> = Mutex::new(&mut [1]);
// ERROR: error[E0716]: temporary value dropped while borrowed

But with an empty slice, it gets promoted, and so there's no issue with pointing to a temporary -- and so we reach const validation, where it gets rejected as expected.

We may have to adjust validity to make it so that "mutable reference to 0 bytes" is allowed. But I'm concerned that people might then unsafely turn that into a reference that covers more than 0 bytes and now we need subobject provenance to explain why their code is UB and I don't want to have subobject provenance.

What we could allow is a mutable reference that doesn't point to actual memory. I don't know what exactly the internal representation of the empty slice here will be, but that should be sufficient for this specific case.

[saethlin]

🤔 Could we do something as restrictive as ZST promoteds?

For OP's use-case (which seems reasonable) the mutable reference is never going to be used, so it's all the same if it never points to an allocation, right? Oh that's what you just said. I guess my brain is warming up.

[RalfJung]

If this is in fact a mistake in the compiler, I'd be glad to try to provide a pull request with a fix. In my head the compiler need to look at the Sync trait and if it is implement by a type, this is not UB.

This has nothing to do with Sync, no. We just want to be extremely careful with when we allow anything mutable to be created implicitly, and your code looks to the compiler as if it was doing that.

The compiler is wrong though, there is nothing mutable, so I fixed the check: #121179.

[RalfJung]

However even with that PR, we still reject this code:

#![feature(const_mut_refs)]
use std::sync::Mutex;

static BAR: Mutex<&mut [u8]> = unsafe {
  static mut FOO: [u8; 1] = [42]; // a private static that we are sure nobody else will reference
  Mutex::new(&mut FOO)
};

Arguably there is no issue with implicitly creating mutable memory here. It's not even unsound, since we are hiding the static mut in a way that we definitely have the only reference to it. So... do we have to reject this? Or should we accept it?

It already passes the static checks. (Interestingly, it passes them even before #120932 landed. Turns out the local that stores the static mut address does have a StorageDead here so we consider this a transient borrow. MIR building has some odd inconsistencies...)

[RalfJung]

Reopening due to my comment above.

[oli-obk]

I'm slowly wondering why we reject mutable allocs within immutable statics at all 😅. You can only use them mutably if they are only used via raw pointers or protected by some mutex-like guard. Even

static FOO: &mut u8 = &mut 42;

Is just surprising, but not actually mutable

[RalfJung]

I think for your example I'd say it's not nice that the mutable allocation doesn't have a "name", that it just exists implicitly as a local variable in the static. If I could rewind time I'd entirely reject having such implicit statics (i.e., not apply the "enclosing scope" rule to static at all), I'd rather not extend their power.

[oli-obk]

The reason I'm bringing it up is that we could eliminate static mut from the compiler after ast lowering. If we have to have the complexity anyway, and with interior mutability for immut statics, too. Then why not just permit actual mutable references?

[RalfJung]

with interior mutability for immut statics, too.

We don't allow mutable "inner" allocations in static, not even with interior mutability. It's only static mut where we allow this.

[RalfJung]

I think before #119044, this code actually got accepted. That PR introduced

    fn may_contain_mutable_ref(self) -> bool {
        match self {
            CtfeValidationMode::Static { mutbl } => mutbl == Mutability::Mut,
            CtfeValidationMode::Promoted { .. } => false,
            CtfeValidationMode::Const { .. } => false,
        }
    }

whereas previously we only rejected mutable references in Const.

So maybe we should just change that to say true for all statics? The interner still ensures that there are no new mutable allocations, and validity ensures that mutable references point to mutable allocations.

[oli-obk]

So maybe we should just change that to say true for all statics?

👍

The interner still ensures that there are no new mutable allocations, and validity ensures that mutable references point to mutable allocations.

jup