From b4641b2b3fd0616fa9597537f8fd954c34e6535f Mon Sep 17 00:00:00 2001 From: Michael Goulet Date: Wed, 5 Feb 2025 19:36:41 +0000 Subject: [PATCH 1/2] Detect (non-raw) borrows of null ZST pointers in CheckNull --- .../src/check_alignment.rs | 2 + .../rustc_mir_transform/src/check_null.rs | 69 ++++++++++++------- .../rustc_mir_transform/src/check_pointers.rs | 14 ++-- tests/ui/mir/null/borrowed_null_zst.rs | 8 +++ tests/ui/mir/null/place_without_read.rs | 1 + tests/ui/mir/null/place_without_read_zst.rs | 11 +++ 6 files changed, 76 insertions(+), 29 deletions(-) create mode 100644 tests/ui/mir/null/borrowed_null_zst.rs create mode 100644 tests/ui/mir/null/place_without_read_zst.rs 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>, stmts: &mut Vec>, 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>, stmts: &mut Vec>, 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>, /* stmts: */ &mut Vec>, /* 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/tests/ui/mir/null/borrowed_null_zst.rs b/tests/ui/mir/null/borrowed_null_zst.rs new file mode 100644 index 00000000000..7b8743e7044 --- /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 occured + +fn main() { + let ptr: *const () = std::ptr::null(); + let _ptr: &() = unsafe { &*ptr }; +} 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; + } +} From a61537f6c068eab87c79173c23837d74b7f7d0ef Mon Sep 17 00:00:00 2001 From: Michael Goulet Date: Sat, 8 Feb 2025 22:28:21 +0000 Subject: [PATCH 2/2] occured -> occurred --- compiler/rustc_middle/src/mir/terminator.rs | 2 +- compiler/stable_mir/src/mir/body.rs | 2 +- compiler/stable_mir/src/mir/pretty.rs | 2 +- library/core/src/panicking.rs | 2 +- tests/ui/mir/null/borrowed_mut_null.rs | 2 +- tests/ui/mir/null/borrowed_null.rs | 2 +- tests/ui/mir/null/borrowed_null_zst.rs | 2 +- tests/ui/mir/null/null_lhs.rs | 2 +- tests/ui/mir/null/null_rhs.rs | 2 +- tests/ui/mir/null/two_pointers.rs | 2 +- 10 files changed, 10 insertions(+), 10 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 AssertKind { "\"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/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(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 index 7b8743e7044..835727c068b 100644 --- a/tests/ui/mir/null/borrowed_null_zst.rs +++ b/tests/ui/mir/null/borrowed_null_zst.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 () = std::ptr::null(); 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/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();