From ccc50b21c149686e7620253b10936e79665404f1 Mon Sep 17 00:00:00 2001 From: Adwin White Date: Mon, 24 Jun 2024 14:40:04 +0800 Subject: [PATCH 1/9] add syscall `dup()` --- src/tools/miri/src/shims/unix/fd.rs | 28 +++++++++++++++++++ .../miri/src/shims/unix/foreign_items.rs | 13 +++++++++ src/tools/miri/tests/pass-dep/libc/libc-fs.rs | 26 +++++++++++++++++ 3 files changed, 67 insertions(+) diff --git a/src/tools/miri/src/shims/unix/fd.rs b/src/tools/miri/src/shims/unix/fd.rs index 599f78e712a..87e20954a70 100644 --- a/src/tools/miri/src/shims/unix/fd.rs +++ b/src/tools/miri/src/shims/unix/fd.rs @@ -273,6 +273,34 @@ impl FdTable { impl<'tcx> EvalContextExt<'tcx> for crate::MiriInterpCx<'tcx> {} pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { + fn dup(&mut self, old_fd: i32) -> InterpResult<'tcx, i32> { + let this = self.eval_context_mut(); + + match this.machine.fds.dup(old_fd) { + Some(dup_fd) => Ok(this.machine.fds.insert_fd_with_min_fd(dup_fd, 0)), + None => this.fd_not_found(), + } + } + + fn dup2(&mut self, old_fd: i32, new_fd: i32) -> InterpResult<'tcx, i32> { + let this = self.eval_context_mut(); + + match this.machine.fds.dup(old_fd) { + Some(dup_fd) => { + if new_fd != old_fd { + // Close new_fd if it is previously opened. + // If old_fd and new_fd point to the same description, then `dup_fd` ensures we keep the underlying file description alive. + if let Some(file_descriptor) = this.machine.fds.fds.insert(new_fd, dup_fd) { + // Ignore close error (not interpreter's) according to dup2() doc. + file_descriptor.close(this.machine.communicate())?.ok(); + } + } + Ok(new_fd) + } + None => this.fd_not_found(), + } + } + fn fcntl(&mut self, args: &[OpTy<'tcx>]) -> InterpResult<'tcx, i32> { let this = self.eval_context_mut(); diff --git a/src/tools/miri/src/shims/unix/foreign_items.rs b/src/tools/miri/src/shims/unix/foreign_items.rs index 53ad40cfd2c..2421f9244f3 100644 --- a/src/tools/miri/src/shims/unix/foreign_items.rs +++ b/src/tools/miri/src/shims/unix/foreign_items.rs @@ -115,6 +115,19 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { let result = this.fcntl(args)?; this.write_scalar(Scalar::from_i32(result), dest)?; } + "dup" => { + let [old_fd] = this.check_shim(abi, Abi::C { unwind: false }, link_name, args)?; + let old_fd = this.read_scalar(old_fd)?.to_i32()?; + let new_fd = this.dup(old_fd)?; + this.write_scalar(Scalar::from_i32(new_fd), dest)?; + } + "dup2" => { + let [old_fd, new_fd] = this.check_shim(abi, Abi::C { unwind: false }, link_name, args)?; + let old_fd = this.read_scalar(old_fd)?.to_i32()?; + let new_fd = this.read_scalar(new_fd)?.to_i32()?; + let result = this.dup2(old_fd, new_fd)?; + this.write_scalar(Scalar::from_i32(result), dest)?; + } // File and file system access "open" | "open64" => { diff --git a/src/tools/miri/tests/pass-dep/libc/libc-fs.rs b/src/tools/miri/tests/pass-dep/libc/libc-fs.rs index 80c9757e9c9..da685e5c6b7 100644 --- a/src/tools/miri/tests/pass-dep/libc/libc-fs.rs +++ b/src/tools/miri/tests/pass-dep/libc/libc-fs.rs @@ -15,6 +15,7 @@ use std::path::PathBuf; mod utils; fn main() { + test_dup(); test_dup_stdout_stderr(); test_canonicalize_too_long(); test_rename(); @@ -74,6 +75,31 @@ fn test_dup_stdout_stderr() { } } +fn test_dup() { + let bytes = b"dup and dup2"; + let path = utils::prepare_with_content("miri_test_libc_dup.txt", bytes); + + let mut name = path.into_os_string(); + name.push("\0"); + let name_ptr = name.as_bytes().as_ptr().cast::(); + unsafe { + let fd = libc::open(name_ptr, libc::O_RDONLY); + let mut first_buf = [0u8; 4]; + libc::read(fd, first_buf.as_mut_ptr() as *mut libc::c_void, 4); + assert_eq!(&first_buf, b"dup "); + + let new_fd = libc::dup(fd); + let mut second_buf = [0u8; 4]; + libc::read(new_fd, second_buf.as_mut_ptr() as *mut libc::c_void, 4); + assert_eq!(&second_buf, b"and "); + + let new_fd2 = libc::dup2(fd, 8); + let mut third_buf = [0u8; 4]; + libc::read(new_fd2, third_buf.as_mut_ptr() as *mut libc::c_void, 4); + assert_eq!(&third_buf, b"dup2"); + } +} + fn test_canonicalize_too_long() { // Make sure we get an error for long paths. let too_long = "x/".repeat(libc::PATH_MAX.try_into().unwrap()); From 18049b73ec1c0b899d5ede199ba991f51fd652ed Mon Sep 17 00:00:00 2001 From: bjorn3 <17426603+bjorn3@users.noreply.github.com> Date: Mon, 1 Jul 2024 17:40:00 +0000 Subject: [PATCH 2/9] Use the symbol_name query instead of trying to infer from the link_name attribute This prevents the calculated name from going out of sync with exported_symbols. It also avoids having to special case the panic_impl lang item. --- src/tools/miri/src/helpers.rs | 10 +--------- src/tools/miri/src/machine.rs | 4 ++-- src/tools/miri/src/shims/foreign_items.rs | 15 --------------- 3 files changed, 3 insertions(+), 26 deletions(-) diff --git a/src/tools/miri/src/helpers.rs b/src/tools/miri/src/helpers.rs index a7a6f8cfd87..590e8984e99 100644 --- a/src/tools/miri/src/helpers.rs +++ b/src/tools/miri/src/helpers.rs @@ -24,7 +24,7 @@ use rustc_middle::ty::{ FloatTy, IntTy, Ty, TyCtxt, UintTy, }; use rustc_session::config::CrateType; -use rustc_span::{sym, Span, Symbol}; +use rustc_span::{Span, Symbol}; use rustc_target::abi::{Align, FieldIdx, FieldsShape, Size, Variants}; use rustc_target::spec::abi::Abi; @@ -1182,14 +1182,6 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { this.alloc_mark_immutable(provenance.get_alloc_id().unwrap()).unwrap(); } - fn item_link_name(&self, def_id: DefId) -> Symbol { - let tcx = self.eval_context_ref().tcx; - match tcx.get_attrs(def_id, sym::link_name).filter_map(|a| a.value_str()).next() { - Some(name) => name, - None => tcx.item_name(def_id), - } - } - /// Converts `src` from floating point to integer type `dest_ty` /// after rounding with mode `round`. /// Returns `None` if `f` is NaN or out of range. diff --git a/src/tools/miri/src/machine.rs b/src/tools/miri/src/machine.rs index 0d91279f9f4..e321237bb4a 100644 --- a/src/tools/miri/src/machine.rs +++ b/src/tools/miri/src/machine.rs @@ -954,7 +954,7 @@ impl<'tcx> Machine<'tcx> for MiriMachine<'tcx> { // foreign function // Any needed call to `goto_block` will be performed by `emulate_foreign_item`. let args = ecx.copy_fn_args(args); // FIXME: Should `InPlace` arguments be reset to uninit? - let link_name = ecx.item_link_name(instance.def_id()); + let link_name = Symbol::intern(ecx.tcx.symbol_name(instance).name); return ecx.emulate_foreign_item(link_name, abi, &args, dest, ret, unwind); } @@ -1050,7 +1050,7 @@ impl<'tcx> Machine<'tcx> for MiriMachine<'tcx> { ecx: &MiriInterpCx<'tcx>, def_id: DefId, ) -> InterpResult<'tcx, StrictPointer> { - let link_name = ecx.item_link_name(def_id); + let link_name = Symbol::intern(ecx.tcx.symbol_name(Instance::mono(*ecx.tcx, def_id)).name); if let Some(&ptr) = ecx.machine.extern_statics.get(&link_name) { // Various parts of the engine rely on `get_alloc_info` for size and alignment // information. That uses the type information of this static. diff --git a/src/tools/miri/src/shims/foreign_items.rs b/src/tools/miri/src/shims/foreign_items.rs index f9ccc6ad4d2..9004f7efc8b 100644 --- a/src/tools/miri/src/shims/foreign_items.rs +++ b/src/tools/miri/src/shims/foreign_items.rs @@ -46,24 +46,9 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { unwind: mir::UnwindAction, ) -> InterpResult<'tcx, Option<(&'tcx mir::Body<'tcx>, ty::Instance<'tcx>)>> { let this = self.eval_context_mut(); - let tcx = this.tcx.tcx; // Some shims forward to other MIR bodies. match link_name.as_str() { - // This matches calls to the foreign item `panic_impl`. - // The implementation is provided by the function with the `#[panic_handler]` attribute. - "panic_impl" => { - // We don't use `check_shim` here because we are just forwarding to the lang - // item. Argument count checking will be performed when the returned `Body` is - // called. - this.check_abi_and_shim_symbol_clash(abi, Abi::Rust, link_name)?; - let panic_impl_id = tcx.lang_items().panic_impl().unwrap(); - let panic_impl_instance = ty::Instance::mono(tcx, panic_impl_id); - return Ok(Some(( - this.load_mir(panic_impl_instance.def, None)?, - panic_impl_instance, - ))); - } "__rust_alloc_error_handler" => { // Forward to the right symbol that implements this function. let Some(handler_kind) = this.tcx.alloc_error_handler_kind(()) else { From 3e44883606ad4df77018c157971ba7a47e540088 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Tue, 2 Jul 2024 08:21:44 +0200 Subject: [PATCH 3/9] Preparing for merge from rustc --- src/tools/miri/rust-version | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/tools/miri/rust-version b/src/tools/miri/rust-version index fd59ad3b8f1..912aa11ded0 100644 --- a/src/tools/miri/rust-version +++ b/src/tools/miri/rust-version @@ -1 +1 @@ -9ed2ab3790ff41bf741dd690befd6a1c1e2b23ca +7d97c59438e933e86f557ed999da3b8dfc6855a7 From c96bec1860f6a95c045e76a3a456495d7c1dc21f Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Tue, 2 Jul 2024 21:03:13 +0200 Subject: [PATCH 4/9] use let-else to avoid rightwards drift --- src/tools/miri/src/shims/unix/fd.rs | 45 ++++++++++++++--------------- 1 file changed, 21 insertions(+), 24 deletions(-) diff --git a/src/tools/miri/src/shims/unix/fd.rs b/src/tools/miri/src/shims/unix/fd.rs index 87e20954a70..7f6a0978103 100644 --- a/src/tools/miri/src/shims/unix/fd.rs +++ b/src/tools/miri/src/shims/unix/fd.rs @@ -276,29 +276,27 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { fn dup(&mut self, old_fd: i32) -> InterpResult<'tcx, i32> { let this = self.eval_context_mut(); - match this.machine.fds.dup(old_fd) { - Some(dup_fd) => Ok(this.machine.fds.insert_fd_with_min_fd(dup_fd, 0)), - None => this.fd_not_found(), - } + let Some(dup_fd) = this.machine.fds.dup(old_fd) else { + return this.fd_not_found(); + }; + Ok(this.machine.fds.insert_fd_with_min_fd(dup_fd, 0)) } fn dup2(&mut self, old_fd: i32, new_fd: i32) -> InterpResult<'tcx, i32> { let this = self.eval_context_mut(); - match this.machine.fds.dup(old_fd) { - Some(dup_fd) => { - if new_fd != old_fd { - // Close new_fd if it is previously opened. - // If old_fd and new_fd point to the same description, then `dup_fd` ensures we keep the underlying file description alive. - if let Some(file_descriptor) = this.machine.fds.fds.insert(new_fd, dup_fd) { - // Ignore close error (not interpreter's) according to dup2() doc. - file_descriptor.close(this.machine.communicate())?.ok(); - } - } - Ok(new_fd) + let Some(dup_fd) = this.machine.fds.dup(old_fd) else { + return this.fd_not_found(); + }; + if new_fd != old_fd { + // Close new_fd if it is previously opened. + // If old_fd and new_fd point to the same description, then `dup_fd` ensures we keep the underlying file description alive. + if let Some(file_descriptor) = this.machine.fds.fds.insert(new_fd, dup_fd) { + // Ignore close error (not interpreter's) according to dup2() doc. + file_descriptor.close(this.machine.communicate())?.ok(); } - None => this.fd_not_found(), } + Ok(new_fd) } fn fcntl(&mut self, args: &[OpTy<'tcx>]) -> InterpResult<'tcx, i32> { @@ -362,14 +360,13 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { let fd = this.read_scalar(fd_op)?.to_i32()?; - Ok(Scalar::from_i32(if let Some(file_descriptor) = this.machine.fds.remove(fd) { - let result = file_descriptor.close(this.machine.communicate())?; - // return `0` if close is successful - let result = result.map(|()| 0i32); - this.try_unwrap_io_result(result)? - } else { - this.fd_not_found()? - })) + let Some(file_descriptor) = this.machine.fds.remove(fd) else { + return Ok(Scalar::from_i32(this.fd_not_found()?)); + }; + let result = file_descriptor.close(this.machine.communicate())?; + // return `0` if close is successful + let result = result.map(|()| 0i32); + Ok(Scalar::from_i32(this.try_unwrap_io_result(result)?)) } /// Function used when a file descriptor does not exist. It returns `Ok(-1)`and sets From d982844b47c88c2d78cde42de88f27ce75586972 Mon Sep 17 00:00:00 2001 From: Tobias Decking Date: Mon, 1 Jul 2024 21:01:49 +0200 Subject: [PATCH 5/9] Implement the `_mm256_zeroupper` and `_mm256_zeroall` intrinsics --- src/tools/miri/src/shims/x86/avx.rs | 11 +++++++++++ .../miri/tests/pass/shims/x86/intrinsics-x86-avx.rs | 5 +++++ 2 files changed, 16 insertions(+) diff --git a/src/tools/miri/src/shims/x86/avx.rs b/src/tools/miri/src/shims/x86/avx.rs index 0d2977b7b6f..f36bb4826e4 100644 --- a/src/tools/miri/src/shims/x86/avx.rs +++ b/src/tools/miri/src/shims/x86/avx.rs @@ -338,6 +338,17 @@ pub(super) trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { this.write_scalar(Scalar::from_i32(res.into()), dest)?; } + // Used to implement the `_mm256_zeroupper` and `_mm256_zeroall` functions. + // These function clear out the upper 128 bits of all avx registers or + // zero out all avx registers respectively. + "vzeroupper" | "vzeroall" => { + // These functions are purely a performance hint for the CPU. + // Any registers currently in use will be saved beforehand by the + // compiler, making these functions no-ops. + + // The only thing that needs to be ensured is the correct calling convention. + let [] = this.check_shim(abi, Abi::C { unwind: false }, link_name, args)?; + } _ => return Ok(EmulateItemResult::NotSupported), } Ok(EmulateItemResult::NeedsReturn) diff --git a/src/tools/miri/tests/pass/shims/x86/intrinsics-x86-avx.rs b/src/tools/miri/tests/pass/shims/x86/intrinsics-x86-avx.rs index 7d43cc596ae..728f57d48f1 100644 --- a/src/tools/miri/tests/pass/shims/x86/intrinsics-x86-avx.rs +++ b/src/tools/miri/tests/pass/shims/x86/intrinsics-x86-avx.rs @@ -1342,6 +1342,11 @@ unsafe fn test_avx() { assert_eq!(r, 1); } test_mm_testnzc_ps(); + + // These intrinsics are functionally no-ops. The only thing + // that needs to be tested is that they can be executed. + _mm256_zeroupper(); + _mm256_zeroall(); } #[target_feature(enable = "sse2")] From 5c2946a4beb927d5c34519d5310b6507efb91724 Mon Sep 17 00:00:00 2001 From: The Miri Cronjob Bot Date: Thu, 4 Jul 2024 04:54:26 +0000 Subject: [PATCH 6/9] Preparing for merge from rustc --- src/tools/miri/rust-version | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/tools/miri/rust-version b/src/tools/miri/rust-version index 912aa11ded0..5a35166769e 100644 --- a/src/tools/miri/rust-version +++ b/src/tools/miri/rust-version @@ -1 +1 @@ -7d97c59438e933e86f557ed999da3b8dfc6855a7 +66b4f0021bfb11a8c20d084c99a40f4a78ce1d38 From 8ef2c6c6afd8f5b4d5c69278cc3dd986e37c574d Mon Sep 17 00:00:00 2001 From: Johannes Hostert Date: Wed, 3 Jul 2024 22:56:31 +0200 Subject: [PATCH 7/9] TB: Make FnEntry access on protected locations be a write under certain circumstances --- .../tree_borrows/diagnostics.rs | 7 ++-- .../src/borrow_tracker/tree_borrows/mod.rs | 19 ++------- .../src/borrow_tracker/tree_borrows/perms.rs | 4 ++ .../src/borrow_tracker/tree_borrows/tree.rs | 41 +++++++++++-------- 4 files changed, 35 insertions(+), 36 deletions(-) diff --git a/src/tools/miri/src/borrow_tracker/tree_borrows/diagnostics.rs b/src/tools/miri/src/borrow_tracker/tree_borrows/diagnostics.rs index 8abc8530f7c..498b7dc3e42 100644 --- a/src/tools/miri/src/borrow_tracker/tree_borrows/diagnostics.rs +++ b/src/tools/miri/src/borrow_tracker/tree_borrows/diagnostics.rs @@ -19,7 +19,7 @@ pub enum AccessCause { Explicit(AccessKind), Reborrow, Dealloc, - FnExit, + FnExit(AccessKind), } impl fmt::Display for AccessCause { @@ -28,7 +28,8 @@ impl fmt::Display for AccessCause { Self::Explicit(kind) => write!(f, "{kind}"), Self::Reborrow => write!(f, "reborrow"), Self::Dealloc => write!(f, "deallocation"), - Self::FnExit => write!(f, "protector release"), + Self::FnExit(AccessKind::Read) => write!(f, "protector release read"), + Self::FnExit(AccessKind::Write) => write!(f, "protector release write"), } } } @@ -40,7 +41,7 @@ impl AccessCause { Self::Explicit(kind) => format!("{rel} {kind}"), Self::Reborrow => format!("reborrow (acting as a {rel} read access)"), Self::Dealloc => format!("deallocation (acting as a {rel} write access)"), - Self::FnExit => format!("protector release (acting as a {rel} read access)"), + Self::FnExit(kind) => format!("protector release (acting as a {rel} {kind})"), } } } diff --git a/src/tools/miri/src/borrow_tracker/tree_borrows/mod.rs b/src/tools/miri/src/borrow_tracker/tree_borrows/mod.rs index 77e003ab8a7..86074384084 100644 --- a/src/tools/miri/src/borrow_tracker/tree_borrows/mod.rs +++ b/src/tools/miri/src/borrow_tracker/tree_borrows/mod.rs @@ -68,13 +68,11 @@ impl<'tcx> Tree { let global = machine.borrow_tracker.as_ref().unwrap(); let span = machine.current_span(); self.perform_access( - access_kind, tag, - Some(range), + Some((range, access_kind, diagnostics::AccessCause::Explicit(access_kind))), global, alloc_id, span, - diagnostics::AccessCause::Explicit(access_kind), ) } @@ -115,15 +113,8 @@ impl<'tcx> Tree { alloc_id: AllocId, // diagnostics ) -> InterpResult<'tcx> { let span = machine.current_span(); - self.perform_access( - AccessKind::Read, - tag, - None, // no specified range because it occurs on the entire allocation - global, - alloc_id, - span, - diagnostics::AccessCause::FnExit, - ) + // `None` makes it the magic on-protector-end operation + self.perform_access(tag, None, global, alloc_id, span) } } @@ -297,13 +288,11 @@ trait EvalContextPrivExt<'tcx>: crate::MiriInterpCxExt<'tcx> { // All reborrows incur a (possibly zero-sized) read access to the parent tree_borrows.perform_access( - AccessKind::Read, orig_tag, - Some(range), + Some((range, AccessKind::Read, diagnostics::AccessCause::Reborrow)), this.machine.borrow_tracker.as_ref().unwrap(), alloc_id, this.machine.current_span(), - diagnostics::AccessCause::Reborrow, )?; // Record the parent-child pair in the tree. tree_borrows.new_child(orig_tag, new_tag, new_perm.initial_state, range, span)?; diff --git a/src/tools/miri/src/borrow_tracker/tree_borrows/perms.rs b/src/tools/miri/src/borrow_tracker/tree_borrows/perms.rs index fb3a4c8dad9..7aa9c3e862b 100644 --- a/src/tools/miri/src/borrow_tracker/tree_borrows/perms.rs +++ b/src/tools/miri/src/borrow_tracker/tree_borrows/perms.rs @@ -186,6 +186,10 @@ impl Permission { pub fn is_disabled(&self) -> bool { self.inner == Disabled } + /// Check if `self` is the post-child-write state of a pointer (is `Active`). + pub fn is_active(&self) -> bool { + self.inner == Active + } /// Default initial permission of the root of a new tree at inbounds positions. /// Must *only* be used for the root, this is not in general an "initial" permission! diff --git a/src/tools/miri/src/borrow_tracker/tree_borrows/tree.rs b/src/tools/miri/src/borrow_tracker/tree_borrows/tree.rs index ff4589657af..90bd1103218 100644 --- a/src/tools/miri/src/borrow_tracker/tree_borrows/tree.rs +++ b/src/tools/miri/src/borrow_tracker/tree_borrows/tree.rs @@ -530,13 +530,11 @@ impl<'tcx> Tree { span: Span, // diagnostics ) -> InterpResult<'tcx> { self.perform_access( - AccessKind::Write, tag, - Some(access_range), + Some((access_range, AccessKind::Write, diagnostics::AccessCause::Dealloc)), global, alloc_id, span, - diagnostics::AccessCause::Dealloc, )?; for (perms_range, perms) in self.rperms.iter_mut(access_range.start, access_range.size) { TreeVisitor { nodes: &mut self.nodes, tag_mapping: &self.tag_mapping, perms } @@ -570,12 +568,16 @@ impl<'tcx> Tree { } /// Map the per-node and per-location `LocationState::perform_access` - /// to each location of `access_range`, on every tag of the allocation. + /// to each location of the first component of `access_range_and_kind`, + /// on every tag of the allocation. /// - /// If `access_range` is `None`, this is interpreted as the special + /// If `access_range_and_kind` is `None`, this is interpreted as the special /// access that is applied on protector release: /// - the access will be applied only to initialized locations of the allocation, - /// - and it will not be visible to children. + /// - it will not be visible to children, + /// - it will be recorded as a `FnExit` diagnostic access + /// - and it will be a read except if the location is `Active`, i.e. has been written to, + /// in which case it will be a write. /// /// `LocationState::perform_access` will take care of raising transition /// errors and updating the `initialized` status of each location, @@ -585,13 +587,11 @@ impl<'tcx> Tree { /// - recording the history. pub fn perform_access( &mut self, - access_kind: AccessKind, tag: BorTag, - access_range: Option, + access_range_and_kind: Option<(AllocRange, AccessKind, diagnostics::AccessCause)>, global: &GlobalState, - alloc_id: AllocId, // diagnostics - span: Span, // diagnostics - access_cause: diagnostics::AccessCause, // diagnostics + alloc_id: AllocId, // diagnostics + span: Span, // diagnostics ) -> InterpResult<'tcx> { use std::ops::Range; // Performs the per-node work: @@ -605,6 +605,8 @@ impl<'tcx> Tree { // `perms_range` is only for diagnostics (it is the range of // the `RangeMap` on which we are currently working). let node_app = |perms_range: Range, + access_kind: AccessKind, + access_cause: diagnostics::AccessCause, args: NodeAppArgs<'_>| -> Result { let NodeAppArgs { node, mut perm, rel_pos } = args; @@ -618,14 +620,13 @@ impl<'tcx> Tree { let protected = global.borrow().protected_tags.contains_key(&node.tag); let transition = old_state.perform_access(access_kind, rel_pos, protected)?; - // Record the event as part of the history if !transition.is_noop() { node.debug_info.history.push(diagnostics::Event { transition, is_foreign: rel_pos.is_foreign(), access_cause, - access_range, + access_range: access_range_and_kind.map(|x| x.0), transition_range: perms_range, span, }); @@ -636,6 +637,7 @@ impl<'tcx> Tree { // Error handler in case `node_app` goes wrong. // Wraps the faulty transition in more context for diagnostics. let err_handler = |perms_range: Range, + access_cause: diagnostics::AccessCause, args: ErrHandlerArgs<'_, TransitionError>| -> InterpError<'tcx> { let ErrHandlerArgs { error_kind, conflicting_info, accessed_info } = args; @@ -650,7 +652,7 @@ impl<'tcx> Tree { .build() }; - if let Some(access_range) = access_range { + if let Some((access_range, access_kind, access_cause)) = access_range_and_kind { // Default branch: this is a "normal" access through a known range. // We iterate over affected locations and traverse the tree for each of them. for (perms_range, perms) in self.rperms.iter_mut(access_range.start, access_range.size) @@ -658,8 +660,8 @@ impl<'tcx> Tree { TreeVisitor { nodes: &mut self.nodes, tag_mapping: &self.tag_mapping, perms } .traverse_parents_this_children_others( tag, - |args| node_app(perms_range.clone(), args), - |args| err_handler(perms_range.clone(), args), + |args| node_app(perms_range.clone(), access_kind, access_cause, args), + |args| err_handler(perms_range.clone(), access_cause, args), )?; } } else { @@ -678,11 +680,14 @@ impl<'tcx> Tree { if let Some(p) = perms.get(idx) && p.initialized { + let access_kind = + if p.permission.is_active() { AccessKind::Write } else { AccessKind::Read }; + let access_cause = diagnostics::AccessCause::FnExit(access_kind); TreeVisitor { nodes: &mut self.nodes, tag_mapping: &self.tag_mapping, perms } .traverse_nonchildren( tag, - |args| node_app(perms_range.clone(), args), - |args| err_handler(perms_range.clone(), args), + |args| node_app(perms_range.clone(), access_kind, access_cause, args), + |args| err_handler(perms_range.clone(), access_cause, args), )?; } } From 0a86e7966442e12fa2080727d08047fdd2d3e619 Mon Sep 17 00:00:00 2001 From: Johannes Hostert Date: Thu, 4 Jul 2024 10:59:30 +0200 Subject: [PATCH 8/9] Add UI test for protector end write semantics --- .../fail/tree_borrows/protector-write-lazy.rs | 35 +++++++++++++++++++ .../tree_borrows/protector-write-lazy.stderr | 27 ++++++++++++++ 2 files changed, 62 insertions(+) create mode 100644 src/tools/miri/tests/fail/tree_borrows/protector-write-lazy.rs create mode 100644 src/tools/miri/tests/fail/tree_borrows/protector-write-lazy.stderr diff --git a/src/tools/miri/tests/fail/tree_borrows/protector-write-lazy.rs b/src/tools/miri/tests/fail/tree_borrows/protector-write-lazy.rs new file mode 100644 index 00000000000..238f6dba9d3 --- /dev/null +++ b/src/tools/miri/tests/fail/tree_borrows/protector-write-lazy.rs @@ -0,0 +1,35 @@ +//@compile-flags: -Zmiri-tree-borrows +// This test tests that TB's protector end semantics correctly ensure +// that protected activated writes can be reordered. +fn the_other_function(ref_to_fst_elem: &mut i32, ptr_to_vec: *mut i32) -> *mut i32 { + // Activate the reference. Afterwards, we should be able to reorder arbitrary writes. + *ref_to_fst_elem = 0; + // Here is such an arbitrary write. + // It could be moved down after the retag, in which case the `funky_ref` would be invalidated. + // We need to ensure that the `funky_ptr` is unusable even if the write to `ref_to_fst_elem` + // happens before the retag. + *ref_to_fst_elem = 42; + // this creates a reference that is Reserved Lazy on the first element (offset 0). + // It does so by doing a proper retag on the second element (offset 1), which is fine + // since nothing else happens at that offset, but the lazy init mechanism means it's + // also reserved at offset 0, but not initialized. + let funky_ptr_lazy_on_fst_elem = + unsafe { (&mut *(ptr_to_vec.wrapping_add(1))) as *mut i32 }.wrapping_sub(1); + // If we write to `ref_to_fst_elem` here, then any further access to `funky_ptr_lazy_on_fst_elem` would + // definitely be UB. Since the compiler ought to be able to reorder the write of `42` above down to + // here, that means we want this program to also be UB. + return funky_ptr_lazy_on_fst_elem; +} + +fn main() { + let mut v = vec![0, 1]; + // get a pointer to the root of the allocation + // note that it's not important it's the actual root, what matters is that it's a parent + // of both references that will be created + let ptr_to_vec = v.as_mut_ptr(); + let ref_to_fst_elem = unsafe { &mut *ptr_to_vec }; + let funky_ptr_lazy_on_fst_elem = the_other_function(ref_to_fst_elem, ptr_to_vec); + // now we try to use the funky lazy pointer. + // It should be UB, since the write-on-protector-end should disable it. + unsafe { println!("Value of funky: {}", *funky_ptr_lazy_on_fst_elem) } //~ ERROR: /reborrow through .* is forbidden/ +} diff --git a/src/tools/miri/tests/fail/tree_borrows/protector-write-lazy.stderr b/src/tools/miri/tests/fail/tree_borrows/protector-write-lazy.stderr new file mode 100644 index 00000000000..955abd144c7 --- /dev/null +++ b/src/tools/miri/tests/fail/tree_borrows/protector-write-lazy.stderr @@ -0,0 +1,27 @@ +error: Undefined Behavior: reborrow through at ALLOC[0x0] is forbidden + --> $DIR/protector-write-lazy.rs:LL:CC + | +LL | unsafe { println!("Value of funky: {}", *funky_ptr_lazy_on_fst_elem) } + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ reborrow through at ALLOC[0x0] is forbidden + | + = 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 has state Disabled which forbids this reborrow (acting as a child read access) +help: the accessed tag was created here, in the initial state Reserved + --> $DIR/protector-write-lazy.rs:LL:CC + | +LL | unsafe { (&mut *(ptr_to_vec.wrapping_add(1))) as *mut i32 }.wrapping_sub(1); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +help: the accessed tag later transitioned to Disabled due to a protector release (acting as a foreign write access) on every location previously accessed by this tag + --> $DIR/protector-write-lazy.rs:LL:CC + | +LL | } + | ^ + = help: this transition corresponds to a loss of read and write permissions + = note: BACKTRACE (of the first span): + = note: inside `main` at $DIR/protector-write-lazy.rs:LL:CC + = note: this error originates in the macro `$crate::format_args_nl` which comes from the expansion of the macro `println` (in Nightly builds, run with -Z macro-backtrace for more info) + +note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace + +error: aborting due to 1 previous error + From 02bec40930d7256d290c6a61b60df8284e460dda Mon Sep 17 00:00:00 2001 From: Johannes Hostert Date: Thu, 4 Jul 2024 12:01:49 +0200 Subject: [PATCH 9/9] TB: protector end semantics never causes immediate UB --- .../miri/src/borrow_tracker/tree_borrows/diagnostics.rs | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/tools/miri/src/borrow_tracker/tree_borrows/diagnostics.rs b/src/tools/miri/src/borrow_tracker/tree_borrows/diagnostics.rs index 498b7dc3e42..a753de28a04 100644 --- a/src/tools/miri/src/borrow_tracker/tree_borrows/diagnostics.rs +++ b/src/tools/miri/src/borrow_tracker/tree_borrows/diagnostics.rs @@ -28,8 +28,11 @@ impl fmt::Display for AccessCause { Self::Explicit(kind) => write!(f, "{kind}"), Self::Reborrow => write!(f, "reborrow"), Self::Dealloc => write!(f, "deallocation"), - Self::FnExit(AccessKind::Read) => write!(f, "protector release read"), - Self::FnExit(AccessKind::Write) => write!(f, "protector release write"), + // This is dead code, since the protector release access itself can never + // cause UB (while the protector is active, if some other access invalidates + // further use of the protected tag, that is immediate UB). + // Describing the cause of UB is the only time this function is called. + Self::FnExit(_) => unreachable!("protector accesses can never be the source of UB"), } } }