Rollup merge of #136601 - compiler-errors:borrow-null-zst, r=saethlin

Detect (non-raw) borrows of null ZST pointers in CheckNull

Fixes #136568. Ensures that we check that borrows of derefs are non-null in the `CheckNull` pass **even if** it's a ZST pointee.

I'm actually surprised that this is UB in Miri, but if it's certainly UB, then this PR modifies the null check to be stricter. I couldn't find anywhere in https://doc.rust-lang.org/reference/behavior-considered-undefined.html that discusses this case specifically, but I didn't read it too closely, or perhaps it's just missing a bullet point.

On the contrary, if this is actually erroneous UB in Miri, then I'm happy to close this (and perhaps fix the null check in Miri to exclude ZSTs?)

On the double contrary, if this is still an "open question", I'm also happy to close this and wait for a decision to be made.

r? ``@saethlin`` cc ``@RalfJung`` (perhaps you feel strongly about this change)

15 files changed, 85 insertions, 38 deletions

diff --git a/compiler/rustc_middle/src/mir/terminator.rs b/compiler/rustc_middle/src/mir/terminator.rs
index 2fe116212eb..9357e19f7c5 100644
--- a/compiler/rustc_middle/src/mir/terminator.rs
+++ b/compiler/rustc_middle/src/mir/terminator.rs
@@ -272,7 +272,7 @@ impl<O> AssertKind<O> {
                     "\"misaligned pointer dereference: address must be a multiple of {{}} but is {{}}\", {required:?}, {found:?}"
                 )
             }
-            NullPointerDereference => write!(f, "\"null pointer dereference occured\""),
+            NullPointerDereference => write!(f, "\"null pointer dereference occurred\""),
             ResumedAfterReturn(CoroutineKind::Coroutine(_)) => {
                 write!(f, "\"coroutine resumed after completion\"")
             }
diff --git a/compiler/rustc_mir_transform/src/check_alignment.rs b/compiler/rustc_mir_transform/src/check_alignment.rs
index ca5564e447a..b70cca14840 100644
--- a/compiler/rustc_mir_transform/src/check_alignment.rs
+++ b/compiler/rustc_mir_transform/src/check_alignment.rs
@@ -1,5 +1,6 @@
 use rustc_index::IndexVec;
 use rustc_middle::mir::interpret::Scalar;
+use rustc_middle::mir::visit::PlaceContext;
 use rustc_middle::mir::*;
 use rustc_middle::ty::{Ty, TyCtxt};
 use rustc_session::Session;
@@ -44,6 +45,7 @@ fn insert_alignment_check<'tcx>(
     tcx: TyCtxt<'tcx>,
     pointer: Place<'tcx>,
     pointee_ty: Ty<'tcx>,
+    _context: PlaceContext,
     local_decls: &mut IndexVec<Local, LocalDecl<'tcx>>,
     stmts: &mut Vec<Statement<'tcx>>,
     source_info: SourceInfo,
diff --git a/compiler/rustc_mir_transform/src/check_null.rs b/compiler/rustc_mir_transform/src/check_null.rs
index 0b6c0ceaac1..543e1845e65 100644
--- a/compiler/rustc_mir_transform/src/check_null.rs
+++ b/compiler/rustc_mir_transform/src/check_null.rs
@@ -1,5 +1,5 @@
 use rustc_index::IndexVec;
-use rustc_middle::mir::interpret::Scalar;
+use rustc_middle::mir::visit::{MutatingUseContext, NonMutatingUseContext, PlaceContext};
 use rustc_middle::mir::*;
 use rustc_middle::ty::{Ty, TyCtxt};
 use rustc_session::Session;
@@ -26,6 +26,7 @@ fn insert_null_check<'tcx>(
     tcx: TyCtxt<'tcx>,
     pointer: Place<'tcx>,
     pointee_ty: Ty<'tcx>,
+    context: PlaceContext,
     local_decls: &mut IndexVec<Local, LocalDecl<'tcx>>,
     stmts: &mut Vec<Statement<'tcx>>,
     source_info: SourceInfo,
@@ -42,30 +43,51 @@ fn insert_null_check<'tcx>(
     let addr = local_decls.push(LocalDecl::with_source_info(tcx.types.usize, source_info)).into();
     stmts.push(Statement { source_info, kind: StatementKind::Assign(Box::new((addr, rvalue))) });
 
-    // Get size of the pointee (zero-sized reads and writes are allowed).
-    let rvalue = Rvalue::NullaryOp(NullOp::SizeOf, pointee_ty);
-    let sizeof_pointee =
-        local_decls.push(LocalDecl::with_source_info(tcx.types.usize, source_info)).into();
-    stmts.push(Statement {
-        source_info,
-        kind: StatementKind::Assign(Box::new((sizeof_pointee, rvalue))),
-    });
-
-    // Check that the pointee is not a ZST.
     let zero = Operand::Constant(Box::new(ConstOperand {
         span: source_info.span,
         user_ty: None,
-        const_: Const::Val(ConstValue::Scalar(Scalar::from_target_usize(0, &tcx)), tcx.types.usize),
+        const_: Const::Val(ConstValue::from_target_usize(0, &tcx), tcx.types.usize),
     }));
-    let is_pointee_no_zst =
-        local_decls.push(LocalDecl::with_source_info(tcx.types.bool, source_info)).into();
-    stmts.push(Statement {
-        source_info,
-        kind: StatementKind::Assign(Box::new((
-            is_pointee_no_zst,
-            Rvalue::BinaryOp(BinOp::Ne, Box::new((Operand::Copy(sizeof_pointee), zero.clone()))),
-        ))),
-    });
+
+    let pointee_should_be_checked = match context {
+        // Borrows pointing to "null" are UB even if the pointee is a ZST.
+        PlaceContext::NonMutatingUse(NonMutatingUseContext::SharedBorrow)
+        | PlaceContext::MutatingUse(MutatingUseContext::Borrow) => {
+            // Pointer should be checked unconditionally.
+            Operand::Constant(Box::new(ConstOperand {
+                span: source_info.span,
+                user_ty: None,
+                const_: Const::Val(ConstValue::from_bool(true), tcx.types.bool),
+            }))
+        }
+        // Other usages of null pointers only are UB if the pointee is not a ZST.
+        _ => {
+            let rvalue = Rvalue::NullaryOp(NullOp::SizeOf, pointee_ty);
+            let sizeof_pointee =
+                local_decls.push(LocalDecl::with_source_info(tcx.types.usize, source_info)).into();
+            stmts.push(Statement {
+                source_info,
+                kind: StatementKind::Assign(Box::new((sizeof_pointee, rvalue))),
+            });
+
+            // Check that the pointee is not a ZST.
+            let is_pointee_not_zst =
+                local_decls.push(LocalDecl::with_source_info(tcx.types.bool, source_info)).into();
+            stmts.push(Statement {
+                source_info,
+                kind: StatementKind::Assign(Box::new((
+                    is_pointee_not_zst,
+                    Rvalue::BinaryOp(
+                        BinOp::Ne,
+                        Box::new((Operand::Copy(sizeof_pointee), zero.clone())),
+                    ),
+                ))),
+            });
+
+            // Pointer needs to be checked only if pointee is not a ZST.
+            Operand::Copy(is_pointee_not_zst)
+        }
+    };
 
     // Check whether the pointer is null.
     let is_null = local_decls.push(LocalDecl::with_source_info(tcx.types.bool, source_info)).into();
@@ -77,7 +99,8 @@ fn insert_null_check<'tcx>(
         ))),
     });
 
-    // We want to throw an exception if the pointer is null and doesn't point to a ZST.
+    // We want to throw an exception if the pointer is null and the pointee is not unconditionally
+    // allowed (which for all non-borrow place uses, is when the pointee is ZST).
     let should_throw_exception =
         local_decls.push(LocalDecl::with_source_info(tcx.types.bool, source_info)).into();
     stmts.push(Statement {
@@ -86,7 +109,7 @@ fn insert_null_check<'tcx>(
             should_throw_exception,
             Rvalue::BinaryOp(
                 BinOp::BitAnd,
-                Box::new((Operand::Copy(is_null), Operand::Copy(is_pointee_no_zst))),
+                Box::new((Operand::Copy(is_null), pointee_should_be_checked)),
             ),
         ))),
     });
