Rollup merge of #126265 - RalfJung:interpret-cast-validity, r=oli-obk

jieyouxu · web-flow · commit cfd48bdd7e12 · 2024-06-11T14:16:47.000+01:00
interpret: ensure we check bool/char for validity when they are used in a cast

In general, `Scalar::to_bits` is a bit dangerous as it bypasses all type information. We should usually prefer matching on the type and acting according to that. So I also refactored `unary_op` handling of integers to do that. The remaining `to_bits` uses are operations that just fundamentally don't care about the sign (and only work on integers).

invalid_char_cast.rs is the key new test, the others already passed before this PR.

r? `@oli-obk`
diff --git a/compiler/rustc_const_eval/src/interpret/cast.rs b/compiler/rustc_const_eval/src/interpret/cast.rs
@@ -274,9 +274,13 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> {
         // Let's make sure v is sign-extended *if* it has a signed type.
         let signed = src_layout.abi.is_signed(); // Also asserts that abi is `Scalar`.
 
-        let v = scalar.to_bits(src_layout.size)?;
-        let v = if signed { self.sign_extend(v, src_layout) } else { v };
-        trace!("cast_from_scalar: {}, {} -> {}", v, src_layout.ty, cast_ty);
+        let v = match src_layout.ty.kind() {
+            Uint(_) | RawPtr(..) | FnPtr(..) => scalar.to_uint(src_layout.size)?,
+            Int(_) => scalar.to_int(src_layout.size)? as u128, // we will cast back to `i128` below if the sign matters
+            Bool => scalar.to_bool()?.into(),
+            Char => scalar.to_char()?.into(),
+            _ => span_bug!(self.cur_span(), "invalid int-like cast from {}", src_layout.ty),
+        };
 
         Ok(match *cast_ty.kind() {
             // int -> int
diff --git a/compiler/rustc_const_eval/src/interpret/intrinsics.rs b/compiler/rustc_const_eval/src/interpret/intrinsics.rs
@@ -197,7 +197,7 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> {
                 // rotate_right: (X << ((BW - S) % BW)) | (X >> (S % BW))
                 let layout_val = self.layout_of(instance_args.type_at(0))?;
                 let val = self.read_scalar(&args[0])?;
-                let val_bits = val.to_bits(layout_val.size)?;
+                let val_bits = val.to_bits(layout_val.size)?; // sign is ignored here
 
                 let layout_raw_shift = self.layout_of(self.tcx.types.u32)?;
                 let raw_shift = self.read_scalar(&args[1])?;
@@ -484,7 +484,7 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> {
         ret_layout: TyAndLayout<'tcx>,
     ) -> InterpResult<'tcx, Scalar<M::Provenance>> {
         assert!(layout.ty.is_integral(), "invalid type for numeric intrinsic: {}", layout.ty);
-        let bits = val.to_bits(layout.size)?;
+        let bits = val.to_bits(layout.size)?; // these operations all ignore the sign
         let extra = 128 - u128::from(layout.size.bits());
         let bits_out = match name {
             sym::ctpop => u128::from(bits.count_ones()),
@@ -519,6 +519,7 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> {
         // `x % y != 0` or `y == 0` or `x == T::MIN && y == -1`.
         // First, check x % y != 0 (or if that computation overflows).
         let rem = self.binary_op(BinOp::Rem, a, b)?;
+        // sign does not matter for 0 test, so `to_bits` is fine
         if rem.to_scalar().to_bits(a.layout.size)? != 0 {
             throw_ub_custom!(
                 fluent::const_eval_exact_div_has_remainder,
@@ -545,22 +546,19 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> {
             self.binary_op(mir_op.wrapping_to_overflowing().unwrap(), l, r)?.to_scalar_pair();
         Ok(if overflowed.to_bool()? {
             let size = l.layout.size;
-            let num_bits = size.bits();
             if l.layout.abi.is_signed() {
                 // For signed ints the saturated value depends on the sign of the first
                 // term since the sign of the second term can be inferred from this and
                 // the fact that the operation has overflowed (if either is 0 no
                 // overflow can occur)
-                let first_term: u128 = l.to_scalar().to_bits(l.layout.size)?;
-                let first_term_positive = first_term & (1 << (num_bits - 1)) == 0;
-                if first_term_positive {
+                let first_term: i128 = l.to_scalar().to_int(l.layout.size)?;
+                if first_term >= 0 {
                     // Negative overflow not possible since the positive first term
                     // can only increase an (in range) negative term for addition
-                    // or corresponding negated positive term for subtraction
+                    // or corresponding negated positive term for subtraction.
                     Scalar::from_int(size.signed_int_max(), size)
                 } else {
-                    // Positive overflow not possible for similar reason
-                    // max negative
+                    // Positive overflow not possible for similar reason.
                     Scalar::from_int(size.signed_int_min(), size)
                 }
             } else {
diff --git a/compiler/rustc_const_eval/src/interpret/operator.rs b/compiler/rustc_const_eval/src/interpret/operator.rs
@@ -437,23 +437,24 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> {
                 };
                 Ok(ImmTy::from_scalar(res, layout))
             }
-            _ if layout.ty.is_integral() => {
-                let val = val.to_scalar();
-                let val = val.to_bits(layout.size)?;
+            ty::Int(..) => {
+                let val = val.to_scalar().to_int(layout.size)?;
                 let res = match un_op {
-                    Not => self.truncate(!val, layout), // bitwise negation, then truncate
-                    Neg => {
-                        // arithmetic negation
-                        assert!(layout.abi.is_signed());
-                        let val = self.sign_extend(val, layout) as i128;
-                        let res = val.wrapping_neg();
-                        let res = res as u128;
-                        // Truncate to target type.
-                        self.truncate(res, layout)
-                    }
+                    Not => !val,
+                    Neg => val.wrapping_neg(),
                     _ => span_bug!(self.cur_span(), "Invalid integer op {:?}", un_op),
                 };
-                Ok(ImmTy::from_uint(res, layout))
+                let res = ScalarInt::truncate_from_int(res, layout.size).0;
+                Ok(ImmTy::from_scalar(res.into(), layout))
+            }
+            ty::Uint(..) => {
+                let val = val.to_scalar().to_uint(layout.size)?;
+                let res = match un_op {
+                    Not => !val,
+                    _ => span_bug!(self.cur_span(), "Invalid unsigned integer op {:?}", un_op),
+                };
+                let res = ScalarInt::truncate_from_uint(res, layout.size).0;
+                Ok(ImmTy::from_scalar(res.into(), layout))
             }
             ty::RawPtr(..) => {
                 assert_eq!(un_op, PtrMetadata);
diff --git a/compiler/rustc_middle/src/ty/consts/int.rs b/compiler/rustc_middle/src/ty/consts/int.rs
@@ -209,8 +209,8 @@ impl ScalarInt {
 
     #[inline]
     pub fn try_from_uint(i: impl Into<u128>, size: Size) -> Option<Self> {
-        let data = i.into();
-        if size.truncate(data) == data { Some(Self::raw(data, size)) } else { None }
+        let (r, overflow) = Self::truncate_from_uint(i, size);
+        if overflow { None } else { Some(r) }
     }
 
     /// Returns the truncated result, and whether truncation changed the value.
@@ -223,20 +223,15 @@ impl ScalarInt {
 
     #[inline]
     pub fn try_from_int(i: impl Into<i128>, size: Size) -> Option<Self> {
-        let i = i.into();
-        // `into` performed sign extension, we have to truncate
-        let truncated = size.truncate(i as u128);
-        if size.sign_extend(truncated) as i128 == i {
-            Some(Self::raw(truncated, size))
-        } else {
-            None
-        }
+        let (r, overflow) = Self::truncate_from_int(i, size);
+        if overflow { None } else { Some(r) }
     }
 
     /// Returns the truncated result, and whether truncation changed the value.
     #[inline]
     pub fn truncate_from_int(i: impl Into<i128>, size: Size) -> (Self, bool) {
         let data = i.into();
+        // `into` performed sign extension, we have to truncate
         let r = Self::raw(size.truncate(data as u128), size);
         (r, size.sign_extend(r.data) as i128 != data)
     }
diff --git a/src/tools/miri/tests/fail/validity/invalid_char_cast.rs b/src/tools/miri/tests/fail/validity/invalid_char_cast.rs
@@ -0,0 +1,21 @@
+// Make sure we find these even with many checks disabled.
+//@compile-flags: -Zmiri-disable-alignment-check -Zmiri-disable-stacked-borrows -Zmiri-disable-validation
+#![feature(core_intrinsics)]
+#![feature(custom_mir)]
+
+use std::intrinsics::mir::*;
+
+#[custom_mir(dialect = "runtime", phase = "optimized")]
+fn cast(ptr: *const char) -> u32 {
+    mir! {
+        {
+            RET = *ptr as u32; //~ERROR: interpreting an invalid 32-bit value as a char
+            Return()
+        }
+    }
+}
+
+pub fn main() {
+    let v = u32::MAX;
+    cast(&v as *const u32 as *const char);
+}
diff --git a/src/tools/miri/tests/fail/validity/invalid_char_cast.stderr b/src/tools/miri/tests/fail/validity/invalid_char_cast.stderr
@@ -0,0 +1,20 @@
+error: Undefined Behavior: interpreting an invalid 32-bit value as a char: $HEX
+  --> $DIR/invalid_char_cast.rs:LL:CC
+   |
+LL |             RET = *ptr as u32;
+   |             ^^^^^^^^^^^^^^^^^ interpreting an invalid 32-bit value as a char: $HEX
+   |
+   = 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 `cast` at $DIR/invalid_char_cast.rs:LL:CC
+note: inside `main`
+  --> $DIR/invalid_char_cast.rs:LL:CC
+   |
+LL |     cast(&v as *const u32 as *const char);
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace
+
+error: aborting due to 1 previous error
+
diff --git a/src/tools/miri/tests/fail/validity/invalid_char_match.rs b/src/tools/miri/tests/fail/validity/invalid_char_match.rs
@@ -0,0 +1,26 @@
+// Make sure we find these even with many checks disabled.
+//@compile-flags: -Zmiri-disable-alignment-check -Zmiri-disable-stacked-borrows -Zmiri-disable-validation
+#![feature(core_intrinsics)]
+#![feature(custom_mir)]
+
+use std::intrinsics::mir::*;
+
+#[custom_mir(dialect = "runtime", phase = "optimized")]
+fn switch_int(ptr: *const char) {
+    mir! {
+        {
+            match *ptr { //~ERROR: interpreting an invalid 32-bit value as a char
+                '0' => ret,
+                _ => ret,
+            }
+        }
+        ret = {
+            Return()
+        }
+    }
+}
+
+pub fn main() {
+    let v = u32::MAX;
+    switch_int(&v as *const u32 as *const char);
+}
diff --git a/src/tools/miri/tests/fail/validity/invalid_char_match.stderr b/src/tools/miri/tests/fail/validity/invalid_char_match.stderr
@@ -0,0 +1,23 @@
+error: Undefined Behavior: interpreting an invalid 32-bit value as a char: $HEX
+  --> $DIR/invalid_char_match.rs:LL:CC
+   |
+LL | /             match *ptr {
+LL | |                 '0' => ret,
+LL | |                 _ => ret,
+LL | |             }
+   | |_____________^ interpreting an invalid 32-bit value as a char: $HEX
+   |
+   = 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 `switch_int` at $DIR/invalid_char_match.rs:LL:CC
+note: inside `main`
+  --> $DIR/invalid_char_match.rs:LL:CC
+   |
+LL |     switch_int(&v as *const u32 as *const char);
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace
+
+error: aborting due to 1 previous error
+
diff --git a/src/tools/miri/tests/fail/validity/invalid_enum_cast.rs b/src/tools/miri/tests/fail/validity/invalid_enum_cast.rs
@@ -0,0 +1,15 @@
+// Make sure we find these even with many checks disabled.
+//@compile-flags: -Zmiri-disable-alignment-check -Zmiri-disable-stacked-borrows -Zmiri-disable-validation
+
+#[derive(Copy, Clone)]
+#[allow(unused)]
+enum E {A, B, C }
+
+fn cast(ptr: *const E) { unsafe {
+    let _val = *ptr as u32; //~ERROR: enum value has invalid tag
+}}
+
+pub fn main() {
+    let v = u32::MAX;
+    cast(&v as *const u32 as *const E);
+}
diff --git a/src/tools/miri/tests/fail/validity/invalid_enum_cast.stderr b/src/tools/miri/tests/fail/validity/invalid_enum_cast.stderr
@@ -0,0 +1,20 @@
+error: Undefined Behavior: enum value has invalid tag: 0xff
+  --> $DIR/invalid_enum_cast.rs:LL:CC
+   |
+LL |     let _val = *ptr as u32;
+   |                ^^^^^^^^^^^ enum value has invalid tag: 0xff
+   |
+   = 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 `cast` at $DIR/invalid_enum_cast.rs:LL:CC
+note: inside `main`
+  --> $DIR/invalid_enum_cast.rs:LL:CC
+   |
+LL |     cast(&v as *const u32 as *const E);
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace
+
+error: aborting due to 1 previous error
+
diff --git a/tests/ui/consts/const-eval/ub-invalid-values.rs b/tests/ui/consts/const-eval/ub-invalid-values.rs
@@ -0,0 +1,11 @@
+const fn bool_cast(ptr: *const bool) { unsafe {
+    let _val = *ptr as u32; //~ERROR: evaluation of constant value failed
+    //~^ interpreting an invalid 8-bit value as a bool
+}}
+
+const _: () = {
+    let v = 3_u8;
+    bool_cast(&v as *const u8 as *const bool);
+};
+
+fn main() {}
diff --git a/tests/ui/consts/const-eval/ub-invalid-values.stderr b/tests/ui/consts/const-eval/ub-invalid-values.stderr
@@ -0,0 +1,20 @@
+error[E0080]: evaluation of constant value failed
+  --> $DIR/ub-invalid-values.rs:2:16
+   |
+LL |     let _val = *ptr as u32;
+   |                ^^^^^^^^^^^ interpreting an invalid 8-bit value as a bool: 0x03
+   |
+note: inside `bool_cast`
+  --> $DIR/ub-invalid-values.rs:2:16
+   |
+LL |     let _val = *ptr as u32;
+   |                ^^^^^^^^^^^
+note: inside `_`
+  --> $DIR/ub-invalid-values.rs:8:5
+   |
+LL |     bool_cast(&v as *const u8 as *const bool);
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+error: aborting due to 1 previous error
+
+For more information about this error, try `rustc --explain E0080`.
