Rollup merge of #130342 - RalfJung:slice-idx-overflow, r=saethlin

interpret, miri: fix dealing with overflow during slice indexing and allocation

This is mostly to fix https://github.com/rust-lang/rust/issues/130284.

I then realized we're using somewhat sketchy arguments for a similar multiplication in `copy`/`copy_nonoverlapping`/`write_bytes`,  so I made them all share the same function that checks exactly the right thing. (The intrinsics would previously fail on allocations larger than `1 << 47` bytes... which are theoretically possible maybe? Anyway it seems conceptually wrong to use any other bound than `isize::MAX` here.)
This commit is contained in:
Matthias Krüger 2024-09-15 16:01:38 +02:00 committed by GitHub
commit 96195a5e24
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
11 changed files with 109 additions and 45 deletions

View file

@ -216,7 +216,7 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> {
self.copy_intrinsic(&args[0], &args[1], &args[2], /*nonoverlapping*/ false)?;
}
sym::write_bytes => {
self.write_bytes_intrinsic(&args[0], &args[1], &args[2])?;
self.write_bytes_intrinsic(&args[0], &args[1], &args[2], "write_bytes")?;
}
sym::compare_bytes => {
let result = self.compare_bytes_intrinsic(&args[0], &args[1], &args[2])?;
@ -599,9 +599,8 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> {
let count = self.read_target_usize(count)?;
let layout = self.layout_of(src.layout.ty.builtin_deref(true).unwrap())?;
let (size, align) = (layout.size, layout.align.abi);
// `checked_mul` enforces a too small bound (the correct one would probably be target_isize_max),
// but no actual allocation can be big enough for the difference to be noticeable.
let size = size.checked_mul(count, self).ok_or_else(|| {
let size = self.compute_size_in_bytes(size, count).ok_or_else(|| {
err_ub_custom!(
fluent::const_eval_size_overflow,
name = if nonoverlapping { "copy_nonoverlapping" } else { "copy" }
@ -635,11 +634,12 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> {
Ok(())
}
pub(crate) fn write_bytes_intrinsic(
pub fn write_bytes_intrinsic(
&mut self,
dst: &OpTy<'tcx, <M as Machine<'tcx>>::Provenance>,
byte: &OpTy<'tcx, <M as Machine<'tcx>>::Provenance>,
count: &OpTy<'tcx, <M as Machine<'tcx>>::Provenance>,
name: &'static str,
) -> InterpResult<'tcx> {
let layout = self.layout_of(dst.layout.ty.builtin_deref(true).unwrap())?;
@ -649,9 +649,9 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> {
// `checked_mul` enforces a too small bound (the correct one would probably be target_isize_max),
// but no actual allocation can be big enough for the difference to be noticeable.
let len = layout.size.checked_mul(count, self).ok_or_else(|| {
err_ub_custom!(fluent::const_eval_size_overflow, name = "write_bytes")
})?;
let len = self
.compute_size_in_bytes(layout.size, count)
.ok_or_else(|| err_ub_custom!(fluent::const_eval_size_overflow, name = name))?;
let bytes = std::iter::repeat(byte).take(len.bytes_usize());
self.write_bytes_ptr(dst, bytes)

View file

@ -222,7 +222,7 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> {
} else {
Allocation::try_uninit(size, align)?
};
self.allocate_raw_ptr(alloc, kind)
self.insert_allocation(alloc, kind)
}
pub fn allocate_bytes_ptr(
@ -233,14 +233,15 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> {
mutability: Mutability,
) -> InterpResult<'tcx, Pointer<M::Provenance>> {
let alloc = Allocation::from_bytes(bytes, align, mutability);
self.allocate_raw_ptr(alloc, kind)
self.insert_allocation(alloc, kind)
}
pub fn allocate_raw_ptr(
pub fn insert_allocation(
&mut self,
alloc: Allocation<M::Provenance, (), M::Bytes>,
kind: MemoryKind<M::MemoryKind>,
) -> InterpResult<'tcx, Pointer<M::Provenance>> {
assert!(alloc.size() <= self.max_size_of_val());
let id = self.tcx.reserve_alloc_id();
debug_assert_ne!(
Some(kind),

View file

@ -1,11 +1,12 @@
use either::Either;
use rustc_apfloat::{Float, FloatConvert};
use rustc_middle::mir::interpret::{InterpResult, Scalar};
use rustc_middle::mir::interpret::{InterpResult, PointerArithmetic, Scalar};
use rustc_middle::mir::NullOp;
use rustc_middle::ty::layout::{LayoutOf, TyAndLayout};
use rustc_middle::ty::{self, FloatTy, ScalarInt, Ty};
use rustc_middle::{bug, mir, span_bug};
use rustc_span::symbol::sym;
use rustc_target::abi::Size;
use tracing::trace;
use super::{throw_ub, ImmTy, InterpCx, Machine, MemPlaceMeta};
@ -287,6 +288,20 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> {
})
}
/// Computes the total size of this access, `count * elem_size`,
/// checking for overflow beyond isize::MAX.
pub fn compute_size_in_bytes(&self, elem_size: Size, count: u64) -> Option<Size> {
// `checked_mul` applies `u64` limits independent of the target pointer size... but the
// subsequent check for `max_size_of_val` means we also handle 32bit targets correctly.
// (We cannot use `Size::checked_mul` as that enforces `obj_size_bound` as the limit, which
// would be wrong here.)
elem_size
.bytes()
.checked_mul(count)
.map(Size::from_bytes)
.filter(|&total| total <= self.max_size_of_val())
}
fn binary_ptr_op(
&self,
bin_op: mir::BinOp,

View file

@ -17,7 +17,7 @@ use rustc_target::abi::{self, Size, VariantIdx};
use tracing::{debug, instrument};
use super::{
throw_ub, throw_unsup, InterpCx, InterpResult, MPlaceTy, Machine, MemPlaceMeta, OpTy,
err_ub, throw_ub, throw_unsup, InterpCx, InterpResult, MPlaceTy, Machine, MemPlaceMeta, OpTy,
Provenance, Scalar,
};
@ -229,7 +229,11 @@ where
// This can only be reached in ConstProp and non-rustc-MIR.
throw_ub!(BoundsCheckFailed { len, index });
}
let offset = stride * index; // `Size` multiplication
// With raw slices, `len` can be so big that this *can* overflow.
let offset = self
.compute_size_in_bytes(stride, index)
.ok_or_else(|| err_ub!(PointerArithOverflow))?;
// All fields have the same layout.
let field_layout = base.layout().field(self, 0);
(offset, field_layout)