From 3fdd8d5ef3a8b26c3e24d9baf706fc7fb16717f9 Mon Sep 17 00:00:00 2001 From: Jubilee Young Date: Fri, 31 May 2024 18:48:42 -0700 Subject: [PATCH] compiler: treat `&raw (const|mut) UNSAFE_STATIC` implied deref as safe The implied deref to statics introduced by HIR->THIR lowering is only used to create place expressions, it lacks unsafe semantics. It is also confusing, as there is no visible `*ident` in the source. For both classes of "unsafe static" (extern static and static mut) allow this operation. We lack a clear story around `thread_local! { static mut }`, which is actually its own category of item that reuses the static syntax but has its own rules. It's possible they should be similarly included, but in the absence of a good reason one way or another, we do not bless it. --- .../rustc_mir_build/src/check_unsafety.rs | 18 +++++++++++++ compiler/rustc_mir_build/src/thir/cx/expr.rs | 6 +++-- tests/ui/consts/const_refs_to_static.rs | 2 +- tests/ui/consts/mut-ptr-to-static.rs | 9 +++---- tests/ui/static/raw-ref-deref-with-unsafe.rs | 16 +++++++++++ .../ui/static/raw-ref-deref-without-unsafe.rs | 18 +++++++++++++ .../raw-ref-deref-without-unsafe.stderr | 19 +++++++++++++ tests/ui/static/raw-ref-extern-static.rs | 27 +++++++++++++++++++ tests/ui/static/raw-ref-static-mut.rs | 17 ++++++++++++ 9 files changed, 123 insertions(+), 9 deletions(-) create mode 100644 tests/ui/static/raw-ref-deref-with-unsafe.rs create mode 100644 tests/ui/static/raw-ref-deref-without-unsafe.rs create mode 100644 tests/ui/static/raw-ref-deref-without-unsafe.stderr create mode 100644 tests/ui/static/raw-ref-extern-static.rs create mode 100644 tests/ui/static/raw-ref-static-mut.rs diff --git a/compiler/rustc_mir_build/src/check_unsafety.rs b/compiler/rustc_mir_build/src/check_unsafety.rs index 24098282d93..b4a69d7d7b7 100644 --- a/compiler/rustc_mir_build/src/check_unsafety.rs +++ b/compiler/rustc_mir_build/src/check_unsafety.rs @@ -449,6 +449,24 @@ impl<'a, 'tcx> Visitor<'a, 'tcx> for UnsafetyVisitor<'a, 'tcx> { } } } + ExprKind::AddressOf { arg, .. } => { + if let ExprKind::Scope { value: arg, .. } = self.thir[arg].kind + // THIR desugars UNSAFE_STATIC into *UNSAFE_STATIC_REF, where + // UNSAFE_STATIC_REF holds the addr of the UNSAFE_STATIC, so: take two steps + && let ExprKind::Deref { arg } = self.thir[arg].kind + // FIXME(workingjubiee): we lack a clear reason to reject ThreadLocalRef here, + // but we also have no conclusive reason to allow it either! + && let ExprKind::StaticRef { .. } = self.thir[arg].kind + { + // A raw ref to a place expr, even an "unsafe static", is okay! + // We short-circuit to not recursively traverse this expression. + return; + // note: const_mut_refs enables this code, and it currently remains unsafe: + // static mut BYTE: u8 = 0; + // static mut BYTE_PTR: *mut u8 = unsafe { addr_of_mut!(BYTE) }; + // static mut DEREF_BYTE_PTR: *mut u8 = unsafe { addr_of_mut!(*BYTE_PTR) }; + } + } ExprKind::Deref { arg } => { if let ExprKind::StaticRef { def_id, .. } | ExprKind::ThreadLocalRef(def_id) = self.thir[arg].kind diff --git a/compiler/rustc_mir_build/src/thir/cx/expr.rs b/compiler/rustc_mir_build/src/thir/cx/expr.rs index bd66257e6b6..0ba31f820b5 100644 --- a/compiler/rustc_mir_build/src/thir/cx/expr.rs +++ b/compiler/rustc_mir_build/src/thir/cx/expr.rs @@ -939,9 +939,11 @@ impl<'tcx> Cx<'tcx> { } } - // We encode uses of statics as a `*&STATIC` where the `&STATIC` part is - // a constant reference (or constant raw pointer for `static mut`) in MIR + // A source Rust `path::to::STATIC` is a place expr like *&ident is. + // In THIR, we make them exactly equivalent by inserting the implied *& or *&raw, + // but distinguish between &STATIC and &THREAD_LOCAL as they have different semantics Res::Def(DefKind::Static { .. }, id) => { + // this is &raw for extern static or static mut, and & for other statics let ty = self.tcx.static_ptr_ty(id); let temp_lifetime = self .rvalue_scopes diff --git a/tests/ui/consts/const_refs_to_static.rs b/tests/ui/consts/const_refs_to_static.rs index 1baa8535b2c..f41725b786e 100644 --- a/tests/ui/consts/const_refs_to_static.rs +++ b/tests/ui/consts/const_refs_to_static.rs @@ -9,7 +9,7 @@ const C1: &i32 = &S; const C1_READ: () = { assert!(*C1 == 0); }; -const C2: *const i32 = unsafe { std::ptr::addr_of!(S_MUT) }; +const C2: *const i32 = std::ptr::addr_of!(S_MUT); fn main() { assert_eq!(*C1, 0); diff --git a/tests/ui/consts/mut-ptr-to-static.rs b/tests/ui/consts/mut-ptr-to-static.rs index d8a788bb37d..e921365c7d4 100644 --- a/tests/ui/consts/mut-ptr-to-static.rs +++ b/tests/ui/consts/mut-ptr-to-static.rs @@ -16,12 +16,9 @@ static mut STATIC: u32 = 42; static INTERIOR_MUTABLE_STATIC: SyncUnsafeCell = SyncUnsafeCell::new(42); // A static that mutably points to STATIC. -static PTR: SyncPtr = SyncPtr { - foo: unsafe { ptr::addr_of_mut!(STATIC) }, -}; -static INTERIOR_MUTABLE_PTR: SyncPtr = SyncPtr { - foo: ptr::addr_of!(INTERIOR_MUTABLE_STATIC) as *mut u32, -}; +static PTR: SyncPtr = SyncPtr { foo: ptr::addr_of_mut!(STATIC) }; +static INTERIOR_MUTABLE_PTR: SyncPtr = + SyncPtr { foo: ptr::addr_of!(INTERIOR_MUTABLE_STATIC) as *mut u32 }; fn main() { let ptr = PTR.foo; diff --git a/tests/ui/static/raw-ref-deref-with-unsafe.rs b/tests/ui/static/raw-ref-deref-with-unsafe.rs new file mode 100644 index 00000000000..4b8f72de5b7 --- /dev/null +++ b/tests/ui/static/raw-ref-deref-with-unsafe.rs @@ -0,0 +1,16 @@ +//@ check-pass +#![feature(const_mut_refs)] +use std::ptr; + +// This code should remain unsafe because of the two unsafe operations here, +// even if in a hypothetical future we deem all &raw (const|mut) *ptr exprs safe. + +static mut BYTE: u8 = 0; +static mut BYTE_PTR: *mut u8 = ptr::addr_of_mut!(BYTE); +// An unsafe static's ident is a place expression in its own right, so despite the above being safe +// (it's fine to create raw refs to places!) the following derefs the ptr before creating its ref +static mut DEREF_BYTE_PTR: *mut u8 = unsafe { ptr::addr_of_mut!(*BYTE_PTR) }; + +fn main() { + let _ = unsafe { DEREF_BYTE_PTR }; +} diff --git a/tests/ui/static/raw-ref-deref-without-unsafe.rs b/tests/ui/static/raw-ref-deref-without-unsafe.rs new file mode 100644 index 00000000000..f9bce4368c1 --- /dev/null +++ b/tests/ui/static/raw-ref-deref-without-unsafe.rs @@ -0,0 +1,18 @@ +#![feature(const_mut_refs)] + +use std::ptr; + +// This code should remain unsafe because of the two unsafe operations here, +// even if in a hypothetical future we deem all &raw (const|mut) *ptr exprs safe. + +static mut BYTE: u8 = 0; +static mut BYTE_PTR: *mut u8 = ptr::addr_of_mut!(BYTE); +// An unsafe static's ident is a place expression in its own right, so despite the above being safe +// (it's fine to create raw refs to places!) the following derefs the ptr before creating its ref! +static mut DEREF_BYTE_PTR: *mut u8 = ptr::addr_of_mut!(*BYTE_PTR); +//~^ ERROR: use of mutable static +//~| ERROR: dereference of raw pointer + +fn main() { + let _ = unsafe { DEREF_BYTE_PTR }; +} diff --git a/tests/ui/static/raw-ref-deref-without-unsafe.stderr b/tests/ui/static/raw-ref-deref-without-unsafe.stderr new file mode 100644 index 00000000000..ac4df8b410c --- /dev/null +++ b/tests/ui/static/raw-ref-deref-without-unsafe.stderr @@ -0,0 +1,19 @@ +error[E0133]: dereference of raw pointer is unsafe and requires unsafe function or block + --> $DIR/raw-ref-deref-without-unsafe.rs:12:56 + | +LL | static mut DEREF_BYTE_PTR: *mut u8 = ptr::addr_of_mut!(*BYTE_PTR); + | ^^^^^^^^^ dereference of raw pointer + | + = note: raw pointers may be null, dangling or unaligned; they can violate aliasing rules and cause data races: all of these are undefined behavior + +error[E0133]: use of mutable static is unsafe and requires unsafe function or block + --> $DIR/raw-ref-deref-without-unsafe.rs:12:57 + | +LL | static mut DEREF_BYTE_PTR: *mut u8 = ptr::addr_of_mut!(*BYTE_PTR); + | ^^^^^^^^ use of mutable static + | + = note: mutable statics can be mutated by multiple threads: aliasing violations or data races will cause undefined behavior + +error: aborting due to 2 previous errors + +For more information about this error, try `rustc --explain E0133`. diff --git a/tests/ui/static/raw-ref-extern-static.rs b/tests/ui/static/raw-ref-extern-static.rs new file mode 100644 index 00000000000..95a53a8640d --- /dev/null +++ b/tests/ui/static/raw-ref-extern-static.rs @@ -0,0 +1,27 @@ +//@ check-pass +#![feature(raw_ref_op)] +use std::ptr; + +// see https://github.com/rust-lang/rust/issues/125833 +// notionally, taking the address of an extern static is a safe operation, +// as we only point at it instead of generating a true reference to it + +// it may potentially induce linker errors, but the safety of that is not about taking addresses! +// any safety obligation of the extern static's correctness in declaration is on the extern itself, +// see RFC 3484 for more on that: https://rust-lang.github.io/rfcs/3484-unsafe-extern-blocks.html + +extern "C" { + static THERE: u8; + static mut SOMEWHERE: u8; +} + +fn main() { + let ptr2there = ptr::addr_of!(THERE); + let ptr2somewhere = ptr::addr_of!(SOMEWHERE); + let ptr2somewhere = ptr::addr_of_mut!(SOMEWHERE); + + // testing both addr_of and the expression it directly expands to + let raw2there = &raw const THERE; + let raw2somewhere = &raw const SOMEWHERE; + let raw2somewhere = &raw mut SOMEWHERE; +} diff --git a/tests/ui/static/raw-ref-static-mut.rs b/tests/ui/static/raw-ref-static-mut.rs new file mode 100644 index 00000000000..6855cc7b050 --- /dev/null +++ b/tests/ui/static/raw-ref-static-mut.rs @@ -0,0 +1,17 @@ +//@ check-pass +#![feature(raw_ref_op)] +use std::ptr; + +// see https://github.com/rust-lang/rust/issues/125833 +// notionally, taking the address of a static mut is a safe operation, +// as we only point at it instead of generating a true reference to it +static mut NOWHERE: usize = 0; + +fn main() { + let p2nowhere = ptr::addr_of!(NOWHERE); + let p2nowhere = ptr::addr_of_mut!(NOWHERE); + + // testing both addr_of and the expression it directly expands to + let raw2nowhere = &raw const NOWHERE; + let raw2nowhere = &raw mut NOWHERE; +}