swap_typed_nonoverlapping: properly detect overlap even when swapping scalar values

Author: RalfJung

compiler/rustc_const_eval/src/interpret/call.rs

@@ -883,19 +883,7 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> {
                 .local_to_op(mir::RETURN_PLACE, None)
                 .expect("return place should always be live");
             let dest = self.frame().return_place.clone();
-            let res = if self.stack().len() == 1 {
-                // The initializer of constants and statics will get validated separately
-                // after the constant has been fully evaluated. While we could fall back to the default
-                // code path, that will cause -Zenforce-validity to cycle on static initializers.
-                // Reading from a static's memory is not allowed during its evaluation, and will always
-                // trigger a cycle error. Validation must read from the memory of the current item.
-                // For Miri this means we do not validate the root frame return value,
-                // but Miri anyway calls `read_target_isize` on that so separate validation
-                // is not needed.
-                self.copy_op_no_dest_validation(&op, &dest)
-            } else {
-                self.copy_op_allow_transmute(&op, &dest)
-            };
+            let res = self.copy_op_allow_transmute(&op, &dest);
             trace!("return value: {:?}", self.dump_place(&dest.into()));
             // We delay actually short-circuiting on this error until *after* the stack frame is
             // popped, since we want this error to be attributed to the caller, whose type defines

compiler/rustc_const_eval/src/interpret/intrinsics.rs

@@ -425,7 +425,7 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> {
                 self.write_scalar(result, dest)?;
             }
             sym::typed_swap_nonoverlapping => {
-                self.typed_swap_intrinsic(&args[0], &args[1])?;
+                self.typed_swap_nonoverlapping_intrinsic(&args[0], &args[1])?;
             }
 
             sym::vtable_size => {
@@ -638,19 +638,35 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> {
     }
 
     /// Does a *typed* swap of `*left` and `*right`.
-    fn typed_swap_intrinsic(
+    fn typed_swap_nonoverlapping_intrinsic(
         &mut self,
         left: &OpTy<'tcx, <M as Machine<'tcx>>::Provenance>,
         right: &OpTy<'tcx, <M as Machine<'tcx>>::Provenance>,
     ) -> InterpResult<'tcx> {
         let left = self.deref_pointer(left)?;
         let right = self.deref_pointer(right)?;
-        debug_assert_eq!(left.layout, right.layout);
+        assert_eq!(left.layout, right.layout);
+        assert!(left.layout.is_sized());
         let kind = MemoryKind::Stack;
         let temp = self.allocate(left.layout, kind)?;
-        self.copy_op(&left, &temp)?;
-        self.copy_op(&right, &left)?; // this checks that they are non-overlapping
-        self.copy_op(&temp, &right)?;
+        self.copy_op(&left, &temp)?; // checks alignment of `left`
+
+        // We want to always enforce non-overlapping, even if this is a scalar type.
+        // Therefore we directly use the underlying `mem_copy` here.
+        self.mem_copy(right.ptr(), left.ptr(), left.layout.size, /*nonoverlapping*/ true)?;
+        // This means we also need to do the validation of the value that used to be in `right`
+        // ourselves. This value is now in `left.` The one that started out in `left` already got
+        // validated by the copy above.
+        if M::enforce_validity(self, left.layout) {
+            self.validate_operand(
+                &left.clone().into(),
+                M::enforce_validity_recursively(self, left.layout),
+                /*reset_provenance_and_padding*/ true,
+            )?;
+        }
+
+        self.copy_op(&temp, &right)?; // checks alignment of `right`
+
         self.deallocate_ptr(temp.ptr(), None, kind)?;
         interp_ok(())
     }

compiler/rustc_const_eval/src/interpret/place.rs

@@ -773,22 +773,6 @@ where
         interp_ok(())
     }
 
-    /// Copies the data from an operand to a place.
-    /// The layouts of the `src` and `dest` may disagree.
-    /// Does not perform validation of the destination.
-    /// The only known use case for this function is checking the return
-    /// value of a static during stack frame popping.
-    #[inline(always)]
-    pub(super) fn copy_op_no_dest_validation(
-        &mut self,
-        src: &impl Projectable<'tcx, M::Provenance>,
-        dest: &impl Writeable<'tcx, M::Provenance>,
-    ) -> InterpResult<'tcx> {
-        self.copy_op_inner(
-            src, dest, /* allow_transmute */ true, /* validate_dest */ false,
-        )
-    }
-
     /// Copies the data from an operand to a place.
     /// The layouts of the `src` and `dest` may disagree.
     #[inline(always)]
@@ -797,9 +781,7 @@ where
         src: &impl Projectable<'tcx, M::Provenance>,
         dest: &impl Writeable<'tcx, M::Provenance>,
     ) -> InterpResult<'tcx> {
-        self.copy_op_inner(
-            src, dest, /* allow_transmute */ true, /* validate_dest */ true,
-        )
+        self.copy_op_inner(src, dest, /* allow_transmute */ true)
     }
 
     /// Copies the data from an operand to a place.
@@ -810,9 +792,7 @@ where
         src: &impl Projectable<'tcx, M::Provenance>,
         dest: &impl Writeable<'tcx, M::Provenance>,
     ) -> InterpResult<'tcx> {
-        self.copy_op_inner(
-            src, dest, /* allow_transmute */ false, /* validate_dest */ true,
-        )
+        self.copy_op_inner(src, dest, /* allow_transmute */ false)
     }
 
     /// Copies the data from an operand to a place.
@@ -824,22 +804,21 @@ where
         src: &impl Projectable<'tcx, M::Provenance>,
         dest: &impl Writeable<'tcx, M::Provenance>,
         allow_transmute: bool,
-        validate_dest: bool,
     ) -> InterpResult<'tcx> {
         // These are technically *two* typed copies: `src` is a not-yet-loaded value,
-        // so we're going a typed copy at `src` type from there to some intermediate storage.
+        // so we're doing a typed copy at `src` type from there to some intermediate storage.
         // And then we're doing a second typed copy from that intermediate storage to `dest`.
         // But as an optimization, we only make a single direct copy here.
 
         // Do the actual copy.
         self.copy_op_no_validate(src, dest, allow_transmute)?;
 
-        if validate_dest && M::enforce_validity(self, dest.layout()) {
+        if M::enforce_validity(self, dest.layout()) {
             let dest = dest.to_place();
             // Given that there were two typed copies, we have to ensure this is valid at both types,
             // and we have to ensure this loses provenance and padding according to both types.
             // But if the types are identical, we only do one pass.
-            if allow_transmute && src.layout().ty != dest.layout().ty {
+            if src.layout().ty != dest.layout().ty {
                 self.validate_operand(
                     &dest.transmute(src.layout(), self)?,
                     M::enforce_validity_recursively(self, src.layout()),

src/tools/miri/tests/fail/intrinsics/typed-swap-invalid-scalar.stderr renamed to src/tools/miri/tests/fail/intrinsics/typed-swap-invalid-scalar.left.stderr

@@ -0,0 +1,20 @@
+error: Undefined Behavior: constructing invalid value: encountered 0x03, but expected a boolean
+  --> tests/fail/intrinsics/typed-swap-invalid-scalar.rs:LL:CC
+   |
+LL |         typed_swap_nonoverlapping(a, b);
+   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ constructing invalid value: encountered 0x03, but expected a boolean
+   |
+   = 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 `invalid_scalar` at tests/fail/intrinsics/typed-swap-invalid-scalar.rs:LL:CC
+note: inside `main`
+  --> tests/fail/intrinsics/typed-swap-invalid-scalar.rs:LL:CC
+   |
+LL |     invalid_scalar();
+   |     ^^^^^^^^^^^^^^^^
+
+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/intrinsics/typed-swap-invalid-scalar.right.stderr

@@ -1,12 +1,14 @@
+//@revisions: left right
 #![feature(core_intrinsics)]
 #![feature(rustc_attrs)]
 
 use std::intrinsics::typed_swap_nonoverlapping;
 use std::ptr::addr_of_mut;
 
 fn invalid_scalar() {
-    let mut a = 1_u8;
-    let mut b = 2_u8;
+    // We run the test twice, with either the left or the right side being invalid.
+    let mut a = if cfg!(left) { 2_u8} else { 1_u8 };
+    let mut b = if cfg!(right) { 3_u8} else { 1_u8 };
     unsafe {
         let a = addr_of_mut!(a).cast::<bool>();
         let b = addr_of_mut!(b).cast::<bool>();

src/tools/miri/tests/fail/intrinsics/typed-swap-invalid-scalar.rs

@@ -5,7 +5,7 @@ use std::intrinsics::typed_swap_nonoverlapping;
 use std::ptr::addr_of_mut;
 
 fn main() {
-    let mut a = [0_u8; 100];
+    let mut a = 0_u8;
     unsafe {
         let a = addr_of_mut!(a);
         typed_swap_nonoverlapping(a, a); //~ERROR: called on overlapping ranges