diff --git a/compiler/rustc_mir_transform/src/check_pointers.rs b/compiler/rustc_mir_transform/src/check_pointers.rs
index 72460542f87..ccaa83fd9e2 100644
--- a/compiler/rustc_mir_transform/src/check_pointers.rs
+++ b/compiler/rustc_mir_transform/src/check_pointers.rs
@@ -40,10 +40,10 @@ pub(crate) enum BorrowCheckMode {
 ///   success and fail the check otherwise.
 /// This utility will insert a terminator block that asserts on the condition
 /// and panics on failure.
-pub(crate) fn check_pointers<'a, 'tcx, F>(
+pub(crate) fn check_pointers<'tcx, F>(
     tcx: TyCtxt<'tcx>,
     body: &mut Body<'tcx>,
-    excluded_pointees: &'a [Ty<'tcx>],
+    excluded_pointees: &[Ty<'tcx>],
     on_finding: F,
     borrow_check_mode: BorrowCheckMode,
 ) where
@@ -51,6 +51,7 @@ pub(crate) fn check_pointers<'a, 'tcx, F>(
         /* tcx: */ TyCtxt<'tcx>,
         /* pointer: */ Place<'tcx>,
         /* pointee_ty: */ Ty<'tcx>,
+        /* context: */ PlaceContext,
         /* local_decls: */ &mut IndexVec<Local, LocalDecl<'tcx>>,
         /* stmts: */ &mut Vec<Statement<'tcx>>,
         /* source_info: */ SourceInfo,
@@ -86,7 +87,7 @@ pub(crate) fn check_pointers<'a, 'tcx, F>(
             );
             finder.visit_statement(statement, location);
 
-            for (local, ty) in finder.into_found_pointers() {
+            for (local, ty, context) in finder.into_found_pointers() {
                 debug!("Inserting check for {:?}", ty);
                 let new_block = split_block(basic_blocks, location);
 
@@ -98,6 +99,7 @@ pub(crate) fn check_pointers<'a, 'tcx, F>(
                     tcx,
                     local,
                     ty,
+                    context,
                     local_decls,
                     &mut block_data.statements,
                     source_info,
@@ -125,7 +127,7 @@ struct PointerFinder<'a, 'tcx> {
     tcx: TyCtxt<'tcx>,
     local_decls: &'a mut LocalDecls<'tcx>,
     typing_env: ty::TypingEnv<'tcx>,
-    pointers: Vec<(Place<'tcx>, Ty<'tcx>)>,
+    pointers: Vec<(Place<'tcx>, Ty<'tcx>, PlaceContext)>,
     excluded_pointees: &'a [Ty<'tcx>],
     borrow_check_mode: BorrowCheckMode,
 }
@@ -148,7 +150,7 @@ impl<'a, 'tcx> PointerFinder<'a, 'tcx> {
         }
     }
 
-    fn into_found_pointers(self) -> Vec<(Place<'tcx>, Ty<'tcx>)> {
+    fn into_found_pointers(self) -> Vec<(Place<'tcx>, Ty<'tcx>, PlaceContext)> {
         self.pointers
     }
 
@@ -211,7 +213,7 @@ impl<'a, 'tcx> Visitor<'tcx> for PointerFinder<'a, 'tcx> {
             return;
         }
 
-        self.pointers.push((pointer, pointee_ty));
+        self.pointers.push((pointer, pointee_ty, context));
 
         self.super_place(place, context, location);
     }
diff --git a/compiler/stable_mir/src/mir/body.rs b/compiler/stable_mir/src/mir/body.rs
index a6406e9db8e..bc2e427f50c 100644
--- a/compiler/stable_mir/src/mir/body.rs
+++ b/compiler/stable_mir/src/mir/body.rs
@@ -307,7 +307,7 @@ impl AssertMessage {
             AssertMessage::MisalignedPointerDereference { .. } => {
                 Ok("misaligned pointer dereference")
             }
-            AssertMessage::NullPointerDereference => Ok("null pointer dereference occured"),
+            AssertMessage::NullPointerDereference => Ok("null pointer dereference occurred"),
         }
     }
 }
diff --git a/compiler/stable_mir/src/mir/pretty.rs b/compiler/stable_mir/src/mir/pretty.rs
index 6638f93e555..8278afb7a2f 100644
--- a/compiler/stable_mir/src/mir/pretty.rs
+++ b/compiler/stable_mir/src/mir/pretty.rs
@@ -299,7 +299,7 @@ fn pretty_assert_message<W: Write>(writer: &mut W, msg: &AssertMessage) -> io::R
             )
         }
         AssertMessage::NullPointerDereference => {
-            write!(writer, "\"null pointer dereference occured.\"")
+            write!(writer, "\"null pointer dereference occurred\"")
         }
         AssertMessage::ResumedAfterReturn(_) | AssertMessage::ResumedAfterPanic(_) => {
             write!(writer, "{}", msg.description().unwrap())
diff --git a/library/core/src/panicking.rs b/library/core/src/panicking.rs
index a89de12c02b..b97f19e1baa 100644
--- a/library/core/src/panicking.rs
+++ b/library/core/src/panicking.rs
@@ -302,7 +302,7 @@ fn panic_null_pointer_dereference() -> ! {
     }
 
     panic_nounwind_fmt(
-        format_args!("null pointer dereference occured"),
+        format_args!("null pointer dereference occurred"),
         /* force_no_backtrace */ false,
     )
 }
