#[thread_local] static mut is allowed to have 'static lifetime

[nikomatsakis]

Spinning off from #51269:

The MIR borrow checker (i.e., NLL) permits #[thread_local] static mut to have 'static lifetime. This is probably a bug. It arises because we ignore borrows of "unsafe places", which includes static mut.

We probably ought to stop doing that — at least not ignoring them entirely — but if we do so, we have to ensure that we continue to accept overlapping borrows of static mut (even though that is basically guaranteed UB), since it compiles today:

fn main() {
 static mut X: usize = 22;
 unsafe {
  let p = &mut X;
  let q = &mut X;
  *p += 1;
  *q += 1;
  *p += 1;
 }
}

[pnkfelix]

Re-triaging for #56754. Based on #29594 (comment) I am tagging this as P-medium.

[oli-obk]

Fixing this issue is easy now, but it will be super hard to do with a future incompat lint, because the real change is happening elsewhere. The fix is to change https://github.com/rust-lang/rust/blob/a916ac22b9f7f1f0f7aba0a41a789b3ecd765018/src/librustc/ty/util.rs#L666.L667 to

if self.is_mutable_static(def_id) {
    self.mk_mut_ref(self.lifetimes.re_erased, static_ty)

Because then borrowck will pick it up correctly and detect the thread local annotation and not give it the static lifetime

[bstrie]

@oli-obk, is a future incompat lint necessary if this feature is still unstable? It doesn't sound like that change would affect stable users.

[nikomatsakis]

I agree a lint is probably not needed. I'm not sure whether I think @oli-obk's suggested patch is correct (like, I literally haven't looked into it -- it probably is).

[oli-obk]

While attempting to fix #70685 (see #71192 for the changes) I got all of our tests to pass, except for https://github.com/rust-lang/rust/blob/master/src/test/ui/nll/borrowck-thread-local-static-mut-borrow-outlives-fn.rs which references this issue. I have reinstated the current behaviour, but the site of the required change that I suggested above has changed. It's now in the Rvalue::ty method.

[RalfJung]

Curiously, doing the same thing with a non-mut static is detected by the borrow checker.

[RalfJung]

@oli-obk

The fix is to change https://github.com/rust-lang/rust/blob/a916ac22b9f7f1f0f7aba0a41a789b3ecd765018/src/librustc/ty/util.rs#L666.L667 to

Isn't that a problem with unsafety checking? You said elsewhere that this crucially is a raw pointer to ensure that accesses are unsafe.

[oli-obk]

We do unsafety checking properly nowadays:

rust/compiler/rustc_mir/src/transform/check_unsafety.rs

Lines 207 to 216 in c9c57fa

	// If the projection root is an artifical local that we introduced when
	// desugaring `static`, give a more specific error message
	// (avoid the general "raw pointer" clause below, that would only be confusing).
	if let Some(box LocalInfo::StaticRef { def_id, .. }) = decl.local_info {
	if self.tcx.is_mutable_static(def_id) {
	self.require_unsafe(
	UnsafetyViolationKind::General,
	UnsafetyViolationDetails::UseOfMutableStatic,
	);
	return;

So the raw pointer checks aren't actually necessary, even though they are a good safety net for our use of LocalInfo for soundness.

[RalfJung]

"properly" for some notion of "properly". ;) Relying on LocalInfo feels rather fragile.

[VorpalBlade]

Will this be solved in edition 2024 by the changes to static mut being done? Or will this still be an issue?

[arielb1]

What is the bug here? Why is borrowing a static mut for 'static "less sound" than anything else unsafe you can do with a static mut?

Is the problem that static mut tempts people to write functions taking &'static mut Self in an incorrect manner? In that case I believe the 2024 changes would solve it.

[RalfJung]

The borrow checker clearly tries to account for the fact that thread_local statics don't have 'static lifetime... but then that doesn't apply to thread_local static mut. IMO that's clearly a bug. But it's not a soundness bug since creating such a reference is still unsafe.

[arielb1]

So the annoying thing is that the thread local rvalue needs to return *mut T to avoid taking a reference (which would invalidate all pre-existing shared pointers), but that does not remember the lifetime.

Or does

static mut X: (u32, u32) = (0, 0);

fn a() {
    &mut X.1;
}

translating to

_0: &mut (u32, u32) = ThreadLocalRef(X);
_1: &mut u32 = &mut (*_0).0

actually does not annoy MIRI too much

Therefore, there is a semantic reason for static muts being a dereference of a raw pointer, which means no lifetime checking.

Other way of describing this:

static mut X: Foo = ...;
X

is the same as

static X: RacyUnsafeCell<Foo> = ...;
X.get()

and since the latter allows getting 'static references, the former should too.

[RalfJung]

Again, we already must have special code for immutable thread-local statics handling this. The exact same logic should "just" also be applied to mutable thread-local statics. Or is there some reason that does not work?

[arielb1]

@RalfJung

All kinds of statics and thread locals work by

_0: Pointer<T> = addr_of_static()
use(_0)

For non-mut statics, Pointer<T> is &T - for thread-locals it has a lifetime that is bounded to the function, for statics it has lifetime 'static.
For mut statics, Pointer<T> can't be &mut T since that would have aliasing problems. So it is *mut T, which doesn't come with a lifetime attached.

So you basically end up with the RacyUnsafeCell "desugaring".

[arielb1]

The thing I like about static mut being defined to act like the syntactic sugar for RacyUnsafeCell + a lint that prevents you from taking &mut in the most tempting-unsafe way, is that it makes it 1 less thing for the language to care about.