Skip to content

Commit 5d37e8e

Browse files
jieyouxujieyouxu
authored
Rollup merge of #145585 - RalfJung:miri-inplace-arg-checks, r=compiler-errors
Miri: fix handling of in-place argument and return place handling This fixes two separate bugs (in two separate commits): - If the return place is `_local` and not `*ptr`, we didn't always properly protect it if there were other pointers pointing to that return place. - If two in-place arguments are *the same* local variable, we didn't always detect that aliasing.
2 parents 5e979cb + 7dfbc0a commit 5d37e8e

16 files changed

+185
-59
lines changed

‎compiler/rustc_const_eval/src/interpret/call.rs‎

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -27,8 +27,9 @@ use crate::{enter_trace_span, fluent_generated as fluent};
2727
pub enum FnArg<'tcx, Prov: Provenance = CtfeProvenance> {
2828
/// Pass a copy of the given operand.
2929
Copy(OpTy<'tcx, Prov>),
30-
/// Allow for the argument to be passed in-place: destroy the value originally stored at that place and
31-
/// make the place inaccessible for the duration of the function call.
30+
/// Allow for the argument to be passed in-place: destroy the value originally stored at that
31+
/// place and make the place inaccessible for the duration of the function call. This *must* be
32+
/// an in-memory place so that we can do the proper alias checks.
3233
InPlace(MPlaceTy<'tcx, Prov>),
3334
}
3435

@@ -379,6 +380,11 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> {
379380
}
380381
}
381382

383+
// *Before* pushing the new frame, determine whether the return destination is in memory.
384+
// Need to use `place_to_op` to be *sure* we get the mplace if there is one.
385+
let destination_mplace = self.place_to_op(destination)?.as_mplace_or_imm().left();
386+
387+
// Push the "raw" frame -- this leaves locals uninitialized.
382388
self.push_stack_frame_raw(instance, body, destination, cont)?;
383389

