Skip to content

Fix validation and reborrowing of integer pointers #830

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
Jul 11, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion rust-version
Original file line number Diff line number Diff line change
@@ -1 +1 @@
0b680cfce544ff9a59d720020e397c4bf3346650
97b1128589fdaa786a7cf65c5a6ff7ed37a1d2f3
3 changes: 1 addition & 2 deletions src/eval.rs
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,6 @@ pub fn create_ecx<'mir, 'tcx: 'mir>(
ecx.machine.argc = Some(argc_place.ptr.to_ptr()?);
}

// FIXME: extract main source file path.
// Third argument (`argv`): created from `config.args`.
let dest = ecx.eval_place(&mir::Place::Base(mir::PlaceBase::Local(args.next().unwrap())))?;
// For Windows, construct a command string with all the aguments.
Expand Down Expand Up @@ -136,7 +135,7 @@ pub fn create_ecx<'mir, 'tcx: 'mir>(
let place = ecx.mplace_field(argvs_place, idx as u64)?;
ecx.write_scalar(Scalar::Ptr(arg), place.into())?;
}
ecx.memory_mut().mark_immutable(argvs_place.to_ptr()?.alloc_id)?;
ecx.memory_mut().mark_immutable(argvs_place.ptr.assert_ptr().alloc_id)?;
// Write a pointer to that place as the argument.
let argv = argvs_place.ptr;
ecx.write_scalar(argv, dest)?;
Expand Down
24 changes: 10 additions & 14 deletions src/helpers.rs
Original file line number Diff line number Diff line change
Expand Up @@ -116,36 +116,32 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
.map(|(size, _)| size)
.unwrap_or_else(|| place.layout.size)
);
assert!(size.bytes() > 0);
// Store how far we proceeded into the place so far. Everything to the left of
// this offset has already been handled, in the sense that the frozen parts
// have had `action` called on them.
let mut end_ptr = place.ptr;
let mut end_ptr = place.ptr.assert_ptr();
// Called when we detected an `UnsafeCell` at the given offset and size.
// Calls `action` and advances `end_ptr`.
let mut unsafe_cell_action = |unsafe_cell_ptr: Scalar<Tag>, unsafe_cell_size: Size| {
if unsafe_cell_size != Size::ZERO {
debug_assert_eq!(unsafe_cell_ptr.to_ptr().unwrap().alloc_id,
end_ptr.to_ptr().unwrap().alloc_id);
debug_assert_eq!(unsafe_cell_ptr.to_ptr().unwrap().tag,
end_ptr.to_ptr().unwrap().tag);
}
let unsafe_cell_ptr = unsafe_cell_ptr.assert_ptr();
debug_assert_eq!(unsafe_cell_ptr.alloc_id, end_ptr.alloc_id);
debug_assert_eq!(unsafe_cell_ptr.tag, end_ptr.tag);
// We assume that we are given the fields in increasing offset order,
// and nothing else changes.
let unsafe_cell_offset = unsafe_cell_ptr.get_ptr_offset(this);
let end_offset = end_ptr.get_ptr_offset(this);
let unsafe_cell_offset = unsafe_cell_ptr.offset;
let end_offset = end_ptr.offset;
assert!(unsafe_cell_offset >= end_offset);
let frozen_size = unsafe_cell_offset - end_offset;
// Everything between the end_ptr and this `UnsafeCell` is frozen.
if frozen_size != Size::ZERO {
action(end_ptr.to_ptr()?, frozen_size, /*frozen*/true)?;
action(end_ptr, frozen_size, /*frozen*/true)?;
}
// This `UnsafeCell` is NOT frozen.
if unsafe_cell_size != Size::ZERO {
action(unsafe_cell_ptr.to_ptr()?, unsafe_cell_size, /*frozen*/false)?;
action(unsafe_cell_ptr, unsafe_cell_size, /*frozen*/false)?;
}
// Update end end_ptr.
end_ptr = unsafe_cell_ptr.ptr_wrapping_offset(unsafe_cell_size, this);
end_ptr = unsafe_cell_ptr.wrapping_offset(unsafe_cell_size, this);
// Done
Ok(())
};
Expand Down Expand Up @@ -234,7 +230,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
layout::FieldPlacement::Arbitrary { .. } => {
// Gather the subplaces and sort them before visiting.
let mut places = fields.collect::<InterpResult<'tcx, Vec<MPlaceTy<'tcx, Tag>>>>()?;
places.sort_by_key(|place| place.ptr.get_ptr_offset(self.ecx()));
places.sort_by_key(|place| place.ptr.assert_ptr().offset);
self.walk_aggregate(place, places.into_iter().map(Ok))
}
layout::FieldPlacement::Union { .. } => {
Expand Down
8 changes: 1 addition & 7 deletions src/shims/foreign_items.rs
Original file line number Diff line number Diff line change
Expand Up @@ -842,13 +842,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
},
"GetSystemInfo" => {
let system_info = this.deref_operand(args[0])?;
let (system_info_ptr, align) = system_info.to_scalar_ptr_align();
let system_info_ptr = this.memory()
.check_ptr_access(
system_info_ptr,
system_info.layout.size,
align,
)?
let system_info_ptr = this.check_mplace_access(system_info, None)?
.expect("cannot be a ZST");
// Initialize with `0`.
this.memory_mut().get_mut(system_info_ptr.alloc_id)?
Expand Down
22 changes: 13 additions & 9 deletions src/shims/intrinsics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -166,17 +166,21 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
let elem_size = elem_layout.size.bytes();
let count = this.read_scalar(args[2])?.to_usize(this)?;
let elem_align = elem_layout.align.abi;
// erase tags: this is a raw ptr operation

let size = Size::from_bytes(count * elem_size);
let src = this.read_scalar(args[0])?.not_undef()?;
let src = this.memory().check_ptr_access(src, size, elem_align)?;
let dest = this.read_scalar(args[1])?.not_undef()?;
this.memory_mut().copy(
src,
elem_align,
dest,
elem_align,
Size::from_bytes(count * elem_size),
intrinsic_name.ends_with("_nonoverlapping"),
)?;
let dest = this.memory().check_ptr_access(dest, size, elem_align)?;

if let (Some(src), Some(dest)) = (src, dest) {
this.memory_mut().copy(
src,
dest,
size,
intrinsic_name.ends_with("_nonoverlapping"),
)?;
}
}

"discriminant_value" => {
Expand Down
1 change: 1 addition & 0 deletions src/stacked_borrows.rs
Original file line number Diff line number Diff line change
Expand Up @@ -587,6 +587,7 @@ trait EvalContextPrivExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
// Nothing to do for ZSTs.
return Ok(*val);
}
let place = this.force_mplace_ptr(place)?;

// Compute new borrow.
let new_tag = match kind {
Expand Down
3 changes: 1 addition & 2 deletions test-cargo-miri/run-test.py
Original file line number Diff line number Diff line change
Expand Up @@ -52,9 +52,8 @@ def test_cargo_miri_run():
)

def test_cargo_miri_test():
# FIXME: enable validation again, once that no longer conflicts with intptrcast
test("cargo miri test",
cargo_miri("test") + ["--", "-Zmiri-seed=feed", "-Zmiri-disable-validation"],
cargo_miri("test") + ["--", "-Zmiri-seed=feed"],
"test.stdout.ref", "test.stderr.ref"
)
test("cargo miri test (with filter)",
Expand Down
5 changes: 5 additions & 0 deletions tests/run-pass/unique-send.rs → tests/run-pass/mpsc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,4 +7,9 @@ pub fn main() {
tx.send(box 100).unwrap();
let v = rx.recv().unwrap();
assert_eq!(v, box 100);

tx.send(box 101).unwrap();
tx.send(box 102).unwrap();
assert_eq!(rx.recv().unwrap(), box 101);
assert_eq!(rx.recv().unwrap(), box 102);
}
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
// FIXME move this to run-pass, it should work with intptrcast.
use std::mem;
use std::ptr;

Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,3 @@
// FIXME move this to run-pass, it should work with intptrcast.

fn f() -> i32 { 42 }

fn main() {
Expand Down