Skip to content

Commit

Permalink
Rollup merge of #126265 - RalfJung:interpret-cast-validity, r=oli-obk
Browse files Browse the repository at this point in the history
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`
  • Loading branch information
jieyouxu authored Jun 11, 2024
2 parents 2a94a5b + de4ac0c commit cfd48bd
Show file tree
Hide file tree
Showing 12 changed files with 190 additions and 36 deletions.
10 changes: 7 additions & 3 deletions compiler/rustc_const_eval/src/interpret/cast.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
16 changes: 7 additions & 9 deletions compiler/rustc_const_eval/src/interpret/intrinsics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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])?;
Expand Down Expand Up @@ -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()),
Expand Down Expand Up @@ -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,
Expand All @@ -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 {
Expand Down
29 changes: 15 additions & 14 deletions compiler/rustc_const_eval/src/interpret/operator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
15 changes: 5 additions & 10 deletions compiler/rustc_middle/src/ty/consts/int.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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)
}
Expand Down
21 changes: 21 additions & 0 deletions src/tools/miri/tests/fail/validity/invalid_char_cast.rs
Original file line number Diff line number Diff line change
@@ -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);
}
20 changes: 20 additions & 0 deletions src/tools/miri/tests/fail/validity/invalid_char_cast.stderr
Original file line number Diff line number Diff line change
@@ -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

26 changes: 26 additions & 0 deletions src/tools/miri/tests/fail/validity/invalid_char_match.rs
Original file line number Diff line number Diff line change
@@ -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);
}
23 changes: 23 additions & 0 deletions src/tools/miri/tests/fail/validity/invalid_char_match.stderr
Original file line number Diff line number Diff line change
@@ -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

15 changes: 15 additions & 0 deletions src/tools/miri/tests/fail/validity/invalid_enum_cast.rs
Original file line number Diff line number Diff line change
@@ -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);
}
20 changes: 20 additions & 0 deletions src/tools/miri/tests/fail/validity/invalid_enum_cast.stderr
Original file line number Diff line number Diff line change
@@ -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

11 changes: 11 additions & 0 deletions tests/ui/consts/const-eval/ub-invalid-values.rs
Original file line number Diff line number Diff line change
@@ -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() {}
20 changes: 20 additions & 0 deletions tests/ui/consts/const-eval/ub-invalid-values.stderr
Original file line number Diff line number Diff line change
@@ -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`.

0 comments on commit cfd48bd

Please sign in to comment.