From 58be2fdba4580be41649198f7cc27ed0c03bafa3 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Wed, 28 Aug 2024 17:39:09 +0200 Subject: [PATCH 1/5] interpret: remove Readable trait, we can use Projectable instead --- src/helpers.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/helpers.rs b/src/helpers.rs index a546ad20fe..cba99c0bd7 100644 --- a/src/helpers.rs +++ b/src/helpers.rs @@ -869,7 +869,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { /// Dereference a pointer operand to a place using `layout` instead of the pointer's declared type fn deref_pointer_as( &self, - op: &impl Readable<'tcx, Provenance>, + op: &impl Projectable<'tcx, Provenance>, layout: TyAndLayout<'tcx>, ) -> InterpResult<'tcx, MPlaceTy<'tcx>> { let this = self.eval_context_ref(); @@ -880,7 +880,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { /// Calculates the MPlaceTy given the offset and layout of an access on an operand fn deref_pointer_and_offset( &self, - op: &impl Readable<'tcx, Provenance>, + op: &impl Projectable<'tcx, Provenance>, offset: u64, base_layout: TyAndLayout<'tcx>, value_layout: TyAndLayout<'tcx>, @@ -897,7 +897,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { fn deref_pointer_and_read( &self, - op: &impl Readable<'tcx, Provenance>, + op: &impl Projectable<'tcx, Provenance>, offset: u64, base_layout: TyAndLayout<'tcx>, value_layout: TyAndLayout<'tcx>, @@ -909,7 +909,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { fn deref_pointer_and_write( &mut self, - op: &impl Readable<'tcx, Provenance>, + op: &impl Projectable<'tcx, Provenance>, offset: u64, value: impl Into, base_layout: TyAndLayout<'tcx>, From fd02b78c5779bd878a31fbe66229788b389f6fe1 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Thu, 29 Aug 2024 08:59:52 +0200 Subject: [PATCH 2/5] interpret: reset provenance on typed copies --- src/concurrency/data_race.rs | 2 +- src/intrinsics/mod.rs | 6 ++-- .../provenance/int_copy_looses_provenance0.rs | 10 +++++++ .../int_copy_looses_provenance0.stderr | 15 ++++++++++ .../provenance/int_copy_looses_provenance1.rs | 10 +++++++ .../int_copy_looses_provenance1.stderr | 15 ++++++++++ .../provenance/int_copy_looses_provenance2.rs | 12 ++++++++ .../int_copy_looses_provenance2.stderr | 15 ++++++++++ .../provenance/int_copy_looses_provenance3.rs | 29 +++++++++++++++++++ .../int_copy_looses_provenance3.stderr | 15 ++++++++++ .../ptr_copy_loses_partial_provenance0.rs | 17 +++++++++++ .../ptr_copy_loses_partial_provenance0.stderr | 15 ++++++++++ .../ptr_copy_loses_partial_provenance1.rs | 17 +++++++++++ .../ptr_copy_loses_partial_provenance1.stderr | 15 ++++++++++ tests/pass/provenance.rs | 22 ++++++++++++++ 15 files changed, 212 insertions(+), 3 deletions(-) create mode 100644 tests/fail/provenance/int_copy_looses_provenance0.rs create mode 100644 tests/fail/provenance/int_copy_looses_provenance0.stderr create mode 100644 tests/fail/provenance/int_copy_looses_provenance1.rs create mode 100644 tests/fail/provenance/int_copy_looses_provenance1.stderr create mode 100644 tests/fail/provenance/int_copy_looses_provenance2.rs create mode 100644 tests/fail/provenance/int_copy_looses_provenance2.stderr create mode 100644 tests/fail/provenance/int_copy_looses_provenance3.rs create mode 100644 tests/fail/provenance/int_copy_looses_provenance3.stderr create mode 100644 tests/fail/provenance/ptr_copy_loses_partial_provenance0.rs create mode 100644 tests/fail/provenance/ptr_copy_loses_partial_provenance0.stderr create mode 100644 tests/fail/provenance/ptr_copy_loses_partial_provenance1.rs create mode 100644 tests/fail/provenance/ptr_copy_loses_partial_provenance1.stderr diff --git a/src/concurrency/data_race.rs b/src/concurrency/data_race.rs index 6fd207c92b..b604fd868a 100644 --- a/src/concurrency/data_race.rs +++ b/src/concurrency/data_race.rs @@ -637,7 +637,7 @@ pub trait EvalContextExt<'tcx>: MiriInterpCxExt<'tcx> { // The program didn't actually do a read, so suppress the memory access hooks. // This is also a very special exception where we just ignore an error -- if this read // was UB e.g. because the memory is uninitialized, we don't want to know! - let old_val = this.run_for_validation(|| this.read_scalar(dest)).ok(); + let old_val = this.run_for_validation(|this| this.read_scalar(dest)).ok(); this.allow_data_races_mut(move |this| this.write_scalar(val, dest))?; this.validate_atomic_store(dest, atomic)?; this.buffered_atomic_write(val, dest, atomic, old_val) diff --git a/src/intrinsics/mod.rs b/src/intrinsics/mod.rs index 18b22827bd..0ab1b9dfb6 100644 --- a/src/intrinsics/mod.rs +++ b/src/intrinsics/mod.rs @@ -152,8 +152,10 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { // ``` // Would not be considered UB, or the other way around (`is_val_statically_known(0)`). "is_val_statically_known" => { - let [arg] = check_arg_count(args)?; - this.validate_operand(arg, /*recursive*/ false)?; + let [_arg] = check_arg_count(args)?; + // FIXME: should we check for validity here? It's tricky because we do not have a + // place. Codegen does not seem to set any attributes like `noundef` for intrinsic + // calls, so we don't *have* to do anything. let branch: bool = this.machine.rng.get_mut().gen(); this.write_scalar(Scalar::from_bool(branch), dest)?; } diff --git a/tests/fail/provenance/int_copy_looses_provenance0.rs b/tests/fail/provenance/int_copy_looses_provenance0.rs new file mode 100644 index 0000000000..fd0773ed91 --- /dev/null +++ b/tests/fail/provenance/int_copy_looses_provenance0.rs @@ -0,0 +1,10 @@ +use std::mem; + +// Doing a copy at integer type should lose provenance. +// This tests the unoptimized base case. +fn main() { + let ptrs = [(&42, true)]; + let ints: [(usize, bool); 1] = unsafe { mem::transmute(ptrs) }; + let ptr = (&raw const ints[0].0).cast::<&i32>(); + let _val = unsafe { *ptr.read() }; //~ERROR: dangling +} diff --git a/tests/fail/provenance/int_copy_looses_provenance0.stderr b/tests/fail/provenance/int_copy_looses_provenance0.stderr new file mode 100644 index 0000000000..fc012af3ad --- /dev/null +++ b/tests/fail/provenance/int_copy_looses_provenance0.stderr @@ -0,0 +1,15 @@ +error: Undefined Behavior: constructing invalid value: encountered a dangling reference ($HEX[noalloc] has no provenance) + --> $DIR/int_copy_looses_provenance0.rs:LL:CC + | +LL | let _val = unsafe { *ptr.read() }; + | ^^^^^^^^^^ constructing invalid value: encountered a dangling reference ($HEX[noalloc] has no provenance) + | + = 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 `main` at $DIR/int_copy_looses_provenance0.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 + diff --git a/tests/fail/provenance/int_copy_looses_provenance1.rs b/tests/fail/provenance/int_copy_looses_provenance1.rs new file mode 100644 index 0000000000..ce64dcc1a0 --- /dev/null +++ b/tests/fail/provenance/int_copy_looses_provenance1.rs @@ -0,0 +1,10 @@ +use std::mem; + +// Doing a copy at integer type should lose provenance. +// This tests the optimized-array case of integer copies. +fn main() { + let ptrs = [&42]; + let ints: [usize; 1] = unsafe { mem::transmute(ptrs) }; + let ptr = (&raw const ints[0]).cast::<&i32>(); + let _val = unsafe { *ptr.read() }; //~ERROR: dangling +} diff --git a/tests/fail/provenance/int_copy_looses_provenance1.stderr b/tests/fail/provenance/int_copy_looses_provenance1.stderr new file mode 100644 index 0000000000..375262655d --- /dev/null +++ b/tests/fail/provenance/int_copy_looses_provenance1.stderr @@ -0,0 +1,15 @@ +error: Undefined Behavior: constructing invalid value: encountered a dangling reference ($HEX[noalloc] has no provenance) + --> $DIR/int_copy_looses_provenance1.rs:LL:CC + | +LL | let _val = unsafe { *ptr.read() }; + | ^^^^^^^^^^ constructing invalid value: encountered a dangling reference ($HEX[noalloc] has no provenance) + | + = 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 `main` at $DIR/int_copy_looses_provenance1.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 + diff --git a/tests/fail/provenance/int_copy_looses_provenance2.rs b/tests/fail/provenance/int_copy_looses_provenance2.rs new file mode 100644 index 0000000000..e8966c53d7 --- /dev/null +++ b/tests/fail/provenance/int_copy_looses_provenance2.rs @@ -0,0 +1,12 @@ +use std::mem; + +// Doing a copy at integer type should lose provenance. +// This tests the case where provenacne is hiding in the metadata of a pointer. +fn main() { + let ptrs = [(&42, &42)]; + // Typed copy at wide pointer type (with integer-typed metadata). + let ints: [*const [usize]; 1] = unsafe { mem::transmute(ptrs) }; + // Get a pointer to the metadata field. + let ptr = (&raw const ints[0]).wrapping_byte_add(mem::size_of::<*const ()>()).cast::<&i32>(); + let _val = unsafe { *ptr.read() }; //~ERROR: dangling +} diff --git a/tests/fail/provenance/int_copy_looses_provenance2.stderr b/tests/fail/provenance/int_copy_looses_provenance2.stderr new file mode 100644 index 0000000000..8402c7b5e1 --- /dev/null +++ b/tests/fail/provenance/int_copy_looses_provenance2.stderr @@ -0,0 +1,15 @@ +error: Undefined Behavior: constructing invalid value: encountered a dangling reference ($HEX[noalloc] has no provenance) + --> $DIR/int_copy_looses_provenance2.rs:LL:CC + | +LL | let _val = unsafe { *ptr.read() }; + | ^^^^^^^^^^ constructing invalid value: encountered a dangling reference ($HEX[noalloc] has no provenance) + | + = 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 `main` at $DIR/int_copy_looses_provenance2.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 + diff --git a/tests/fail/provenance/int_copy_looses_provenance3.rs b/tests/fail/provenance/int_copy_looses_provenance3.rs new file mode 100644 index 0000000000..48a48ce458 --- /dev/null +++ b/tests/fail/provenance/int_copy_looses_provenance3.rs @@ -0,0 +1,29 @@ +#![feature(strict_provenance)] +use std::mem; + +#[repr(C, usize)] +#[allow(unused)] +enum E { + Var1(usize), + Var2(usize), +} + +// Doing a copy at integer type should lose provenance. +// This tests the case where provenacne is hiding in the discriminant of an enum. +fn main() { + assert_eq!(mem::size_of::(), 2*mem::size_of::()); + + // We want to store provenance in the enum discriminant, but the value still needs to + // be valid atfor the type. So we split provenance and data. + let ptr = &42; + let ptr = ptr as *const i32; + let ptrs = [(ptr.with_addr(0), ptr)]; + // Typed copy at the enum type. + let ints: [E; 1] = unsafe { mem::transmute(ptrs) }; + // Read the discriminant. + let discr = unsafe { (&raw const ints[0]).cast::<*const i32>().read() }; + // Take the provenance from there, together with the original address. + let ptr = discr.with_addr(ptr.addr()); + // There should be no provenance is `discr`, so this should be UB. + let _val = unsafe { *ptr }; //~ERROR: dangling +} diff --git a/tests/fail/provenance/int_copy_looses_provenance3.stderr b/tests/fail/provenance/int_copy_looses_provenance3.stderr new file mode 100644 index 0000000000..b50e23da96 --- /dev/null +++ b/tests/fail/provenance/int_copy_looses_provenance3.stderr @@ -0,0 +1,15 @@ +error: Undefined Behavior: memory access failed: expected a pointer to 4 bytes of memory, but got $HEX[noalloc] which is a dangling pointer (it has no provenance) + --> $DIR/int_copy_looses_provenance3.rs:LL:CC + | +LL | let _val = unsafe { *ptr }; + | ^^^^ memory access failed: expected a pointer to 4 bytes of memory, but got $HEX[noalloc] which is a dangling pointer (it has no provenance) + | + = 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 `main` at $DIR/int_copy_looses_provenance3.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 + diff --git a/tests/fail/provenance/ptr_copy_loses_partial_provenance0.rs b/tests/fail/provenance/ptr_copy_loses_partial_provenance0.rs new file mode 100644 index 0000000000..0c6666b424 --- /dev/null +++ b/tests/fail/provenance/ptr_copy_loses_partial_provenance0.rs @@ -0,0 +1,17 @@ +fn main() { + unsafe { + let mut bytes = [1u8; 16]; + let bytes = bytes.as_mut_ptr(); + + // Put a pointer in the middle. + bytes.add(4).cast::<&i32>().write_unaligned(&42); + // Typed copy of the entire thing as two pointers, but not perfectly + // overlapping with the pointer we have in there. + let copy = bytes.cast::<[*const (); 2]>().read_unaligned(); + let copy_bytes = copy.as_ptr().cast::(); + // Now go to the middle of the copy and get the pointer back out. + let ptr = copy_bytes.add(4).cast::<*const i32>().read_unaligned(); + // Dereferencing this should fail as the copy has removed the provenance. + let _val = *ptr; //~ERROR: dangling + } +} diff --git a/tests/fail/provenance/ptr_copy_loses_partial_provenance0.stderr b/tests/fail/provenance/ptr_copy_loses_partial_provenance0.stderr new file mode 100644 index 0000000000..ed38572a5f --- /dev/null +++ b/tests/fail/provenance/ptr_copy_loses_partial_provenance0.stderr @@ -0,0 +1,15 @@ +error: Undefined Behavior: memory access failed: expected a pointer to 4 bytes of memory, but got $HEX[noalloc] which is a dangling pointer (it has no provenance) + --> $DIR/ptr_copy_loses_partial_provenance0.rs:LL:CC + | +LL | let _val = *ptr; + | ^^^^ memory access failed: expected a pointer to 4 bytes of memory, but got $HEX[noalloc] which is a dangling pointer (it has no provenance) + | + = 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 `main` at $DIR/ptr_copy_loses_partial_provenance0.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 + diff --git a/tests/fail/provenance/ptr_copy_loses_partial_provenance1.rs b/tests/fail/provenance/ptr_copy_loses_partial_provenance1.rs new file mode 100644 index 0000000000..30d826413d --- /dev/null +++ b/tests/fail/provenance/ptr_copy_loses_partial_provenance1.rs @@ -0,0 +1,17 @@ +fn main() { + unsafe { + let mut bytes = [1u8; 16]; + let bytes = bytes.as_mut_ptr(); + + // Put a pointer in the middle. + bytes.add(4).cast::<&i32>().write_unaligned(&42); + // Typed copy of the entire thing as two *function* pointers, but not perfectly + // overlapping with the pointer we have in there. + let copy = bytes.cast::<[fn(); 2]>().read_unaligned(); + let copy_bytes = copy.as_ptr().cast::(); + // Now go to the middle of the copy and get the pointer back out. + let ptr = copy_bytes.add(4).cast::<*const i32>().read_unaligned(); + // Dereferencing this should fail as the copy has removed the provenance. + let _val = *ptr; //~ERROR: dangling + } +} diff --git a/tests/fail/provenance/ptr_copy_loses_partial_provenance1.stderr b/tests/fail/provenance/ptr_copy_loses_partial_provenance1.stderr new file mode 100644 index 0000000000..2e11687175 --- /dev/null +++ b/tests/fail/provenance/ptr_copy_loses_partial_provenance1.stderr @@ -0,0 +1,15 @@ +error: Undefined Behavior: memory access failed: expected a pointer to 4 bytes of memory, but got $HEX[noalloc] which is a dangling pointer (it has no provenance) + --> $DIR/ptr_copy_loses_partial_provenance1.rs:LL:CC + | +LL | let _val = *ptr; + | ^^^^ memory access failed: expected a pointer to 4 bytes of memory, but got $HEX[noalloc] which is a dangling pointer (it has no provenance) + | + = 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 `main` at $DIR/ptr_copy_loses_partial_provenance1.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 + diff --git a/tests/pass/provenance.rs b/tests/pass/provenance.rs index 9e8a9651b3..2e4d240cc4 100644 --- a/tests/pass/provenance.rs +++ b/tests/pass/provenance.rs @@ -12,6 +12,7 @@ fn main() { bytewise_custom_memcpy(); bytewise_custom_memcpy_chunked(); int_load_strip_provenance(); + maybe_uninit_preserves_partial_provenance(); } /// Some basic smoke tests for provenance. @@ -145,3 +146,24 @@ fn int_load_strip_provenance() { let ints: [usize; 1] = unsafe { mem::transmute(ptrs) }; assert_eq!(ptrs[0] as *const _ as usize, ints[0]); } + +fn maybe_uninit_preserves_partial_provenance() { + // This is the same test as ptr_copy_loses_partial_provenance.rs, but using MaybeUninit and thus + // properly preserving partial provenance. + unsafe { + let mut bytes = [1u8; 16]; + let bytes = bytes.as_mut_ptr(); + + // Put a pointer in the middle. + bytes.add(4).cast::<&i32>().write_unaligned(&42); + // Copy the entire thing as two pointers but not perfectly + // overlapping with the pointer we have in there. + let copy = bytes.cast::<[mem::MaybeUninit<*const ()>; 2]>().read_unaligned(); + let copy_bytes = copy.as_ptr().cast::(); + // Now go to the middle of the copy and get the pointer back out. + let ptr = copy_bytes.add(4).cast::<*const i32>().read_unaligned(); + // And deref this to ensure we get the right value. + let val = *ptr; + assert_eq!(val, 42); + } +} From 4c77f86ff71986f5bbbcc6aaa0485970f31ee62e Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Thu, 29 Aug 2024 19:24:31 +0200 Subject: [PATCH 3/5] interpret: reset padding during validation --- src/machine.rs | 13 +++++++ .../ptr_copy_loses_partial_provenance0.rs | 11 +++--- .../ptr_copy_loses_partial_provenance1.rs | 11 +++--- tests/fail/uninit/padding-enum.rs | 23 +++++++++++ tests/fail/uninit/padding-enum.stderr | 15 ++++++++ tests/fail/uninit/padding-pair.rs | 25 ++++++++++++ tests/fail/uninit/padding-pair.stderr | 15 ++++++++ tests/fail/uninit/padding-struct-in-union.rs | 32 ++++++++++++++++ .../uninit/padding-struct-in-union.stderr | 15 ++++++++ tests/fail/uninit/padding-struct.rs | 11 ++++++ tests/fail/uninit/padding-struct.stderr | 15 ++++++++ tests/fail/uninit/padding-union.rs | 14 +++++++ tests/fail/uninit/padding-union.stderr | 15 ++++++++ .../{ => uninit}/transmute-pair-uninit.rs | 11 +++--- .../{ => uninit}/transmute-pair-uninit.stderr | 0 tests/pass/enums.rs | 38 +++++++++++++++++++ 16 files changed, 249 insertions(+), 15 deletions(-) create mode 100644 tests/fail/uninit/padding-enum.rs create mode 100644 tests/fail/uninit/padding-enum.stderr create mode 100644 tests/fail/uninit/padding-pair.rs create mode 100644 tests/fail/uninit/padding-pair.stderr create mode 100644 tests/fail/uninit/padding-struct-in-union.rs create mode 100644 tests/fail/uninit/padding-struct-in-union.stderr create mode 100644 tests/fail/uninit/padding-struct.rs create mode 100644 tests/fail/uninit/padding-struct.stderr create mode 100644 tests/fail/uninit/padding-union.rs create mode 100644 tests/fail/uninit/padding-union.stderr rename tests/fail/{ => uninit}/transmute-pair-uninit.rs (61%) rename tests/fail/{ => uninit}/transmute-pair-uninit.stderr (100%) diff --git a/src/machine.rs b/src/machine.rs index 66c6966d1a..d1267cd686 100644 --- a/src/machine.rs +++ b/src/machine.rs @@ -572,6 +572,9 @@ pub struct MiriMachine<'tcx> { /// Invariant: the promised alignment will never be less than the native alignment of the /// allocation. pub(crate) symbolic_alignment: RefCell>, + + /// A cache of "data range" computations for unions (i.e., the offsets of non-padding bytes). + union_data_ranges: FxHashMap, RangeSet>, } impl<'tcx> MiriMachine<'tcx> { @@ -714,6 +717,7 @@ impl<'tcx> MiriMachine<'tcx> { allocation_spans: RefCell::new(FxHashMap::default()), const_cache: RefCell::new(FxHashMap::default()), symbolic_alignment: RefCell::new(FxHashMap::default()), + union_data_ranges: FxHashMap::default(), } } @@ -826,6 +830,7 @@ impl VisitProvenance for MiriMachine<'_> { allocation_spans: _, const_cache: _, symbolic_alignment: _, + union_data_ranges: _, } = self; threads.visit_provenance(visit); @@ -1627,4 +1632,12 @@ impl<'tcx> Machine<'tcx> for MiriMachine<'tcx> { ecx.machine.rng.borrow_mut().gen::() % ADDRS_PER_ANON_GLOBAL } } + + fn cached_union_data_range<'e>( + ecx: &'e mut InterpCx<'tcx, Self>, + ty: Ty<'tcx>, + compute_range: impl FnOnce() -> RangeSet, + ) -> Cow<'e, RangeSet> { + Cow::Borrowed(ecx.machine.union_data_ranges.entry(ty).or_insert_with(compute_range)) + } } diff --git a/tests/fail/provenance/ptr_copy_loses_partial_provenance0.rs b/tests/fail/provenance/ptr_copy_loses_partial_provenance0.rs index 0c6666b424..ff94f2263c 100644 --- a/tests/fail/provenance/ptr_copy_loses_partial_provenance0.rs +++ b/tests/fail/provenance/ptr_copy_loses_partial_provenance0.rs @@ -1,16 +1,17 @@ fn main() { - unsafe { - let mut bytes = [1u8; 16]; - let bytes = bytes.as_mut_ptr(); + let half_ptr = std::mem::size_of::<*const ()>() / 2; + let mut bytes = [1u8; 16]; + let bytes = bytes.as_mut_ptr(); + unsafe { // Put a pointer in the middle. - bytes.add(4).cast::<&i32>().write_unaligned(&42); + bytes.add(half_ptr).cast::<&i32>().write_unaligned(&42); // Typed copy of the entire thing as two pointers, but not perfectly // overlapping with the pointer we have in there. let copy = bytes.cast::<[*const (); 2]>().read_unaligned(); let copy_bytes = copy.as_ptr().cast::(); // Now go to the middle of the copy and get the pointer back out. - let ptr = copy_bytes.add(4).cast::<*const i32>().read_unaligned(); + let ptr = copy_bytes.add(half_ptr).cast::<*const i32>().read_unaligned(); // Dereferencing this should fail as the copy has removed the provenance. let _val = *ptr; //~ERROR: dangling } diff --git a/tests/fail/provenance/ptr_copy_loses_partial_provenance1.rs b/tests/fail/provenance/ptr_copy_loses_partial_provenance1.rs index 30d826413d..d0e3dac779 100644 --- a/tests/fail/provenance/ptr_copy_loses_partial_provenance1.rs +++ b/tests/fail/provenance/ptr_copy_loses_partial_provenance1.rs @@ -1,16 +1,17 @@ fn main() { - unsafe { - let mut bytes = [1u8; 16]; - let bytes = bytes.as_mut_ptr(); + let half_ptr = std::mem::size_of::<*const ()>() / 2; + let mut bytes = [1u8; 16]; + let bytes = bytes.as_mut_ptr(); + unsafe { // Put a pointer in the middle. - bytes.add(4).cast::<&i32>().write_unaligned(&42); + bytes.add(half_ptr).cast::<&i32>().write_unaligned(&42); // Typed copy of the entire thing as two *function* pointers, but not perfectly // overlapping with the pointer we have in there. let copy = bytes.cast::<[fn(); 2]>().read_unaligned(); let copy_bytes = copy.as_ptr().cast::(); // Now go to the middle of the copy and get the pointer back out. - let ptr = copy_bytes.add(4).cast::<*const i32>().read_unaligned(); + let ptr = copy_bytes.add(half_ptr).cast::<*const i32>().read_unaligned(); // Dereferencing this should fail as the copy has removed the provenance. let _val = *ptr; //~ERROR: dangling } diff --git a/tests/fail/uninit/padding-enum.rs b/tests/fail/uninit/padding-enum.rs new file mode 100644 index 0000000000..0ab9be2758 --- /dev/null +++ b/tests/fail/uninit/padding-enum.rs @@ -0,0 +1,23 @@ +use std::mem; + +// We have three fields to avoid the ScalarPair optimization. +#[allow(unused)] +enum E { + None, + Some(&'static (), &'static (), usize), +} + +fn main() { unsafe { + let mut p: mem::MaybeUninit = mem::MaybeUninit::zeroed(); + // The copy when `E` is returned from `transmute` should destroy padding + // (even when we use `write_unaligned`, which under the hood uses an untyped copy). + p.as_mut_ptr().write_unaligned(mem::transmute((0usize, 0usize, 0usize))); + // This is a `None`, so everything but the discriminant is padding. + assert!(matches!(*p.as_ptr(), E::None)); + + // Turns out the discriminant is (currently) stored + // in the 2nd pointer, so the first half is padding. + let c = &p as *const _ as *const u8; + let _val = *c.add(0); // Get the padding byte. + //~^ERROR: uninitialized +} } diff --git a/tests/fail/uninit/padding-enum.stderr b/tests/fail/uninit/padding-enum.stderr new file mode 100644 index 0000000000..a507a5d49b --- /dev/null +++ b/tests/fail/uninit/padding-enum.stderr @@ -0,0 +1,15 @@ +error: Undefined Behavior: using uninitialized data, but this operation requires initialized memory + --> $DIR/padding-enum.rs:LL:CC + | +LL | let _val = *c.add(0); // Get the padding byte. + | ^^^^^^^^^ using uninitialized data, but this operation requires initialized memory + | + = 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 `main` at $DIR/padding-enum.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 + diff --git a/tests/fail/uninit/padding-pair.rs b/tests/fail/uninit/padding-pair.rs new file mode 100644 index 0000000000..c8c00b3c65 --- /dev/null +++ b/tests/fail/uninit/padding-pair.rs @@ -0,0 +1,25 @@ +#![feature(core_intrinsics)] + +use std::mem::{self, MaybeUninit}; + +fn main() { + // This constructs a `(usize, bool)` pair: 9 bytes initialized, the rest not. + // Ensure that these 9 bytes are indeed initialized, and the rest is indeed not. + // This should be the case even if we write into previously initialized storage. + let mut x: MaybeUninit> = MaybeUninit::zeroed(); + let z = std::intrinsics::add_with_overflow(0usize, 0usize); + unsafe { x.as_mut_ptr().cast::<(usize, bool)>().write(z) }; + // Now read this bytewise. There should be (`ptr_size + 1`) def bytes followed by + // (`ptr_size - 1`) undef bytes (the padding after the bool) in there. + let z: *const u8 = &x as *const _ as *const _; + let first_undef = mem::size_of::() as isize + 1; + for i in 0..first_undef { + let byte = unsafe { *z.offset(i) }; + assert_eq!(byte, 0); + } + let v = unsafe { *z.offset(first_undef) }; + //~^ ERROR: uninitialized + if v == 0 { + println!("it is zero"); + } +} diff --git a/tests/fail/uninit/padding-pair.stderr b/tests/fail/uninit/padding-pair.stderr new file mode 100644 index 0000000000..d35934d83d --- /dev/null +++ b/tests/fail/uninit/padding-pair.stderr @@ -0,0 +1,15 @@ +error: Undefined Behavior: using uninitialized data, but this operation requires initialized memory + --> $DIR/padding-pair.rs:LL:CC + | +LL | let v = unsafe { *z.offset(first_undef) }; + | ^^^^^^^^^^^^^^^^^^^^^^ using uninitialized data, but this operation requires initialized memory + | + = 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 `main` at $DIR/padding-pair.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 + diff --git a/tests/fail/uninit/padding-struct-in-union.rs b/tests/fail/uninit/padding-struct-in-union.rs new file mode 100644 index 0000000000..132b858283 --- /dev/null +++ b/tests/fail/uninit/padding-struct-in-union.rs @@ -0,0 +1,32 @@ +#[repr(C)] +#[derive(Debug, Copy, Clone)] +struct Foo { + val16: u16, + // Padding bytes go here! + val32: u32, +} + +#[repr(C)] +#[derive(Debug, Copy, Clone)] +struct Bar { + bytes: [u8; 8], +} + +#[repr(C)] +union FooBar { + foo: Foo, + bar: Bar, +} + +pub fn main() { + // Initialize as u8 to ensure padding bytes are zeroed. + let mut foobar = FooBar { bar: Bar { bytes: [0u8; 8] } }; + // Reading either field is ok. + let _val = unsafe { (foobar.foo, foobar.bar) }; + // Does this assignment copy the uninitialized padding bytes + // over the initialized padding bytes? miri doesn't seem to think so. + foobar.foo = Foo { val16: 1, val32: 2 }; + // This resets the padding to uninit. + let _val = unsafe { (foobar.foo, foobar.bar) }; + //~^ ERROR: uninitialized +} diff --git a/tests/fail/uninit/padding-struct-in-union.stderr b/tests/fail/uninit/padding-struct-in-union.stderr new file mode 100644 index 0000000000..e122249af1 --- /dev/null +++ b/tests/fail/uninit/padding-struct-in-union.stderr @@ -0,0 +1,15 @@ +error: Undefined Behavior: constructing invalid value at .bytes[2]: encountered uninitialized memory, but expected an integer + --> $DIR/padding-struct-in-union.rs:LL:CC + | +LL | let _val = unsafe { (foobar.foo, foobar.bar) }; + | ^^^^^^^^^^ constructing invalid value at .bytes[2]: encountered uninitialized memory, but expected an integer + | + = 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 `main` at $DIR/padding-struct-in-union.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 + diff --git a/tests/fail/uninit/padding-struct.rs b/tests/fail/uninit/padding-struct.rs new file mode 100644 index 0000000000..dd3be50343 --- /dev/null +++ b/tests/fail/uninit/padding-struct.rs @@ -0,0 +1,11 @@ +use std::mem; + +#[repr(C)] +struct Pair(u8, u16); + +fn main() { unsafe { + let p: Pair = mem::transmute(0u32); // The copy when `Pair` is returned from `transmute` should destroy padding. + let c = &p as *const _ as *const u8; + let _val = *c.add(1); // Get the padding byte. + //~^ERROR: uninitialized +} } diff --git a/tests/fail/uninit/padding-struct.stderr b/tests/fail/uninit/padding-struct.stderr new file mode 100644 index 0000000000..8dc40a482a --- /dev/null +++ b/tests/fail/uninit/padding-struct.stderr @@ -0,0 +1,15 @@ +error: Undefined Behavior: using uninitialized data, but this operation requires initialized memory + --> $DIR/padding-struct.rs:LL:CC + | +LL | let _val = *c.add(1); // Get the padding byte. + | ^^^^^^^^^ using uninitialized data, but this operation requires initialized memory + | + = 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 `main` at $DIR/padding-struct.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 + diff --git a/tests/fail/uninit/padding-union.rs b/tests/fail/uninit/padding-union.rs new file mode 100644 index 0000000000..2e9e0a40d6 --- /dev/null +++ b/tests/fail/uninit/padding-union.rs @@ -0,0 +1,14 @@ +use std::mem; + +#[allow(unused)] +#[repr(C)] +union U { + field: (u8, u16), +} + +fn main() { unsafe { + let p: U = mem::transmute(0u32); // The copy when `U` is returned from `transmute` should destroy padding. + let c = &p as *const _ as *const [u8; 4]; + let _val = *c; // Read the entire thing, definitely contains the padding byte. + //~^ERROR: uninitialized +} } diff --git a/tests/fail/uninit/padding-union.stderr b/tests/fail/uninit/padding-union.stderr new file mode 100644 index 0000000000..04002da4f1 --- /dev/null +++ b/tests/fail/uninit/padding-union.stderr @@ -0,0 +1,15 @@ +error: Undefined Behavior: constructing invalid value at [1]: encountered uninitialized memory, but expected an integer + --> $DIR/padding-union.rs:LL:CC + | +LL | let _val = *c; // Read the entire thing, definitely contains the padding byte. + | ^^ constructing invalid value at [1]: encountered uninitialized memory, but expected an integer + | + = 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 `main` at $DIR/padding-union.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 + diff --git a/tests/fail/transmute-pair-uninit.rs b/tests/fail/uninit/transmute-pair-uninit.rs similarity index 61% rename from tests/fail/transmute-pair-uninit.rs rename to tests/fail/uninit/transmute-pair-uninit.rs index bc95f3cb7a..0ba5520a54 100644 --- a/tests/fail/transmute-pair-uninit.rs +++ b/tests/fail/uninit/transmute-pair-uninit.rs @@ -1,16 +1,17 @@ #![feature(core_intrinsics)] -use std::mem; +use std::mem::{self, MaybeUninit}; fn main() { - let x: Option> = unsafe { + // This constructs a `(usize, bool)` pair: 9 bytes initialized, the rest not. + // Ensure that these 9 bytes are indeed initialized, and the rest is indeed not. + let x: MaybeUninit> = unsafe { let z = std::intrinsics::add_with_overflow(0usize, 0usize); - std::mem::transmute::<(usize, bool), Option>>(z) + std::mem::transmute::<(usize, bool), MaybeUninit>>(z) }; - let y = &x; // Now read this bytewise. There should be (`ptr_size + 1`) def bytes followed by // (`ptr_size - 1`) undef bytes (the padding after the bool) in there. - let z: *const u8 = y as *const _ as *const _; + let z: *const u8 = &x as *const _ as *const _; let first_undef = mem::size_of::() as isize + 1; for i in 0..first_undef { let byte = unsafe { *z.offset(i) }; diff --git a/tests/fail/transmute-pair-uninit.stderr b/tests/fail/uninit/transmute-pair-uninit.stderr similarity index 100% rename from tests/fail/transmute-pair-uninit.stderr rename to tests/fail/uninit/transmute-pair-uninit.stderr diff --git a/tests/pass/enums.rs b/tests/pass/enums.rs index 1dafef025e..9fc61f07c0 100644 --- a/tests/pass/enums.rs +++ b/tests/pass/enums.rs @@ -132,6 +132,43 @@ fn overaligned_casts() { assert_eq!(aligned as u8, 0); } +// This hits a corner case in the logic for clearing padding on typed copies. +fn padding_clear_corner_case() { + #[allow(unused)] + #[derive(Copy, Clone)] + #[repr(C)] + pub struct Decoded { + /// The scaled mantissa. + pub mant: u64, + /// The lower error range. + pub minus: u64, + /// The upper error range. + pub plus: u64, + /// The shared exponent in base 2. + pub exp: i16, + /// True when the error range is inclusive. + /// + /// In IEEE 754, this is true when the original mantissa was even. + pub inclusive: bool, + } + + #[allow(unused)] + #[derive(Copy, Clone)] + pub enum FullDecoded { + /// Not-a-number. + Nan, + /// Infinities, either positive or negative. + Infinite, + /// Zero, either positive or negative. + Zero, + /// Finite numbers with further decoded fields. + Finite(Decoded), + } + + let val = FullDecoded::Finite(Decoded { mant: 0, minus: 0, plus: 0, exp: 0, inclusive: false }); + let _val2 = val; // trigger typed copy +} + fn main() { test(MyEnum::MyEmptyVariant); test(MyEnum::MyNewtypeVariant(42)); @@ -141,4 +178,5 @@ fn main() { discriminant_overflow(); more_discriminant_overflow(); overaligned_casts(); + padding_clear_corner_case(); } From cb82cdca9051cce20b515db5919f790f13c5be82 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Mon, 9 Sep 2024 14:42:03 +0200 Subject: [PATCH 4/5] union padding computation: add fast-path for ZST Also avoid even tracking empty ranges, and add fast-path for arrays of scalars --- tests/pass/arrays.rs | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/tests/pass/arrays.rs b/tests/pass/arrays.rs index 61b44453e9..b0c6f54cab 100644 --- a/tests/pass/arrays.rs +++ b/tests/pass/arrays.rs @@ -61,6 +61,20 @@ fn debug() { println!("{:?}", array); } +fn huge_zst() { + fn id(x: T) -> T { x } + + // A "huge" zero-sized array. Make sure we don't loop over it in any part of Miri. + let val = [(); usize::MAX]; + id(val); // make a copy + + let val = [val; 2]; + id(val); + + // Also wrap it in a union (which, in particular, hits the logic for computing union padding). + let _copy = std::mem::MaybeUninit::new(val); +} + fn main() { assert_eq!(empty_array(), []); assert_eq!(index_unsafe(), 20); @@ -73,4 +87,5 @@ fn main() { from(); eq(); debug(); + huge_zst(); } From ed3e1c4afe7f673c179cb76c12485e95505112c8 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Mon, 9 Sep 2024 16:11:17 +0200 Subject: [PATCH 5/5] fix UB in a test also add an explicit test for the fact that a Option has padding when it is None --- tests/fail/uninit/padding-enum.rs | 2 +- tests/fail/uninit/padding-enum.stderr | 2 +- tests/fail/uninit/padding-wide-ptr.rs | 18 ++++++++++++++++++ tests/fail/uninit/padding-wide-ptr.stderr | 15 +++++++++++++++ 4 files changed, 35 insertions(+), 2 deletions(-) create mode 100644 tests/fail/uninit/padding-wide-ptr.rs create mode 100644 tests/fail/uninit/padding-wide-ptr.stderr diff --git a/tests/fail/uninit/padding-enum.rs b/tests/fail/uninit/padding-enum.rs index 0ab9be2758..3852ac5c47 100644 --- a/tests/fail/uninit/padding-enum.rs +++ b/tests/fail/uninit/padding-enum.rs @@ -18,6 +18,6 @@ fn main() { unsafe { // Turns out the discriminant is (currently) stored // in the 2nd pointer, so the first half is padding. let c = &p as *const _ as *const u8; - let _val = *c.add(0); // Get the padding byte. + let _val = *c.add(0); // Get a padding byte. //~^ERROR: uninitialized } } diff --git a/tests/fail/uninit/padding-enum.stderr b/tests/fail/uninit/padding-enum.stderr index a507a5d49b..c571f18874 100644 --- a/tests/fail/uninit/padding-enum.stderr +++ b/tests/fail/uninit/padding-enum.stderr @@ -1,7 +1,7 @@ error: Undefined Behavior: using uninitialized data, but this operation requires initialized memory --> $DIR/padding-enum.rs:LL:CC | -LL | let _val = *c.add(0); // Get the padding byte. +LL | let _val = *c.add(0); // Get a padding byte. | ^^^^^^^^^ using uninitialized data, but this operation requires initialized memory | = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior diff --git a/tests/fail/uninit/padding-wide-ptr.rs b/tests/fail/uninit/padding-wide-ptr.rs new file mode 100644 index 0000000000..0403a9caba --- /dev/null +++ b/tests/fail/uninit/padding-wide-ptr.rs @@ -0,0 +1,18 @@ +use std::mem; + +// If this is `None`, the metadata becomes padding. +type T = Option<&'static str>; + +fn main() { unsafe { + let mut p: mem::MaybeUninit = mem::MaybeUninit::zeroed(); + // The copy when `T` is returned from `transmute` should destroy padding + // (even when we use `write_unaligned`, which under the hood uses an untyped copy). + p.as_mut_ptr().write_unaligned(mem::transmute((0usize, 0usize))); + // Null epresents `None`. + assert!(matches!(*p.as_ptr(), None)); + + // The second part, with the length, becomes padding. + let c = &p as *const _ as *const u8; + let _val = *c.add(mem::size_of::<*const u8>()); // Get a padding byte. + //~^ERROR: uninitialized +} } diff --git a/tests/fail/uninit/padding-wide-ptr.stderr b/tests/fail/uninit/padding-wide-ptr.stderr new file mode 100644 index 0000000000..0da72550b2 --- /dev/null +++ b/tests/fail/uninit/padding-wide-ptr.stderr @@ -0,0 +1,15 @@ +error: Undefined Behavior: using uninitialized data, but this operation requires initialized memory + --> $DIR/padding-wide-ptr.rs:LL:CC + | +LL | let _val = *c.add(mem::size_of::<*const u8>()); // Get a padding byte. + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ using uninitialized data, but this operation requires initialized memory + | + = 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 `main` at $DIR/padding-wide-ptr.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 +