384390
// If an error is raised here, pop the frame again to get an accurate backtrace.
@@ -496,7 +502,7 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> {
496502

497503
// Protect return place for in-place return value passing.
498504
// We only need to protect anything if this is actually an in-memory place.
499-
if let Left(mplace) = destination.as_mplace_or_local() {
505+
if let Some(mplace) = destination_mplace {
500506
M::protect_in_place_function_argument(self, &mplace)?;
501507
}
502508

‎compiler/rustc_const_eval/src/interpret/place.rs‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -234,6 +234,12 @@ impl<'tcx, Prov: Provenance> PlaceTy<'tcx, Prov> {
234234
}
235235

236236
/// A place is either an mplace or some local.
237+
///
238+
/// Note that the return value can be different even for logically identical places!
239+
/// Specifically, if a local is stored in-memory, this may return `Local` or `MPlaceTy`
240+
/// depending on how the place was constructed. In other words, seeing `Local` here does *not*
241+
/// imply that this place does not point to memory. Every caller must therefore always handle
242+
/// both cases.
237243
#[inline(always)]
238244
pub fn as_mplace_or_local(
239245
&self,

‎compiler/rustc_const_eval/src/interpret/step.rs‎

Lines changed: 41 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
55
use either::Either;
66
use rustc_abi::{FIRST_VARIANT, FieldIdx};
7+
use rustc_data_structures::fx::FxHashSet;
78
use rustc_index::IndexSlice;
89
use rustc_middle::ty::{self, Instance, Ty};
910
use rustc_middle::{bug, mir, span_bug};
@@ -389,8 +390,9 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> {
389390

390391
/// Evaluate the arguments of a function call
391392
fn eval_fn_call_argument(
392-
&self,
393+
&mut self,
393394
op: &mir::Operand<'tcx>,
395+
move_definitely_disjoint: bool,
394396
) -> InterpResult<'tcx, FnArg<'tcx, M::Provenance>> {
395397
interp_ok(match op {
396398
mir::Operand::Copy(_) | mir::Operand::Constant(_) => {
@@ -399,24 +401,19 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> {
399401
FnArg::Copy(op)
400402
}
401403
mir::Operand::Move(place) => {
402-
// If this place lives in memory, preserve its location.
403-
// We call `place_to_op` which will be an `MPlaceTy` whenever there exists
404-
// an mplace for this place. (This is in contrast to `PlaceTy::as_mplace_or_local`
405-
// which can return a local even if that has an mplace.)
406404
let place = self.eval_place(*place)?;
407-
let op = self.place_to_op(&place)?;
408-
409-
match op.as_mplace_or_imm() {
410-
Either::Left(mplace) => FnArg::InPlace(mplace),
411-
Either::Right(_imm) => {
412-
// This argument doesn't live in memory, so there's no place
413-
// to make inaccessible during the call.
414-
// We rely on there not being any stray `PlaceTy` that would let the
415-
// caller directly access this local!
416-
// This is also crucial for tail calls, where we want the `FnArg` to
417-
// stay valid when the old stack frame gets popped.
418-
FnArg::Copy(op)
405+
if move_definitely_disjoint {
406+
// We still have to ensure that no *other* pointers are used to access this place,
407+
// so *if* it is in memory then we have to treat it as `InPlace`.
408+
// Use `place_to_op` to guarantee that we notice it being in memory.
409+
let op = self.place_to_op(&place)?;
410+
match op.as_mplace_or_imm() {
411+
Either::Left(mplace) => FnArg::InPlace(mplace),
412+
Either::Right(_imm) => FnArg::Copy(op),
419413
}
414+
} else {
415+
// We have to force this into memory to detect aliasing among `Move` arguments.
416+
FnArg::InPlace(self.force_allocation(&place)?)
420417
}
421418
}
422419
})
@@ -425,15 +422,40 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> {
425422
/// Shared part of `Call` and `TailCall` implementation — finding and evaluating all the
426423
/// necessary information about callee and arguments to make a call.
427424
fn eval_callee_and_args(
428-
&self,
425+
&mut self,
429426
terminator: &mir::Terminator<'tcx>,
430427
func: &mir::Operand<'tcx>,
431428
args: &[Spanned<mir::Operand<'tcx>>],
432429
) -> InterpResult<'tcx, EvaluatedCalleeAndArgs<'tcx, M>> {
433430
let func = self.eval_operand(func, None)?;
431+
432+
// Evaluating function call arguments. The tricky part here is dealing with `Move`
433+
// arguments: we have to ensure no two such arguments alias. This would be most easily done
434+
// by just forcing them all into memory and then doing the usual in-place argument
435+
// protection, but then we'd force *a lot* of arguments into memory. So we do some syntactic
436+
// pre-processing here where if all `move` arguments are syntactically distinct local
437+
// variables (and none is indirect), we can skip the in-memory forcing.
438+
let move_definitely_disjoint = 'move_definitely_disjoint: {
439+
let mut previous_locals = FxHashSet::<mir::Local>::default();
440+
for arg in args {
441+
let mir::Operand::Move(place) = arg.node else {
442+
continue; // we can skip non-`Move` arguments.
443+
};
444+
if place.is_indirect_first_projection() {
445+
// An indirect `Move` argument could alias with anything else...
446+
break 'move_definitely_disjoint false;
447+
}
448+
if !previous_locals.insert(place.local) {
449+
// This local is the base for two arguments! They might overlap.
450+
break 'move_definitely_disjoint false;
451+
}
452+
}
453+
// We found no violation so they are all definitely disjoint.
454+
true
455+
};
434456
let args = args
435457
.iter()
436-
.map(|arg| self.eval_fn_call_argument(&arg.node))
458+
.map(|arg| self.eval_fn_call_argument(&arg.node, move_definitely_disjoint))
437459
.collect::<InterpResult<'tcx, Vec<_>>>()?;
438460

439461
let fn_sig_binder = {

‎src/tools/miri/tests/fail/function_calls/arg_inplace_locals_alias.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+
// Validation forces more things into memory, which we can't have here.
4+
//@compile-flags: -Zmiri-disable-validation
5+
#![feature(custom_mir, core_intrinsics)]
6+
use std::intrinsics::mir::*;
7+
8+
pub struct S(i32);
9+
10+
#[custom_mir(dialect = "runtime", phase = "optimized")]
11+
fn main() {
12+
mir! {
13+
let _unit: ();
14+
{
15+
let staging = S(42); // This forces `staging` into memory...
16+
let non_copy = staging; // ... so we move it to a non-inmemory local here.
17+
// This specifically uses a type with scalar representation to tempt Miri to use the
18+
// efficient way of storing local variables (outside adressable memory).
19+
Call(_unit = callee(Move(non_copy), Move(non_copy)), ReturnTo(after_call), UnwindContinue())
20+
//~[stack]^ ERROR: not granting access
21+
//~[tree]| ERROR: /read access .* forbidden/
22+
}
23+
after_call = {
24+
Return()
25+
}
26+
}
27+
}
28+
29+
pub fn callee(x: S, mut y: S) {
30+
// With the setup above, if `x` and `y` are both moved,
31+
// then writing to `y` will change the value stored in `x`!
32+
y.0 = 0;
33+
assert_eq!(x.0, 42);
34+
}

‎src/tools/miri/tests/fail/function_calls/arg_inplace_locals_alias.stack.stderr‎

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
error: Undefined Behavior: not granting access to tag <TAG> because that would remove [Unique for <TAG>] which is strongly protected
2+
--> tests/fail/function_calls/arg_inplace_locals_alias.rs:LL:CC
3+
|
4+
LL | Call(_unit = callee(Move(non_copy), Move(non_copy)), ReturnTo(after_call), UnwindContinue())
5+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Undefined Behavior occurred here
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 here, as the root tag for ALLOC
10+
--> tests/fail/function_calls/arg_inplace_locals_alias.rs:LL:CC
11+
|
12+
LL | Call(_unit = callee(Move(non_copy), Move(non_copy)), ReturnTo(after_call), UnwindContinue())
13+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
14+
help: <TAG> is this argument
15+
--> tests/fail/function_calls/arg_inplace_locals_alias.rs:LL:CC
16+
|
17+
LL | y.0 = 0;
18+
| ^^^^^^^
19+
= note: BACKTRACE (of the first span):
20+
= note: inside `main` at tests/fail/function_calls/arg_inplace_locals_alias.rs:LL:CC
21+
22+
note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace
23+
24+
error: aborting due to 1 previous error
25+

‎src/tools/miri/tests/fail/function_calls/arg_inplace_locals_alias.tree.stderr‎

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,33 @@
1+
error: Undefined Behavior: read access through <TAG> (root of the allocation) at ALLOC[0x0] is forbidden
2+
--> tests/fail/function_calls/arg_inplace_locals_alias.rs:LL:CC
3+
|
4+
LL | Call(_unit = callee(Move(non_copy), Move(non_copy)), ReturnTo(after_call), UnwindContinue())
5+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Undefined Behavior occurred here
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 read access would cause the protected tag <TAG> (currently Active) to become Disabled
10+
= help: protected tags must never be Disabled
11+
help: the accessed tag <TAG> was created here
12+
--> tests/fail/function_calls/arg_inplace_locals_alias.rs:LL:CC
13+
|
14+
LL | Call(_unit = callee(Move(non_copy), Move(non_copy)), ReturnTo(after_call), UnwindContinue())
15+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
16+
help: the protected tag <TAG> was created here, in the initial state Reserved
17+
--> tests/fail/function_calls/arg_inplace_locals_alias.rs:LL:CC
18+
|
19+
LL | y.0 = 0;
20+
| ^^^^^^^
21+
help: the protected tag <TAG> later transitioned to Active due to a child write access at offsets [0x0..0x4]
22+
--> tests/fail/function_calls/arg_inplace_locals_alias.rs:LL:CC
23+
|
24+
LL | y.0 = 0;
25+
| ^^^^^^^
26+
= help: this transition corresponds to the first write to a 2-phase borrowed mutable reference
27+
= note: BACKTRACE (of the first span):
28+
= note: inside `main` at tests/fail/function_calls/arg_inplace_locals_alias.rs:LL:CC
29+
30+
note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace
31+
32+
error: aborting due to 1 previous error
33+

‎src/tools/miri/tests/fail/function_calls/return_pointer_aliasing_read.none.stderr‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -11,8 +11,8 @@ LL | unsafe { ptr.read() };
1111
note: inside `main`
1212
--> tests/fail/function_calls/return_pointer_aliasing_read.rs:LL:CC
1313
|
14-
LL | Call(*ptr = myfun(ptr), ReturnTo(after_call), UnwindContinue())
15-
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
14+
LL | Call(_x = myfun(ptr), ReturnTo(after_call), UnwindContinue())
15+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
1616

1717
Uninitialized memory occurred at ALLOC[0x0..0x4], in this allocation:
1818
ALLOC (stack variable, size: 4, align: 4) {

‎src/tools/miri/tests/fail/function_calls/return_pointer_aliasing_read.rs‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -10,11 +10,11 @@ use std::intrinsics::mir::*;
1010
pub fn main() {
1111
mir! {
1212
{
13-
let x = 0;
14-
let ptr = &raw mut x;
13+
let _x = 0;
14+
let ptr = &raw mut _x;
1515
// We arrange for `myfun` to have a pointer that aliases
1616
// its return place. Even just reading from that pointer is UB.
17-
Call(*ptr = myfun(ptr), ReturnTo(after_call), UnwindContinue())
17+
Call(_x = myfun(ptr), ReturnTo(after_call), UnwindContinue())
1818
}
1919

2020
after_call = {
@@ -25,7 +25,7 @@ pub fn main() {
2525

2626
fn myfun(ptr: *mut i32) -> i32 {
2727
unsafe { ptr.read() };
28-
//~[stack]^ ERROR: not granting access
28+
//~[stack]^ ERROR: does not exist in the borrow stack
2929
//~[tree]| ERROR: /read access .* forbidden/
3030
//~[none]| ERROR: uninitialized
3131
// Without an aliasing model, reads are "fine" but at least they return uninit data.

‎src/tools/miri/tests/fail/function_calls/return_pointer_aliasing_read.stack.stderr‎

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,8 @@
1-
error: Undefined Behavior: not granting access to tag <TAG> because that would remove [Unique for <TAG>] which is strongly protected
1+
error: Undefined Behavior: attempting a read access using <TAG> at ALLOC[0x0], but that tag does not exist in the borrow stack for this location
22
--> tests/fail/function_calls/return_pointer_aliasing_read.rs:LL:CC
33
|
44
LL | unsafe { ptr.read() };
5-
| ^^^^^^^^^^ Undefined Behavior occurred here
5+
| ^^^^^^^^^^ this error occurs as part of an access at ALLOC[0x0..0x4]
66
|
77
= help: this indicates a potential bug in the program: it performed an invalid operation, but the Stacked Borrows rules it violated are still experimental
88
= help: see https://github.com/rust-lang/unsafe-code-guidelines/blob/master/wip/stacked-borrows.md for further information
@@ -11,12 +11,12 @@ help: <TAG> was created by a SharedReadWrite retag at offsets [0x0..0x4]
1111
|
1212
LL | / mir! {
1313
LL | | {
14-
LL | | let x = 0;
15-
LL | | let ptr = &raw mut x;
14+
LL | | let _x = 0;
15+
LL | | let ptr = &raw mut _x;
1616
... |
1717
LL | | }
1818
| |_____^
19-
help: <TAG> is this argument
19+
help: <TAG> was later invalidated at offsets [0x0..0x4] by a Unique in-place function argument/return passing protection
2020
--> tests/fail/function_calls/return_pointer_aliasing_read.rs:LL:CC
2121
|
2222
LL | unsafe { ptr.read() };
@@ -26,8 +26,8 @@ LL | unsafe { ptr.read() };
2626
note: inside `main`
2727
--> tests/fail/function_calls/return_pointer_aliasing_read.rs:LL:CC
2828
|
29-
LL | Call(*ptr = myfun(ptr), ReturnTo(after_call), UnwindContinue())
30-
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
29+
LL | Call(_x = myfun(ptr), ReturnTo(after_call), UnwindContinue())
30+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
3131
= 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)
3232

3333
note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace

‎src/tools/miri/tests/fail/function_calls/return_pointer_aliasing_read.tree.stderr‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -13,8 +13,8 @@ help: the accessed tag <TAG> was created here
1313
|
1414
LL | / mir! {
1515
LL | | {
16-
LL | | let x = 0;
17-
LL | | let ptr = &raw mut x;
16+
LL | | let _x = 0;
17+
LL | | let ptr = &raw mut _x;
1818
... |
1919
LL | | }
2020
| |_____^
@@ -34,8 +34,8 @@ LL | unsafe { ptr.read() };
3434
note: inside `main`
3535
--> tests/fail/function_calls/return_pointer_aliasing_read.rs:LL:CC
3636
|
37-
LL | Call(*ptr = myfun(ptr), ReturnTo(after_call), UnwindContinue())
38-
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
37+
LL | Call(_x = myfun(ptr), ReturnTo(after_call), UnwindContinue())
38+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
3939
= 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)
4040

4141
note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace

0 commit comments

Comments
 (0)