Rollup merge of rust-lang#129248 - compiler-errors:raw-ref-deref, r=nnethercote

Taking a raw ref (`&raw (const|mut)`) of a deref of pointer (`*ptr`) is always safe

T-opsem decided in rust-lang/reference#1387 that `*ptr` is only unsafe if the place is accessed. This means that taking a raw ref of a deref expr is always safe, since it doesn't constitute a read.

This also relaxes the `DEREF_NULLPTR` lint to stop warning in the case of raw ref of a deref'd nullptr, and updates its docs to reflect that change in the UB specification.

This does not change the behavior of `addr_of!((*ptr).field)`, since field projections still require the projection is in-bounds.

I'm on the fence whether this requires an FCP, since it's something that is guaranteed by the reference you could ostensibly call this a bugfix since we were counting truly safe operations as unsafe. Perhaps someone on opsem has a strong opinion? cc `@rust-lang/opsem`

Author: matthiaskrgr

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,
                 });

compiler/rustc_mir_build/src/check_unsafety.rs

@@ -509,20 +509,12 @@ 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
-                    // 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 } => {

tests/ui/lint/lint-deref-nullptr.rs

@@ -27,9 +27,9 @@ fn f() {
         let ub = &*ptr::null_mut::<i32>();
         //~^ ERROR dereferencing a null pointer
         ptr::addr_of!(*ptr::null::<i32>());
-        //~^ ERROR dereferencing a null pointer
+        // ^^ OKAY
         ptr::addr_of_mut!(*ptr::null_mut::<i32>());
-        //~^ ERROR dereferencing a null pointer
+        // ^^ OKAY
         let offset = ptr::addr_of!((*ptr::null::<Struct>()).field);
         //~^ ERROR dereferencing a null pointer
     }

tests/ui/lint/lint-deref-nullptr.stderr

@@ -46,23 +46,11 @@ error: dereferencing a null pointer
 LL |         let ub = &*ptr::null_mut::<i32>();
    |                   ^^^^^^^^^^^^^^^^^^^^^^^ 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::<i32>());
-   |                       ^^^^^^^^^^^^^^^^^^^ 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::<i32>());
-   |                           ^^^^^^^^^^^^^^^^^^^^^^^ 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::<Struct>()).field);
    |                                    ^^^^^^^^^^^^^^^^^^^^^^^^ this code causes undefined behavior when executed
 
-error: aborting due to 10 previous errors
+error: aborting due to 8 previous errors
 

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() {

tests/ui/static/raw-ref-deref-without-unsafe.rs

@@ -1,15 +1,14 @@
 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);
 //~^ ERROR: use of mutable static
-//~| ERROR: dereference of raw pointer
 
 fn main() {
     let _ = unsafe { DEREF_BYTE_PTR };

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
    |
@@ -14,6 +6,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`.

tests/ui/unsafe/place-expr-safe.rs

@@ -0,0 +1,14 @@
+//@ check-pass
+
+fn main() {
+    let ptr = std::ptr::null_mut::<i32>();
+    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;
+}