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

Author: compiler-errors

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,

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

compiler/rustc_mir_transform/src/check_pointers.rs

@@ -40,17 +40,18 @@ 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
     F: Fn(
         /* 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);
     }

tests/ui/mir/null/borrowed_null_zst.rs

@@ -0,0 +1,8 @@
+//@ run-fail
+//@ compile-flags: -C debug-assertions
+//@ error-pattern: null pointer dereference occured
+
+fn main() {
+    let ptr: *const () = std::ptr::null();
+    let _ptr: &() = unsafe { &*ptr };
+}

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

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;
+    }
+}