From d8064d7d49073ff9962369a40678c934d700f7e0 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Thu, 24 Feb 2022 19:38:37 -0500 Subject: [PATCH] Miri fn ptr check: don't use conservative null check --- .../src/const_eval/machine.rs | 5 ++-- .../src/interpret/eval_context.rs | 29 +++++++++++++++++-- .../rustc_const_eval/src/interpret/memory.rs | 15 ---------- .../rustc_const_eval/src/interpret/operand.rs | 3 +- .../src/interpret/validity.rs | 19 +++++++----- .../consts/const-eval/ub-ref-ptr.32bit.stderr | 2 +- .../consts/const-eval/ub-ref-ptr.64bit.stderr | 2 +- 7 files changed, 43 insertions(+), 32 deletions(-) diff --git a/compiler/rustc_const_eval/src/const_eval/machine.rs b/compiler/rustc_const_eval/src/const_eval/machine.rs index b2019ce40c3..99888992bc8 100644 --- a/compiler/rustc_const_eval/src/const_eval/machine.rs +++ b/compiler/rustc_const_eval/src/const_eval/machine.rs @@ -217,8 +217,9 @@ impl<'mir, 'tcx: 'mir> CompileTimeEvalContext<'mir, 'tcx> { // Comparisons of abstract pointers with null pointers are known if the pointer // is in bounds, because if they are in bounds, the pointer can't be null. // Inequality with integers other than null can never be known for sure. - (Scalar::Int(int), Scalar::Ptr(ptr, _)) | (Scalar::Ptr(ptr, _), Scalar::Int(int)) => { - int.is_null() && !self.memory.ptr_may_be_null(ptr.into()) + (Scalar::Int(int), ptr @ Scalar::Ptr(..)) + | (ptr @ Scalar::Ptr(..), Scalar::Int(int)) => { + int.is_null() && !self.scalar_may_be_null(ptr) } // FIXME: return `true` for at least some comparisons where we can reliably // determine the result of runtime inequality tests at compile-time. diff --git a/compiler/rustc_const_eval/src/interpret/eval_context.rs b/compiler/rustc_const_eval/src/interpret/eval_context.rs index ab50c709143..45ac3cd1f84 100644 --- a/compiler/rustc_const_eval/src/interpret/eval_context.rs +++ b/compiler/rustc_const_eval/src/interpret/eval_context.rs @@ -22,9 +22,9 @@ use rustc_span::{Pos, Span}; use rustc_target::abi::{call::FnAbi, Align, HasDataLayout, Size, TargetDataLayout}; use super::{ - AllocId, GlobalId, Immediate, InterpErrorInfo, InterpResult, MPlaceTy, Machine, MemPlace, - MemPlaceMeta, Memory, MemoryKind, Operand, Place, PlaceTy, Pointer, Provenance, Scalar, - ScalarMaybeUninit, StackPopJump, + AllocCheck, AllocId, GlobalId, Immediate, InterpErrorInfo, InterpResult, MPlaceTy, Machine, + MemPlace, MemPlaceMeta, Memory, MemoryKind, Operand, Place, PlaceTy, Pointer, Provenance, + Scalar, ScalarMaybeUninit, StackPopJump, }; use crate::transform::validate::equal_up_to_regions; @@ -440,6 +440,29 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { self.memory.scalar_to_ptr(scalar) } + /// Test if this value might be null. + /// If the machine does not support ptr-to-int casts, this is conservative. + pub fn scalar_may_be_null(&self, scalar: Scalar) -> bool { + match scalar.try_to_int() { + Ok(int) => int.is_null(), + Err(_) => { + let ptr = self.scalar_to_ptr(scalar); + match self.memory.ptr_try_get_alloc(ptr) { + Ok((alloc_id, offset, _)) => { + let (size, _align) = self + .memory + .get_size_and_align(alloc_id, AllocCheck::MaybeDead) + .expect("alloc info with MaybeDead cannot fail"); + // If the pointer is out-of-bounds, it may be null. + // Note that one-past-the-end (offset == size) is still inbounds, and never null. + offset > size + } + Err(offset) => offset == 0, + } + } + } + } + /// Call this to turn untagged "global" pointers (obtained via `tcx`) into /// the machine pointer to the allocation. Must never be used /// for any other pointers, nor for TLS statics. diff --git a/compiler/rustc_const_eval/src/interpret/memory.rs b/compiler/rustc_const_eval/src/interpret/memory.rs index 73e7d862ad6..04a6209990c 100644 --- a/compiler/rustc_const_eval/src/interpret/memory.rs +++ b/compiler/rustc_const_eval/src/interpret/memory.rs @@ -483,21 +483,6 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> Memory<'mir, 'tcx, M> { } }) } - - /// Test if the pointer might be null. - pub fn ptr_may_be_null(&self, ptr: Pointer>) -> bool { - match self.ptr_try_get_alloc(ptr) { - Ok((alloc_id, offset, _)) => { - let (size, _align) = self - .get_size_and_align(alloc_id, AllocCheck::MaybeDead) - .expect("alloc info with MaybeDead cannot fail"); - // If the pointer is out-of-bounds, it may be null. - // Note that one-past-the-end (offset == size) is still inbounds, and never null. - offset > size - } - Err(offset) => offset == 0, - } - } } /// Allocation accessors diff --git a/compiler/rustc_const_eval/src/interpret/operand.rs b/compiler/rustc_const_eval/src/interpret/operand.rs index cd147b03bca..eba5c2bbe5a 100644 --- a/compiler/rustc_const_eval/src/interpret/operand.rs +++ b/compiler/rustc_const_eval/src/interpret/operand.rs @@ -720,12 +720,11 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { Err(dbg_val) => { // So this is a pointer then, and casting to an int failed. // Can only happen during CTFE. - let ptr = self.scalar_to_ptr(tag_val); // The niche must be just 0, and the ptr not null, then we know this is // okay. Everything else, we conservatively reject. let ptr_valid = niche_start == 0 && variants_start == variants_end - && !self.memory.ptr_may_be_null(ptr); + && !self.scalar_may_be_null(tag_val); if !ptr_valid { throw_ub!(InvalidTag(dbg_val)) } diff --git a/compiler/rustc_const_eval/src/interpret/validity.rs b/compiler/rustc_const_eval/src/interpret/validity.rs index 0bf86d52080..5cacab82386 100644 --- a/compiler/rustc_const_eval/src/interpret/validity.rs +++ b/compiler/rustc_const_eval/src/interpret/validity.rs @@ -572,21 +572,25 @@ impl<'rt, 'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> ValidityVisitor<'rt, 'mir, ' err_unsup!(ReadPointerAsBytes) => { "part of a pointer" } expected { "a proper pointer or integer value" }, err_ub!(InvalidUninitBytes(None)) => { "uninitialized bytes" } expected { "a proper pointer or integer value" }, ); - let ptr = self.ecx.scalar_to_ptr(value); - // Ensure the pointer is non-null. - if self.ecx.memory.ptr_may_be_null(ptr) { - throw_validation_failure!(self.path, { "a potentially null function pointer" }); - } + // If we check references recursively, also check that this points to a function. if let Some(_) = self.ref_tracking { + let ptr = self.ecx.scalar_to_ptr(value); let _fn = try_validation!( self.ecx.memory.get_fn(ptr), self.path, + err_ub!(DanglingIntPointer(0, _)) => + { "a null function pointer" }, err_ub!(DanglingIntPointer(..)) | err_ub!(InvalidFunctionPointer(..)) => { "{:x}", value } expected { "a function pointer" }, ); // FIXME: Check if the signature matches + } else { + // Otherwise (for standalone Miri), we have to still check it to be non-null. + if self.ecx.scalar_may_be_null(value) { + throw_validation_failure!(self.path, { "a null function pointer" }); + } } Ok(true) } @@ -644,10 +648,9 @@ impl<'rt, 'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> ValidityVisitor<'rt, 'mir, ' Err(_) => { // So this is a pointer then, and casting to an int failed. // Can only happen during CTFE. - let ptr = self.ecx.scalar_to_ptr(value); if start == 1 && end == max_value { // Only null is the niche. So make sure the ptr is NOT null. - if self.ecx.memory.ptr_may_be_null(ptr) { + if self.ecx.scalar_may_be_null(value) { throw_validation_failure!(self.path, { "a potentially null pointer" } expected { @@ -758,7 +761,7 @@ impl<'rt, 'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> ValueVisitor<'mir, 'tcx, M> fn visit_value(&mut self, op: &OpTy<'tcx, M::PointerTag>) -> InterpResult<'tcx> { trace!("visit_value: {:?}, {:?}", *op, op.layout); - // Check primitive types -- the leafs of our recursive descend. + // Check primitive types -- the leaves of our recursive descent. if self.try_visit_primitive(op)? { return Ok(()); } diff --git a/src/test/ui/consts/const-eval/ub-ref-ptr.32bit.stderr b/src/test/ui/consts/const-eval/ub-ref-ptr.32bit.stderr index bf4e2926d51..f6f2432f2d7 100644 --- a/src/test/ui/consts/const-eval/ub-ref-ptr.32bit.stderr +++ b/src/test/ui/consts/const-eval/ub-ref-ptr.32bit.stderr @@ -112,7 +112,7 @@ error[E0080]: it is undefined behavior to use this value --> $DIR/ub-ref-ptr.rs:49:1 | LL | const NULL_FN_PTR: fn() = unsafe { mem::transmute(0usize) }; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered a potentially null function pointer + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered a null function pointer | = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rustc repository if you believe it should not be considered undefined behavior. = note: the raw bytes of the constant (size: 4, align: 4) { diff --git a/src/test/ui/consts/const-eval/ub-ref-ptr.64bit.stderr b/src/test/ui/consts/const-eval/ub-ref-ptr.64bit.stderr index ef25e279a06..28bd040e223 100644 --- a/src/test/ui/consts/const-eval/ub-ref-ptr.64bit.stderr +++ b/src/test/ui/consts/const-eval/ub-ref-ptr.64bit.stderr @@ -112,7 +112,7 @@ error[E0080]: it is undefined behavior to use this value --> $DIR/ub-ref-ptr.rs:49:1 | LL | const NULL_FN_PTR: fn() = unsafe { mem::transmute(0usize) }; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered a potentially null function pointer + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered a null function pointer | = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rustc repository if you believe it should not be considered undefined behavior. = note: the raw bytes of the constant (size: 8, align: 8) {