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

addr_of! a static mut should not require unsafe #125833

Closed
gfaster opened this issue May 31, 2024 · 11 comments · Fixed by #125834
Closed

addr_of! a static mut should not require unsafe #125833

gfaster opened this issue May 31, 2024 · 11 comments · Fixed by #125834
Labels
C-bug Category: This is a bug.

Comments

@gfaster
Copy link

gfaster commented May 31, 2024

I tried this code (playground):

static mut FLAG: bool = false;
fn main() {
    let p = std::ptr::addr_of!(FLAG);
    println!("{p:p}")
}

I expected this to compile without adding an unsafe block, since the act of getting the address of the static on its own cannot(?) cause Undefined Behavior. Making addr_of! and addr_of_mut! safe when used on a static mut would make the behavior consistent with UnsafeCell::get, which has identical safety concerns in a multithreaded environment.

Meta

Exists on stable and the nightly version used by playground (2024-05-30).

@gfaster gfaster added the C-bug Category: This is a bug. label May 31, 2024
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label May 31, 2024
@workingjubilee
Copy link
Member

Making addr_of! and addr_of_mut! safe when used on a static mut would make the behavior consistent with UnsafeCell::get, which has identical safety concerns in a multithreaded environment.

It has been in general our expressed preference over time that you use static FLAG: UnsafeCell<bool> = false; (or perhaps static FLAG: AtomicBool = AtomicBool::new(false);, which is also an UnsafeCell) as static mut is quite counterintuitive in many different ways.

@workingjubilee
Copy link
Member

see #123758

@workingjubilee
Copy link
Member

workingjubilee commented May 31, 2024

Hmm, it seems we don't take into account this around here:

ExprKind::Deref { arg } => {
if let ExprKind::StaticRef { def_id, .. } | ExprKind::ThreadLocalRef(def_id) =
self.thir[arg].kind
{
if self.tcx.is_mutable_static(def_id) {
self.requires_unsafe(expr.span, UseOfMutableStatic);
} else if self.tcx.is_foreign_item(def_id) {
self.requires_unsafe(expr.span, UseOfExternStatic);
}
} else if self.thir[arg].ty.is_unsafe_ptr() {
self.requires_unsafe(expr.span, DerefOfRawPointer);
}
}

Simple fix. I think?

@gfaster
Copy link
Author

gfaster commented May 31, 2024

see #123758

It seems like #114447 even floats this idea exactly.

It has been in general our expressed preference over time that you use static FLAG: UnsafeCell<bool> = false;

Not being able to use UnsafeCell directly in statics isn't so nice since it isn't Sync, but there's #95439 for that. That being said, maybe I ought to write a Clippy lint for some of those alternatives, particularly for flag booleans.

I should clarify that I just ran into this when writing some code for a demonstration, so direct replacements aren't exactly applicable. Regardless, I believe the inconsistency stands.

@workingjubilee
Copy link
Member

Hmm, looking at things more closely, I don't see why it's a Deref. That should be an ExprKind::AddressOf...?

@gfaster
Copy link
Author

gfaster commented May 31, 2024

#95439 (comment) claims that arithmetic on static mut is unsafe. I'm not sure why that would be - can addr_of! violate LLVM's noalias?

Maybe something like this causes problems?

@workingjubilee
Copy link
Member

I think that might be just descriptive (of the problem being discussed here).

@workingjubilee
Copy link
Member

well, that was an unexpected THIR desugaring.

