From f42917b81bd958ef05d5d36bd5781629f51ad8c1 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sun, 9 Jul 2023 16:43:50 +0200 Subject: [PATCH 1/5] test and fix return place alias restrictions --- src/borrow_tracker/tree_borrows/mod.rs | 3 +- src/borrow_tracker/tree_borrows/perms.rs | 5 +++ .../both_borrows/return_pointer_aliasing.rs | 32 ++++++++++++++ .../return_pointer_aliasing.stack.stderr | 42 ++++++++++++++++++ .../return_pointer_aliasing.tree.stderr | 44 +++++++++++++++++++ 5 files changed, 124 insertions(+), 2 deletions(-) create mode 100644 tests/fail/both_borrows/return_pointer_aliasing.rs create mode 100644 tests/fail/both_borrows/return_pointer_aliasing.stack.stderr create mode 100644 tests/fail/both_borrows/return_pointer_aliasing.tree.stderr diff --git a/src/borrow_tracker/tree_borrows/mod.rs b/src/borrow_tracker/tree_borrows/mod.rs index e134b73988..3bb38a249f 100644 --- a/src/borrow_tracker/tree_borrows/mod.rs +++ b/src/borrow_tracker/tree_borrows/mod.rs @@ -514,9 +514,8 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { let ptr_layout = this.layout_of(Ty::new_mut_ptr(this.tcx.tcx, return_place.layout.ty))?; let val = ImmTy::from_immediate(return_place.to_ref(this), ptr_layout); // Reborrow it. With protection! That is part of the point. - // FIXME: do we truly want a 2phase borrow here? let new_perm = Some(NewPermission { - initial_state: Permission::new_unique_2phase(/*freeze*/ false), + initial_state: Permission::new_active(), zero_size: false, protector: Some(ProtectorKind::StrongProtector), }); diff --git a/src/borrow_tracker/tree_borrows/perms.rs b/src/borrow_tracker/tree_borrows/perms.rs index 6b1e722b65..362070f185 100644 --- a/src/borrow_tracker/tree_borrows/perms.rs +++ b/src/borrow_tracker/tree_borrows/perms.rs @@ -138,6 +138,11 @@ impl Permission { Self { inner: Reserved { ty_is_freeze } } } + /// Default initial permission for return place. + pub fn new_active() -> Self { + Self { inner: Active } + } + /// Default initial permission of a reborrowed shared reference pub fn new_frozen() -> Self { Self { inner: Frozen } diff --git a/tests/fail/both_borrows/return_pointer_aliasing.rs b/tests/fail/both_borrows/return_pointer_aliasing.rs new file mode 100644 index 0000000000..03886e7874 --- /dev/null +++ b/tests/fail/both_borrows/return_pointer_aliasing.rs @@ -0,0 +1,32 @@ +//@revisions: stack tree +//@[tree]compile-flags: -Zmiri-tree-borrows +#![feature(raw_ref_op)] +#![feature(core_intrinsics)] +#![feature(custom_mir)] + +use std::intrinsics::mir::*; +use std::mem::MaybeUninit; + +#[custom_mir(dialect = "runtime", phase = "optimized")] +pub fn main() { + mir! { + { + let x = 0; + let ptr = &raw mut x; + // We arrange for `myfun` to have a pointer that aliases + // its return place. Even just reading from that pointer is UB. + Call(*ptr, after_call, myfun(ptr)) + } + + after_call = { + Return() + } + } +} + +fn myfun(ptr: *mut i32) -> i32 { + unsafe { ptr.cast::>().read() }; + //~[stack]^ ERROR: /not granting access/ + //~[tree]| ERROR: /read access .* forbidden/ + 13 +} diff --git a/tests/fail/both_borrows/return_pointer_aliasing.stack.stderr b/tests/fail/both_borrows/return_pointer_aliasing.stack.stderr new file mode 100644 index 0000000000..5bdddf5e49 --- /dev/null +++ b/tests/fail/both_borrows/return_pointer_aliasing.stack.stderr @@ -0,0 +1,42 @@ +error: Undefined Behavior: not granting access to tag because that would remove [Unique for ] which is strongly protected because it is an argument of call ID + --> $DIR/return_pointer_aliasing.rs:LL:CC + | +LL | unsafe { ptr.cast::>().read() }; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ not granting access to tag because that would remove [Unique for ] which is strongly protected because it is an argument of call ID + | + = help: this indicates a potential bug in the program: it performed an invalid operation, but the Stacked Borrows rules it violated are still experimental + = help: see https://github.com/rust-lang/unsafe-code-guidelines/blob/master/wip/stacked-borrows.md for further information +help: was created by a SharedReadWrite retag at offsets [0x0..0x4] + --> $DIR/return_pointer_aliasing.rs:LL:CC + | +LL | / mir! { +LL | | { +LL | | let x = 0; +LL | | let ptr = &raw mut x; +... | +LL | | } +LL | | } + | |_____^ +help: is this argument + --> $DIR/return_pointer_aliasing.rs:LL:CC + | +LL | / fn myfun(ptr: *mut i32) -> i32 { +LL | | unsafe { ptr.cast::>().read() }; +LL | | +LL | | +LL | | 13 +LL | | } + | |_^ + = note: BACKTRACE (of the first span): + = note: inside `myfun` at $DIR/return_pointer_aliasing.rs:LL:CC +note: inside `main` + --> $DIR/return_pointer_aliasing.rs:LL:CC + | +LL | Call(*ptr, after_call, myfun(ptr)) + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + = note: this error originates in the macro `::core::intrinsics::mir::__internal_remove_let` which comes from the expansion of the macro `mir` (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 + +error: aborting due to previous error + diff --git a/tests/fail/both_borrows/return_pointer_aliasing.tree.stderr b/tests/fail/both_borrows/return_pointer_aliasing.tree.stderr new file mode 100644 index 0000000000..dd76d65c7c --- /dev/null +++ b/tests/fail/both_borrows/return_pointer_aliasing.tree.stderr @@ -0,0 +1,44 @@ +error: Undefined Behavior: read access through (root of the allocation) is forbidden + --> $DIR/return_pointer_aliasing.rs:LL:CC + | +LL | unsafe { ptr.cast::>().read() }; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ read access through (root of the allocation) is forbidden + | + = help: this indicates a potential bug in the program: it performed an invalid operation, but the Tree Borrows rules it violated are still experimental + = help: the accessed tag (root of the allocation) is foreign to the protected tag (i.e., it is not a child) + = help: this foreign read access would cause the protected tag to transition from Active to Frozen + = help: this transition would be a loss of write permissions, which is not allowed for protected tags +help: the accessed tag was created here + --> $DIR/return_pointer_aliasing.rs:LL:CC + | +LL | / mir! { +LL | | { +LL | | let x = 0; +LL | | let ptr = &raw mut x; +... | +LL | | } +LL | | } + | |_____^ +help: the protected tag was created here, in the initial state Active + --> $DIR/return_pointer_aliasing.rs:LL:CC + | +LL | / fn myfun(ptr: *mut i32) -> i32 { +LL | | unsafe { ptr.cast::>().read() }; +LL | | +LL | | +LL | | 13 +LL | | } + | |_^ + = note: BACKTRACE (of the first span): + = note: inside `myfun` at $DIR/return_pointer_aliasing.rs:LL:CC +note: inside `main` + --> $DIR/return_pointer_aliasing.rs:LL:CC + | +LL | Call(*ptr, after_call, myfun(ptr)) + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + = note: this error originates in the macro `::core::intrinsics::mir::__internal_remove_let` which comes from the expansion of the macro `mir` (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 + +error: aborting due to previous error + From b5490c7d06efcb97600da958843d9772b0475bc4 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Mon, 10 Jul 2023 10:15:54 +0200 Subject: [PATCH 2/5] Preparing for merge from rustc --- rust-version | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rust-version b/rust-version index 02b0dd16f9..17b1d2b112 100644 --- a/rust-version +++ b/rust-version @@ -1 +1 @@ -d4096e0412ac5de785d739a0aa2b1c1c7b9d3b7d +743333f3dd90721461c09387ec73d09c080d5f5f From 6fe2fc87dab2d57b3311a64864040782727faf91 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Mon, 10 Jul 2023 22:07:07 +0200 Subject: [PATCH 3/5] miri: protect Move() function arguments during the call --- src/borrow_tracker/mod.rs | 6 +-- .../stacked_borrows/diagnostics.rs | 4 +- src/borrow_tracker/stacked_borrows/mod.rs | 32 ++++------- src/borrow_tracker/tree_borrows/mod.rs | 33 ++++-------- src/lib.rs | 1 + src/machine.rs | 53 ++++++++++++++----- src/shims/mod.rs | 8 +-- .../fail/function_calls/arg_inplace_mutate.rs | 34 ++++++++++++ .../arg_inplace_mutate.stack.stderr | 37 +++++++++++++ .../arg_inplace_mutate.tree.stderr | 39 ++++++++++++++ .../arg_inplace_observe_after.rs | 27 ++++++++++ .../arg_inplace_observe_after.stderr | 22 ++++++++ .../arg_inplace_observe_during.none.stderr | 20 +++++++ .../arg_inplace_observe_during.rs | 34 ++++++++++++ .../arg_inplace_observe_during.stack.stderr | 37 +++++++++++++ .../arg_inplace_observe_during.tree.stderr | 39 ++++++++++++++ .../return_pointer_aliasing.none.stderr | 20 +++++++ .../return_pointer_aliasing.rs | 10 ++-- .../return_pointer_aliasing.stack.stderr | 13 ++--- .../return_pointer_aliasing.tree.stderr | 13 ++--- .../function_calls/return_place_on_heap.rs | 25 +++++++++ 21 files changed, 422 insertions(+), 85 deletions(-) create mode 100644 tests/fail/function_calls/arg_inplace_mutate.rs create mode 100644 tests/fail/function_calls/arg_inplace_mutate.stack.stderr create mode 100644 tests/fail/function_calls/arg_inplace_mutate.tree.stderr create mode 100644 tests/fail/function_calls/arg_inplace_observe_after.rs create mode 100644 tests/fail/function_calls/arg_inplace_observe_after.stderr create mode 100644 tests/fail/function_calls/arg_inplace_observe_during.none.stderr create mode 100644 tests/fail/function_calls/arg_inplace_observe_during.rs create mode 100644 tests/fail/function_calls/arg_inplace_observe_during.stack.stderr create mode 100644 tests/fail/function_calls/arg_inplace_observe_during.tree.stderr create mode 100644 tests/fail/function_calls/return_pointer_aliasing.none.stderr rename tests/fail/{both_borrows => function_calls}/return_pointer_aliasing.rs (70%) rename tests/fail/{both_borrows => function_calls}/return_pointer_aliasing.stack.stderr (78%) rename tests/fail/{both_borrows => function_calls}/return_pointer_aliasing.tree.stderr (83%) create mode 100644 tests/pass/function_calls/return_place_on_heap.rs diff --git a/src/borrow_tracker/mod.rs b/src/borrow_tracker/mod.rs index faa23fd262..a2cf7c8095 100644 --- a/src/borrow_tracker/mod.rs +++ b/src/borrow_tracker/mod.rs @@ -302,12 +302,12 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { } } - fn retag_return_place(&mut self) -> InterpResult<'tcx> { + fn protect_place(&mut self, place: &MPlaceTy<'tcx, Provenance>) -> InterpResult<'tcx> { let this = self.eval_context_mut(); let method = this.machine.borrow_tracker.as_ref().unwrap().borrow().borrow_tracker_method; match method { - BorrowTrackerMethod::StackedBorrows => this.sb_retag_return_place(), - BorrowTrackerMethod::TreeBorrows => this.tb_retag_return_place(), + BorrowTrackerMethod::StackedBorrows => this.sb_protect_place(place), + BorrowTrackerMethod::TreeBorrows => this.tb_protect_place(place), } } diff --git a/src/borrow_tracker/stacked_borrows/diagnostics.rs b/src/borrow_tracker/stacked_borrows/diagnostics.rs index de307a3c5f..5ec8d80fb3 100644 --- a/src/borrow_tracker/stacked_borrows/diagnostics.rs +++ b/src/borrow_tracker/stacked_borrows/diagnostics.rs @@ -189,7 +189,7 @@ struct RetagOp { #[derive(Debug, Clone, Copy, PartialEq)] pub enum RetagCause { Normal, - FnReturnPlace, + InPlaceFnPassing, FnEntry, TwoPhase, } @@ -501,7 +501,7 @@ impl RetagCause { match self { RetagCause::Normal => "retag", RetagCause::FnEntry => "function-entry retag", - RetagCause::FnReturnPlace => "return-place retag", + RetagCause::InPlaceFnPassing => "in-place function argument/return passing protection", RetagCause::TwoPhase => "two-phase retag", } .to_string() diff --git a/src/borrow_tracker/stacked_borrows/mod.rs b/src/borrow_tracker/stacked_borrows/mod.rs index ca0f69450c..e22b352e74 100644 --- a/src/borrow_tracker/stacked_borrows/mod.rs +++ b/src/borrow_tracker/stacked_borrows/mod.rs @@ -994,35 +994,25 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { } } - /// After a stack frame got pushed, retag the return place so that we are sure - /// it does not alias with anything. - /// - /// This is a HACK because there is nothing in MIR that would make the retag - /// explicit. Also see . - fn sb_retag_return_place(&mut self) -> InterpResult<'tcx> { + /// Protect a place so that it cannot be used any more for the duration of the current function + /// call. + /// + /// This is used to ensure soundness of in-place function argument/return passing. + fn sb_protect_place(&mut self, place: &MPlaceTy<'tcx, Provenance>) -> InterpResult<'tcx> { let this = self.eval_context_mut(); - let return_place = &this.frame().return_place; - if return_place.layout.is_zst() { - // There may not be any memory here, nothing to do. - return Ok(()); - } - // We need this to be in-memory to use tagged pointers. - let return_place = this.force_allocation(&return_place.clone())?; - // We have to turn the place into a pointer to use the existing code. + // We have to turn the place into a pointer to use the usual retagging logic. // (The pointer type does not matter, so we use a raw pointer.) - let ptr_layout = this.layout_of(Ty::new_mut_ptr(this.tcx.tcx, return_place.layout.ty))?; - let val = ImmTy::from_immediate(return_place.to_ref(this), ptr_layout); - // Reborrow it. With protection! That is part of the point. + let ptr_layout = this.layout_of(Ty::new_mut_ptr(this.tcx.tcx, place.layout.ty))?; + let ptr = ImmTy::from_immediate(place.to_ref(this), ptr_layout); + // Reborrow it. With protection! That is the entire point. let new_perm = NewPermission::Uniform { perm: Permission::Unique, access: Some(AccessKind::Write), protector: Some(ProtectorKind::StrongProtector), }; - let val = this.sb_retag_reference(&val, new_perm, RetagCause::FnReturnPlace)?; - // And use reborrowed pointer for return place. - let return_place = this.ref_to_mplace(&val)?; - this.frame_mut().return_place = return_place.into(); + let _new_ptr = this.sb_retag_reference(&ptr, new_perm, RetagCause::InPlaceFnPassing)?; + // We just throw away `new_ptr`, so nobody can access this memory while it is protected. Ok(()) } diff --git a/src/borrow_tracker/tree_borrows/mod.rs b/src/borrow_tracker/tree_borrows/mod.rs index 3bb38a249f..a3f0310f1d 100644 --- a/src/borrow_tracker/tree_borrows/mod.rs +++ b/src/borrow_tracker/tree_borrows/mod.rs @@ -493,36 +493,25 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { } } - /// After a stack frame got pushed, retag the return place so that we are sure - /// it does not alias with anything. - /// - /// This is a HACK because there is nothing in MIR that would make the retag - /// explicit. Also see . - fn tb_retag_return_place(&mut self) -> InterpResult<'tcx> { + /// Protect a place so that it cannot be used any more for the duration of the current function + /// call. + /// + /// This is used to ensure soundness of in-place function argument/return passing. + fn tb_protect_place(&mut self, place: &MPlaceTy<'tcx, Provenance>) -> InterpResult<'tcx> { let this = self.eval_context_mut(); - //this.debug_hint_location(); - let return_place = &this.frame().return_place; - if return_place.layout.is_zst() { - // There may not be any memory here, nothing to do. - return Ok(()); - } - // We need this to be in-memory to use tagged pointers. - let return_place = this.force_allocation(&return_place.clone())?; - // We have to turn the place into a pointer to use the existing code. + // We have to turn the place into a pointer to use the usual retagging logic. // (The pointer type does not matter, so we use a raw pointer.) - let ptr_layout = this.layout_of(Ty::new_mut_ptr(this.tcx.tcx, return_place.layout.ty))?; - let val = ImmTy::from_immediate(return_place.to_ref(this), ptr_layout); - // Reborrow it. With protection! That is part of the point. + let ptr_layout = this.layout_of(Ty::new_mut_ptr(this.tcx.tcx, place.layout.ty))?; + let ptr = ImmTy::from_immediate(place.to_ref(this), ptr_layout); + // Reborrow it. With protection! That is the entire point. let new_perm = Some(NewPermission { initial_state: Permission::new_active(), zero_size: false, protector: Some(ProtectorKind::StrongProtector), }); - let val = this.tb_retag_reference(&val, new_perm)?; - // And use reborrowed pointer for return place. - let return_place = this.ref_to_mplace(&val)?; - this.frame_mut().return_place = return_place.into(); + let _new_ptr = this.tb_retag_reference(&ptr, new_perm)?; + // We just throw away `new_ptr`, so nobody can access this memory while it is protected. Ok(()) } diff --git a/src/lib.rs b/src/lib.rs index 7e92dc7a0c..4a093d7bcc 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -55,6 +55,7 @@ extern crate rustc_index; extern crate rustc_session; extern crate rustc_span; extern crate rustc_target; +extern crate either; // the one from rustc // Necessary to pull in object code as the rest of the rustc crates are shipped only as rmeta // files. diff --git a/src/machine.rs b/src/machine.rs index ac2bad2211..5510e3f94b 100644 --- a/src/machine.rs +++ b/src/machine.rs @@ -7,6 +7,7 @@ use std::fmt; use std::path::Path; use std::process; +use either::Either; use rand::rngs::StdRng; use rand::SeedableRng; @@ -533,7 +534,7 @@ impl<'mir, 'tcx> MiriMachine<'mir, 'tcx> { let target = &tcx.sess.target; match target.arch.as_ref() { "wasm32" | "wasm64" => 64 * 1024, // https://webassembly.github.io/spec/core/exec/runtime.html#memory-instances - "aarch64" => + "aarch64" => { if target.options.vendor.as_ref() == "apple" { // No "definitive" source, but see: // https://www.wwdcnotes.com/notes/wwdc20/10214/ @@ -541,7 +542,8 @@ impl<'mir, 'tcx> MiriMachine<'mir, 'tcx> { 16 * 1024 } else { 4 * 1024 - }, + } + } _ => 4 * 1024, } }; @@ -892,7 +894,7 @@ impl<'mir, 'tcx> Machine<'mir, 'tcx> for MiriMachine<'mir, 'tcx> { ecx: &mut MiriInterpCx<'mir, 'tcx>, instance: ty::Instance<'tcx>, abi: Abi, - args: &[OpTy<'tcx, Provenance>], + args: &[FnArg<'tcx, Provenance>], dest: &PlaceTy<'tcx, Provenance>, ret: Option, unwind: mir::UnwindAction, @@ -905,12 +907,13 @@ impl<'mir, 'tcx> Machine<'mir, 'tcx> for MiriMachine<'mir, 'tcx> { ecx: &mut MiriInterpCx<'mir, 'tcx>, fn_val: Dlsym, abi: Abi, - args: &[OpTy<'tcx, Provenance>], + args: &[FnArg<'tcx, Provenance>], dest: &PlaceTy<'tcx, Provenance>, ret: Option, _unwind: mir::UnwindAction, ) -> InterpResult<'tcx> { - ecx.call_dlsym(fn_val, abi, args, dest, ret) + let args = ecx.copy_fn_args(args)?; // FIXME: Should `InPlace` arguments be reset to uninit? + ecx.call_dlsym(fn_val, abi, &args, dest, ret) } #[inline(always)] @@ -1094,8 +1097,9 @@ impl<'mir, 'tcx> Machine<'mir, 'tcx> for MiriMachine<'mir, 'tcx> { ptr: Pointer, ) -> InterpResult<'tcx> { match ptr.provenance { - Provenance::Concrete { alloc_id, tag } => - intptrcast::GlobalStateInner::expose_ptr(ecx, alloc_id, tag), + Provenance::Concrete { alloc_id, tag } => { + intptrcast::GlobalStateInner::expose_ptr(ecx, alloc_id, tag) + } Provenance::Wildcard => { // No need to do anything for wildcard pointers as // their provenances have already been previously exposed. @@ -1206,6 +1210,25 @@ impl<'mir, 'tcx> Machine<'mir, 'tcx> for MiriMachine<'mir, 'tcx> { Ok(()) } + fn protect_in_place_function_argument( + ecx: &mut InterpCx<'mir, 'tcx, Self>, + place: &PlaceTy<'tcx, Provenance>, + ) -> InterpResult<'tcx> { + // We do need to write `uninit` so that even after the call ends, the former contents of + // this place cannot be observed any more. + ecx.write_uninit(place)?; + // If we have a borrow tracker, we also have it set up protection so that all reads *and + // writes* during this call are insta-UB. + if ecx.machine.borrow_tracker.is_some() { + if let Either::Left(place) = place.as_mplace_or_local() { + ecx.protect_place(&place)?; + } else { + // Locals that don't have their address taken are as protected as they can ever be. + } + } + Ok(()) + } + #[inline(always)] fn init_frame_extra( ecx: &mut InterpCx<'mir, 'tcx, Self>, @@ -1288,8 +1311,17 @@ impl<'mir, 'tcx> Machine<'mir, 'tcx> for MiriMachine<'mir, 'tcx> { let stack_len = ecx.active_thread_stack().len(); ecx.active_thread_mut().set_top_user_relevant_frame(stack_len - 1); } - if ecx.machine.borrow_tracker.is_some() { - ecx.retag_return_place()?; + Ok(()) + } + + fn before_stack_pop( + ecx: &InterpCx<'mir, 'tcx, Self>, + frame: &Frame<'mir, 'tcx, Self::Provenance, Self::FrameExtra>, + ) -> InterpResult<'tcx> { + // We want this *before* the return value copy, because the return place itself is protected + // until we do `end_call` here. + if let Some(borrow_tracker) = &ecx.machine.borrow_tracker { + borrow_tracker.borrow_mut().end_call(&frame.extra); } Ok(()) } @@ -1308,9 +1340,6 @@ impl<'mir, 'tcx> Machine<'mir, 'tcx> for MiriMachine<'mir, 'tcx> { ecx.active_thread_mut().recompute_top_user_relevant_frame(); } let timing = frame.extra.timing.take(); - if let Some(borrow_tracker) = &ecx.machine.borrow_tracker { - borrow_tracker.borrow_mut().end_call(&frame.extra); - } let res = ecx.handle_stack_pop_unwind(frame.extra, unwinding); if let Some(profiler) = ecx.machine.profiler.as_ref() { profiler.finish_recording_interval_event(timing.unwrap()); diff --git a/src/shims/mod.rs b/src/shims/mod.rs index a423a0786b..1027b24e30 100644 --- a/src/shims/mod.rs +++ b/src/shims/mod.rs @@ -31,7 +31,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { &mut self, instance: ty::Instance<'tcx>, abi: Abi, - args: &[OpTy<'tcx, Provenance>], + args: &[FnArg<'tcx, Provenance>], dest: &PlaceTy<'tcx, Provenance>, ret: Option, unwind: mir::UnwindAction, @@ -41,7 +41,8 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { // There are some more lang items we want to hook that CTFE does not hook (yet). if this.tcx.lang_items().align_offset_fn() == Some(instance.def.def_id()) { - let [ptr, align] = check_arg_count(args)?; + let args = this.copy_fn_args(args)?; + let [ptr, align] = check_arg_count(&args)?; if this.align_offset(ptr, align, dest, ret, unwind)? { return Ok(None); } @@ -55,7 +56,8 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { // to run extra MIR), and Ok(Some(body)) if we found MIR to run for the // foreign function // Any needed call to `goto_block` will be performed by `emulate_foreign_item`. - return this.emulate_foreign_item(instance.def_id(), abi, args, dest, ret, unwind); + let args = this.copy_fn_args(args)?; // FIXME: Should `InPlace` arguments be reset to uninit? + return this.emulate_foreign_item(instance.def_id(), abi, &args, dest, ret, unwind); } // Otherwise, load the MIR. diff --git a/tests/fail/function_calls/arg_inplace_mutate.rs b/tests/fail/function_calls/arg_inplace_mutate.rs new file mode 100644 index 0000000000..625a8bda8a --- /dev/null +++ b/tests/fail/function_calls/arg_inplace_mutate.rs @@ -0,0 +1,34 @@ +//@revisions: stack tree +//@[tree]compile-flags: -Zmiri-tree-borrows +#![feature(custom_mir, core_intrinsics)] +use std::intrinsics::mir::*; + +pub struct S(i32); + +#[custom_mir(dialect = "runtime", phase = "optimized")] +fn main() { + mir! { + let unit: (); + { + let non_copy = S(42); + let ptr = std::ptr::addr_of_mut!(non_copy); + // Inside `callee`, the first argument and `*ptr` are basically + // aliasing places! + Call(unit, after_call, callee(Move(*ptr), ptr)) + } + after_call = { + Return() + } + + } +} + +pub fn callee(x: S, ptr: *mut S) { + // With the setup above, if `x` is indeed moved in + // (i.e. we actually just get a pointer to the underlying storage), + // then writing to `ptr` will change the value stored in `x`! + unsafe { ptr.write(S(0)) }; + //~[stack]^ ERROR: not granting access + //~[tree]| ERROR: /write access .* forbidden/ + assert_eq!(x.0, 42); +} diff --git a/tests/fail/function_calls/arg_inplace_mutate.stack.stderr b/tests/fail/function_calls/arg_inplace_mutate.stack.stderr new file mode 100644 index 0000000000..471dc1dd6d --- /dev/null +++ b/tests/fail/function_calls/arg_inplace_mutate.stack.stderr @@ -0,0 +1,37 @@ +error: Undefined Behavior: not granting access to tag because that would remove [Unique for ] which is strongly protected because it is an argument of call ID + --> $DIR/arg_inplace_mutate.rs:LL:CC + | +LL | unsafe { ptr.write(S(0)) }; + | ^^^^^^^^^^^^^^^ not granting access to tag because that would remove [Unique for ] which is strongly protected because it is an argument of call ID + | + = help: this indicates a potential bug in the program: it performed an invalid operation, but the Stacked Borrows rules it violated are still experimental + = help: see https://github.com/rust-lang/unsafe-code-guidelines/blob/master/wip/stacked-borrows.md for further information +help: was created by a SharedReadWrite retag at offsets [0x0..0x4] + --> $DIR/arg_inplace_mutate.rs:LL:CC + | +LL | / mir! { +LL | | let unit: (); +LL | | { +LL | | let non_copy = S(42); +... | +LL | | +LL | | } + | |_____^ +help: is this argument + --> $DIR/arg_inplace_mutate.rs:LL:CC + | +LL | unsafe { ptr.write(S(0)) }; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^ + = note: BACKTRACE (of the first span): + = note: inside `callee` at $DIR/arg_inplace_mutate.rs:LL:CC +note: inside `main` + --> $DIR/arg_inplace_mutate.rs:LL:CC + | +LL | Call(unit, after_call, callee(Move(*ptr), ptr)) + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + = note: this error originates in the macro `::core::intrinsics::mir::__internal_remove_let` which comes from the expansion of the macro `mir` (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 + +error: aborting due to previous error + diff --git a/tests/fail/function_calls/arg_inplace_mutate.tree.stderr b/tests/fail/function_calls/arg_inplace_mutate.tree.stderr new file mode 100644 index 0000000000..35c02cc2eb --- /dev/null +++ b/tests/fail/function_calls/arg_inplace_mutate.tree.stderr @@ -0,0 +1,39 @@ +error: Undefined Behavior: write access through (root of the allocation) is forbidden + --> $DIR/arg_inplace_mutate.rs:LL:CC + | +LL | unsafe { ptr.write(S(0)) }; + | ^^^^^^^^^^^^^^^ write access through (root of the allocation) is forbidden + | + = help: this indicates a potential bug in the program: it performed an invalid operation, but the Tree Borrows rules it violated are still experimental + = help: the accessed tag (root of the allocation) is foreign to the protected tag (i.e., it is not a child) + = help: this foreign write access would cause the protected tag to transition from Active to Disabled + = help: this transition would be a loss of read and write permissions, which is not allowed for protected tags +help: the accessed tag was created here + --> $DIR/arg_inplace_mutate.rs:LL:CC + | +LL | / mir! { +LL | | let unit: (); +LL | | { +LL | | let non_copy = S(42); +... | +LL | | +LL | | } + | |_____^ +help: the protected tag was created here, in the initial state Active + --> $DIR/arg_inplace_mutate.rs:LL:CC + | +LL | unsafe { ptr.write(S(0)) }; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^ + = note: BACKTRACE (of the first span): + = note: inside `callee` at $DIR/arg_inplace_mutate.rs:LL:CC +note: inside `main` + --> $DIR/arg_inplace_mutate.rs:LL:CC + | +LL | Call(unit, after_call, callee(Move(*ptr), ptr)) + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + = note: this error originates in the macro `::core::intrinsics::mir::__internal_remove_let` which comes from the expansion of the macro `mir` (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 + +error: aborting due to previous error + diff --git a/tests/fail/function_calls/arg_inplace_observe_after.rs b/tests/fail/function_calls/arg_inplace_observe_after.rs new file mode 100644 index 0000000000..8eda913feb --- /dev/null +++ b/tests/fail/function_calls/arg_inplace_observe_after.rs @@ -0,0 +1,27 @@ +#![feature(custom_mir, core_intrinsics)] +use std::intrinsics::mir::*; + +pub struct S(i32); + +#[custom_mir(dialect = "runtime", phase = "optimized")] +fn main() { + // FIXME: the span is not great (probably caused by custom MIR) + mir! { //~ERROR: uninitialized + let unit: (); + { + let non_copy = S(42); + // This could change `non_copy` in-place + Call(unit, after_call, change_arg(Move(non_copy))) + } + after_call = { + // So now we must not be allowed to observe non-copy again. + let _observe = non_copy.0; + Return() + } + + } +} + +pub fn change_arg(mut x: S) { + x.0 = 0; +} diff --git a/tests/fail/function_calls/arg_inplace_observe_after.stderr b/tests/fail/function_calls/arg_inplace_observe_after.stderr new file mode 100644 index 0000000000..3ff7976c70 --- /dev/null +++ b/tests/fail/function_calls/arg_inplace_observe_after.stderr @@ -0,0 +1,22 @@ +error: Undefined Behavior: using uninitialized data, but this operation requires initialized memory + --> $DIR/arg_inplace_observe_after.rs:LL:CC + | +LL | / mir! { +LL | | let unit: (); +LL | | { +LL | | let non_copy = S(42); +... | +LL | | +LL | | } + | |_____^ using uninitialized data, but this operation requires initialized memory + | + = 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 RUSTLIB/core/src/intrinsics/mir.rs:LL:CC + = note: this error originates in the macro `::core::intrinsics::mir::__internal_remove_let` which comes from the expansion of the macro `mir` (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 + +error: aborting due to previous error + diff --git a/tests/fail/function_calls/arg_inplace_observe_during.none.stderr b/tests/fail/function_calls/arg_inplace_observe_during.none.stderr new file mode 100644 index 0000000000..baa9148479 --- /dev/null +++ b/tests/fail/function_calls/arg_inplace_observe_during.none.stderr @@ -0,0 +1,20 @@ +error: Undefined Behavior: using uninitialized data, but this operation requires initialized memory + --> $DIR/arg_inplace_observe_during.rs:LL:CC + | +LL | unsafe { ptr.read() }; + | ^^^^^^^^^^ using uninitialized data, but this operation requires initialized memory + | + = 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 `change_arg` at $DIR/arg_inplace_observe_during.rs:LL:CC +note: inside `main` + --> $DIR/arg_inplace_observe_during.rs:LL:CC + | +LL | Call(unit, after_call, change_arg(Move(*ptr), ptr)) + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +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/function_calls/arg_inplace_observe_during.rs b/tests/fail/function_calls/arg_inplace_observe_during.rs new file mode 100644 index 0000000000..2e57872db9 --- /dev/null +++ b/tests/fail/function_calls/arg_inplace_observe_during.rs @@ -0,0 +1,34 @@ +//@revisions: stack tree none +//@[tree]compile-flags: -Zmiri-tree-borrows +//@[none]compile-flags: -Zmiri-disable-stacked-borrows +#![feature(custom_mir, core_intrinsics)] +use std::intrinsics::mir::*; + +pub struct S(i32); + +#[custom_mir(dialect = "runtime", phase = "optimized")] +fn main() { + mir! { + let unit: (); + { + let non_copy = S(42); + let ptr = std::ptr::addr_of_mut!(non_copy); + // This could change `non_copy` in-place + Call(unit, after_call, change_arg(Move(*ptr), ptr)) + } + after_call = { + Return() + } + + } +} + +pub fn change_arg(mut x: S, ptr: *mut S) { + x.0 = 0; + // If `x` got passed in-place, we'd see the write through `ptr`! + // Make sure we are not allowed to do that read. + unsafe { ptr.read() }; + //~[stack]^ ERROR: not granting access + //~[tree]| ERROR: /read access .* forbidden/ + //~[none]| ERROR: uninitialized +} diff --git a/tests/fail/function_calls/arg_inplace_observe_during.stack.stderr b/tests/fail/function_calls/arg_inplace_observe_during.stack.stderr new file mode 100644 index 0000000000..a842d3a804 --- /dev/null +++ b/tests/fail/function_calls/arg_inplace_observe_during.stack.stderr @@ -0,0 +1,37 @@ +error: Undefined Behavior: not granting access to tag because that would remove [Unique for ] which is strongly protected because it is an argument of call ID + --> $DIR/arg_inplace_observe_during.rs:LL:CC + | +LL | unsafe { ptr.read() }; + | ^^^^^^^^^^ not granting access to tag because that would remove [Unique for ] which is strongly protected because it is an argument of call ID + | + = help: this indicates a potential bug in the program: it performed an invalid operation, but the Stacked Borrows rules it violated are still experimental + = help: see https://github.com/rust-lang/unsafe-code-guidelines/blob/master/wip/stacked-borrows.md for further information +help: was created by a SharedReadWrite retag at offsets [0x0..0x4] + --> $DIR/arg_inplace_observe_during.rs:LL:CC + | +LL | / mir! { +LL | | let unit: (); +LL | | { +LL | | let non_copy = S(42); +... | +LL | | +LL | | } + | |_____^ +help: is this argument + --> $DIR/arg_inplace_observe_during.rs:LL:CC + | +LL | x.0 = 0; + | ^^^^^^^ + = note: BACKTRACE (of the first span): + = note: inside `change_arg` at $DIR/arg_inplace_observe_during.rs:LL:CC +note: inside `main` + --> $DIR/arg_inplace_observe_during.rs:LL:CC + | +LL | Call(unit, after_call, change_arg(Move(*ptr), ptr)) + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + = note: this error originates in the macro `::core::intrinsics::mir::__internal_remove_let` which comes from the expansion of the macro `mir` (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 + +error: aborting due to previous error + diff --git a/tests/fail/function_calls/arg_inplace_observe_during.tree.stderr b/tests/fail/function_calls/arg_inplace_observe_during.tree.stderr new file mode 100644 index 0000000000..cbd76c38f6 --- /dev/null +++ b/tests/fail/function_calls/arg_inplace_observe_during.tree.stderr @@ -0,0 +1,39 @@ +error: Undefined Behavior: read access through (root of the allocation) is forbidden + --> $DIR/arg_inplace_observe_during.rs:LL:CC + | +LL | unsafe { ptr.read() }; + | ^^^^^^^^^^ read access through (root of the allocation) is forbidden + | + = help: this indicates a potential bug in the program: it performed an invalid operation, but the Tree Borrows rules it violated are still experimental + = help: the accessed tag (root of the allocation) is foreign to the protected tag (i.e., it is not a child) + = help: this foreign read access would cause the protected tag to transition from Active to Frozen + = help: this transition would be a loss of write permissions, which is not allowed for protected tags +help: the accessed tag was created here + --> $DIR/arg_inplace_observe_during.rs:LL:CC + | +LL | / mir! { +LL | | let unit: (); +LL | | { +LL | | let non_copy = S(42); +... | +LL | | +LL | | } + | |_____^ +help: the protected tag was created here, in the initial state Active + --> $DIR/arg_inplace_observe_during.rs:LL:CC + | +LL | x.0 = 0; + | ^^^^^^^ + = note: BACKTRACE (of the first span): + = note: inside `change_arg` at $DIR/arg_inplace_observe_during.rs:LL:CC +note: inside `main` + --> $DIR/arg_inplace_observe_during.rs:LL:CC + | +LL | Call(unit, after_call, change_arg(Move(*ptr), ptr)) + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + = note: this error originates in the macro `::core::intrinsics::mir::__internal_remove_let` which comes from the expansion of the macro `mir` (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 + +error: aborting due to previous error + diff --git a/tests/fail/function_calls/return_pointer_aliasing.none.stderr b/tests/fail/function_calls/return_pointer_aliasing.none.stderr new file mode 100644 index 0000000000..9d9dfc89f8 --- /dev/null +++ b/tests/fail/function_calls/return_pointer_aliasing.none.stderr @@ -0,0 +1,20 @@ +error: Undefined Behavior: using uninitialized data, but this operation requires initialized memory + --> $DIR/return_pointer_aliasing.rs:LL:CC + | +LL | unsafe { ptr.read() }; + | ^^^^^^^^^^ using uninitialized data, but this operation requires initialized memory + | + = 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 `myfun` at $DIR/return_pointer_aliasing.rs:LL:CC +note: inside `main` + --> $DIR/return_pointer_aliasing.rs:LL:CC + | +LL | Call(*ptr, after_call, myfun(ptr)) + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +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/both_borrows/return_pointer_aliasing.rs b/tests/fail/function_calls/return_pointer_aliasing.rs similarity index 70% rename from tests/fail/both_borrows/return_pointer_aliasing.rs rename to tests/fail/function_calls/return_pointer_aliasing.rs index 03886e7874..829809102f 100644 --- a/tests/fail/both_borrows/return_pointer_aliasing.rs +++ b/tests/fail/function_calls/return_pointer_aliasing.rs @@ -1,11 +1,11 @@ -//@revisions: stack tree +//@revisions: stack tree none //@[tree]compile-flags: -Zmiri-tree-borrows +//@[none]compile-flags: -Zmiri-disable-stacked-borrows #![feature(raw_ref_op)] #![feature(core_intrinsics)] #![feature(custom_mir)] use std::intrinsics::mir::*; -use std::mem::MaybeUninit; #[custom_mir(dialect = "runtime", phase = "optimized")] pub fn main() { @@ -25,8 +25,10 @@ pub fn main() { } fn myfun(ptr: *mut i32) -> i32 { - unsafe { ptr.cast::>().read() }; - //~[stack]^ ERROR: /not granting access/ + unsafe { ptr.read() }; + //~[stack]^ ERROR: not granting access //~[tree]| ERROR: /read access .* forbidden/ + //~[none]| ERROR: uninitialized + // Without an aliasing model, reads are "fine" but at least they return uninit data. 13 } diff --git a/tests/fail/both_borrows/return_pointer_aliasing.stack.stderr b/tests/fail/function_calls/return_pointer_aliasing.stack.stderr similarity index 78% rename from tests/fail/both_borrows/return_pointer_aliasing.stack.stderr rename to tests/fail/function_calls/return_pointer_aliasing.stack.stderr index 5bdddf5e49..d486dcb95d 100644 --- a/tests/fail/both_borrows/return_pointer_aliasing.stack.stderr +++ b/tests/fail/function_calls/return_pointer_aliasing.stack.stderr @@ -1,8 +1,8 @@ error: Undefined Behavior: not granting access to tag because that would remove [Unique for ] which is strongly protected because it is an argument of call ID --> $DIR/return_pointer_aliasing.rs:LL:CC | -LL | unsafe { ptr.cast::>().read() }; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ not granting access to tag because that would remove [Unique for ] which is strongly protected because it is an argument of call ID +LL | unsafe { ptr.read() }; + | ^^^^^^^^^^ not granting access to tag because that would remove [Unique for ] which is strongly protected because it is an argument of call ID | = help: this indicates a potential bug in the program: it performed an invalid operation, but the Stacked Borrows rules it violated are still experimental = help: see https://github.com/rust-lang/unsafe-code-guidelines/blob/master/wip/stacked-borrows.md for further information @@ -20,13 +20,8 @@ LL | | } help: is this argument --> $DIR/return_pointer_aliasing.rs:LL:CC | -LL | / fn myfun(ptr: *mut i32) -> i32 { -LL | | unsafe { ptr.cast::>().read() }; -LL | | -LL | | -LL | | 13 -LL | | } - | |_^ +LL | unsafe { ptr.read() }; + | ^^^^^^^^^^^^^^^^^^^^^ = note: BACKTRACE (of the first span): = note: inside `myfun` at $DIR/return_pointer_aliasing.rs:LL:CC note: inside `main` diff --git a/tests/fail/both_borrows/return_pointer_aliasing.tree.stderr b/tests/fail/function_calls/return_pointer_aliasing.tree.stderr similarity index 83% rename from tests/fail/both_borrows/return_pointer_aliasing.tree.stderr rename to tests/fail/function_calls/return_pointer_aliasing.tree.stderr index dd76d65c7c..c2c9de3f4e 100644 --- a/tests/fail/both_borrows/return_pointer_aliasing.tree.stderr +++ b/tests/fail/function_calls/return_pointer_aliasing.tree.stderr @@ -1,8 +1,8 @@ error: Undefined Behavior: read access through (root of the allocation) is forbidden --> $DIR/return_pointer_aliasing.rs:LL:CC | -LL | unsafe { ptr.cast::>().read() }; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ read access through (root of the allocation) is forbidden +LL | unsafe { ptr.read() }; + | ^^^^^^^^^^ read access through (root of the allocation) is forbidden | = help: this indicates a potential bug in the program: it performed an invalid operation, but the Tree Borrows rules it violated are still experimental = help: the accessed tag (root of the allocation) is foreign to the protected tag (i.e., it is not a child) @@ -22,13 +22,8 @@ LL | | } help: the protected tag was created here, in the initial state Active --> $DIR/return_pointer_aliasing.rs:LL:CC | -LL | / fn myfun(ptr: *mut i32) -> i32 { -LL | | unsafe { ptr.cast::>().read() }; -LL | | -LL | | -LL | | 13 -LL | | } - | |_^ +LL | unsafe { ptr.read() }; + | ^^^^^^^^^^^^^^^^^^^^^ = note: BACKTRACE (of the first span): = note: inside `myfun` at $DIR/return_pointer_aliasing.rs:LL:CC note: inside `main` diff --git a/tests/pass/function_calls/return_place_on_heap.rs b/tests/pass/function_calls/return_place_on_heap.rs new file mode 100644 index 0000000000..dcfebd0f82 --- /dev/null +++ b/tests/pass/function_calls/return_place_on_heap.rs @@ -0,0 +1,25 @@ +#![feature(raw_ref_op)] +#![feature(core_intrinsics)] +#![feature(custom_mir)] + +use std::intrinsics::mir::*; + +// Make sure calls with the return place "on the heap" work. +#[custom_mir(dialect = "runtime", phase = "optimized")] +pub fn main() { + mir! { + { + let x = 0; + let ptr = &raw mut x; + Call(*ptr, after_call, myfun()) + } + + after_call = { + Return() + } + } +} + +fn myfun() -> i32 { + 13 +} From b6df7e9f5833fff962b5ccf420b29aec5987daad Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Mon, 10 Jul 2023 22:14:28 +0200 Subject: [PATCH 4/5] miri tree borrows: skip retag_reference early if there is no NewPermission --- src/borrow_tracker/tree_borrows/mod.rs | 28 ++++++++++++++------------ 1 file changed, 15 insertions(+), 13 deletions(-) diff --git a/src/borrow_tracker/tree_borrows/mod.rs b/src/borrow_tracker/tree_borrows/mod.rs index a3f0310f1d..274a4a0aab 100644 --- a/src/borrow_tracker/tree_borrows/mod.rs +++ b/src/borrow_tracker/tree_borrows/mod.rs @@ -178,7 +178,7 @@ trait EvalContextPrivExt<'mir: 'ecx, 'tcx: 'mir, 'ecx>: crate::MiriInterpCxExt<' &mut self, place: &MPlaceTy<'tcx, Provenance>, // parent tag extracted from here ptr_size: Size, - new_perm: Option, + new_perm: NewPermission, new_tag: BorTag, ) -> InterpResult<'tcx, Option<(AllocId, BorTag)>> { let this = self.eval_context_mut(); @@ -256,10 +256,6 @@ trait EvalContextPrivExt<'mir: 'ecx, 'tcx: 'mir, 'ecx>: crate::MiriInterpCxExt<' ptr_size.bytes() ); - let Some(new_perm) = new_perm else { - return Ok(Some((alloc_id, orig_tag))); - }; - if let Some(protect) = new_perm.protector { // We register the protection in two different places. // This makes creating a protector slower, but checking whether a tag @@ -305,7 +301,7 @@ trait EvalContextPrivExt<'mir: 'ecx, 'tcx: 'mir, 'ecx>: crate::MiriInterpCxExt<' fn tb_retag_reference( &mut self, val: &ImmTy<'tcx, Provenance>, - new_perm: Option, + new_perm: NewPermission, ) -> InterpResult<'tcx, ImmTy<'tcx, Provenance>> { let this = self.eval_context_mut(); // We want a place for where the ptr *points to*, so we get one. @@ -317,7 +313,7 @@ trait EvalContextPrivExt<'mir: 'ecx, 'tcx: 'mir, 'ecx>: crate::MiriInterpCxExt<' // - if the pointer is not reborrowed (raw pointer) or if `zero_size` is set // then we override the size to do a zero-length reborrow. let reborrow_size = match new_perm { - Some(NewPermission { zero_size: false, .. }) => + NewPermission { zero_size: false, .. } => this.size_and_align_of_mplace(&place)? .map(|(size, _)| size) .unwrap_or(place.layout.size), @@ -374,7 +370,11 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { NewPermission::from_ref_ty(pointee, mutability, kind, this), _ => None, }; - this.tb_retag_reference(val, new_perm) + if let Some(new_perm) = new_perm { + this.tb_retag_reference(val, new_perm) + } else { + Ok(val.clone()) + } } /// Retag all pointers that are stored in this place. @@ -405,9 +405,11 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { place: &PlaceTy<'tcx, Provenance>, new_perm: Option, ) -> InterpResult<'tcx> { - let val = self.ecx.read_immediate(&self.ecx.place_to_op(place)?)?; - let val = self.ecx.tb_retag_reference(&val, new_perm)?; - self.ecx.write_immediate(*val, place)?; + if let Some(new_perm) = new_perm { + let val = self.ecx.read_immediate(&self.ecx.place_to_op(place)?)?; + let val = self.ecx.tb_retag_reference(&val, new_perm)?; + self.ecx.write_immediate(*val, place)?; + } Ok(()) } } @@ -505,11 +507,11 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { let ptr_layout = this.layout_of(Ty::new_mut_ptr(this.tcx.tcx, place.layout.ty))?; let ptr = ImmTy::from_immediate(place.to_ref(this), ptr_layout); // Reborrow it. With protection! That is the entire point. - let new_perm = Some(NewPermission { + let new_perm = NewPermission { initial_state: Permission::new_active(), zero_size: false, protector: Some(ProtectorKind::StrongProtector), - }); + }; let _new_ptr = this.tb_retag_reference(&ptr, new_perm)?; // We just throw away `new_ptr`, so nobody can access this memory while it is protected. From f8a331a08df8c299e9e7bbacfad0957838f779b2 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Tue, 11 Jul 2023 15:50:11 +0200 Subject: [PATCH 5/5] fix handling of alignment for dyn-sized places --- tests/fail/unaligned_pointers/dyn_alignment.rs | 7 ++++--- tests/fail/unaligned_pointers/dyn_alignment.stderr | 4 ++-- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/tests/fail/unaligned_pointers/dyn_alignment.rs b/tests/fail/unaligned_pointers/dyn_alignment.rs index 6d31ded75c..555aa57de3 100644 --- a/tests/fail/unaligned_pointers/dyn_alignment.rs +++ b/tests/fail/unaligned_pointers/dyn_alignment.rs @@ -1,5 +1,6 @@ -// should find the bug even without validation and stacked borrows, but gets masked by optimizations -//@compile-flags: -Zmiri-disable-validation -Zmiri-disable-stacked-borrows -Zmir-opt-level=0 -Cdebug-assertions=no +// should find the bug even without, but gets masked by optimizations +//@compile-flags: -Zmiri-disable-stacked-borrows -Zmir-opt-level=0 -Cdebug-assertions=no +//@normalize-stderr-test: "but found [0-9]+" -> "but found $$ALIGN" #[repr(align(256))] #[derive(Debug)] @@ -19,6 +20,6 @@ fn main() { (&mut ptr as *mut _ as *mut *const u8).write(&buf as *const _ as *const u8); } // Re-borrow that. This should be UB. - let _ptr = &*ptr; //~ERROR: alignment 256 is required + let _ptr = &*ptr; //~ERROR: required 256 byte alignment } } diff --git a/tests/fail/unaligned_pointers/dyn_alignment.stderr b/tests/fail/unaligned_pointers/dyn_alignment.stderr index a900b46612..503721b955 100644 --- a/tests/fail/unaligned_pointers/dyn_alignment.stderr +++ b/tests/fail/unaligned_pointers/dyn_alignment.stderr @@ -1,8 +1,8 @@ -error: Undefined Behavior: accessing memory with alignment ALIGN, but alignment ALIGN is required +error: Undefined Behavior: constructing invalid value: encountered an unaligned reference (required 256 byte alignment but found $ALIGN) --> $DIR/dyn_alignment.rs:LL:CC | LL | let _ptr = &*ptr; - | ^^^^^ accessing memory with alignment ALIGN, but alignment ALIGN is required + | ^^^^^ constructing invalid value: encountered an unaligned reference (required 256 byte alignment but found $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