From 3fdd8d5ef3a8b26c3e24d9baf706fc7fb16717f9 Mon Sep 17 00:00:00 2001 From: Jubilee Young Date: Fri, 31 May 2024 18:48:42 -0700 Subject: [PATCH 1/3] 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 @@ fn visit_expr(&mut self, expr: &'a Expr<'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 @@ fn convert_path_expr(&mut self, expr: &'tcx hir::Expr<'tcx>, res: Res) -> ExprKi } } - // 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_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 @@ unsafe impl Sync for SyncPtr {} 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; +} From bf454afcaabb2406dda5e5d880ad061750caf4a8 Mon Sep 17 00:00:00 2001 From: Jubilee Young Date: Fri, 31 May 2024 18:28:03 -0700 Subject: [PATCH 2/3] library: vary unsafety in bootstrapping for SEH --- library/panic_unwind/src/seh.rs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/library/panic_unwind/src/seh.rs b/library/panic_unwind/src/seh.rs index 04c3d3bf9c3..82c248c5a7b 100644 --- a/library/panic_unwind/src/seh.rs +++ b/library/panic_unwind/src/seh.rs @@ -157,7 +157,10 @@ pub fn new(ptr: *mut u8) -> Self { // going to be cross-lang LTOed anyway. However, using expose is shorter and // requires less unsafe. let addr: usize = ptr.expose_provenance(); + #[cfg(bootstrap)] let image_base = unsafe { addr_of!(__ImageBase) }.addr(); + #[cfg(not(bootstrap))] + let image_base = addr_of!(__ImageBase).addr(); let offset: usize = addr - image_base; Self(offset as u32) } @@ -250,7 +253,10 @@ pub struct _TypeDescriptor { // This is fine since the MSVC runtime uses string comparison on the type name // to match TypeDescriptors rather than pointer equality. static mut TYPE_DESCRIPTOR: _TypeDescriptor = _TypeDescriptor { + #[cfg(bootstrap)] pVFTable: unsafe { addr_of!(TYPE_INFO_VTABLE) } as *const _, + #[cfg(not(bootstrap))] + pVFTable: addr_of!(TYPE_INFO_VTABLE) as *const _, spare: core::ptr::null_mut(), name: TYPE_NAME, }; From b3cd9b5cd3470da7c8f05ed27a8c1db02422b805 Mon Sep 17 00:00:00 2001 From: Jubilee Young Date: Fri, 31 May 2024 23:40:11 -0700 Subject: [PATCH 3/3] miri: fixup for allowing &raw UNSAFE_STATIC --- src/tools/miri/tests/fail/extern_static.rs | 2 +- src/tools/miri/tests/fail/extern_static.stderr | 4 ++-- src/tools/miri/tests/pass/static_mut.rs | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/tools/miri/tests/fail/extern_static.rs b/src/tools/miri/tests/fail/extern_static.rs index f8805db8d14..0cbf1646be9 100644 --- a/src/tools/miri/tests/fail/extern_static.rs +++ b/src/tools/miri/tests/fail/extern_static.rs @@ -5,5 +5,5 @@ } fn main() { - let _val = unsafe { std::ptr::addr_of!(FOO) }; //~ ERROR: is not supported by Miri + let _val = std::ptr::addr_of!(FOO); //~ ERROR: is not supported by Miri } diff --git a/src/tools/miri/tests/fail/extern_static.stderr b/src/tools/miri/tests/fail/extern_static.stderr index 21759f96019..c7ab128e2fe 100644 --- a/src/tools/miri/tests/fail/extern_static.stderr +++ b/src/tools/miri/tests/fail/extern_static.stderr @@ -1,8 +1,8 @@ error: unsupported operation: extern static `FOO` is not supported by Miri --> $DIR/extern_static.rs:LL:CC | -LL | let _val = unsafe { std::ptr::addr_of!(FOO) }; - | ^^^ extern static `FOO` is not supported by Miri +LL | let _val = std::ptr::addr_of!(FOO); + | ^^^ extern static `FOO` is not supported by Miri | = help: this is likely not a bug in the program; it indicates that the program performed an operation that Miri does not support = note: BACKTRACE: diff --git a/src/tools/miri/tests/pass/static_mut.rs b/src/tools/miri/tests/pass/static_mut.rs index 1b416cc4e9b..4488b5a09d5 100644 --- a/src/tools/miri/tests/pass/static_mut.rs +++ b/src/tools/miri/tests/pass/static_mut.rs @@ -2,7 +2,7 @@ static mut FOO: i32 = 42; -static BAR: Foo = Foo(unsafe { addr_of!(FOO) }); +static BAR: Foo = Foo(addr_of!(FOO)); #[allow(dead_code)] struct Foo(*const i32);