From 367183bc0c64d2835dbfe498d995072c3d426ffe Mon Sep 17 00:00:00 2001 From: Michael Goulet Date: Sun, 18 Aug 2024 12:33:21 -0400 Subject: [PATCH] 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 8bd9c899a62..0d66ea54bd9 100644 --- a/compiler/rustc_lint/src/builtin.rs +++ b/compiler/rustc_lint/src/builtin.rs @@ -2657,8 +2657,8 @@ fn ty_find_init_error<'tcx>( /// /// ### 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 @@ fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &hir::Expr<'_>) { /// 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 @@ fn is_null_ptr(cx: &LateContext<'_>, expr: &hir::Expr<'_>) -> bool { /// 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 @@ fn is_zero(expr: &hir::Expr<'_>) -> bool { 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 0f0286fab29..f3e6301d9d1 100644 --- a/compiler/rustc_mir_build/src/check_unsafety.rs +++ b/compiler/rustc_mir_build/src/check_unsafety.rs @@ -509,8 +509,6 @@ fn visit_expr(&mut self, expr: &'a Expr<'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 d052dbd9b64..f83d88309b9 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 c6f432e4e42..175431b2994 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 0974948b3a0..5beed697179 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 7146c564e91..97d08c815bf 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 d654d4b6e6b..b9c294e5c1f 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 |