Expr {
    ty: *const bool
    temp_lifetime: Some(Node(1))
    span: /home/jubilee/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ptr/mod.rs:2203:5: 2203:22 (#4)
    kind: 
    Scope {
        region_scope: Node(3)
        lint_level: Explicit(HirId(DefId(0:4 ~ static_mut[60a8]::main).3))
        value:
            Expr {
                ty: *const bool
                temp_lifetime: Some(Node(1))
                span: /home/jubilee/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ptr/mod.rs:2203:5: 2203:22 (#4)
                kind: 
                    AddressOf {
                        mutability: Not
                        arg:
                            Expr {
                                ty: bool
                                temp_lifetime: Some(Remainder { block: 68, first_statement_index: 0})
                                span: static_mut.rs:3:41: 3:45 (#0)
                                kind: 
                                    Scope {
                                        region_scope: Node(4)
                                        lint_level: Explicit(HirId(DefId(0:4 ~ static_mut[60a8]::main).4))
                                        value:
                                            Expr {
                                                ty: bool
                                                temp_lifetime: Some(Remainder { block: 68, first_statement_index: 0})
                                                span: static_mut.rs:3:41: 3:45 (#0)
                                                kind: 
                                                    Deref {
                                                        Expr {
                                                            ty: *mut bool
                                                            temp_lifetime: Some(Remainder { block: 68, first_statement_index: 0})
                                                            span: static_mut.rs:3:41: 3:45 (#0)
                                                            kind: 
                                                                StaticRef {
                                                                    def_id: DefId(0:3 ~ static_mut[60a8]::FLAG)
                                                                    ty: *mut bool
                                                                    alloc_id: alloc1
                                                                }
                                                        }
                                                    }
                                            }
                                    }
                            }
                    }
            }
        }
    }

@workingjubilee
Copy link
Member

Oh! Oh, I see, the way that this is defined is that naming a static generates an *mut Static and then derefs it, creating the appropriate place expression.

@traviscross traviscross added T-lang Relevant to the language team, which will review and decide on the PR/issue. and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Jun 1, 2024
@workingjubilee
Copy link
Member

PR up at #125834

@traviscross traviscross removed the T-lang Relevant to the language team, which will review and decide on the PR/issue. label Jun 1, 2024
@workingjubilee
Copy link
Member

I wanted to say that the compiler should fix it by choosing a different lowering for naming STATIC_MUT, but that doesn't seem like anything reasonable to implement Soon, so I implemented my original thought of complicating the unsafety check.

tgross35 added a commit to tgross35/rust that referenced this issue Jul 23, 2024
…k-for-addr-of-static-mut, r=compiler-errors

treat `&raw (const|mut) UNSAFE_STATIC` implied deref as safe

Fixes rust-lang#125833

As reported in that and related issues, `static mut STATIC_MUT: T` is very often used in embedded code, and is in many ways equivalent to `static STATIC_CELL: SyncUnsafeCell<T>`. The Rust expression of `&raw mut STATIC_MUT` and `SyncUnsafeCell::get(&STATIC_CELL)` are approximately equal, and both evaluate to `*mut T`. The library function is safe because it has *declared itself* to be safe. However, the raw ref operator is unsafe because all uses of `static mut` are considered unsafe, even though the static's value is not used by this expression (unlike, for example, `&STATIC_MUT`).

We can fix this unnatural difference by simply adding the proper exclusion for the safety check inside the THIR unsafeck, so that we do not declare it unsafe if it is not.

While the primary concern here is `static mut`, this change is made for all instances of an "unsafe static", which includes a static declared inside `extern "abi" {}`. Hypothetically, we could go as far as generalizing this to all instances of `&raw (const|mut) *ptr`, but today we do not, as we have not actually considered the range of possible expressions that use a similar encoding. We do not even extend this to thread-local equivalents, because they have less clear semantics.
@bors bors closed this as completed in 1b4b0e9 Jul 23, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Jul 23, 2024
Rollup merge of rust-lang#125834 - workingjubilee:weaken-thir-unsafeck-for-addr-of-static-mut, r=compiler-errors

treat `&raw (const|mut) UNSAFE_STATIC` implied deref as safe

Fixes rust-lang#125833

As reported in that and related issues, `static mut STATIC_MUT: T` is very often used in embedded code, and is in many ways equivalent to `static STATIC_CELL: SyncUnsafeCell<T>`. The Rust expression of `&raw mut STATIC_MUT` and `SyncUnsafeCell::get(&STATIC_CELL)` are approximately equal, and both evaluate to `*mut T`. The library function is safe because it has *declared itself* to be safe. However, the raw ref operator is unsafe because all uses of `static mut` are considered unsafe, even though the static's value is not used by this expression (unlike, for example, `&STATIC_MUT`).

We can fix this unnatural difference by simply adding the proper exclusion for the safety check inside the THIR unsafeck, so that we do not declare it unsafe if it is not.

While the primary concern here is `static mut`, this change is made for all instances of an "unsafe static", which includes a static declared inside `extern "abi" {}`. Hypothetically, we could go as far as generalizing this to all instances of `&raw (const|mut) *ptr`, but today we do not, as we have not actually considered the range of possible expressions that use a similar encoding. We do not even extend this to thread-local equivalents, because they have less clear semantics.
github-actions bot pushed a commit to rust-lang/miri that referenced this issue Jul 24, 2024
…r-of-static-mut, r=compiler-errors

treat `&raw (const|mut) UNSAFE_STATIC` implied deref as safe

Fixes rust-lang/rust#125833

As reported in that and related issues, `static mut STATIC_MUT: T` is very often used in embedded code, and is in many ways equivalent to `static STATIC_CELL: SyncUnsafeCell<T>`. The Rust expression of `&raw mut STATIC_MUT` and `SyncUnsafeCell::get(&STATIC_CELL)` are approximately equal, and both evaluate to `*mut T`. The library function is safe because it has *declared itself* to be safe. However, the raw ref operator is unsafe because all uses of `static mut` are considered unsafe, even though the static's value is not used by this expression (unlike, for example, `&STATIC_MUT`).

We can fix this unnatural difference by simply adding the proper exclusion for the safety check inside the THIR unsafeck, so that we do not declare it unsafe if it is not.

While the primary concern here is `static mut`, this change is made for all instances of an "unsafe static", which includes a static declared inside `extern "abi" {}`. Hypothetically, we could go as far as generalizing this to all instances of `&raw (const|mut) *ptr`, but today we do not, as we have not actually considered the range of possible expressions that use a similar encoding. We do not even extend this to thread-local equivalents, because they have less clear semantics.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants