miri: detect passing the same local twice as an in-place argument

compiler/rustc_const_eval/src/interpret/step.rs

@@ -4,6 +4,7 @@
 
 use either::Either;
 use rustc_abi::{FIRST_VARIANT, FieldIdx};
+use rustc_data_structures::fx::FxHashSet;
 use rustc_index::IndexSlice;
 use rustc_middle::ty::{self, Instance, Ty};
 use rustc_middle::{bug, mir, span_bug};
@@ -389,8 +390,9 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> {
 
     /// Evaluate the arguments of a function call
     fn eval_fn_call_argument(
-        &self,
+        &mut self,
         op: &mir::Operand<'tcx>,
+        move_definitely_disjoint: bool,
     ) -> InterpResult<'tcx, FnArg<'tcx, M::Provenance>> {
         interp_ok(match op {
             mir::Operand::Copy(_) | mir::Operand::Constant(_) => {
@@ -399,25 +401,19 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> {
                 FnArg::Copy(op)
             }
             mir::Operand::Move(place) => {
-                // If this place lives in memory, preserve its location.
-                // We call `place_to_op` which will be an `MPlaceTy` whenever there exists
-                // an mplace for this place. (This is in contrast to `PlaceTy::as_mplace_or_local`
-                // which can return a local even if that has an mplace.)
                 let place = self.eval_place(*place)?;
-                let op = self.place_to_op(&place)?;
-
-                match op.as_mplace_or_imm() {
-                    Either::Left(mplace) => FnArg::InPlace(mplace),
-                    Either::Right(_imm) => {
-                        // This argument doesn't live in memory, so there's no place
-                        // to make inaccessible during the call.
-                        // We rely on there not being any stray `PlaceTy` that would let the
-                        // caller directly access this local!
-                        // This is also crucial for tail calls, where we want the `FnArg` to
-                        // stay valid when the old stack frame gets popped.
-                        // FIXME: How can this be right for aliasing arguments?
-                        FnArg::Copy(op)
+                if move_definitely_disjoint {
+                    // We still have to ensure that no *other* pointers are used to access this place,
+                    // so *if* it is in memory then we have to treat it as `InPlace`.
+                    // Use `place_to_op` to guarantee that we notice it being in memory.
+                    let op = self.place_to_op(&place)?;
+                    match op.as_mplace_or_imm() {
+                        Either::Left(mplace) => FnArg::InPlace(mplace),
+                        Either::Right(_imm) => FnArg::Copy(op),
                     }
+                } else {
+                    // We have to force this into memory to detect aliasing among `Move` arguments.
+                    FnArg::InPlace(self.force_allocation(&place)?)
                 }
             }
         })
@@ -426,15 +422,40 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> {
     /// Shared part of `Call` and `TailCall` implementation — finding and evaluating all the
     /// necessary information about callee and arguments to make a call.
     fn eval_callee_and_args(
-        &self,
+        &mut self,
         terminator: &mir::Terminator<'tcx>,
         func: &mir::Operand<'tcx>,
         args: &[Spanned<mir::Operand<'tcx>>],
     ) -> InterpResult<'tcx, EvaluatedCalleeAndArgs<'tcx, M>> {
         let func = self.eval_operand(func, None)?;
+
+        // Evaluating function call arguments. The tricky part here is dealing with `Move`
+        // arguments: we have to ensure no two such arguments alias. This would be most easily done
+        // by just forcing them all into memory and then doing the usual in-place argument
+        // protection, but then we'd force *a lot* of arguments into memory. So we do some syntactic
+        // pre-processing here where if all `move` arguments are syntactically distinct local
+        // variables (and none is indirect), we can skip the in-memory forcing.
+        let move_definitely_disjoint = 'move_definitely_disjoint: {
+            let mut previous_locals = FxHashSet::<mir::Local>::default();
+            for arg in args {
+                let mir::Operand::Move(place) = arg.node else {
+                    continue; // we can skip non-`Move` arguments.
+                };
+                if place.is_indirect_first_projection() {
+                    // An indirect `Move` argument could alias with anything else...
+                    break 'move_definitely_disjoint false;
+                }
+                if !previous_locals.insert(place.local) {
+                    // This local is the base for two arguments! They might overlap.
+                    break 'move_definitely_disjoint false;
+                }
+            }
+            // We found no violation so they are all definitely disjoint.
+            true
+        };
         let args = args
             .iter()
-            .map(|arg| self.eval_fn_call_argument(&arg.node))
+            .map(|arg| self.eval_fn_call_argument(&arg.node, move_definitely_disjoint))
             .collect::<InterpResult<'tcx, Vec<_>>>()?;
 
         let fn_sig_binder = func.layout.ty.fn_sig(*self.tcx);

src/tools/miri/tests/fail/function_calls/arg_inplace_locals_alias.rs

@@ -0,0 +1,34 @@
+//@revisions: stack tree
+//@[tree]compile-flags: -Zmiri-tree-borrows
+// Validation forces more things into memory, which we can't have here.
+//@compile-flags: -Zmiri-disable-validation
+#![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 staging = S(42); // This forces `staging` into memory...
+            let non_copy = staging; // ... so we move it to a non-inmemory local here.
+            // This specifically uses a type with scalar representation to tempt Miri to use the
+            // efficient way of storing local variables (outside adressable memory).
+            Call(_unit = callee(Move(non_copy), Move(non_copy)), ReturnTo(after_call), UnwindContinue())
+            //~[stack]^ ERROR: not granting access
+            //~[tree]| ERROR: /read access .* forbidden/
+        }
+        after_call = {
+            Return()
+        }
+    }
+}
+
+pub fn callee(x: S, mut y: S) {
+    // With the setup above, if `x` and `y` are both moved,
+    // then writing to `y` will change the value stored in `x`!
+    y.0 = 0;
+    assert_eq!(x.0, 42);
+}

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

@@ -0,0 +1,25 @@
+error: Undefined Behavior: not granting access to tag <TAG> because that would remove [Unique for <TAG>] which is strongly protected
+  --> tests/fail/function_calls/arg_inplace_locals_alias.rs:LL:CC
+   |
+LL |             Call(_unit = callee(Move(non_copy), Move(non_copy)), ReturnTo(after_call), UnwindContinue())
+   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Undefined Behavior occurred here
+   |
+   = 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: <TAG> was created here, as the root tag for ALLOC
+  --> tests/fail/function_calls/arg_inplace_locals_alias.rs:LL:CC
+   |
+LL |             Call(_unit = callee(Move(non_copy), Move(non_copy)), ReturnTo(after_call), UnwindContinue())
+   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+help: <TAG> is this argument
+  --> tests/fail/function_calls/arg_inplace_locals_alias.rs:LL:CC
+   |
+LL |     y.0 = 0;
+   |     ^^^^^^^
+   = note: BACKTRACE (of the first span):
+   = note: inside `main` at tests/fail/function_calls/arg_inplace_locals_alias.rs:LL:CC
+
+note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace
+
+error: aborting due to 1 previous error
+

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

@@ -0,0 +1,33 @@
+error: Undefined Behavior: read access through <TAG> (root of the allocation) at ALLOC[0x0] is forbidden
+  --> tests/fail/function_calls/arg_inplace_locals_alias.rs:LL:CC
+   |
+LL |             Call(_unit = callee(Move(non_copy), Move(non_copy)), ReturnTo(after_call), UnwindContinue())
+   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Undefined Behavior occurred here
+   |
+   = 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 <TAG> (root of the allocation) is foreign to the protected tag <TAG> (i.e., it is not a child)
+   = help: this foreign read access would cause the protected tag <TAG> (currently Active) to become Disabled
+   = help: protected tags must never be Disabled
+help: the accessed tag <TAG> was created here
+  --> tests/fail/function_calls/arg_inplace_locals_alias.rs:LL:CC
+   |
+LL |             Call(_unit = callee(Move(non_copy), Move(non_copy)), ReturnTo(after_call), UnwindContinue())
+   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+help: the protected tag <TAG> was created here, in the initial state Reserved
+  --> tests/fail/function_calls/arg_inplace_locals_alias.rs:LL:CC
+   |
+LL |     y.0 = 0;
+   |     ^^^^^^^
+help: the protected tag <TAG> later transitioned to Active due to a child write access at offsets [0x0..0x4]
+  --> tests/fail/function_calls/arg_inplace_locals_alias.rs:LL:CC
+   |
+LL |     y.0 = 0;
+   |     ^^^^^^^
+   = help: this transition corresponds to the first write to a 2-phase borrowed mutable reference
+   = note: BACKTRACE (of the first span):
+   = note: inside `main` at tests/fail/function_calls/arg_inplace_locals_alias.rs:LL:CC
+
+note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace
+
+error: aborting due to 1 previous error
+