Skip to content

Commit 6fe2fc8

Browse files
RalfJungRalfJung
committed
miri: protect Move() function arguments during the call
1 parent 742db95 commit 6fe2fc8

21 files changed

+422
-85
lines changed

‎src/borrow_tracker/mod.rs

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -302,12 +302,12 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
302302
}
303303
}
304304

305-
fn retag_return_place(&mut self) -> InterpResult<'tcx> {
305+
fn protect_place(&mut self, place: &MPlaceTy<'tcx, Provenance>) -> InterpResult<'tcx> {
306306
let this = self.eval_context_mut();
307307
let method = this.machine.borrow_tracker.as_ref().unwrap().borrow().borrow_tracker_method;
308308
match method {
309-
BorrowTrackerMethod::StackedBorrows => this.sb_retag_return_place(),
310-
BorrowTrackerMethod::TreeBorrows => this.tb_retag_return_place(),
309+
BorrowTrackerMethod::StackedBorrows => this.sb_protect_place(place),
310+
BorrowTrackerMethod::TreeBorrows => this.tb_protect_place(place),
311311
}
312312
}
313313

‎src/borrow_tracker/stacked_borrows/diagnostics.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -189,7 +189,7 @@ struct RetagOp {
189189
#[derive(Debug, Clone, Copy, PartialEq)]
190190
pub enum RetagCause {
191191
Normal,
192-
FnReturnPlace,
192+
InPlaceFnPassing,
193193
FnEntry,
194194
TwoPhase,
195195
}
@@ -501,7 +501,7 @@ impl RetagCause {
501501
match self {
502502
RetagCause::Normal => "retag",
503503
RetagCause::FnEntry => "function-entry retag",
504-
RetagCause::FnReturnPlace => "return-place retag",
504+
RetagCause::InPlaceFnPassing => "in-place function argument/return passing protection",
505505
RetagCause::TwoPhase => "two-phase retag",
506506
}
507507
.to_string()

‎src/borrow_tracker/stacked_borrows/mod.rs

Lines changed: 11 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -994,35 +994,25 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
994994
}
995995
}
996996

997-
/// After a stack frame got pushed, retag the return place so that we are sure
998-
/// it does not alias with anything.
999-
///
1000-
/// This is a HACK because there is nothing in MIR that would make the retag
1001-
/// explicit. Also see <https://github.com/rust-lang/rust/issues/71117>.
1002-
fn sb_retag_return_place(&mut self) -> InterpResult<'tcx> {
997+
/// Protect a place so that it cannot be used any more for the duration of the current function
998+
/// call.
999+
///
1000+
/// This is used to ensure soundness of in-place function argument/return passing.
1001+
fn sb_protect_place(&mut self, place: &MPlaceTy<'tcx, Provenance>) -> InterpResult<'tcx> {
10031002
let this = self.eval_context_mut();
1004-
let return_place = &this.frame().return_place;
1005-
if return_place.layout.is_zst() {
1006-
// There may not be any memory here, nothing to do.
1007-
return Ok(());
1008-
}
1009-
// We need this to be in-memory to use tagged pointers.
1010-
let return_place = this.force_allocation(&return_place.clone())?;
10111003

1012-
// We have to turn the place into a pointer to use the existing code.
1004+
// We have to turn the place into a pointer to use the usual retagging logic.
10131005
// (The pointer type does not matter, so we use a raw pointer.)
1014-
let ptr_layout = this.layout_of(Ty::new_mut_ptr(this.tcx.tcx, return_place.layout.ty))?;
1015-
let val = ImmTy::from_immediate(return_place.to_ref(this), ptr_layout);
1016-
// Reborrow it. With protection! That is part of the point.
1006+
let ptr_layout = this.layout_of(Ty::new_mut_ptr(this.tcx.tcx, place.layout.ty))?;
1007+
let ptr = ImmTy::from_immediate(place.to_ref(this), ptr_layout);
1008+
// Reborrow it. With protection! That is the entire point.
10171009
let new_perm = NewPermission::Uniform {
10181010
perm: Permission::Unique,
10191011
access: Some(AccessKind::Write),
10201012
protector: Some(ProtectorKind::StrongProtector),
10211013
};
1022-
let val = this.sb_retag_reference(&val, new_perm, RetagCause::FnReturnPlace)?;
1023-
// And use reborrowed pointer for return place.
1024-
let return_place = this.ref_to_mplace(&val)?;
1025-
this.frame_mut().return_place = return_place.into();
1014+
let _new_ptr = this.sb_retag_reference(&ptr, new_perm, RetagCause::InPlaceFnPassing)?;
1015+
// We just throw away `new_ptr`, so nobody can access this memory while it is protected.
10261016

10271017
Ok(())
10281018
}

‎src/borrow_tracker/tree_borrows/mod.rs

Lines changed: 11 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -493,36 +493,25 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
493493
}
494494
}
495495