diff --git a/tests/ui/mir/null/borrowed_mut_null.rs b/tests/ui/mir/null/borrowed_mut_null.rs
index 437955c452b..d26452b9dac 100644
--- a/tests/ui/mir/null/borrowed_mut_null.rs
+++ b/tests/ui/mir/null/borrowed_mut_null.rs
@@ -1,6 +1,6 @@
 //@ run-fail
 //@ compile-flags: -C debug-assertions
-//@ error-pattern: null pointer dereference occured
+//@ error-pattern: null pointer dereference occurred
 
 fn main() {
     let ptr: *mut u32 = std::ptr::null_mut();
diff --git a/tests/ui/mir/null/borrowed_null.rs b/tests/ui/mir/null/borrowed_null.rs
index eb0794efaa5..fefac3a7212 100644
--- a/tests/ui/mir/null/borrowed_null.rs
+++ b/tests/ui/mir/null/borrowed_null.rs
@@ -1,6 +1,6 @@
 //@ run-fail
 //@ compile-flags: -C debug-assertions
-//@ error-pattern: null pointer dereference occured
+//@ error-pattern: null pointer dereference occurred
 
 fn main() {
     let ptr: *const u32 = std::ptr::null();
diff --git a/tests/ui/mir/null/borrowed_null_zst.rs b/tests/ui/mir/null/borrowed_null_zst.rs
new file mode 100644
index 00000000000..835727c068b
--- /dev/null
+++ b/tests/ui/mir/null/borrowed_null_zst.rs
@@ -0,0 +1,8 @@
+//@ run-fail
+//@ compile-flags: -C debug-assertions
+//@ error-pattern: null pointer dereference occurred
+
+fn main() {
+    let ptr: *const () = std::ptr::null();
+    let _ptr: &() = unsafe { &*ptr };
+}
diff --git a/tests/ui/mir/null/null_lhs.rs b/tests/ui/mir/null/null_lhs.rs
index fd3bc3a78b8..238d350d1bd 100644
--- a/tests/ui/mir/null/null_lhs.rs
+++ b/tests/ui/mir/null/null_lhs.rs
@@ -1,6 +1,6 @@
 //@ run-fail
 //@ compile-flags: -C debug-assertions
-//@ error-pattern: null pointer dereference occured
+//@ error-pattern: null pointer dereference occurred
 
 fn main() {
     let ptr: *mut u32 = std::ptr::null_mut();
diff --git a/tests/ui/mir/null/null_rhs.rs b/tests/ui/mir/null/null_rhs.rs
index 45c8beb3fe8..18eafb61869 100644
--- a/tests/ui/mir/null/null_rhs.rs
+++ b/tests/ui/mir/null/null_rhs.rs
@@ -1,6 +1,6 @@
 //@ run-fail
 //@ compile-flags: -C debug-assertions
-//@ error-pattern: null pointer dereference occured
+//@ error-pattern: null pointer dereference occurred
 
 fn main() {
     let ptr: *mut u32 = std::ptr::null_mut();
diff --git a/tests/ui/mir/null/place_without_read.rs b/tests/ui/mir/null/place_without_read.rs
index d6bfb089b01..a259053d22b 100644
--- a/tests/ui/mir/null/place_without_read.rs
+++ b/tests/ui/mir/null/place_without_read.rs
@@ -6,5 +6,6 @@ fn main() {
     let ptr: *const u16 = std::ptr::null();
     unsafe {
         let _ = *ptr;
+        let _ = &raw const *ptr;
     }
 }
diff --git a/tests/ui/mir/null/place_without_read_zst.rs b/tests/ui/mir/null/place_without_read_zst.rs
new file mode 100644
index 00000000000..c923ccea9a8
--- /dev/null
+++ b/tests/ui/mir/null/place_without_read_zst.rs
@@ -0,0 +1,11 @@
+// Make sure that we don't insert a check for places that do not read.
+//@ run-pass
+//@ compile-flags: -C debug-assertions
+
+fn main() {
+    let ptr: *const () = std::ptr::null();
+    unsafe {
+        let _ = *ptr;
+        let _ = &raw const *ptr;
+    }
+}
diff --git a/tests/ui/mir/null/two_pointers.rs b/tests/ui/mir/null/two_pointers.rs
index d9f0687fe0d..52b9510be12 100644
--- a/tests/ui/mir/null/two_pointers.rs
+++ b/tests/ui/mir/null/two_pointers.rs
@@ -1,6 +1,6 @@
 //@ run-fail
 //@ compile-flags: -C debug-assertions
-//@ error-pattern: null pointer dereference occured
+//@ error-pattern: null pointer dereference occurred
 
 fn main() {
     let ptr = std::ptr::null();