Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Disallow *references* to static mut [Edition Idea] #114447

Closed
Tracked by #172
scottmcm opened this issue Aug 4, 2023 · 45 comments · Fixed by #117556
Closed
Tracked by #172

Disallow *references* to static mut [Edition Idea] #114447

scottmcm opened this issue Aug 4, 2023 · 45 comments · Fixed by #117556
Assignees
Labels
A-edition-2024 Area: The 2024 edition A-maybe-future-edition Something we may consider for a future edition. E-help-wanted Call for participation: Help is requested to fix this issue. E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@scottmcm
Copy link
Member

scottmcm commented Aug 4, 2023

Now that we have ptr::addr_of_mut!, the 2024 edition would be a great time to do @RalfJung's idea from #53639 (comment):

Disallow &MY_STATIC_MUT and &mut MY_STATIC_MUT entirely.

ptr::addr_of_mut!(MY_STATIC_MUT) could even be safe -- unlike taking a reference which is unsafe and often insta-UB -- and helps encourage avoiding races in the accesses, which is more practical than trying to avoid the UB from concurrent existence of references.

(Maybe people will end up just doing &mut *ptr::addr_of_mut!(MY_STATIC_MUT) without thinking through all the safety requirements -- in particular, making sure that the reference stops being used before a second reference is created -- but we can't force them to drink.)

@scottmcm scottmcm added T-lang Relevant to the language team, which will review and decide on the PR/issue. A-maybe-future-edition Something we may consider for a future edition. I-lang-nominated The issue / PR has been nominated for discussion during a lang team meeting. labels Aug 4, 2023
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Aug 4, 2023
@Nilstrieb Nilstrieb removed the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Aug 4, 2023
@scottmcm scottmcm added E-help-wanted Call for participation: Help is requested to fix this issue. A-edition-2024 Area: The 2024 edition labels Sep 5, 2023
@Lokathor
Copy link
Contributor

Lokathor commented Sep 5, 2023

I think that this would be much better if methods on a type could take *mut self, instead of only allowing &mut self, but that's not a hard blocker.

@scottmcm scottmcm added the E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. label Sep 5, 2023
@scottmcm
Copy link
Member Author

scottmcm commented Sep 5, 2023

We discussed this in the lang team meeting today, and the consensus was in favour of going in this direction.

(With https://doc.rust-lang.org/std/cell/struct.SyncUnsafeCell.html and https://doc.rust-lang.org/std/ptr/macro.addr_of_mut.html we have options that we didn't back in 2018.)

Steps needed, I think:

  • Add a lint that warns on using a & or &mut to a static mut, with some help suggesting other options.
  • Make doing so a hard error in 2024 (in addition to the lint that affects the other editions)

@obeis
Copy link
Contributor

obeis commented Sep 14, 2023

@rustbot claim

@traviscross
Copy link
Contributor

@rustbot labels -I-lang-nominated

I'm going to unnominate this, since there's nothing more to discuss until there is a PR to review. Please nominate such a PR as soon as it is posted.

(This is correctly labeled so we can track it with other ideas for the next edition without the nomination.)

@rustbot rustbot removed the I-lang-nominated The issue / PR has been nominated for discussion during a lang team meeting. label Oct 24, 2023
@scottmcm
Copy link
Member Author

scottmcm commented Nov 1, 2023

@obeis Any updates? Have you managed to make progress here?

@obeis
Copy link
Contributor

obeis commented Nov 1, 2023

@scottmcm almost done. I will submit my pull request within the next two days at the latest. I apologize for the delay.

@RalfJung
Copy link
Member

RalfJung commented Dec 2, 2023

While implementing the lint in #117556, the question came up whether various cases of implicitly taking a reference should be linted against:

static mut STATIC = &[0, 1, 2];

let _ = STATIC.len();
let _ = STATIC[0];
let _ = format!("{}", STATIC);

The three listed cases are (fairly) harmless. However, let val = z.get() might not be harmless if get returns a 'static reference.

@scottmcm (and t-lang in general), what is your opinion on that?

@est31
Copy link
Member

est31 commented Dec 2, 2023

Personally I lean towards deprecating these cases, too, if the idea is that we want to mostly deprecate static mut anyways. As pointed out above, dot syntax can be used to create references without involving any &, so the creation of the potentially dangerous reference is quite hidden. By extension [] syntax and even field access syntax is in similar positions as well, when the type that they give access to is a static reference: but there you don't create a new reference but only access an existing one (example: static mut ST = (&[&0, &1, &2], &3); let _ = ST.0[0]; let _ = ST.1;).

@scottmcm
Copy link
Member Author

scottmcm commented Dec 4, 2023

I think this should be linting about anything that's created enough of a reference to trigger the reference rules -- if it can trigger UB when someone else holds a &mut to the static, that's worth linting about here.

@ravenexp
Copy link

ravenexp commented Dec 5, 2023

Are Vec::leak() and the like still OK under the new rules?

@Lokathor
Copy link
Contributor

Lokathor commented Dec 5, 2023

This discussion is only about static mut variables (which can easily cause UB), not &mut T references with a 'static lifetime (which are 100% and UB free).

So Vec::leak and Box::leak and similar are unaffected.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Jan 3, 2024
…twco

Disallow reference to `static mut` and adding `static_mut_ref` lint

Closes rust-lang#114447

r? `@scottmcm`
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Jan 5, 2024
…twco

Disallow reference to `static mut` and adding `static_mut_ref` lint

Closes rust-lang#114447

r? `@scottmcm`
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Jan 5, 2024
…twco

Disallow reference to `static mut` and adding `static_mut_ref` lint

Closes rust-lang#114447

r? ``@scottmcm``
bors added a commit to rust-lang-ci/rust that referenced this issue Jan 7, 2024
Disallow reference to `static mut` and adding `static_mut_ref` lint

Closes rust-lang#114447

r? `@scottmcm`
bors added a commit to rust-lang-ci/rust that referenced this issue Jan 8, 2024
Disallow reference to `static mut` and adding `static_mut_ref` lint

Closes rust-lang#114447

r? `@scottmcm`
Yunlongs added a commit to Yunlongs/Hopper that referenced this issue Apr 25, 2024
I found there were clippy warnings introduced in recent rust versions.

I fixed the following three types of warnings:
1. creating a mutable reference to mutable static is discouraged
rust-lang/rust#114447
2. initializer for `thread_local` value can be made `const`
 https://rust-lang.github.io/rust-clippy/master/index.html#thread_local_initializer_can_be_made_const
 3. unused imported libraries
@WorldSEnder
Copy link

WorldSEnder commented May 10, 2024

I have found a use-case of static mut in one of my crates that isn't directly covered by the migration advice and may be obscure so for posteriority I want to document it here, since this issue is linked by the compiler.

static mut has the non-immediately obvious behaviour of allowing the specified type to be non-Sync even though it is shared between threads. This is clearly dangerous but can be used in a correct program. One example is returning a reference to an empty vector. That is

impl Foo {
  fn get(&self) -> &Vec<Bar> {
    // Note: mut to avoid a Sync bound that is required by non-mut statics
    static mut FOO: Vec<Bar> = Vec::new();
    // SAFETY: only take mutable references to the shared empty vector.
    unsafe { &FOO }
  }
}

was used where Bar and thus Vec<Bar> was not Sync. This is perfectly fine as far as I can see: no Bar is ever exposed to the user and the reference is only immutable. I do not expect to be able to rewrite this without unsafe. My reworked implementation is a wrapper struct that contains the expected Sync behaviour but requires no mut:

impl Foo {
  fn get(&self) -> &Vec<Bar> {
    struct EmptyBars(Vec<Bar>); // Bit of a sad type, who likes an empty bar? :/
    // SAFETY: This struct only ever contains an empty vec, hence exposes no Bar to un-Sync users
    unsafe impl Sync for EmptyBars {}
    static FOO: EmptyBars = EmptyBars(Vec::new());
    &FOO.0
  }
}

Ah someone else has found this out too :)

crawford added a commit to crawford/PoE that referenced this issue May 12, 2024
This was spawned by the following error message, introduced in 1.77.0:

    warning: creating a shared reference to mutable static is discouraged
      --> src/log/mod.rs:36:27
       |
    36 |     let logger = unsafe { &LOGGER };
       |                           ^^^^^^^ shared reference to mutable static
       |
       = note: for more information, see issue #114447 <rust-lang/rust#114447>

This changes the implementation slightly so that it no longer returns
a reference to the logger itself; instead returning a marker type that
is called in the same way as the original. It also removes the use of
`cortex_m::singleton!`, since the singleton value wasn't actually
being used.

Before this implementation, I tried removing the builder pattern
entirely and just taking the ITM and RTT loggers in the init function.
That ended up being fine, but it prevented any logging (RTT or
otherwise) before the GPIOs were configured.
benma added a commit to benma/bitbox02-firmware that referenced this issue May 13, 2024
This reduces the fw binary size by 4176 bytes as compared to the
v9.18.0 release.

Also bump LLVM 18 - it is unrelated but I did it anyway as the Rust
release notes also mentioned that they moved to LLVM 18.

The workflow.rs change is due to this deprecation warning:

```
warning: creating a shared reference to mutable static is discouraged
   --> bitbox02-rust-c/src/workflow.rs:103:11
    |
103 |     match &CONFIRM_STATE {
    |           ^^^^^^^^^^^^^^ shared reference to mutable static
    |
    = note: for more information, see issue #114447 <rust-lang/rust#114447>
    = note: this will be a hard error in the 2024 edition
    = note: this shared reference has lifetime `'static`, but if the static ever gets mutated, or a mutable reference is created, then any further use of this shared reference is Undefined Behavior
    = note: `#[warn(static_mut_refs)]` on by default
help: use `addr_of!` instead to create a raw pointer
    |
103 |     match addr_of!(CONFIRM_STATE) {
```
benma added a commit to benma/bitbox02-firmware that referenced this issue May 13, 2024
This reduces the fw binary size by 4176 bytes as compared to the
v9.18.0 release.

Also bump LLVM 18 - it is unrelated but I did it anyway as the Rust
release notes also mentioned that they moved to LLVM 18.

The workflow.rs change is due to this deprecation warning:

```
warning: creating a shared reference to mutable static is discouraged
   --> bitbox02-rust-c/src/workflow.rs:103:11
    |
103 |     match &CONFIRM_STATE {
    |           ^^^^^^^^^^^^^^ shared reference to mutable static
    |
    = note: for more information, see issue #114447 <rust-lang/rust#114447>
    = note: this will be a hard error in the 2024 edition
    = note: this shared reference has lifetime `'static`, but if the static ever gets mutated, or a mutable reference is created, then any further use of this shared reference is Undefined Behavior
    = note: `#[warn(static_mut_refs)]` on by default
help: use `addr_of!` instead to create a raw pointer
    |
103 |     match addr_of!(CONFIRM_STATE) {
```

The C code changes are clang-format changes.
benma added a commit to benma/bitbox02-firmware that referenced this issue May 13, 2024
This reduces the fw binary size by 4176 bytes as compared to the
v9.18.0 release.

Also bump LLVM 18 - it is unrelated but I did it anyway as the Rust
release notes also mentioned that they moved to LLVM 18.

The workflow.rs change is due to this deprecation warning:

```
warning: creating a shared reference to mutable static is discouraged
   --> bitbox02-rust-c/src/workflow.rs:103:11
    |
103 |     match &CONFIRM_STATE {
    |           ^^^^^^^^^^^^^^ shared reference to mutable static
    |
    = note: for more information, see issue #114447 <rust-lang/rust#114447>
    = note: this will be a hard error in the 2024 edition
    = note: this shared reference has lifetime `'static`, but if the static ever gets mutated, or a mutable reference is created, then any further use of this shared reference is Undefined Behavior
    = note: `#[warn(static_mut_refs)]` on by default
help: use `addr_of!` instead to create a raw pointer
    |
103 |     match addr_of!(CONFIRM_STATE) {
```

The C code changes are clang-format changes.
benma added a commit to benma/bitbox02-firmware that referenced this issue May 13, 2024
This reduces the fw binary size by 4176 bytes as compared to the
v9.18.0 release.

Also bump LLVM 18 - it is unrelated but I did it anyway as the Rust
release notes also mentioned that they moved to LLVM 18.

The workflow.rs change is due to this deprecation warning:

```
warning: creating a shared reference to mutable static is discouraged
   --> bitbox02-rust-c/src/workflow.rs:103:11
    |
103 |     match &CONFIRM_STATE {
    |           ^^^^^^^^^^^^^^ shared reference to mutable static
    |
    = note: for more information, see issue #114447 <rust-lang/rust#114447>
    = note: this will be a hard error in the 2024 edition
    = note: this shared reference has lifetime `'static`, but if the static ever gets mutated, or a mutable reference is created, then any further use of this shared reference is Undefined Behavior
    = note: `#[warn(static_mut_refs)]` on by default
help: use `addr_of!` instead to create a raw pointer
    |
103 |     match addr_of!(CONFIRM_STATE) {
```

The C code changes are clang-format changes.
benma added a commit to benma/bitbox02-firmware that referenced this issue May 13, 2024
This reduces the fw binary size by 4176 bytes as compared to the
v9.18.0 release.

Also bump LLVM 18 - it is unrelated but I did it anyway as the Rust
release notes also mentioned that they moved to LLVM 18.

The workflow.rs change is due to this deprecation warning:

```
warning: creating a shared reference to mutable static is discouraged
   --> bitbox02-rust-c/src/workflow.rs:103:11
    |
103 |     match &CONFIRM_STATE {
    |           ^^^^^^^^^^^^^^ shared reference to mutable static
    |
    = note: for more information, see issue #114447 <rust-lang/rust#114447>
    = note: this will be a hard error in the 2024 edition
    = note: this shared reference has lifetime `'static`, but if the static ever gets mutated, or a mutable reference is created, then any further use of this shared reference is Undefined Behavior
    = note: `#[warn(static_mut_refs)]` on by default
help: use `addr_of!` instead to create a raw pointer
    |
103 |     match addr_of!(CONFIRM_STATE) {
```

The C code changes are clang-format changes. Clang-tidy
`-readability-avoid-unconditional-preprocessor-if` was added because
of the `#if 0` in pukcc.c, which seems worth preserving for now.
@uazu
Copy link

uazu commented May 28, 2024

Please don't deprecate static mut completely. If you have a single blessed "main" thread then globals are a useful and valid optimization when wrapped safely. They are fundamental and efficient, and were there since the very first linkers. They were not bolted on later like thread-locals whose implementation or cost is very much less predictable.

@LunarLambda
Copy link

Going forward the way to use static mut (if you must use them) will be using the addr_of/addr_of_mut macros. If your use case is sound for reasons Rust cannot check then turning that pointer back to a reference is trivial. This RFC is explicitly not about deprecating static mut itself.

Though if you just need a "global" that's only used on a single thread there are still less "problematic" ways to do so, like using Box::leak to get a &'static mut T or Rc<RefCell> or similar.

@uazu
Copy link

uazu commented May 29, 2024

Going forward the way to use static mut (if you must use them) will be using the addr_of/addr_of_mut macros. If your use case is sound for reasons Rust cannot check then turning that pointer back to a reference is trivial. This RFC is explicitly not about deprecating static mut itself.

Though if you just need a "global" that's only used on a single thread there are still less "problematic" ways to do so, like using Box::leak to get a &'static mut T or Rc or similar.

@est31 above mentioned the intention to deprecate static mut, so I thought I should speak up now before it was too late. It doesn't bother me that Rust is adding some extra hoops to jump through.

The alternatives you suggested don't help when there is no way to pass the reference down the stack to where it will be needed. Also even if there is a way to pass the reference around, that's another usize being passed everywhere. A global's address is compiled into the code, so is effectively zero-sized on the stack or in the data. For my code in Stakker, it is an optional internal optimization that saves a usize in loads of places and saves an indirection on actor calls, which are very frequent. I see no advantage in giving up that optimization.

Really we're all having to deal with bad decisions made when multi-threading was added to C. That is why there is all this mess. Single-thread per process is still a valid model, as is main-thread plus worker-threads. static mut still has uses for those cases. It seems pointless to throw away efficiency in those cases just because symmetric multi-threaded runtimes are more common right now.

I wonder whether I'm the only person who cares about this. Maybe people assume that LLVM will take care of it, but it can't if things are fundamentally structured in a way that it can't optimise. There is a lot of talk of effects systems in Rust. Globals or thread-locals are a way to implement that, i.e. the need to get to a reference from deep down in the stack after passing through code which doesn't know anything about the "effect". So there still are valid uses.

Maybe if someone wants to come up with a more restrictive formulation of static mut that also includes some kinds of checks that would also work, so long as it is zero-cost after some kind of low-cost thread-ID check, e.g. on creating a zero-sized non-Send wrapper object. Maybe there should be a way to build for a single-thread process that disables the checks and thread-ID overhead, and also disables creating threads. That would hopefully cover all valid existing uses of static mut. Then you can deprecate static mut.

@LunarLambda
Copy link

The alternatives you suggested don't help when there is no way to pass the reference down the stack to where it will be needed. Also even if there is a way to pass the reference around, that's another usize being passed everywhere.

Both of these things are true regardless of whether you are allowed to take reference to a static mut directly. addr_of! compiles to the exact same code as taking a reference does.

Thread-ID checks and similar mechanisms can be built today using stable APIs on top of UnsafeCell, and don't need static mut.

@scottmcm
Copy link
Member Author

They are fundamental and efficient, and were there since the very first linkers.

Note that I don't find this particularly persuasive, since as far as the linker sees, static mut FOO: i32 and static FOO: SyncUnsafeCell<i32> are exactly the same. They're just as efficient.

Is there something you're seeing that actually needs static mut specifically, as opposed to static-with-UnsafeCell-internally?

Single-thread per process is still a valid model

Note that even if you only have one thread (and assume no interrupts) that's not enough on its own to make static mut sound, because you can read from it at multiple scopes and thus end up with multiple independent &muts to it, which is UB.

I would suggest people look at making some kind of safe library type to help this scenario. Perhaps comment in this zulip thread where I was trying to brainstorm something: https://rust-lang.zulipchat.com/#narrow/stream/327149-t-libs-api.2Fapi-changes/topic/static.20.60RefCell.60.20or.20single-threaded.20.60Mutex.60.3F/near/439487333.

above mentioned the intention to deprecate static mut, so I thought I should speak up now before it was too late.

If that's your concern you should comment in #53639. This issue is specifically about not completely deprecating it.

@uazu
Copy link

uazu commented May 29, 2024

Note that I don't find this particularly persuasive, since as far as the linker sees, static mut FOO: i32 and static FOO: SyncUnsafeCell<i32> are exactly the same. They're just as efficient.

Is there something you're seeing that actually needs static mut specifically, as opposed to static-with-UnsafeCell-internally?

Okay, I thought you were thinking of getting rid of mutable statics entirely. The 1.78 compiler warning which brought me to this page only mentioned addr_of_mut! and didn't say anything about static with UnsafeCell. So that is what we're supposed to use now instead of static mut? I really don't mind. I will switch if that is now the approved way. (Since you can get a *mut either way does it make any difference to safety? The coder still needs to know what they're doing.)

@RalfJung
Copy link
Member

RalfJung commented May 29, 2024

(Since you can get a *mut either way does it make any difference to safety? The coder still needs to know what they're doing.)

That's exactly why we are only deprecating references to static mut for now and encourage moving to raw pointers instead. Some people are arguing we should ban static mut entirely but so far those people have not managed to convince the team(s) that that step would be worth it.

My personal opinion is that switching from static mut with only raw pointers to static SyncUnsafeCell mostly feels like needless extra paperwork. So as far as I am concerned, if you're using static mut and raw pointers throughout, you're good. The moment you create references from those raw pointers you need to be extremely careful, and you should make them as short-lived as possible.

@scottmcm
Copy link
Member Author

Well, the simplest transition is to https://doc.rust-lang.org/std/cell/struct.SyncUnsafeCell.html, but unfortunately that's not yet stable :( You can always also make your own types with appropriate unsafe impls that use UnsafeCell internally -- that's all SyncUnsafeCell is.

Since you can get a *mut either way does it make any difference to safety?

The differences are in whether you get a reference directly. *muts are allowed to alias, so you can make new ones all day long and keep them around -- the only problems are when you actually read or write through them. Whereas multiple independent live &muts at the same time is UB even without a data race. So that's the difference in footgun-ness.

@uazu
Copy link

uazu commented May 30, 2024

This is clearer now, thanks. I can see the justification for some kind of static ... UnsafeCell approach instead of static mut because it does express the problems with mutable globals better in Rust terms, and the coder might be persuaded to read the UnsafeCell docs. It's not any safer for someone who is just copy/pasting incantations, though.

However, I couldn't use SyncUnsafeCell because I'm storing non-Sync types protected by a thread-ID check. So I needed to do what @eddyb explains at the top of #53639, i.e. create my own wrapper type that has an unsafe impl Sync and then justify the safety in comments. So telling everyone to use SyncUnsafeCell isn't going to work, unless the docs for SyncUnsafeCell explain that it doesn't cover all cases.

I don't think my new code is any better or worse without static mut, because I was already careful. It has 40% more LOC and has two more unsafe keywords. There's not much in it.

@yanchith
Copy link
Contributor

yanchith commented May 31, 2024

My personal opinion is that switching from static mut with only raw pointers to static SyncUnsafeCell mostly feels like needless extra paperwork.

I feel this way too, and am glad that you do, too!

Still, if there's some busywork to discourage misuse, I can live with that, just please let me have (the equivalent of) my:

let p = ptr::addr_of_mut!(STATIC_MUT);

There's programs I can't express without. For instance, we have a low-overhead profiler in our company that is just two rdtscs, and a couple of adds, loads and stores per profiling zone. The profiler is very careful to not create references, and it can only be ran in isolated single-threaded scenarios when we are tuning algorithms.

ac000 added a commit to nginx/unit-wasm that referenced this issue Jul 2, 2024
With at least

  rustc 1.79.0 (129f3b996 2024-06-10) (Fedora 1.79.0-3.fc40)

We were getting warnings when building the rust examples like

warning: creating a mutable reference to mutable static is discouraged
  --> src/lib.rs:75:40
   |
75 |     let ctx: *mut luw_ctx_t = unsafe { &mut CTX };
   |                                        ^^^^^^^^ mutable reference to mutable static
   |
   = note: for more information, see issue #114447 <rust-lang/rust#114447>
   = note: this will be a hard error in the 2024 edition
   = note: this mutable reference has lifetime `'static`, but if the static gets accessed (read or written) by any other means, or any other reference is created, then any further use of this mutable reference is Undefined Behavior
   = note: `#[warn(static_mut_refs)]` on by default
help: use `addr_of_mut!` instead to create a raw pointer
   |
75 |     let ctx: *mut luw_ctx_t = unsafe { addr_of_mut!(CTX) };
   |                                        ~~~~~~~~~~~~~~~~~

So do like it says and use the addr_of_mut!() macro.

Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
@ronhombre

This comment was marked as off-topic.

@ChayimFriedman2

This comment was marked as off-topic.

@ronhombre

This comment was marked as off-topic.

@workingjubilee
Copy link
Contributor

As a reminder, the Rust issue tracker is for filing bugs or issues with the language, it is not for use as a helpdesk. This includes creating code of dubious quality via passing a random number generator through a filter (or vector of weights) and asking us to not lint on it: When lints trigger correctly, they are supposed to trigger on code of dubious quality, to discourage reusing it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-edition-2024 Area: The 2024 edition A-maybe-future-edition Something we may consider for a future edition. E-help-wanted Call for participation: Help is requested to fix this issue. E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.