496-
/// After a stack frame got pushed, retag the return place so that we are sure
497-
/// it does not alias with anything.
498-
///
499-
/// This is a HACK because there is nothing in MIR that would make the retag
500-
/// explicit. Also see <https://github.com/rust-lang/rust/issues/71117>.
501-
fn tb_retag_return_place(&mut self) -> InterpResult<'tcx> {
496+
/// Protect a place so that it cannot be used any more for the duration of the current function
497+
/// call.
498+
///
499+
/// This is used to ensure soundness of in-place function argument/return passing.
500+
fn tb_protect_place(&mut self, place: &MPlaceTy<'tcx, Provenance>) -> InterpResult<'tcx> {
502501
let this = self.eval_context_mut();
503-
//this.debug_hint_location();
504-
let return_place = &this.frame().return_place;
505-
if return_place.layout.is_zst() {
506-
// There may not be any memory here, nothing to do.
507-
return Ok(());
508-
}
509-
// We need this to be in-memory to use tagged pointers.
510-
let return_place = this.force_allocation(&return_place.clone())?;
511502

512-
// We have to turn the place into a pointer to use the existing code.
503+
// We have to turn the place into a pointer to use the usual retagging logic.
513504
// (The pointer type does not matter, so we use a raw pointer.)
514-
let ptr_layout = this.layout_of(Ty::new_mut_ptr(this.tcx.tcx, return_place.layout.ty))?;
515-
let val = ImmTy::from_immediate(return_place.to_ref(this), ptr_layout);
516-
// Reborrow it. With protection! That is part of the point.
505+
let ptr_layout = this.layout_of(Ty::new_mut_ptr(this.tcx.tcx, place.layout.ty))?;
506+
let ptr = ImmTy::from_immediate(place.to_ref(this), ptr_layout);
507+
// Reborrow it. With protection! That is the entire point.
517508
let new_perm = Some(NewPermission {
518509
initial_state: Permission::new_active(),
519510
zero_size: false,
520511
protector: Some(ProtectorKind::StrongProtector),
521512
});
522-
let val = this.tb_retag_reference(&val, new_perm)?;
523-
// And use reborrowed pointer for return place.
524-
let return_place = this.ref_to_mplace(&val)?;
525-
this.frame_mut().return_place = return_place.into();
513+
let _new_ptr = this.tb_retag_reference(&ptr, new_perm)?;
514+
// We just throw away `new_ptr`, so nobody can access this memory while it is protected.
526515

527516
Ok(())
528517
}

‎src/lib.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,7 @@ extern crate rustc_index;
5555
extern crate rustc_session;
5656
extern crate rustc_span;
5757
extern crate rustc_target;
58+
extern crate either; // the one from rustc
5859

5960
// Necessary to pull in object code as the rest of the rustc crates are shipped only as rmeta
6061
// files.

‎src/machine.rs

Lines changed: 41 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ use std::fmt;
77
use std::path::Path;
88
use std::process;
99

10+
use either::Either;
1011
use rand::rngs::StdRng;
1112
use rand::SeedableRng;
1213

@@ -533,15 +534,16 @@ impl<'mir, 'tcx> MiriMachine<'mir, 'tcx> {
533534
let target = &tcx.sess.target;
534535
match target.arch.as_ref() {
535536
"wasm32" | "wasm64" => 64 * 1024, // https://webassembly.github.io/spec/core/exec/runtime.html#memory-instances
536-
"aarch64" =>
537+
"aarch64" => {
537538
if target.options.vendor.as_ref() == "apple" {
538539
// No "definitive" source, but see:
539540
// https://www.wwdcnotes.com/notes/wwdc20/10214/
540541
// https://github.com/ziglang/zig/issues/11308 etc.
541542
16 * 1024
542543
} else {
543544
4 * 1024
544-
},
545+
}
546+
}
545547
_ => 4 * 1024,
546548
}
547549
};
@@ -892,7 +894,7 @@ impl<'mir, 'tcx> Machine<'mir, 'tcx> for MiriMachine<'mir, 'tcx> {
892894
ecx: &mut MiriInterpCx<'mir, 'tcx>,
893895
instance: ty::Instance<'tcx>,
894896
abi: Abi,
895-
args: &[OpTy<'tcx, Provenance>],
897+
args: &[FnArg<'tcx, Provenance>],
896898
dest: &PlaceTy<'tcx, Provenance>,
897899
ret: Option<mir::BasicBlock>,
898900
unwind: mir::UnwindAction,
@@ -905,12 +907,13 @@ impl<'mir, 'tcx> Machine<'mir, 'tcx> for MiriMachine<'mir, 'tcx> {
905907
ecx: &mut MiriInterpCx<'mir, 'tcx>,
906908
fn_val: Dlsym,
907909
abi: Abi,
908-
args: &[OpTy<'tcx, Provenance>],
910+
args: &[FnArg<'tcx, Provenance>],
909911
dest: &PlaceTy<'tcx, Provenance>,
910912
ret: Option<mir::BasicBlock>,
911913
_unwind: mir::UnwindAction,
912914
) -> InterpResult<'tcx> {
913-
ecx.call_dlsym(fn_val, abi, args, dest, ret)
915+
let args = ecx.copy_fn_args(args)?; // FIXME: Should `InPlace` arguments be reset to uninit?
916+
ecx.call_dlsym(fn_val, abi, &args, dest, ret)
914917
}
915918

916919
#[inline(always)]
@@ -1094,8 +1097,9 @@ impl<'mir, 'tcx> Machine<'mir, 'tcx> for MiriMachine<'mir, 'tcx> {
10941097
ptr: Pointer<Self::Provenance>,
10951098
) -> InterpResult<'tcx> {
10961099
match ptr.provenance {
1097-
Provenance::Concrete { alloc_id, tag } =>
1098-
intptrcast::GlobalStateInner::expose_ptr(ecx, alloc_id, tag),
1100+
Provenance::Concrete { alloc_id, tag } => {
1101+
intptrcast::GlobalStateInner::expose_ptr(ecx, alloc_id, tag)
1102+
}
10991103
Provenance::Wildcard => {
11001104
// No need to do anything for wildcard pointers as
11011105
// their provenances have already been previously exposed.
@@ -1206,6 +1210,25 @@ impl<'mir, 'tcx> Machine<'mir, 'tcx> for MiriMachine<'mir, 'tcx> {
12061210
Ok(())
12071211
}
12081212

1213+
fn protect_in_place_function_argument(
1214+
ecx: &mut InterpCx<'mir, 'tcx, Self>,
1215+
place: &PlaceTy<'tcx, Provenance>,
1216+
) -> InterpResult<'tcx> {
1217+
// We do need to write `uninit` so that even after the call ends, the former contents of
1218+
// this place cannot be observed any more.
1219+
ecx.write_uninit(place)?;
1220+
// If we have a borrow tracker, we also have it set up protection so that all reads *and
1221+
// writes* during this call are insta-UB.
1222+
if ecx.machine.borrow_tracker.is_some() {
1223+
if let Either::Left(place) = place.as_mplace_or_local() {
1224+
ecx.protect_place(&place)?;
1225+
} else {
1226+
// Locals that don't have their address taken are as protected as they can ever be.
1227+
}
1228+
}
1229+
Ok(())
1230+
}
1231+
12091232
#[inline(always)]
12101233
fn init_frame_extra(
12111234
ecx: &mut InterpCx<'mir, 'tcx, Self>,
@@ -1288,8 +1311,17 @@ impl<'mir, 'tcx> Machine<'mir, 'tcx> for MiriMachine<'mir, 'tcx> {
12881311
let stack_len = ecx.active_thread_stack().len();
12891312
ecx.active_thread_mut().set_top_user_relevant_frame(stack_len - 1);
12901313
}
1291-
if ecx.machine.borrow_tracker.is_some() {
1292-
ecx.retag_return_place()?;
1314+
Ok(())
1315+
}
1316+
1317+
fn before_stack_pop(
1318+
ecx: &InterpCx<'mir, 'tcx, Self>,
1319+
frame: &Frame<'mir, 'tcx, Self::Provenance, Self::FrameExtra>,
1320+
) -> InterpResult<'tcx> {
1321+
// We want this *before* the return value copy, because the return place itself is protected
1322+
// until we do `end_call` here.
1323+
if let Some(borrow_tracker) = &ecx.machine.borrow_tracker {
1324+
borrow_tracker.borrow_mut().end_call(&frame.extra);
12931325
}
12941326
Ok(())
12951327
}
@@ -1308,9 +1340,6 @@ impl<'mir, 'tcx> Machine<'mir, 'tcx> for MiriMachine<'mir, 'tcx> {
13081340
ecx.active_thread_mut().recompute_top_user_relevant_frame();
13091341
}
13101342
let timing = frame.extra.timing.take();
1311-
if let Some(borrow_tracker) = &ecx.machine.borrow_tracker {
1312-
borrow_tracker.borrow_mut().end_call(&frame.extra);
1313-
}
13141343
let res = ecx.handle_stack_pop_unwind(frame.extra, unwinding);
13151344
if let Some(profiler) = ecx.machine.profiler.as_ref() {
13161345
profiler.finish_recording_interval_event(timing.unwrap());

‎src/shims/mod.rs

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
3131
&mut self,
3232
instance: ty::Instance<'tcx>,
3333
abi: Abi,
34-
args: &[OpTy<'tcx, Provenance>],
34+
args: &[FnArg<'tcx, Provenance>],
3535
dest: &PlaceTy<'tcx, Provenance>,
3636
ret: Option<mir::BasicBlock>,
3737
unwind: mir::UnwindAction,
@@ -41,7 +41,8 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
4141

4242
// There are some more lang items we want to hook that CTFE does not hook (yet).
4343
if this.tcx.lang_items().align_offset_fn() == Some(instance.def.def_id()) {
44-
let [ptr, align] = check_arg_count(args)?;
44+
let args = this.copy_fn_args(args)?;
45+
let [ptr, align] = check_arg_count(&args)?;
4546
if this.align_offset(ptr, align, dest, ret, unwind)? {
4647
return Ok(None);
4748
}
@@ -55,7 +56,8 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
5556
// to run extra MIR), and Ok(Some(body)) if we found MIR to run for the
5657
// foreign function
5758
// Any needed call to `goto_block` will be performed by `emulate_foreign_item`.
58-
return this.emulate_foreign_item(instance.def_id(), abi, args, dest, ret, unwind);
59+
let args = this.copy_fn_args(args)?; // FIXME: Should `InPlace` arguments be reset to uninit?
60+
return this.emulate_foreign_item(instance.def_id(), abi, &args, dest, ret, unwind);
5961
}
6062

6163
// Otherwise, load the MIR.

‎tests/fail/function_calls/arg_inplace_mutate.rs

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,34 @@
1+
//@revisions: stack tree
2+
//@[tree]compile-flags: -Zmiri-tree-borrows
3+
#![feature(custom_mir, core_intrinsics)]
4+
use std::intrinsics::mir::*;
5+
6+
pub struct S(i32);
7+
8+
#[custom_mir(dialect = "runtime", phase = "optimized")]
9+
fn main() {
10+
mir! {
11+
let unit: ();
12+
{
13+
let non_copy = S(42);
14+
let ptr = std::ptr::addr_of_mut!(non_copy);
15+
// Inside `callee`, the first argument and `*ptr` are basically
16+
// aliasing places!
17+
Call(unit, after_call, callee(Move(*ptr), ptr))
18+
}
19+
after_call = {
20+
Return()
21+
}
22+
23+
}
24+
}
25+
26+
pub fn callee(x: S, ptr: *mut S) {
27+
// With the setup above, if `x` is indeed moved in
28+
// (i.e. we actually just get a pointer to the underlying storage),
29+
// then writing to `ptr` will change the value stored in `x`!
30+
unsafe { ptr.write(S(0)) };
31+
//~[stack]^ ERROR: not granting access
32+
//~[tree]| ERROR: /write access .* forbidden/
33+
assert_eq!(x.0, 42);
34+
}

‎tests/fail/function_calls/arg_inplace_mutate.stack.stderr

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,37 @@
1+
error: Undefined Behavior: not granting access to tag <TAG> because that would remove [Unique for <TAG>] which is strongly protected because it is an argument of call ID
2+
--> $DIR/arg_inplace_mutate.rs:LL:CC
3+
|
4+
LL | unsafe { ptr.write(S(0)) };
5+
| ^^^^^^^^^^^^^^^ not granting access to tag <TAG> because that would remove [Unique for <TAG>] which is strongly protected because it is an argument of call ID
6+
|
7+
= help: this indicates a potential bug in the program: it performed an invalid operation, but the Stacked Borrows rules it violated are still experimental
8+
= help: see https://github.com/rust-lang/unsafe-code-guidelines/blob/master/wip/stacked-borrows.md for further information
9+
help: <TAG> was created by a SharedReadWrite retag at offsets [0x0..0x4]
10+
--> $DIR/arg_inplace_mutate.rs:LL:CC
11+
|
12+
LL | / mir! {
13+
LL | | let unit: ();
14+
LL | | {
15+
LL | | let non_copy = S(42);
16+
... |
17+
LL | |
18+
LL | | }
19+
| |_____^
20+
help: <TAG> is this argument
21+
--> $DIR/arg_inplace_mutate.rs:LL:CC
22+
|
23+
LL | unsafe { ptr.write(S(0)) };
24+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^
25+
= note: BACKTRACE (of the first span):
26+
= note: inside `callee` at $DIR/arg_inplace_mutate.rs:LL:CC
27+
note: inside `main`
28+
--> $DIR/arg_inplace_mutate.rs:LL:CC
29+
|
30+
LL | Call(unit, after_call, callee(Move(*ptr), ptr))
31+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
32+
= 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)
33+
34+
note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace
35+
36+
error: aborting due to previous error
37+

‎tests/fail/function_calls/arg_inplace_mutate.tree.stderr

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,39 @@
1+
error: Undefined Behavior: write access through <TAG> (root of the allocation) is forbidden
2+
--> $DIR/arg_inplace_mutate.rs:LL:CC
3+
|
4+
LL | unsafe { ptr.write(S(0)) };
5+
| ^^^^^^^^^^^^^^^ write access through <TAG> (root of the allocation) is forbidden
6+
|
7+
= help: this indicates a potential bug in the program: it performed an invalid operation, but the Tree Borrows rules it violated are still experimental
8+
= help: the accessed tag <TAG> (root of the allocation) is foreign to the protected tag <TAG> (i.e., it is not a child)
9+
= help: this foreign write access would cause the protected tag <TAG> to transition from Active to Disabled
10+
= help: this transition would be a loss of read and write permissions, which is not allowed for protected tags
11+
help: the accessed tag <TAG> was created here
12+
--> $DIR/arg_inplace_mutate.rs:LL:CC
13+
|
14+
LL | / mir! {
15+
LL | | let unit: ();
16+
LL | | {
17+
LL | | let non_copy = S(42);
18+
... |
19+
LL | |
20+
LL | | }
21+
| |_____^
22+
help: the protected tag <TAG> was created here, in the initial state Active
23+
--> $DIR/arg_inplace_mutate.rs:LL:CC
24+
|
25+
LL | unsafe { ptr.write(S(0)) };
26+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^
27+
= note: BACKTRACE (of the first span):
28+
= note: inside `callee` at $DIR/arg_inplace_mutate.rs:LL:CC
29+
note: inside `main`
30+
--> $DIR/arg_inplace_mutate.rs:LL:CC
31+
|
32+
LL | Call(unit, after_call, callee(Move(*ptr), ptr))
33+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
34+
= 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)
35+
36+
note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace
37+
38+
error: aborting due to previous error
39+

0 commit comments

Comments
 (0)