From 411d6bc4c70dfdc526d8c1cb111cbfc188b9f342 Mon Sep 17 00:00:00 2001 From: Taras Tsugrii Date: Mon, 7 Aug 2023 20:00:45 -0500 Subject: [PATCH 01/13] [nit][typo] Remove "currently" that breaks reading flow. --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 89d4b29ebb..4483ae242d 100644 --- a/README.md +++ b/README.md @@ -53,7 +53,7 @@ behavior** in your program, and cannot run all programs: positives here, so if your program runs fine in Miri right now that is by no means a guarantee that it is UB-free when these questions get answered. - In particular, Miri does currently not check that references point to valid data. + In particular, Miri does not check that references point to valid data. * If the program relies on unspecified details of how data is laid out, it will still run fine in Miri -- but might break (including causing UB) on different compiler versions or different platforms. From 9bace04e471592396d4b6bc565dc9e438c116858 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Fri, 11 Aug 2023 16:47:23 +0200 Subject: [PATCH 02/13] llvm.prefetch is not a math function --- src/shims/foreign_items.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/shims/foreign_items.rs b/src/shims/foreign_items.rs index 953677d329..99bcce6ab7 100644 --- a/src/shims/foreign_items.rs +++ b/src/shims/foreign_items.rs @@ -942,6 +942,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { this.write_scalar(Scalar::from_u64(res.to_bits()), dest)?; } + // LLVM intrinsics "llvm.prefetch" => { let [p, rw, loc, ty] = this.check_shim(abi, Abi::C { unwind: false }, link_name, args)?; @@ -968,8 +969,6 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { throw_unsup_format!("unsupported `llvm.prefetch` type argument: {}", ty); } } - - // Architecture-specific shims "llvm.x86.addcarry.64" if this.tcx.sess.target.arch == "x86_64" => { // Computes u8+u64+u64, returning tuple (u8,u64) comprising the output carry and truncated sum. let [c_in, a, b] = this.check_shim(abi, Abi::Unadjusted, link_name, args)?; From b57bc6d9ddfd3916ec12d299e4452bf4fc8c8691 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Eduardo=20S=C3=A1nchez=20Mu=C3=B1oz?= Date: Fri, 11 Aug 2023 17:58:09 +0200 Subject: [PATCH 03/13] Add checked float-to-int helper function --- src/helpers.rs | 65 ++++++++++++++++++- src/shims/intrinsics/mod.rs | 51 ++------------- src/shims/x86/sse.rs | 44 +++---------- .../intrinsics/float_to_int_64_neg.stderr | 4 +- 4 files changed, 81 insertions(+), 83 deletions(-) diff --git a/src/helpers.rs b/src/helpers.rs index ea1931f7a1..b5cc5c7e48 100644 --- a/src/helpers.rs +++ b/src/helpers.rs @@ -13,11 +13,11 @@ use rustc_index::IndexVec; use rustc_middle::mir; use rustc_middle::ty::{ self, - layout::{LayoutOf, TyAndLayout}, - List, TyCtxt, + layout::{IntegerExt as _, LayoutOf, TyAndLayout}, + List, Ty, TyCtxt, }; use rustc_span::{def_id::CrateNum, sym, Span, Symbol}; -use rustc_target::abi::{Align, FieldIdx, FieldsShape, Size, Variants}; +use rustc_target::abi::{Align, FieldIdx, FieldsShape, Integer, Size, Variants}; use rustc_target::spec::abi::Abi; use rand::RngCore; @@ -1011,6 +1011,65 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { None => tcx.item_name(def_id), } } + + /// Converts `f` to integer type `dest_ty` after rounding with mode `round`. + /// Returns `None` if `f` is NaN or out of range. + fn float_to_int_checked( + &self, + f: F, + dest_ty: Ty<'tcx>, + round: rustc_apfloat::Round, + ) -> Option> + where + F: rustc_apfloat::Float + Into>, + { + let this = self.eval_context_ref(); + + match dest_ty.kind() { + // Unsigned + ty::Uint(t) => { + let size = Integer::from_uint_ty(this, *t).size(); + let res = f.to_u128_r(size.bits_usize(), round, &mut false); + if res.status.intersects( + rustc_apfloat::Status::INVALID_OP + | rustc_apfloat::Status::OVERFLOW + | rustc_apfloat::Status::UNDERFLOW, + ) { + // Floating point value is NaN (flagged with INVALID_OP) or outside the range + // of values of the integer type (flagged with OVERFLOW or UNDERFLOW). + None + } else { + // Floating point value can be represented by the integer type after rounding. + // The INEXACT flag is ignored on purpose to allow rounding. + Some(Scalar::from_uint(res.value, size)) + } + } + // Signed + ty::Int(t) => { + let size = Integer::from_int_ty(this, *t).size(); + let res = f.to_i128_r(size.bits_usize(), round, &mut false); + if res.status.intersects( + rustc_apfloat::Status::INVALID_OP + | rustc_apfloat::Status::OVERFLOW + | rustc_apfloat::Status::UNDERFLOW, + ) { + // Floating point value is NaN (flagged with INVALID_OP) or outside the range + // of values of the integer type (flagged with OVERFLOW or UNDERFLOW). + None + } else { + // Floating point value can be represented by the integer type after rounding. + // The INEXACT flag is ignored on purpose to allow rounding. + Some(Scalar::from_int(res.value, size)) + } + } + // Nothing else + _ => + span_bug!( + this.cur_span(), + "attempted float-to-int conversion with non-int output type {dest_ty:?}" + ), + } + } } impl<'mir, 'tcx> MiriMachine<'mir, 'tcx> { diff --git a/src/shims/intrinsics/mod.rs b/src/shims/intrinsics/mod.rs index c900ced19c..f0b2850da0 100644 --- a/src/shims/intrinsics/mod.rs +++ b/src/shims/intrinsics/mod.rs @@ -6,12 +6,12 @@ use std::iter; use log::trace; use rustc_apfloat::{Float, Round}; -use rustc_middle::ty::layout::{IntegerExt, LayoutOf}; +use rustc_middle::ty::layout::LayoutOf; use rustc_middle::{ mir, ty::{self, FloatTy, Ty}, }; -use rustc_target::abi::{Integer, Size}; +use rustc_target::abi::Size; use crate::*; use atomic::EvalContextExt as _; @@ -393,47 +393,10 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { F: Float + Into>, { let this = self.eval_context_ref(); - - // Step 1: cut off the fractional part of `f`. The result of this is - // guaranteed to be precisely representable in IEEE floats. - let f = f.round_to_integral(Round::TowardZero).value; - - // Step 2: Cast the truncated float to the target integer type and see if we lose any information in this step. - Ok(match dest_ty.kind() { - // Unsigned - ty::Uint(t) => { - let size = Integer::from_uint_ty(this, *t).size(); - let res = f.to_u128(size.bits_usize()); - if res.status.is_empty() { - // No status flags means there was no further rounding or other loss of precision. - Scalar::from_uint(res.value, size) - } else { - // `f` was not representable in this integer type. - throw_ub_format!( - "`float_to_int_unchecked` intrinsic called on {f} which cannot be represented in target type `{dest_ty:?}`", - ); - } - } - // Signed - ty::Int(t) => { - let size = Integer::from_int_ty(this, *t).size(); - let res = f.to_i128(size.bits_usize()); - if res.status.is_empty() { - // No status flags means there was no further rounding or other loss of precision. - Scalar::from_int(res.value, size) - } else { - // `f` was not representable in this integer type. - throw_ub_format!( - "`float_to_int_unchecked` intrinsic called on {f} which cannot be represented in target type `{dest_ty:?}`", - ); - } - } - // Nothing else - _ => - span_bug!( - this.cur_span(), - "`float_to_int_unchecked` called with non-int output type {dest_ty:?}" - ), - }) + Ok(this + .float_to_int_checked(f, dest_ty, Round::TowardZero) + .ok_or_else(|| err_ub_format!( + "`float_to_int_unchecked` intrinsic called on {f} which cannot be represented in target type `{dest_ty:?}`", + ))?) } } diff --git a/src/shims/x86/sse.rs b/src/shims/x86/sse.rs index deae0875fb..b18441bb40 100644 --- a/src/shims/x86/sse.rs +++ b/src/shims/x86/sse.rs @@ -195,24 +195,12 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { _ => unreachable!(), }; - let mut exact = false; - let cvt = op.to_i128_r(32, rnd, &mut exact); - let res = if cvt.status.intersects( - rustc_apfloat::Status::INVALID_OP - | rustc_apfloat::Status::OVERFLOW - | rustc_apfloat::Status::UNDERFLOW, - ) { - // Input is NaN (flagged with INVALID_OP) or does not fit - // in an i32 (flagged with OVERFLOW or UNDERFLOW), fallback - // to minimum acording to SSE semantics. The INEXACT flag - // is ignored on purpose because rounding can happen during - // float-to-int conversion. - i32::MIN - } else { - i32::try_from(cvt.value).unwrap() - }; + let res = this.float_to_int_checked(op, dest.layout.ty, rnd).unwrap_or_else(|| { + // Fallback to minimum acording to SSE semantics. + Scalar::from_i32(i32::MIN) + }); - this.write_scalar(Scalar::from_i32(res), dest)?; + this.write_scalar(res, dest)?; } // Use to implement _mm_cvtss_si64 and _mm_cvttss_si64. // Converts the first component of `op` from f32 to i64. @@ -232,24 +220,12 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { _ => unreachable!(), }; - let mut exact = false; - let cvt = op.to_i128_r(64, rnd, &mut exact); - let res = if cvt.status.intersects( - rustc_apfloat::Status::INVALID_OP - | rustc_apfloat::Status::OVERFLOW - | rustc_apfloat::Status::UNDERFLOW, - ) { - // Input is NaN (flagged with INVALID_OP) or does not fit - // in an i64 (flagged with OVERFLOW or UNDERFLOW), fallback - // to minimum acording to SSE semantics. The INEXACT flag - // is ignored on purpose because rounding can happen during - // float-to-int conversion. - i64::MIN - } else { - i64::try_from(cvt.value).unwrap() - }; + let res = this.float_to_int_checked(op, dest.layout.ty, rnd).unwrap_or_else(|| { + // Fallback to minimum acording to SSE semantics. + Scalar::from_i64(i64::MIN) + }); - this.write_scalar(Scalar::from_i64(res), dest)?; + this.write_scalar(res, dest)?; } // Used to implement the _mm_cvtsi32_ss function. // Converts `right` from i32 to f32. Returns a SIMD vector with diff --git a/tests/fail/intrinsics/float_to_int_64_neg.stderr b/tests/fail/intrinsics/float_to_int_64_neg.stderr index 7e24f45f65..ddf3249d05 100644 --- a/tests/fail/intrinsics/float_to_int_64_neg.stderr +++ b/tests/fail/intrinsics/float_to_int_64_neg.stderr @@ -1,8 +1,8 @@ -error: Undefined Behavior: `float_to_int_unchecked` intrinsic called on -1 which cannot be represented in target type `u128` +error: Undefined Behavior: `float_to_int_unchecked` intrinsic called on -1.0000000000000999 which cannot be represented in target type `u128` --> $DIR/float_to_int_64_neg.rs:LL:CC | LL | float_to_int_unchecked::(-1.0000000000001f64); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `float_to_int_unchecked` intrinsic called on -1 which cannot be represented in target type `u128` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `float_to_int_unchecked` intrinsic called on -1.0000000000000999 which cannot be represented in target type `u128` | = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information From 514143658e7c01aaf9d4ee6d5eced181fb77187f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Eduardo=20S=C3=A1nchez=20Mu=C3=B1oz?= Date: Fri, 11 Aug 2023 20:23:08 +0200 Subject: [PATCH 04/13] Remove `float_to_int_unchecked` and inline it into its call sites --- src/shims/intrinsics/mod.rs | 44 ++++++++--------- src/shims/intrinsics/simd.rs | 47 ++++++++++++------- .../fail/intrinsics/simd-float-to-int.stderr | 4 +- 3 files changed, 56 insertions(+), 39 deletions(-) diff --git a/src/shims/intrinsics/mod.rs b/src/shims/intrinsics/mod.rs index f0b2850da0..b2c297fe54 100644 --- a/src/shims/intrinsics/mod.rs +++ b/src/shims/intrinsics/mod.rs @@ -9,7 +9,7 @@ use rustc_apfloat::{Float, Round}; use rustc_middle::ty::layout::LayoutOf; use rustc_middle::{ mir, - ty::{self, FloatTy, Ty}, + ty::{self, FloatTy}, }; use rustc_target::abi::Size; @@ -356,10 +356,28 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { let val = this.read_immediate(val)?; let res = match val.layout.ty.kind() { - ty::Float(FloatTy::F32) => - this.float_to_int_unchecked(val.to_scalar().to_f32()?, dest.layout.ty)?, - ty::Float(FloatTy::F64) => - this.float_to_int_unchecked(val.to_scalar().to_f64()?, dest.layout.ty)?, + ty::Float(FloatTy::F32) => { + let f = val.to_scalar().to_f32()?; + this + .float_to_int_checked(f, dest.layout.ty, Round::TowardZero) + .ok_or_else(|| { + err_ub_format!( + "`float_to_int_unchecked` intrinsic called on {f} which cannot be represented in target type `{:?}`", + dest.layout.ty + ) + })? + } + ty::Float(FloatTy::F64) => { + let f = val.to_scalar().to_f64()?; + this + .float_to_int_checked(f, dest.layout.ty, Round::TowardZero) + .ok_or_else(|| { + err_ub_format!( + "`float_to_int_unchecked` intrinsic called on {f} which cannot be represented in target type `{:?}`", + dest.layout.ty + ) + })? + } _ => span_bug!( this.cur_span(), @@ -383,20 +401,4 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { Ok(()) } - - fn float_to_int_unchecked( - &self, - f: F, - dest_ty: Ty<'tcx>, - ) -> InterpResult<'tcx, Scalar> - where - F: Float + Into>, - { - let this = self.eval_context_ref(); - Ok(this - .float_to_int_checked(f, dest_ty, Round::TowardZero) - .ok_or_else(|| err_ub_format!( - "`float_to_int_unchecked` intrinsic called on {f} which cannot be represented in target type `{dest_ty:?}`", - ))?) - } } diff --git a/src/shims/intrinsics/simd.rs b/src/shims/intrinsics/simd.rs index b6225713cd..dd8c4a4f6e 100644 --- a/src/shims/intrinsics/simd.rs +++ b/src/shims/intrinsics/simd.rs @@ -1,4 +1,4 @@ -use rustc_apfloat::Float; +use rustc_apfloat::{Float, Round}; use rustc_middle::ty::layout::{HasParamEnv, LayoutOf}; use rustc_middle::{mir, ty, ty::FloatTy}; use rustc_target::abi::{Endian, HasDataLayout, Size}; @@ -420,7 +420,6 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { } } } - #[rustfmt::skip] "cast" | "as" | "cast_ptr" | "expose_addr" | "from_exposed_addr" => { let [op] = check_arg_count(args)?; let (op, op_len) = this.operand_to_simd(op)?; @@ -440,7 +439,8 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { let val = match (op.layout.ty.kind(), dest.layout.ty.kind()) { // Int-to-(int|float): always safe - (ty::Int(_) | ty::Uint(_), ty::Int(_) | ty::Uint(_) | ty::Float(_)) if safe_cast || unsafe_cast => + (ty::Int(_) | ty::Uint(_), ty::Int(_) | ty::Uint(_) | ty::Float(_)) + if safe_cast || unsafe_cast => this.int_to_int_or_float(&op, dest.layout.ty)?, // Float-to-float: always safe (ty::Float(_), ty::Float(_)) if safe_cast || unsafe_cast => @@ -449,21 +449,36 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { (ty::Float(_), ty::Int(_) | ty::Uint(_)) if safe_cast => this.float_to_float_or_int(&op, dest.layout.ty)?, // Float-to-int in unchecked mode - (ty::Float(FloatTy::F32), ty::Int(_) | ty::Uint(_)) if unsafe_cast => - this.float_to_int_unchecked(op.to_scalar().to_f32()?, dest.layout.ty)?.into(), - (ty::Float(FloatTy::F64), ty::Int(_) | ty::Uint(_)) if unsafe_cast => - this.float_to_int_unchecked(op.to_scalar().to_f64()?, dest.layout.ty)?.into(), - // Ptr-to-ptr cast - (ty::RawPtr(..), ty::RawPtr(..)) if ptr_cast => { - this.ptr_to_ptr(&op, dest.layout.ty)? - } - // Ptr/Int casts - (ty::RawPtr(..), ty::Int(_) | ty::Uint(_)) if expose_cast => { - this.pointer_expose_address_cast(&op, dest.layout.ty)? + (ty::Float(FloatTy::F32), ty::Int(_) | ty::Uint(_)) if unsafe_cast => { + let f = op.to_scalar().to_f32()?; + this.float_to_int_checked(f, dest.layout.ty, Round::TowardZero) + .ok_or_else(|| { + err_ub_format!( + "`simd_cast` intrinsic called on {f} which cannot be represented in target type `{:?}`", + dest.layout.ty + ) + })? + .into() } - (ty::Int(_) | ty::Uint(_), ty::RawPtr(..)) if from_exposed_cast => { - this.pointer_from_exposed_address_cast(&op, dest.layout.ty)? + (ty::Float(FloatTy::F64), ty::Int(_) | ty::Uint(_)) if unsafe_cast => { + let f = op.to_scalar().to_f64()?; + this.float_to_int_checked(f, dest.layout.ty, Round::TowardZero) + .ok_or_else(|| { + err_ub_format!( + "`simd_cast` intrinsic called on {f} which cannot be represented in target type `{:?}`", + dest.layout.ty + ) + })? + .into() } + // Ptr-to-ptr cast + (ty::RawPtr(..), ty::RawPtr(..)) if ptr_cast => + this.ptr_to_ptr(&op, dest.layout.ty)?, + // Ptr/Int casts + (ty::RawPtr(..), ty::Int(_) | ty::Uint(_)) if expose_cast => + this.pointer_expose_address_cast(&op, dest.layout.ty)?, + (ty::Int(_) | ty::Uint(_), ty::RawPtr(..)) if from_exposed_cast => + this.pointer_from_exposed_address_cast(&op, dest.layout.ty)?, // Error otherwise _ => throw_unsup_format!( diff --git a/tests/fail/intrinsics/simd-float-to-int.stderr b/tests/fail/intrinsics/simd-float-to-int.stderr index ea5ad62aea..7b2387944a 100644 --- a/tests/fail/intrinsics/simd-float-to-int.stderr +++ b/tests/fail/intrinsics/simd-float-to-int.stderr @@ -1,8 +1,8 @@ -error: Undefined Behavior: `float_to_int_unchecked` intrinsic called on 3.40282347E+38 which cannot be represented in target type `i32` +error: Undefined Behavior: `simd_cast` intrinsic called on 3.40282347E+38 which cannot be represented in target type `i32` --> $DIR/simd-float-to-int.rs:LL:CC | LL | let _x: i32x2 = f32x2::from_array([f32::MAX, f32::MIN]).to_int_unchecked(); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `float_to_int_unchecked` intrinsic called on 3.40282347E+38 which cannot be represented in target type `i32` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `simd_cast` intrinsic called on 3.40282347E+38 which cannot be represented in target type `i32` | = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information From 07b5601d3b714ac630fac31d9b447768aed0a5ad Mon Sep 17 00:00:00 2001 From: Ben Kimock Date: Sun, 25 Jun 2023 00:55:09 -0400 Subject: [PATCH 05/13] Include spans in use-after-free diagnostics --- src/diagnostics.rs | 16 ++++-- src/machine.rs | 49 ++++++++++++++++++- tests/fail/alloc/deallocate-twice.stderr | 12 ++++- .../fail/alloc/reallocate-change-alloc.stderr | 12 ++++- tests/fail/alloc/reallocate-dangling.stderr | 12 ++++- .../dangling_pointer_addr_of.stderr | 12 ++++- .../dangling_pointer_deref.stderr | 12 ++++- .../dangling_pointer_offset.stderr | 12 ++++- ...dangling_pointer_project_underscore.stderr | 12 ++++- .../dangling_zst_deref.stderr | 12 ++++- .../fail/data_race/dealloc_read_race2.stderr | 16 +++++- .../fail/data_race/dealloc_write_race2.stderr | 16 +++++- tests/fail/generator-pinned-moved.stderr | 12 ++++- tests/fail/rc_as_ptr.stderr | 12 ++++- tests/fail/shims/mmap_use_after_munmap.stderr | 19 ++++++- tests/fail/zst2.stderr | 12 ++++- 16 files changed, 230 insertions(+), 18 deletions(-) diff --git a/src/diagnostics.rs b/src/diagnostics.rs index 6bd4be91e5..4f1ff15b35 100644 --- a/src/diagnostics.rs +++ b/src/diagnostics.rs @@ -304,11 +304,21 @@ pub fn report_error<'tcx, 'mir>( (None, format!("this usually indicates that your program performed an invalid operation and caused Undefined Behavior")), (None, format!("but due to `-Zmiri-symbolic-alignment-check`, alignment errors can also be false positives")), ], - UndefinedBehavior(_) => - vec![ + UndefinedBehavior(info) => { + let mut helps = vec![ (None, format!("this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior")), (None, format!("see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information")), - ], + ]; + if let UndefinedBehaviorInfo::PointerUseAfterFree(alloc_id, _) = info { + if let Some(span) = ecx.machine.allocated_span(*alloc_id) { + helps.push((Some(span), format!("{:?} was allocated here:", alloc_id))); + } + if let Some(span) = ecx.machine.deallocated_span(*alloc_id) { + helps.push((Some(span), format!("{:?} was deallocated here:", alloc_id))); + } + } + helps + } InvalidProgram( InvalidProgramInfo::AlreadyReported(_) ) => { diff --git a/src/machine.rs b/src/machine.rs index e19be417b2..5fc3a5faeb 100644 --- a/src/machine.rs +++ b/src/machine.rs @@ -25,7 +25,7 @@ use rustc_middle::{ }, }; use rustc_span::def_id::{CrateNum, DefId}; -use rustc_span::Symbol; +use rustc_span::{Span, SpanData, Symbol}; use rustc_target::abi::{Align, Size}; use rustc_target::spec::abi::Abi; @@ -135,6 +135,17 @@ impl MayLeak for MiriMemoryKind { } } +impl MiriMemoryKind { + /// Whether we have a useful allocation span for an allocation of this kind. + fn should_save_allocation_span(self) -> bool { + use self::MiriMemoryKind::*; + match self { + Rust | Miri | C | Mmap => true, + Machine | Global | ExternStatic | Tls | WinHeap | Runtime => false, + } + } +} + impl fmt::Display for MiriMemoryKind { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { use self::MiriMemoryKind::*; @@ -497,6 +508,10 @@ pub struct MiriMachine<'mir, 'tcx> { /// Whether to collect a backtrace when each allocation is created, just in case it leaks. pub(crate) collect_leak_backtraces: bool, + + /// The spans we will use to report where an allocation was created and deallocated in + /// diagnostics. + pub(crate) allocation_spans: RefCell, Option)>>, } impl<'mir, 'tcx> MiriMachine<'mir, 'tcx> { @@ -621,6 +636,7 @@ impl<'mir, 'tcx> MiriMachine<'mir, 'tcx> { stack_addr, stack_size, collect_leak_backtraces: config.collect_leak_backtraces, + allocation_spans: RefCell::new(FxHashMap::default()), } } @@ -742,6 +758,22 @@ impl<'mir, 'tcx> MiriMachine<'mir, 'tcx> { pub(crate) fn page_align(&self) -> Align { Align::from_bytes(self.page_size).unwrap() } + + pub(crate) fn allocated_span(&self, alloc_id: AllocId) -> Option { + self.allocation_spans + .borrow() + .get(&alloc_id) + .and_then(|(allocated, _deallocated)| *allocated) + .map(Span::data) + } + + pub(crate) fn deallocated_span(&self, alloc_id: AllocId) -> Option { + self.allocation_spans + .borrow() + .get(&alloc_id) + .and_then(|(_allocated, deallocated)| *deallocated) + .map(Span::data) + } } impl VisitTags for MiriMachine<'_, '_> { @@ -791,6 +823,7 @@ impl VisitTags for MiriMachine<'_, '_> { stack_addr: _, stack_size: _, collect_leak_backtraces: _, + allocation_spans: _, } = self; threads.visit_tags(visit); @@ -1051,6 +1084,16 @@ impl<'mir, 'tcx> Machine<'mir, 'tcx> for MiriMachine<'mir, 'tcx> { }, |ptr| ecx.global_base_pointer(ptr), )?; + + if let MemoryKind::Machine(kind) = kind { + if kind.should_save_allocation_span() { + ecx.machine + .allocation_spans + .borrow_mut() + .insert(id, (Some(ecx.machine.current_span()), None)); + } + } + Ok(Cow::Owned(alloc)) } @@ -1181,6 +1224,10 @@ impl<'mir, 'tcx> Machine<'mir, 'tcx> for MiriMachine<'mir, 'tcx> { if let Some(borrow_tracker) = &mut alloc_extra.borrow_tracker { borrow_tracker.before_memory_deallocation(alloc_id, prove_extra, range, machine)?; } + if let Some((_, deallocated_at)) = machine.allocation_spans.borrow_mut().get_mut(&alloc_id) + { + *deallocated_at = Some(machine.current_span()); + } Ok(()) } diff --git a/tests/fail/alloc/deallocate-twice.stderr b/tests/fail/alloc/deallocate-twice.stderr index 23d145e7d3..48d63e5905 100644 --- a/tests/fail/alloc/deallocate-twice.stderr +++ b/tests/fail/alloc/deallocate-twice.stderr @@ -6,7 +6,17 @@ LL | unsafe { __rust_dealloc(ptr, layout.size(), layout.align()) } | = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information - = note: BACKTRACE: +help: ALLOC was allocated here: + --> $DIR/deallocate-twice.rs:LL:CC + | +LL | let x = alloc(Layout::from_size_align_unchecked(1, 1)); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +help: ALLOC was deallocated here: + --> $DIR/deallocate-twice.rs:LL:CC + | +LL | dealloc(x, Layout::from_size_align_unchecked(1, 1)); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + = note: BACKTRACE (of the first span): = note: inside `std::alloc::dealloc` at RUSTLIB/alloc/src/alloc.rs:LL:CC note: inside `main` --> $DIR/deallocate-twice.rs:LL:CC diff --git a/tests/fail/alloc/reallocate-change-alloc.stderr b/tests/fail/alloc/reallocate-change-alloc.stderr index 7c7cec211b..ff4cb39915 100644 --- a/tests/fail/alloc/reallocate-change-alloc.stderr +++ b/tests/fail/alloc/reallocate-change-alloc.stderr @@ -6,7 +6,17 @@ LL | let _z = *x; | = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information - = note: BACKTRACE: +help: ALLOC was allocated here: + --> $DIR/reallocate-change-alloc.rs:LL:CC + | +LL | let x = alloc(Layout::from_size_align_unchecked(1, 1)); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +help: ALLOC was deallocated here: + --> $DIR/reallocate-change-alloc.rs:LL:CC + | +LL | let _y = realloc(x, Layout::from_size_align_unchecked(1, 1), 1); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + = note: BACKTRACE (of the first span): = note: inside `main` at $DIR/reallocate-change-alloc.rs:LL:CC note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace diff --git a/tests/fail/alloc/reallocate-dangling.stderr b/tests/fail/alloc/reallocate-dangling.stderr index 9c22215471..52cc579c1e 100644 --- a/tests/fail/alloc/reallocate-dangling.stderr +++ b/tests/fail/alloc/reallocate-dangling.stderr @@ -6,7 +6,17 @@ LL | unsafe { __rust_realloc(ptr, layout.size(), layout.align(), new_size) } | = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information - = note: BACKTRACE: +help: ALLOC was allocated here: + --> $DIR/reallocate-dangling.rs:LL:CC + | +LL | let x = alloc(Layout::from_size_align_unchecked(1, 1)); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +help: ALLOC was deallocated here: + --> $DIR/reallocate-dangling.rs:LL:CC + | +LL | dealloc(x, Layout::from_size_align_unchecked(1, 1)); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + = note: BACKTRACE (of the first span): = note: inside `std::alloc::realloc` at RUSTLIB/alloc/src/alloc.rs:LL:CC note: inside `main` --> $DIR/reallocate-dangling.rs:LL:CC diff --git a/tests/fail/dangling_pointers/dangling_pointer_addr_of.stderr b/tests/fail/dangling_pointers/dangling_pointer_addr_of.stderr index 398f216e73..6a3efbdd3d 100644 --- a/tests/fail/dangling_pointers/dangling_pointer_addr_of.stderr +++ b/tests/fail/dangling_pointers/dangling_pointer_addr_of.stderr @@ -6,7 +6,17 @@ LL | let x = unsafe { ptr::addr_of!(*p) }; | = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information - = note: BACKTRACE: +help: ALLOC was allocated here: + --> $DIR/dangling_pointer_addr_of.rs:LL:CC + | +LL | let b = Box::new(42); + | ^^^^^^^^^^^^ +help: ALLOC was deallocated here: + --> $DIR/dangling_pointer_addr_of.rs:LL:CC + | +LL | }; + | ^ + = note: BACKTRACE (of the first span): = note: inside `main` at RUSTLIB/core/src/ptr/mod.rs:LL:CC = note: this error originates in the macro `ptr::addr_of` (in Nightly builds, run with -Z macro-backtrace for more info) diff --git a/tests/fail/dangling_pointers/dangling_pointer_deref.stderr b/tests/fail/dangling_pointers/dangling_pointer_deref.stderr index cb95d71a60..fad4b4be28 100644 --- a/tests/fail/dangling_pointers/dangling_pointer_deref.stderr +++ b/tests/fail/dangling_pointers/dangling_pointer_deref.stderr @@ -6,7 +6,17 @@ LL | let x = unsafe { *p }; | = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information - = note: BACKTRACE: +help: ALLOC was allocated here: + --> $DIR/dangling_pointer_deref.rs:LL:CC + | +LL | let b = Box::new(42); + | ^^^^^^^^^^^^ +help: ALLOC was deallocated here: + --> $DIR/dangling_pointer_deref.rs:LL:CC + | +LL | }; + | ^ + = note: BACKTRACE (of the first span): = note: inside `main` at $DIR/dangling_pointer_deref.rs:LL:CC note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace diff --git a/tests/fail/dangling_pointers/dangling_pointer_offset.stderr b/tests/fail/dangling_pointers/dangling_pointer_offset.stderr index 85bd2bed9c..7ef5fd329a 100644 --- a/tests/fail/dangling_pointers/dangling_pointer_offset.stderr +++ b/tests/fail/dangling_pointers/dangling_pointer_offset.stderr @@ -6,7 +6,17 @@ LL | let x = unsafe { p.offset(42) }; | = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information - = note: BACKTRACE: +help: ALLOC was allocated here: + --> $DIR/dangling_pointer_offset.rs:LL:CC + | +LL | let b = Box::new(42); + | ^^^^^^^^^^^^ +help: ALLOC was deallocated here: + --> $DIR/dangling_pointer_offset.rs:LL:CC + | +LL | }; + | ^ + = note: BACKTRACE (of the first span): = note: inside `main` at $DIR/dangling_pointer_offset.rs:LL:CC note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace diff --git a/tests/fail/dangling_pointers/dangling_pointer_project_underscore.stderr b/tests/fail/dangling_pointers/dangling_pointer_project_underscore.stderr index f2d58fe769..1de6465802 100644 --- a/tests/fail/dangling_pointers/dangling_pointer_project_underscore.stderr +++ b/tests/fail/dangling_pointers/dangling_pointer_project_underscore.stderr @@ -6,7 +6,17 @@ LL | let _ = *p; | = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information - = note: BACKTRACE: +help: ALLOC was allocated here: + --> $DIR/dangling_pointer_project_underscore.rs:LL:CC + | +LL | let b = Box::new(42); + | ^^^^^^^^^^^^ +help: ALLOC was deallocated here: + --> $DIR/dangling_pointer_project_underscore.rs:LL:CC + | +LL | }; + | ^ + = note: BACKTRACE (of the first span): = note: inside `main` at $DIR/dangling_pointer_project_underscore.rs:LL:CC note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace diff --git a/tests/fail/dangling_pointers/dangling_zst_deref.stderr b/tests/fail/dangling_pointers/dangling_zst_deref.stderr index c15f17f3b8..bf6ee775e9 100644 --- a/tests/fail/dangling_pointers/dangling_zst_deref.stderr +++ b/tests/fail/dangling_pointers/dangling_zst_deref.stderr @@ -6,7 +6,17 @@ LL | let _x = unsafe { *p }; | = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information - = note: BACKTRACE: +help: ALLOC was allocated here: + --> $DIR/dangling_zst_deref.rs:LL:CC + | +LL | let b = Box::new(42); + | ^^^^^^^^^^^^ +help: ALLOC was deallocated here: + --> $DIR/dangling_zst_deref.rs:LL:CC + | +LL | }; + | ^ + = note: BACKTRACE (of the first span): = note: inside `main` at $DIR/dangling_zst_deref.rs:LL:CC note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace diff --git a/tests/fail/data_race/dealloc_read_race2.stderr b/tests/fail/data_race/dealloc_read_race2.stderr index 4efc35c15e..810e48d59c 100644 --- a/tests/fail/data_race/dealloc_read_race2.stderr +++ b/tests/fail/data_race/dealloc_read_race2.stderr @@ -6,7 +6,21 @@ LL | *ptr.0 | = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information - = note: BACKTRACE: +help: ALLOC was allocated here: + --> $DIR/dealloc_read_race2.rs:LL:CC + | +LL | let pointer: *mut usize = Box::into_raw(Box::new(0usize)); + | ^^^^^^^^^^^^^^^^ +help: ALLOC was deallocated here: + --> $DIR/dealloc_read_race2.rs:LL:CC + | +LL | / __rust_dealloc( +LL | | ptr.0 as *mut _, +LL | | std::mem::size_of::(), +LL | | std::mem::align_of::(), +LL | | ) + | |_____________^ + = note: BACKTRACE (of the first span): = note: inside closure at $DIR/dealloc_read_race2.rs:LL:CC note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace diff --git a/tests/fail/data_race/dealloc_write_race2.stderr b/tests/fail/data_race/dealloc_write_race2.stderr index fad525830e..7d672cd4d6 100644 --- a/tests/fail/data_race/dealloc_write_race2.stderr +++ b/tests/fail/data_race/dealloc_write_race2.stderr @@ -6,7 +6,21 @@ LL | *ptr.0 = 2; | = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information - = note: BACKTRACE: +help: ALLOC was allocated here: + --> $DIR/dealloc_write_race2.rs:LL:CC + | +LL | let pointer: *mut usize = Box::into_raw(Box::new(0usize)); + | ^^^^^^^^^^^^^^^^ +help: ALLOC was deallocated here: + --> $DIR/dealloc_write_race2.rs:LL:CC + | +LL | / __rust_dealloc( +LL | | ptr.0 as *mut _, +LL | | std::mem::size_of::(), +LL | | std::mem::align_of::(), +LL | | ); + | |_____________^ + = note: BACKTRACE (of the first span): = note: inside closure at $DIR/dealloc_write_race2.rs:LL:CC note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace diff --git a/tests/fail/generator-pinned-moved.stderr b/tests/fail/generator-pinned-moved.stderr index 3eb17f0558..e29e352e64 100644 --- a/tests/fail/generator-pinned-moved.stderr +++ b/tests/fail/generator-pinned-moved.stderr @@ -6,7 +6,17 @@ LL | *num += 1; | = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information - = note: BACKTRACE: +help: ALLOC was allocated here: + --> $DIR/generator-pinned-moved.rs:LL:CC + | +LL | let mut generator_iterator = Box::new(GeneratorIteratorAdapter(firstn())); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +help: ALLOC was deallocated here: + --> $DIR/generator-pinned-moved.rs:LL:CC + | +LL | }; // *deallocate* generator_iterator + | ^ + = note: BACKTRACE (of the first span): = note: inside closure at $DIR/generator-pinned-moved.rs:LL:CC note: inside ` as std::iter::Iterator>::next` --> $DIR/generator-pinned-moved.rs:LL:CC diff --git a/tests/fail/rc_as_ptr.stderr b/tests/fail/rc_as_ptr.stderr index 129916ac73..460ed97713 100644 --- a/tests/fail/rc_as_ptr.stderr +++ b/tests/fail/rc_as_ptr.stderr @@ -6,7 +6,17 @@ LL | assert_eq!(42, **unsafe { &*Weak::as_ptr(&weak) }); | = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information - = note: BACKTRACE: +help: ALLOC was allocated here: + --> $DIR/rc_as_ptr.rs:LL:CC + | +LL | let strong = Rc::new(Box::new(42)); + | ^^^^^^^^^^^^ +help: ALLOC was deallocated here: + --> $DIR/rc_as_ptr.rs:LL:CC + | +LL | drop(strong); + | ^^^^^^^^^^^^ + = note: BACKTRACE (of the first span): = note: inside `main` at RUSTLIB/core/src/macros/mod.rs:LL:CC = note: this error originates in the macro `assert_eq` (in Nightly builds, run with -Z macro-backtrace for more info) diff --git a/tests/fail/shims/mmap_use_after_munmap.stderr b/tests/fail/shims/mmap_use_after_munmap.stderr index 8b9969da8f..44e122330b 100644 --- a/tests/fail/shims/mmap_use_after_munmap.stderr +++ b/tests/fail/shims/mmap_use_after_munmap.stderr @@ -21,7 +21,24 @@ LL | let _x = *(ptr as *mut u8); | = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information - = note: BACKTRACE: +help: ALLOC was allocated here: + --> $DIR/mmap_use_after_munmap.rs:LL:CC + | +LL | let ptr = libc::mmap( + | ___________________^ +LL | | std::ptr::null_mut(), +LL | | 4096, +LL | | libc::PROT_READ | libc::PROT_WRITE, +... | +LL | | 0, +LL | | ); + | |_________^ +help: ALLOC was deallocated here: + --> $DIR/mmap_use_after_munmap.rs:LL:CC + | +LL | libc::munmap(ptr, 4096); + | ^^^^^^^^^^^^^^^^^^^^^^^ + = note: BACKTRACE (of the first span): = note: inside `main` at $DIR/mmap_use_after_munmap.rs:LL:CC note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace diff --git a/tests/fail/zst2.stderr b/tests/fail/zst2.stderr index 63f40ed206..49954b1fd1 100644 --- a/tests/fail/zst2.stderr +++ b/tests/fail/zst2.stderr @@ -6,7 +6,17 @@ LL | unsafe { *x = zst_val }; | = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information - = note: BACKTRACE: +help: ALLOC was allocated here: + --> $DIR/zst2.rs:LL:CC + | +LL | let mut x_box = Box::new(1u8); + | ^^^^^^^^^^^^^ +help: ALLOC was deallocated here: + --> $DIR/zst2.rs:LL:CC + | +LL | drop(x_box); + | ^^^^^^^^^^^ + = note: BACKTRACE (of the first span): = note: inside `main` at $DIR/zst2.rs:LL:CC note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace From 02fc2cf15d80461fd400f722b02c6157e3302cf2 Mon Sep 17 00:00:00 2001 From: The Miri Conjob Bot Date: Sat, 12 Aug 2023 05:25:32 +0000 Subject: [PATCH 06/13] Preparing for merge from rustc --- rust-version | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rust-version b/rust-version index 22a3d43986..52f98965ec 100644 --- a/rust-version +++ b/rust-version @@ -1 +1 @@ -9fa6bdd764a1f7bdf69eccceeace6d13f38cb2e1 +b08dd92552d663e3c877c8e5ce859e212205a09f From a71aef164fbeeb3df09619ebc6263d997f50ed1a Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sun, 9 Jul 2023 16:21:48 +0200 Subject: [PATCH 07/13] C string function shims: consistently treat "invalid" pointers as UB --- src/shims/foreign_items.rs | 12 ++++++++++++ tests/fail/shims/memchr_null.stderr | 4 ++-- tests/fail/shims/memcmp_null.stderr | 4 ++-- tests/fail/shims/memcmp_zero.rs | 13 +++++++++++++ tests/fail/shims/memcmp_zero.stderr | 15 +++++++++++++++ tests/fail/shims/memrchr_null.stderr | 4 ++-- 6 files changed, 46 insertions(+), 6 deletions(-) create mode 100644 tests/fail/shims/memcmp_zero.rs create mode 100644 tests/fail/shims/memcmp_zero.stderr diff --git a/src/shims/foreign_items.rs b/src/shims/foreign_items.rs index 99bcce6ab7..7996e72615 100644 --- a/src/shims/foreign_items.rs +++ b/src/shims/foreign_items.rs @@ -690,6 +690,10 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { let right = this.read_pointer(right)?; let n = Size::from_bytes(this.read_target_usize(n)?); + // C requires that this must always be a valid pointer (C18 §7.1.4). + this.ptr_get_alloc_id(left)?; + this.ptr_get_alloc_id(right)?; + let result = { let left_bytes = this.read_bytes_ptr_strip_provenance(left, n)?; let right_bytes = this.read_bytes_ptr_strip_provenance(right, n)?; @@ -714,6 +718,9 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { #[allow(clippy::cast_sign_loss, clippy::cast_possible_truncation)] let val = val as u8; + // C requires that this must always be a valid pointer (C18 §7.1.4). + this.ptr_get_alloc_id(ptr)?; + if let Some(idx) = this .read_bytes_ptr_strip_provenance(ptr, Size::from_bytes(num))? .iter() @@ -738,6 +745,9 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { #[allow(clippy::cast_sign_loss, clippy::cast_possible_truncation)] let val = val as u8; + // C requires that this must always be a valid pointer (C18 §7.1.4). + this.ptr_get_alloc_id(ptr)?; + let idx = this .read_bytes_ptr_strip_provenance(ptr, Size::from_bytes(num))? .iter() @@ -752,6 +762,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { "strlen" => { let [ptr] = this.check_shim(abi, Abi::C { unwind: false }, link_name, args)?; let ptr = this.read_pointer(ptr)?; + // This reads at least 1 byte, so we are already enforcing that this is a valid pointer. let n = this.read_c_str(ptr)?.len(); this.write_scalar( Scalar::from_target_usize(u64::try_from(n).unwrap(), this), @@ -791,6 +802,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { // pointer provenance is preserved by this implementation of `strcpy`. // That is probably overly cautious, but there also is no fundamental // reason to have `strcpy` destroy pointer provenance. + // This reads at least 1 byte, so we are already enforcing that this is a valid pointer. let n = this.read_c_str(ptr_src)?.len().checked_add(1).unwrap(); this.mem_copy( ptr_src, diff --git a/tests/fail/shims/memchr_null.stderr b/tests/fail/shims/memchr_null.stderr index d48606f34a..54b58f22c6 100644 --- a/tests/fail/shims/memchr_null.stderr +++ b/tests/fail/shims/memchr_null.stderr @@ -1,8 +1,8 @@ -error: Undefined Behavior: memory access failed: null pointer is a dangling pointer (it has no provenance) +error: Undefined Behavior: out-of-bounds pointer use: null pointer is a dangling pointer (it has no provenance) --> $DIR/memchr_null.rs:LL:CC | LL | libc::memchr(ptr::null(), 0, 0); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ memory access failed: null pointer is a dangling pointer (it has no provenance) + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ out-of-bounds pointer use: null pointer is a dangling pointer (it has no provenance) | = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information diff --git a/tests/fail/shims/memcmp_null.stderr b/tests/fail/shims/memcmp_null.stderr index 7a09c77989..8b2882fc24 100644 --- a/tests/fail/shims/memcmp_null.stderr +++ b/tests/fail/shims/memcmp_null.stderr @@ -1,8 +1,8 @@ -error: Undefined Behavior: memory access failed: null pointer is a dangling pointer (it has no provenance) +error: Undefined Behavior: out-of-bounds pointer use: null pointer is a dangling pointer (it has no provenance) --> $DIR/memcmp_null.rs:LL:CC | LL | libc::memcmp(ptr::null(), ptr::null(), 0); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ memory access failed: null pointer is a dangling pointer (it has no provenance) + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ out-of-bounds pointer use: null pointer is a dangling pointer (it has no provenance) | = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information diff --git a/tests/fail/shims/memcmp_zero.rs b/tests/fail/shims/memcmp_zero.rs new file mode 100644 index 0000000000..f2ddc20056 --- /dev/null +++ b/tests/fail/shims/memcmp_zero.rs @@ -0,0 +1,13 @@ +//@ignore-target-windows: No libc on Windows +//@compile-flags: -Zmiri-permissive-provenance + +// C says that passing "invalid" pointers is UB for all string functions. +// It is unclear whether `(int*)42` is "invalid", but there is no actually +// a `char` living at that address, so arguably it cannot be a valid pointer. +// Hence this is UB. +fn main() { + let ptr = 42 as *const u8; + unsafe { + libc::memcmp(ptr.cast(), ptr.cast(), 0); //~ERROR: dangling + } +} diff --git a/tests/fail/shims/memcmp_zero.stderr b/tests/fail/shims/memcmp_zero.stderr new file mode 100644 index 0000000000..e21b9b0600 --- /dev/null +++ b/tests/fail/shims/memcmp_zero.stderr @@ -0,0 +1,15 @@ +error: Undefined Behavior: out-of-bounds pointer use: 0x2a[noalloc] is a dangling pointer (it has no provenance) + --> $DIR/memcmp_zero.rs:LL:CC + | +LL | libc::memcmp(ptr.cast(), ptr.cast(), 0); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ out-of-bounds pointer use: 0x2a[noalloc] is a dangling pointer (it has no provenance) + | + = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior + = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information + = note: BACKTRACE: + = note: inside `main` at $DIR/memcmp_zero.rs:LL:CC + +note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace + +error: aborting due to previous error + diff --git a/tests/fail/shims/memrchr_null.stderr b/tests/fail/shims/memrchr_null.stderr index b5b7630e7f..cc11ba89f8 100644 --- a/tests/fail/shims/memrchr_null.stderr +++ b/tests/fail/shims/memrchr_null.stderr @@ -1,8 +1,8 @@ -error: Undefined Behavior: memory access failed: null pointer is a dangling pointer (it has no provenance) +error: Undefined Behavior: out-of-bounds pointer use: null pointer is a dangling pointer (it has no provenance) --> $DIR/memrchr_null.rs:LL:CC | LL | libc::memrchr(ptr::null(), 0, 0); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ memory access failed: null pointer is a dangling pointer (it has no provenance) + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ out-of-bounds pointer use: null pointer is a dangling pointer (it has no provenance) | = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information From 85ab5f05123d3e542d1c9a7de6a3d69d4b9b84d5 Mon Sep 17 00:00:00 2001 From: Ben Kimock Date: Tue, 15 Aug 2023 19:26:53 -0400 Subject: [PATCH 08/13] Explain why we save spans for some memory types and not others Co-authored-by: Ralf Jung --- src/machine.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/machine.rs b/src/machine.rs index 5fc3a5faeb..99eda0dff6 100644 --- a/src/machine.rs +++ b/src/machine.rs @@ -140,7 +140,9 @@ impl MiriMemoryKind { fn should_save_allocation_span(self) -> bool { use self::MiriMemoryKind::*; match self { + // Heap allocations are fine since the `Allocation` is created immediately. Rust | Miri | C | Mmap => true, + // Everything else is unclear, let's not show potentially confusing spans. Machine | Global | ExternStatic | Tls | WinHeap | Runtime => false, } } From eadad011014a6fd2945942637dbebf5d7b943f55 Mon Sep 17 00:00:00 2001 From: Ben Kimock Date: Tue, 15 Aug 2023 19:31:52 -0400 Subject: [PATCH 09/13] Tidy up the implementation --- src/machine.rs | 21 +++++++++------------ 1 file changed, 9 insertions(+), 12 deletions(-) diff --git a/src/machine.rs b/src/machine.rs index 99eda0dff6..90c3c70ae5 100644 --- a/src/machine.rs +++ b/src/machine.rs @@ -141,9 +141,9 @@ impl MiriMemoryKind { use self::MiriMemoryKind::*; match self { // Heap allocations are fine since the `Allocation` is created immediately. - Rust | Miri | C | Mmap => true, + Rust | Miri | C | WinHeap | Mmap => true, // Everything else is unclear, let's not show potentially confusing spans. - Machine | Global | ExternStatic | Tls | WinHeap | Runtime => false, + Machine | Global | ExternStatic | Tls | Runtime => false, } } } @@ -513,7 +513,7 @@ pub struct MiriMachine<'mir, 'tcx> { /// The spans we will use to report where an allocation was created and deallocated in /// diagnostics. - pub(crate) allocation_spans: RefCell, Option)>>, + pub(crate) allocation_spans: RefCell)>>, } impl<'mir, 'tcx> MiriMachine<'mir, 'tcx> { @@ -765,8 +765,7 @@ impl<'mir, 'tcx> MiriMachine<'mir, 'tcx> { self.allocation_spans .borrow() .get(&alloc_id) - .and_then(|(allocated, _deallocated)| *allocated) - .map(Span::data) + .map(|(allocated, _deallocated)| allocated.data()) } pub(crate) fn deallocated_span(&self, alloc_id: AllocId) -> Option { @@ -1087,13 +1086,11 @@ impl<'mir, 'tcx> Machine<'mir, 'tcx> for MiriMachine<'mir, 'tcx> { |ptr| ecx.global_base_pointer(ptr), )?; - if let MemoryKind::Machine(kind) = kind { - if kind.should_save_allocation_span() { - ecx.machine - .allocation_spans - .borrow_mut() - .insert(id, (Some(ecx.machine.current_span()), None)); - } + if matches!(kind, MemoryKind::Machine(kind) if kind.should_save_allocation_span()) { + ecx.machine + .allocation_spans + .borrow_mut() + .insert(id, (ecx.machine.current_span(), None)); } Ok(Cow::Owned(alloc)) From 4c5ccc25330dd964c69d9153cb3941d60f03bfd8 Mon Sep 17 00:00:00 2001 From: Taras Tsugrii Date: Tue, 15 Aug 2023 18:56:17 -0700 Subject: [PATCH 10/13] Replace hand-written binary search with Vec::binary_search_by. It's similar to https://github.com/rust-lang/miri/pull/3021 and should improve maintainability. --- src/range_map.rs | 35 +++++++++++++++-------------------- 1 file changed, 15 insertions(+), 20 deletions(-) diff --git a/src/range_map.rs b/src/range_map.rs index 146715ddda..228a239600 100644 --- a/src/range_map.rs +++ b/src/range_map.rs @@ -36,26 +36,21 @@ impl RangeMap { /// Finds the index containing the given offset. fn find_offset(&self, offset: u64) -> usize { - // We do a binary search. - let mut left = 0usize; // inclusive - let mut right = self.v.len(); // exclusive - loop { - debug_assert!(left < right, "find_offset: offset {offset} is out-of-bounds"); - let candidate = left.checked_add(right).unwrap() / 2; - let elem = &self.v[candidate]; - if offset < elem.range.start { - // We are too far right (offset is further left). - debug_assert!(candidate < right); // we are making progress - right = candidate; - } else if offset >= elem.range.end { - // We are too far left (offset is further right). - debug_assert!(candidate >= left); // we are making progress - left = candidate + 1; - } else { - // This is it! - return candidate; - } - } + self.v + .binary_search_by(|elem| -> std::cmp::Ordering { + if offset < elem.range.start { + // We are too far right (offset is further left). + // (`Greater` means that `elem` is greater than the desired target.) + std::cmp::Ordering::Greater + } else if offset >= elem.range.end { + // We are too far left (offset is further right). + std::cmp::Ordering::Less + } else { + // This is it! + std::cmp::Ordering::Equal + } + }) + .unwrap() } /// Provides read-only iteration over everything in the given range. This does From 01806af355fb61d14a1bb84c666666317506a4e7 Mon Sep 17 00:00:00 2001 From: The Miri Conjob Bot Date: Wed, 16 Aug 2023 05:25:49 +0000 Subject: [PATCH 11/13] Preparing for merge from rustc --- rust-version | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rust-version b/rust-version index 52f98965ec..30123b92f6 100644 --- a/rust-version +++ b/rust-version @@ -1 +1 @@ -b08dd92552d663e3c877c8e5ce859e212205a09f +656ee47db32e882fb02913f6204e09cc7a41a50e From 2cc7749821adae81bdb2fb862126eb1961349c20 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Wed, 16 Aug 2023 10:49:08 +0200 Subject: [PATCH 12/13] on out-of-bounds error, show where the allocation was created --- src/diagnostics.rs | 2 +- tests/fail/both_borrows/issue-miri-1050-1.stack.stderr | 7 ++++++- tests/fail/both_borrows/issue-miri-1050-1.tree.stderr | 7 ++++++- tests/fail/dangling_pointers/out_of_bounds_read1.stderr | 8 +++++++- tests/fail/dangling_pointers/out_of_bounds_read2.stderr | 8 +++++++- tests/fail/intrinsics/ptr_offset_ptr_plus_0.stderr | 7 ++++++- tests/fail/intrinsics/simd-scatter.stderr | 8 +++++++- tests/fail/zst3.stderr | 7 ++++++- 8 files changed, 46 insertions(+), 8 deletions(-) diff --git a/src/diagnostics.rs b/src/diagnostics.rs index 4f1ff15b35..f3285ccf91 100644 --- a/src/diagnostics.rs +++ b/src/diagnostics.rs @@ -309,7 +309,7 @@ pub fn report_error<'tcx, 'mir>( (None, format!("this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior")), (None, format!("see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information")), ]; - if let UndefinedBehaviorInfo::PointerUseAfterFree(alloc_id, _) = info { + if let UndefinedBehaviorInfo::PointerUseAfterFree(alloc_id, _) | UndefinedBehaviorInfo::PointerOutOfBounds { alloc_id, .. } = info { if let Some(span) = ecx.machine.allocated_span(*alloc_id) { helps.push((Some(span), format!("{:?} was allocated here:", alloc_id))); } diff --git a/tests/fail/both_borrows/issue-miri-1050-1.stack.stderr b/tests/fail/both_borrows/issue-miri-1050-1.stack.stderr index c69a3af293..a5580160c3 100644 --- a/tests/fail/both_borrows/issue-miri-1050-1.stack.stderr +++ b/tests/fail/both_borrows/issue-miri-1050-1.stack.stderr @@ -6,7 +6,12 @@ LL | Box(unsafe { Unique::new_unchecked(raw) }, alloc) | = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information - = note: BACKTRACE: +help: ALLOC was allocated here: + --> $DIR/issue-miri-1050-1.rs:LL:CC + | +LL | let ptr = Box::into_raw(Box::new(0u16)); + | ^^^^^^^^^^^^^^ + = note: BACKTRACE (of the first span): = note: inside `std::boxed::Box::::from_raw_in` at RUSTLIB/alloc/src/boxed.rs:LL:CC = note: inside `std::boxed::Box::::from_raw` at RUSTLIB/alloc/src/boxed.rs:LL:CC note: inside `main` diff --git a/tests/fail/both_borrows/issue-miri-1050-1.tree.stderr b/tests/fail/both_borrows/issue-miri-1050-1.tree.stderr index c69a3af293..a5580160c3 100644 --- a/tests/fail/both_borrows/issue-miri-1050-1.tree.stderr +++ b/tests/fail/both_borrows/issue-miri-1050-1.tree.stderr @@ -6,7 +6,12 @@ LL | Box(unsafe { Unique::new_unchecked(raw) }, alloc) | = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information - = note: BACKTRACE: +help: ALLOC was allocated here: + --> $DIR/issue-miri-1050-1.rs:LL:CC + | +LL | let ptr = Box::into_raw(Box::new(0u16)); + | ^^^^^^^^^^^^^^ + = note: BACKTRACE (of the first span): = note: inside `std::boxed::Box::::from_raw_in` at RUSTLIB/alloc/src/boxed.rs:LL:CC = note: inside `std::boxed::Box::::from_raw` at RUSTLIB/alloc/src/boxed.rs:LL:CC note: inside `main` diff --git a/tests/fail/dangling_pointers/out_of_bounds_read1.stderr b/tests/fail/dangling_pointers/out_of_bounds_read1.stderr index b2461ce423..7d2aed371b 100644 --- a/tests/fail/dangling_pointers/out_of_bounds_read1.stderr +++ b/tests/fail/dangling_pointers/out_of_bounds_read1.stderr @@ -6,8 +6,14 @@ LL | let x = unsafe { *v.as_ptr().wrapping_offset(5) }; | = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information - = note: BACKTRACE: +help: ALLOC was allocated here: + --> $DIR/out_of_bounds_read1.rs:LL:CC + | +LL | let v: Vec = vec![1, 2]; + | ^^^^^^^^^^ + = note: BACKTRACE (of the first span): = note: inside `main` at $DIR/out_of_bounds_read1.rs:LL:CC + = note: this error originates in the macro `vec` (in Nightly builds, run with -Z macro-backtrace for more info) note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace diff --git a/tests/fail/dangling_pointers/out_of_bounds_read2.stderr b/tests/fail/dangling_pointers/out_of_bounds_read2.stderr index b17058b406..69a8498f09 100644 --- a/tests/fail/dangling_pointers/out_of_bounds_read2.stderr +++ b/tests/fail/dangling_pointers/out_of_bounds_read2.stderr @@ -6,8 +6,14 @@ LL | let x = unsafe { *v.as_ptr().wrapping_offset(5) }; | = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information - = note: BACKTRACE: +help: ALLOC was allocated here: + --> $DIR/out_of_bounds_read2.rs:LL:CC + | +LL | let v: Vec = vec![1, 2]; + | ^^^^^^^^^^ + = note: BACKTRACE (of the first span): = note: inside `main` at $DIR/out_of_bounds_read2.rs:LL:CC + = note: this error originates in the macro `vec` (in Nightly builds, run with -Z macro-backtrace for more info) note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace diff --git a/tests/fail/intrinsics/ptr_offset_ptr_plus_0.stderr b/tests/fail/intrinsics/ptr_offset_ptr_plus_0.stderr index b18147ce37..7cc4bc184a 100644 --- a/tests/fail/intrinsics/ptr_offset_ptr_plus_0.stderr +++ b/tests/fail/intrinsics/ptr_offset_ptr_plus_0.stderr @@ -6,7 +6,12 @@ LL | let _x = unsafe { x.offset(0) }; // UB despite offset 0, the pointer is | = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information - = note: BACKTRACE: +help: ALLOC was allocated here: + --> $DIR/ptr_offset_ptr_plus_0.rs:LL:CC + | +LL | let x = Box::into_raw(Box::new(0u32)); + | ^^^^^^^^^^^^^^ + = note: BACKTRACE (of the first span): = note: inside `main` at $DIR/ptr_offset_ptr_plus_0.rs:LL:CC note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace diff --git a/tests/fail/intrinsics/simd-scatter.stderr b/tests/fail/intrinsics/simd-scatter.stderr index a745b61029..5beee034db 100644 --- a/tests/fail/intrinsics/simd-scatter.stderr +++ b/tests/fail/intrinsics/simd-scatter.stderr @@ -11,8 +11,14 @@ LL | | ); | = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information - = note: BACKTRACE: +help: ALLOC was allocated here: + --> $DIR/simd-scatter.rs:LL:CC + | +LL | let mut vec: Vec = vec![10, 11, 12, 13, 14, 15, 16, 17, 18]; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + = note: BACKTRACE (of the first span): = note: inside `main` at $DIR/simd-scatter.rs:LL:CC + = note: this error originates in the macro `vec` (in Nightly builds, run with -Z macro-backtrace for more info) note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace diff --git a/tests/fail/zst3.stderr b/tests/fail/zst3.stderr index c9accf2c8f..b62aef675d 100644 --- a/tests/fail/zst3.stderr +++ b/tests/fail/zst3.stderr @@ -6,7 +6,12 @@ LL | unsafe { *(x as *mut [u8; 0]) = zst_val }; | = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information - = note: BACKTRACE: +help: ALLOC was allocated here: + --> $DIR/zst3.rs:LL:CC + | +LL | let mut x_box = Box::new(1u8); + | ^^^^^^^^^^^^^ + = note: BACKTRACE (of the first span): = note: inside `main` at $DIR/zst3.rs:LL:CC note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace From 6cd057e02400806220708672541fab9d71d07b84 Mon Sep 17 00:00:00 2001 From: Taras Tsugrii Date: Tue, 15 Aug 2023 18:33:57 -0700 Subject: [PATCH 13/13] Avoid unnecessary Vec resize. If `size > 0` current implementation will first create an empty vec and then push an element into it, which will cause a resize that can be easily avoided. It's obviously not a big deal, but this also gets rid of `mut` local variable. --- src/range_map.rs | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/src/range_map.rs b/src/range_map.rs index 146715ddda..2b4544228e 100644 --- a/src/range_map.rs +++ b/src/range_map.rs @@ -27,11 +27,8 @@ impl RangeMap { #[inline(always)] pub fn new(size: Size, init: T) -> RangeMap { let size = size.bytes(); - let mut map = RangeMap { v: Vec::new() }; - if size > 0 { - map.v.push(Elem { range: 0..size, data: init }); - } - map + let v = if size > 0 { vec![Elem { range: 0..size, data: init }] } else { Vec::new() }; + RangeMap { v } } /// Finds the index containing the given offset.