Skip to content

a more reasonable explanation for atomic operation on static is unsafe #1349

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

Closed
wants to merge 1 commit into from

Conversation

zylthinking
Copy link

a more reasonable explanation for atomic operation on static is unsafe

a more reasonable explanation for atomic operation on static is unsafe
@steffahn
Copy link
Member

While you’re at it improving this example:

I think this example should probably also be re-written to use add_of_mut. There’s nothing “safe” about this function as it stands right now, under up-to-date Rust semantics.

Even something like this (that doesn’t even use the reference) is already considered UB by miri:

use std::thread;

static mut LEVELS: u32 = 0;

fn bump_levels_unsafe2() {
    let _ = unsafe { &mut LEVELS };
}

fn main() {
    thread::scope(|s| {
        s.spawn(bump_levels_unsafe2);
        s.spawn(bump_levels_unsafe2);
    });
}
error: Undefined Behavior: Data race detected between (1) non-atomic write on thread `<unnamed>` and (2) non-atomic write on thread `<unnamed>` at alloc1. (2) just happened here
 --> src/main.rs:6:22
  |
6 |     let _ = unsafe { &mut LEVELS };
  |                      ^^^^^^^^^^^ Data race detected between (1) non-atomic write on thread `<unnamed>` and (2) non-atomic write on thread `<unnamed>` at alloc1. (2) just happened here
  |
help: and (1) occurred earlier here
 --> src/main.rs:6:22
  |
6 |     let _ = unsafe { &mut LEVELS };
  |                      ^^^^^^^^^^^
  = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
  = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information
  = note: BACKTRACE (of the first span):
  = note: inside `bump_levels_unsafe2` at src/main.rs:6:22: 6:33

Also, Rustc rightfully warns and suggests usage of addr_of_mut

warning: mutable reference of mutable static is discouraged
 --> src/main.rs:6:22
  |
6 |     let _ = unsafe { &mut LEVELS };
  |                      ^^^^^^^^^^^ mutable reference of mutable static
  |
  = note: for more information, see issue #114447 <https://github.com/rust-lang/rust/issues/114447>
  = note: reference of mutable static is a hard error from 2024 edition
  = note: mutable statics can be written to by multiple threads: aliasing violations or data races will cause undefined behavior
  = note: `#[warn(static_mut_ref)]` on by default
help: mutable references are dangerous since if there's any other pointer or reference used for that static while the reference lives, that's UB; use `addr_of_mut!` instead to create a raw pointer
  |
6 |     let _ = unsafe { addr_of_mut!(LEVELS) };
  |                      ~~~~~~~~~~~~~~~~~~~~

And given that it’s a very low-level thing, this atomic_add function, not using any safe wrapper type (like AtomicU32) but operating on the u32 directly, it’s only natural that *mut u32 is the right argument type here (not &mut u32!)… so the call would then look like atomic_add(addr_of_mut!(LEVELS), 1);


If you want, you can also improve the style and refactor the usage of early-return statements (return EXPR;) at the end of the functions into ordinary final expression in a block for return value.

@ehuss
Copy link
Contributor

ehuss commented Jul 23, 2024

Closing in favor of #1544, thanks!

@ehuss ehuss closed this Jul 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants