Skip to content

Commit

Permalink
separate bounds-check from alignment check
Browse files Browse the repository at this point in the history
  • Loading branch information
RalfJung committed Oct 15, 2023
1 parent 6a5731b commit dd7e34c
Show file tree
Hide file tree
Showing 7 changed files with 13 additions and 24 deletions.
4 changes: 2 additions & 2 deletions src/borrow_tracker/stacked_borrows/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ use log::trace;
use rustc_data_structures::fx::FxHashSet;
use rustc_middle::mir::{Mutability, RetagKind};
use rustc_middle::ty::{self, layout::HasParamEnv, Ty};
use rustc_target::abi::{Abi, Align, Size};
use rustc_target::abi::{Abi, Size};

use crate::borrow_tracker::{
stacked_borrows::diagnostics::{AllocHistory, DiagnosticCx, DiagnosticCxBuilder},
Expand Down Expand Up @@ -616,7 +616,7 @@ trait EvalContextPrivExt<'mir: 'ecx, 'tcx: 'mir, 'ecx>: crate::MiriInterpCxExt<'
) -> InterpResult<'tcx, Option<Provenance>> {
let this = self.eval_context_mut();
// Ensure we bail out if the pointer goes out-of-bounds (see miri#1050).
this.check_ptr_access_align(place.ptr(), size, Align::ONE, CheckInAllocMsg::InboundsTest)?;
this.check_ptr_access(place.ptr(), size, CheckInAllocMsg::InboundsTest)?;

// It is crucial that this gets called on all code paths, to ensure we track tag creation.
let log_creation = |this: &MiriInterpCx<'mir, 'tcx>,
Expand Down
5 changes: 2 additions & 3 deletions src/borrow_tracker/tree_borrows/mod.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use log::trace;

use rustc_target::abi::{Abi, Align, Size};
use rustc_target::abi::{Abi, Size};

use crate::borrow_tracker::{
AccessKind, GlobalState, GlobalStateInner, ProtectorKind, RetagFields,
Expand Down Expand Up @@ -206,10 +206,9 @@ trait EvalContextPrivExt<'mir: 'ecx, 'tcx: 'mir, 'ecx>: crate::MiriInterpCxExt<'
// Make sure the new permission makes sense as the initial permission of a fresh tag.
assert!(new_perm.initial_state.is_initial());
// Ensure we bail out if the pointer goes out-of-bounds (see miri#1050).
this.check_ptr_access_align(
this.check_ptr_access(
place.ptr(),
ptr_size,
Align::ONE,
CheckInAllocMsg::InboundsTest,
)?;

Expand Down
4 changes: 1 addition & 3 deletions src/concurrency/data_race.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1017,11 +1017,9 @@ trait EvalContextPrivExt<'mir, 'tcx: 'mir>: MiriInterpCxExt<'mir, 'tcx> {
// even if the type they wrap would be less aligned (e.g. AtomicU64 on 32bit must
// be 8-aligned).
let align = Align::from_bytes(place.layout.size.bytes()).unwrap();
this.check_ptr_access_align(
this.check_ptr_align(
place.ptr(),
place.layout.size,
align,
CheckInAllocMsg::MemoryAccessTest,
)?;
// Ensure the allocation is mutable. Even failing (read-only) compare_exchange need mutable
// memory on many targets (i.e., they segfault if taht memory is mapped read-only), and
Expand Down
9 changes: 5 additions & 4 deletions src/helpers.rs
Original file line number Diff line number Diff line change
Expand Up @@ -785,7 +785,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
loop {
// FIXME: We are re-getting the allocation each time around the loop.
// Would be nice if we could somehow "extend" an existing AllocRange.
let alloc = this.get_ptr_alloc(ptr.offset(len, this)?, size1, Align::ONE)?.unwrap(); // not a ZST, so we will get a result
let alloc = this.get_ptr_alloc(ptr.offset(len, this)?, size1)?.unwrap(); // not a ZST, so we will get a result
let byte = alloc.read_integer(alloc_range(Size::ZERO, size1))?.to_u8()?;
if byte == 0 {
break;
Expand Down Expand Up @@ -825,13 +825,13 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
fn read_wide_str(&self, mut ptr: Pointer<Option<Provenance>>) -> InterpResult<'tcx, Vec<u16>> {
let this = self.eval_context_ref();
let size2 = Size::from_bytes(2);
let align2 = Align::from_bytes(2).unwrap();
this.check_ptr_align(ptr, Align::from_bytes(2).unwrap())?;

let mut wchars = Vec::new();
loop {
// FIXME: We are re-getting the allocation each time around the loop.
// Would be nice if we could somehow "extend" an existing AllocRange.
let alloc = this.get_ptr_alloc(ptr, size2, align2)?.unwrap(); // not a ZST, so we will get a result
let alloc = this.get_ptr_alloc(ptr, size2)?.unwrap(); // not a ZST, so we will get a result
let wchar = alloc.read_integer(alloc_range(Size::ZERO, size2))?.to_u16()?;
if wchar == 0 {
break;
Expand Down Expand Up @@ -867,8 +867,9 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
// Store the UTF-16 string.
let size2 = Size::from_bytes(2);
let this = self.eval_context_mut();
this.check_ptr_align(ptr, Align::from_bytes(2).unwrap())?;
let mut alloc = this
.get_ptr_alloc_mut(ptr, size2 * string_length, Align::from_bytes(2).unwrap())?
.get_ptr_alloc_mut(ptr, size2 * string_length)?
.unwrap(); // not a ZST, so we will get a result
for (offset, wchar) in wide_str.iter().copied().chain(iter::once(0x0000)).enumerate() {
let offset = u64::try_from(offset).unwrap();
Expand Down
4 changes: 0 additions & 4 deletions src/shims/foreign_items.rs
Original file line number Diff line number Diff line change
Expand Up @@ -807,9 +807,7 @@ trait EvalContextExtPriv<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {

this.mem_copy(
ptr_src,
Align::ONE,
ptr_dest,
Align::ONE,
Size::from_bytes(n),
true,
)?;
Expand All @@ -830,9 +828,7 @@ trait EvalContextExtPriv<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
let n = this.read_c_str(ptr_src)?.len().checked_add(1).unwrap();
this.mem_copy(
ptr_src,
Align::ONE,
ptr_dest,
Align::ONE,
Size::from_bytes(n),
true,
)?;
Expand Down
8 changes: 3 additions & 5 deletions src/shims/unix/fs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ use log::trace;

use rustc_data_structures::fx::FxHashMap;
use rustc_middle::ty::TyCtxt;
use rustc_target::abi::{Align, Size};
use rustc_target::abi::Size;

use crate::shims::os_str::bytes_to_os_str;
use crate::*;
Expand Down Expand Up @@ -756,10 +756,9 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
trace!("Reading from FD {}, size {}", fd, count);

// Check that the *entire* buffer is actually valid memory.
this.check_ptr_access_align(
this.check_ptr_access(
buf,
Size::from_bytes(count),
Align::ONE,
CheckInAllocMsg::MemoryAccessTest,
)?;

Expand Down Expand Up @@ -810,10 +809,9 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
// Isolation check is done via `FileDescriptor` trait.

// Check that the *entire* buffer is actually valid memory.
this.check_ptr_access_align(
this.check_ptr_access(
buf,
Size::from_bytes(count),
Align::ONE,
CheckInAllocMsg::MemoryAccessTest,
)?;

Expand Down
3 changes: 0 additions & 3 deletions src/shims/x86/sse3.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
use rustc_middle::mir;
use rustc_span::Symbol;
use rustc_target::abi::Align;
use rustc_target::spec::abi::Abi;

use super::horizontal_bin_op;
Expand Down Expand Up @@ -76,9 +75,7 @@ pub(super) trait EvalContextExt<'mir, 'tcx: 'mir>:

this.mem_copy(
src_ptr,
Align::ONE,
dest.ptr(),
Align::ONE,
dest.layout.size,
/*nonoverlapping*/ true,
)?;
Expand Down

0 comments on commit dd7e34c

Please sign in to comment.