From f2a80a0f891a09301d43c269b8704bcb2817a419 Mon Sep 17 00:00:00 2001 From: Michael Goulet Date: Sat, 17 Aug 2024 13:03:27 -0400 Subject: [PATCH 1/2] A raw ref of a deref is always safe --- compiler/rustc_mir_build/src/check_unsafety.rs | 12 +++--------- tests/ui/static/raw-ref-deref-without-unsafe.rs | 1 - .../ui/static/raw-ref-deref-without-unsafe.stderr | 2 +- tests/ui/unsafe/place-expr-safe.rs | 14 ++++++++++++++ 4 files changed, 18 insertions(+), 11 deletions(-) create mode 100644 tests/ui/unsafe/place-expr-safe.rs diff --git a/compiler/rustc_mir_build/src/check_unsafety.rs b/compiler/rustc_mir_build/src/check_unsafety.rs index 8512763a595d5..0f0286fab2911 100644 --- a/compiler/rustc_mir_build/src/check_unsafety.rs +++ b/compiler/rustc_mir_build/src/check_unsafety.rs @@ -512,17 +512,11 @@ impl<'a, 'tcx> Visitor<'a, 'tcx> for UnsafetyVisitor<'a, 'tcx> { // 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. + // Taking a raw ref to a deref place expr is always safe. + // Make sure the expression we're deref'ing is safe, though. + visit::walk_expr(self, &self.thir[arg]); 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 } => { diff --git a/tests/ui/static/raw-ref-deref-without-unsafe.rs b/tests/ui/static/raw-ref-deref-without-unsafe.rs index 289e55b76382f..7146c564e91db 100644 --- a/tests/ui/static/raw-ref-deref-without-unsafe.rs +++ b/tests/ui/static/raw-ref-deref-without-unsafe.rs @@ -9,7 +9,6 @@ static mut BYTE_PTR: *mut u8 = ptr::addr_of_mut!(BYTE); // (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 index f034499bbb5ff..d654d4b6e6b70 100644 --- a/tests/ui/static/raw-ref-deref-without-unsafe.stderr +++ b/tests/ui/static/raw-ref-deref-without-unsafe.stderr @@ -14,6 +14,6 @@ LL | static mut DEREF_BYTE_PTR: *mut u8 = ptr::addr_of_mut!(*BYTE_PTR); | = 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 +error: aborting due to 1 previous error For more information about this error, try `rustc --explain E0133`. diff --git a/tests/ui/unsafe/place-expr-safe.rs b/tests/ui/unsafe/place-expr-safe.rs new file mode 100644 index 0000000000000..2590ea7404655 --- /dev/null +++ b/tests/ui/unsafe/place-expr-safe.rs @@ -0,0 +1,14 @@ +//@ check-pass + +fn main() { + let ptr = std::ptr::null_mut::(); + let addr = &raw const *ptr; + + let local = 1; + let ptr = &local as *const i32; + let addr = &raw const *ptr; + + let boxed = Box::new(1); + let ptr = &*boxed as *const i32; + let addr = &raw const *ptr; +} From 367183bc0c64d2835dbfe498d995072c3d426ffe Mon Sep 17 00:00:00 2001 From: Michael Goulet Date: Sun, 18 Aug 2024 12:33:21 -0400 Subject: [PATCH 2/2] Don't emit null pointer lint for raw ref of null deref --- compiler/rustc_lint/src/builtin.rs | 26 ++++++++++++------- .../rustc_mir_build/src/check_unsafety.rs | 2 -- tests/ui/lint/lint-deref-nullptr.rs | 4 +-- tests/ui/lint/lint-deref-nullptr.stderr | 14 +--------- tests/ui/static/raw-ref-deref-with-unsafe.rs | 9 ++++--- .../ui/static/raw-ref-deref-without-unsafe.rs | 6 ++--- .../raw-ref-deref-without-unsafe.stderr | 8 ------ 7 files changed, 28 insertions(+), 41 deletions(-) diff --git a/compiler/rustc_lint/src/builtin.rs b/compiler/rustc_lint/src/builtin.rs index 8bd9c899a6286..0d66ea54bd978 100644 --- a/compiler/rustc_lint/src/builtin.rs +++ b/compiler/rustc_lint/src/builtin.rs @@ -2657,8 +2657,8 @@ declare_lint! { /// /// ### Explanation /// - /// Dereferencing a null pointer causes [undefined behavior] even as a place expression, - /// like `&*(0 as *const i32)` or `addr_of!(*(0 as *const i32))`. + /// Dereferencing a null pointer causes [undefined behavior] if it is accessed + /// (loaded from or stored to). /// /// [undefined behavior]: https://doc.rust-lang.org/reference/behavior-considered-undefined.html pub DEREF_NULLPTR, @@ -2673,14 +2673,14 @@ impl<'tcx> LateLintPass<'tcx> for DerefNullPtr { /// test if expression is a null ptr fn is_null_ptr(cx: &LateContext<'_>, expr: &hir::Expr<'_>) -> bool { match &expr.kind { - rustc_hir::ExprKind::Cast(expr, ty) => { - if let rustc_hir::TyKind::Ptr(_) = ty.kind { + hir::ExprKind::Cast(expr, ty) => { + if let hir::TyKind::Ptr(_) = ty.kind { return is_zero(expr) || is_null_ptr(cx, expr); } } // check for call to `core::ptr::null` or `core::ptr::null_mut` - rustc_hir::ExprKind::Call(path, _) => { - if let rustc_hir::ExprKind::Path(ref qpath) = path.kind { + hir::ExprKind::Call(path, _) => { + if let hir::ExprKind::Path(ref qpath) = path.kind { if let Some(def_id) = cx.qpath_res(qpath, path.hir_id).opt_def_id() { return matches!( cx.tcx.get_diagnostic_name(def_id), @@ -2697,7 +2697,7 @@ impl<'tcx> LateLintPass<'tcx> for DerefNullPtr { /// test if expression is the literal `0` fn is_zero(expr: &hir::Expr<'_>) -> bool { match &expr.kind { - rustc_hir::ExprKind::Lit(lit) => { + hir::ExprKind::Lit(lit) => { if let LitKind::Int(a, _) = lit.node { return a == 0; } @@ -2707,8 +2707,16 @@ impl<'tcx> LateLintPass<'tcx> for DerefNullPtr { false } - if let rustc_hir::ExprKind::Unary(rustc_hir::UnOp::Deref, expr_deref) = expr.kind { - if is_null_ptr(cx, expr_deref) { + if let hir::ExprKind::Unary(hir::UnOp::Deref, expr_deref) = expr.kind + && is_null_ptr(cx, expr_deref) + { + if let hir::Node::Expr(hir::Expr { + kind: hir::ExprKind::AddrOf(hir::BorrowKind::Raw, ..), + .. + }) = cx.tcx.parent_hir_node(expr.hir_id) + { + // `&raw *NULL` is ok. + } else { cx.emit_span_lint(DEREF_NULLPTR, expr.span, BuiltinDerefNullptr { label: expr.span, }); diff --git a/compiler/rustc_mir_build/src/check_unsafety.rs b/compiler/rustc_mir_build/src/check_unsafety.rs index 0f0286fab2911..f3e6301d9d165 100644 --- a/compiler/rustc_mir_build/src/check_unsafety.rs +++ b/compiler/rustc_mir_build/src/check_unsafety.rs @@ -509,8 +509,6 @@ impl<'a, 'tcx> Visitor<'a, 'tcx> for UnsafetyVisitor<'a, 'tcx> { } ExprKind::RawBorrow { 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 { // Taking a raw ref to a deref place expr is always safe. diff --git a/tests/ui/lint/lint-deref-nullptr.rs b/tests/ui/lint/lint-deref-nullptr.rs index d052dbd9b647c..f83d88309b9a7 100644 --- a/tests/ui/lint/lint-deref-nullptr.rs +++ b/tests/ui/lint/lint-deref-nullptr.rs @@ -27,9 +27,9 @@ fn f() { let ub = &*ptr::null_mut::(); //~^ ERROR dereferencing a null pointer ptr::addr_of!(*ptr::null::()); - //~^ ERROR dereferencing a null pointer + // ^^ OKAY ptr::addr_of_mut!(*ptr::null_mut::()); - //~^ ERROR dereferencing a null pointer + // ^^ OKAY let offset = ptr::addr_of!((*ptr::null::()).field); //~^ ERROR dereferencing a null pointer } diff --git a/tests/ui/lint/lint-deref-nullptr.stderr b/tests/ui/lint/lint-deref-nullptr.stderr index c6f432e4e4207..175431b2994bd 100644 --- a/tests/ui/lint/lint-deref-nullptr.stderr +++ b/tests/ui/lint/lint-deref-nullptr.stderr @@ -46,23 +46,11 @@ error: dereferencing a null pointer LL | let ub = &*ptr::null_mut::(); | ^^^^^^^^^^^^^^^^^^^^^^^ this code causes undefined behavior when executed -error: dereferencing a null pointer - --> $DIR/lint-deref-nullptr.rs:29:23 - | -LL | ptr::addr_of!(*ptr::null::()); - | ^^^^^^^^^^^^^^^^^^^ this code causes undefined behavior when executed - -error: dereferencing a null pointer - --> $DIR/lint-deref-nullptr.rs:31:27 - | -LL | ptr::addr_of_mut!(*ptr::null_mut::()); - | ^^^^^^^^^^^^^^^^^^^^^^^ this code causes undefined behavior when executed - error: dereferencing a null pointer --> $DIR/lint-deref-nullptr.rs:33:36 | LL | let offset = ptr::addr_of!((*ptr::null::()).field); | ^^^^^^^^^^^^^^^^^^^^^^^^ this code causes undefined behavior when executed -error: aborting due to 10 previous errors +error: aborting due to 8 previous errors diff --git a/tests/ui/static/raw-ref-deref-with-unsafe.rs b/tests/ui/static/raw-ref-deref-with-unsafe.rs index 0974948b3a06b..5beed6971792e 100644 --- a/tests/ui/static/raw-ref-deref-with-unsafe.rs +++ b/tests/ui/static/raw-ref-deref-with-unsafe.rs @@ -1,13 +1,14 @@ //@ check-pass 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); + +// This code should remain unsafe because reading from a static mut is *always* unsafe. + // 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 +// (it's fine to create raw refs to places!) the following *reads* from the static mut place before +// derefing it explicitly with the `*` below. static mut DEREF_BYTE_PTR: *mut u8 = unsafe { ptr::addr_of_mut!(*BYTE_PTR) }; fn main() { diff --git a/tests/ui/static/raw-ref-deref-without-unsafe.rs b/tests/ui/static/raw-ref-deref-without-unsafe.rs index 7146c564e91db..97d08c815bf93 100644 --- a/tests/ui/static/raw-ref-deref-without-unsafe.rs +++ b/tests/ui/static/raw-ref-deref-without-unsafe.rs @@ -1,10 +1,10 @@ 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); + +// This code should remain unsafe because reading from a static mut is *always* unsafe. + // 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); diff --git a/tests/ui/static/raw-ref-deref-without-unsafe.stderr b/tests/ui/static/raw-ref-deref-without-unsafe.stderr index d654d4b6e6b70..b9c294e5c1f51 100644 --- a/tests/ui/static/raw-ref-deref-without-unsafe.stderr +++ b/tests/ui/static/raw-ref-deref-without-unsafe.stderr @@ -1,11 +1,3 @@ -error[E0133]: dereference of raw pointer is unsafe and requires unsafe function or block - --> $DIR/raw-ref-deref-without-unsafe.rs:10: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:10:57 |