From 8935bc6247ca4e666736eedf1dd4a3cb4a9d2e34 Mon Sep 17 00:00:00 2001 From: Alice Ryhl Date: Sun, 22 Sep 2024 18:07:32 +0200 Subject: [PATCH 1/7] io: avoid pointer casts in IO driver on miri Signed-off-by: Alice Ryhl --- tokio/src/runtime/io/driver.rs | 3 +- tokio/src/runtime/io/mod.rs | 3 ++ tokio/src/runtime/io/registration_set.rs | 1 + tokio/src/runtime/io/scheduled_io.rs | 3 +- tokio/src/util/mod.rs | 3 ++ tokio/src/util/ptr_expose.rs | 64 ++++++++++++++++++++++++ 6 files changed, 73 insertions(+), 4 deletions(-) create mode 100644 tokio/src/util/ptr_expose.rs diff --git a/tokio/src/runtime/io/driver.rs b/tokio/src/runtime/io/driver.rs index 0f7b1e57acb..5b97a8802de 100644 --- a/tokio/src/runtime/io/driver.rs +++ b/tokio/src/runtime/io/driver.rs @@ -168,8 +168,7 @@ impl Driver { self.signal_ready = true; } else { let ready = Ready::from_mio(event); - // Use std::ptr::from_exposed_addr when stable - let ptr: *const ScheduledIo = token.0 as *const _; + let ptr = super::EXPOSE_IO.from_exposed_addr(token.0); // Safety: we ensure that the pointers used as tokens are not freed // until they are both deregistered from mio **and** we know the I/O diff --git a/tokio/src/runtime/io/mod.rs b/tokio/src/runtime/io/mod.rs index e7c721dfb3b..bc748f68d36 100644 --- a/tokio/src/runtime/io/mod.rs +++ b/tokio/src/runtime/io/mod.rs @@ -14,3 +14,6 @@ use scheduled_io::ScheduledIo; mod metrics; use metrics::IoDriverMetrics; + +use crate::util::PtrExposeDomain; +static EXPOSE_IO: PtrExposeDomain = PtrExposeDomain::new(); diff --git a/tokio/src/runtime/io/registration_set.rs b/tokio/src/runtime/io/registration_set.rs index 1a8bd09c310..9b2f3f13c43 100644 --- a/tokio/src/runtime/io/registration_set.rs +++ b/tokio/src/runtime/io/registration_set.rs @@ -115,6 +115,7 @@ impl RegistrationSet { // This function is marked as unsafe, because the caller must make sure that // `io` is part of the registration set. pub(super) unsafe fn remove(&self, synced: &mut Synced, io: &ScheduledIo) { + super::EXPOSE_IO.unexpose_provenance(io); let _ = synced.registrations.remove(io.into()); } } diff --git a/tokio/src/runtime/io/scheduled_io.rs b/tokio/src/runtime/io/scheduled_io.rs index cf25b63867c..ee6977c00e7 100644 --- a/tokio/src/runtime/io/scheduled_io.rs +++ b/tokio/src/runtime/io/scheduled_io.rs @@ -187,8 +187,7 @@ impl Default for ScheduledIo { impl ScheduledIo { pub(crate) fn token(&self) -> mio::Token { - // use `expose_addr` when stable - mio::Token(self as *const _ as usize) + mio::Token(super::EXPOSE_IO.expose_provenance(self)) } /// Invoked when the IO driver is shut down; forces this `ScheduledIo` into a diff --git a/tokio/src/util/mod.rs b/tokio/src/util/mod.rs index 3722b0bc2d4..38fe354e4c2 100644 --- a/tokio/src/util/mod.rs +++ b/tokio/src/util/mod.rs @@ -69,6 +69,9 @@ cfg_rt! { mod rc_cell; pub(crate) use rc_cell::RcCell; + + mod ptr_expose; + pub(crate) use ptr_expose::PtrExposeDomain; } cfg_rt_multi_thread! { diff --git a/tokio/src/util/ptr_expose.rs b/tokio/src/util/ptr_expose.rs new file mode 100644 index 00000000000..89a55c8a358 --- /dev/null +++ b/tokio/src/util/ptr_expose.rs @@ -0,0 +1,64 @@ +//! Utility for helping miri understand our exposed pointers. +//! +//! During normal execution, this module is equivalent to pointer casts. However, when running +//! under miri, pointer casts are replaced with lookups in a hash map. This makes Tokio compatible +//! with strict provenance when running under miri (which comes with a perf cost). + +use std::marker::PhantomData; +#[cfg(miri)] +use {std::collections::HashMap, crate::loom::sync::Mutex}; + +pub(crate) struct PtrExposeDomain { + #[cfg(miri)] + map: Mutex>, + _phantom: PhantomData, +} + +impl PtrExposeDomain { + pub(crate) const fn new() -> Self { + Self { + #[cfg(miri)] + map: Mutex::new(HashMap::new()), + _phantom: PhantomData, + } + } + + #[inline] + pub(crate) fn expose_provenance(&self, ptr: *const T) -> usize { + #[cfg(miri)] + { + // SAFETY: Equivalent to `pointer::addr` which is safe. + let addr: usize = unsafe { std::mem::transmute(ptr) }; + self.map.lock().insert(addr, ptr); + addr + } + + #[cfg(not(miri))] + { + ptr as usize + } + } + + #[inline] + pub(crate) fn from_exposed_addr(&self, addr: usize) -> *const T { + #[cfg(miri)] + { + self.map.lock().get(&addr).expect("Provided address is not exposed.") + } + + #[cfg(not(miri))] + { + addr as *const T + } + } + + #[inline] + pub(crate) fn unexpose_provenance(&self, _ptr: *const T) { + #[cfg(miri)] + { + // SAFETY: Equivalent to `pointer::addr` which is safe. + let addr: usize = unsafe { std::mem::transmute(_ptr) }; + self.map.lock().remove(addr).expect("Provided address is not exposed."); + } + } +} From 085d92e3c2872c20967bbfd6e253727f5c20db2c Mon Sep 17 00:00:00 2001 From: Alice Ryhl Date: Sun, 22 Sep 2024 18:12:39 +0200 Subject: [PATCH 2/7] fmt --- tokio/src/util/ptr_expose.rs | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/tokio/src/util/ptr_expose.rs b/tokio/src/util/ptr_expose.rs index 89a55c8a358..4ea755b2f49 100644 --- a/tokio/src/util/ptr_expose.rs +++ b/tokio/src/util/ptr_expose.rs @@ -6,7 +6,7 @@ use std::marker::PhantomData; #[cfg(miri)] -use {std::collections::HashMap, crate::loom::sync::Mutex}; +use {crate::loom::sync::Mutex, std::collections::HashMap}; pub(crate) struct PtrExposeDomain { #[cfg(miri)] @@ -43,7 +43,10 @@ impl PtrExposeDomain { pub(crate) fn from_exposed_addr(&self, addr: usize) -> *const T { #[cfg(miri)] { - self.map.lock().get(&addr).expect("Provided address is not exposed.") + self.map + .lock() + .get(&addr) + .expect("Provided address is not exposed.") } #[cfg(not(miri))] @@ -58,7 +61,10 @@ impl PtrExposeDomain { { // SAFETY: Equivalent to `pointer::addr` which is safe. let addr: usize = unsafe { std::mem::transmute(_ptr) }; - self.map.lock().remove(addr).expect("Provided address is not exposed."); + self.map + .lock() + .remove(addr) + .expect("Provided address is not exposed."); } } } From 1eb49a1108dba3d025688268261983366a1628f6 Mon Sep 17 00:00:00 2001 From: Alice Ryhl Date: Sun, 22 Sep 2024 18:13:52 +0200 Subject: [PATCH 3/7] clippy --- tokio/src/util/ptr_expose.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/tokio/src/util/ptr_expose.rs b/tokio/src/util/ptr_expose.rs index 4ea755b2f49..8b14c925960 100644 --- a/tokio/src/util/ptr_expose.rs +++ b/tokio/src/util/ptr_expose.rs @@ -40,6 +40,7 @@ impl PtrExposeDomain { } #[inline] + #[allow(clippy::wrong_self_convention)] // mirrors std name pub(crate) fn from_exposed_addr(&self, addr: usize) -> *const T { #[cfg(miri)] { From f9f2695671b9d15098ae28103fb6e2a29da28d59 Mon Sep 17 00:00:00 2001 From: Alice Ryhl Date: Sun, 22 Sep 2024 18:20:48 +0200 Subject: [PATCH 4/7] misc fixes --- spellcheck.dic | 3 ++- tokio/src/runtime/io/mod.rs | 2 +- tokio/src/util/mod.rs | 7 ++++--- tokio/src/util/ptr_expose.rs | 3 ++- 4 files changed, 9 insertions(+), 6 deletions(-) diff --git a/spellcheck.dic b/spellcheck.dic index 83b9d684dcb..23c54b54ad0 100644 --- a/spellcheck.dic +++ b/spellcheck.dic @@ -1,4 +1,4 @@ -286 +287 & + < @@ -152,6 +152,7 @@ metadata mio Mio mio's +miri misconfigured mock's mpmc diff --git a/tokio/src/runtime/io/mod.rs b/tokio/src/runtime/io/mod.rs index bc748f68d36..404359bf528 100644 --- a/tokio/src/runtime/io/mod.rs +++ b/tokio/src/runtime/io/mod.rs @@ -15,5 +15,5 @@ use scheduled_io::ScheduledIo; mod metrics; use metrics::IoDriverMetrics; -use crate::util::PtrExposeDomain; +use crate::util::ptr_expose::PtrExposeDomain; static EXPOSE_IO: PtrExposeDomain = PtrExposeDomain::new(); diff --git a/tokio/src/util/mod.rs b/tokio/src/util/mod.rs index 38fe354e4c2..a93bfe8304f 100644 --- a/tokio/src/util/mod.rs +++ b/tokio/src/util/mod.rs @@ -69,9 +69,6 @@ cfg_rt! { mod rc_cell; pub(crate) use rc_cell::RcCell; - - mod ptr_expose; - pub(crate) use ptr_expose::PtrExposeDomain; } cfg_rt_multi_thread! { @@ -89,3 +86,7 @@ pub(crate) mod memchr; pub(crate) mod markers; pub(crate) mod cacheline; + +cfg_io_driver_impl! { + pub(crate) mod ptr_expose; +} diff --git a/tokio/src/util/ptr_expose.rs b/tokio/src/util/ptr_expose.rs index 8b14c925960..63eb33201d4 100644 --- a/tokio/src/util/ptr_expose.rs +++ b/tokio/src/util/ptr_expose.rs @@ -2,7 +2,7 @@ //! //! During normal execution, this module is equivalent to pointer casts. However, when running //! under miri, pointer casts are replaced with lookups in a hash map. This makes Tokio compatible -//! with strict provenance when running under miri (which comes with a perf cost). +//! with strict provenance when running under miri (which comes with a performance cost). use std::marker::PhantomData; #[cfg(miri)] @@ -27,6 +27,7 @@ impl PtrExposeDomain { pub(crate) fn expose_provenance(&self, ptr: *const T) -> usize { #[cfg(miri)] { + // FIXME: Use `pointer:addr` when it is stable. // SAFETY: Equivalent to `pointer::addr` which is safe. let addr: usize = unsafe { std::mem::transmute(ptr) }; self.map.lock().insert(addr, ptr); From 9611853d667afc5367cc34e7df5470e9b7b46457 Mon Sep 17 00:00:00 2001 From: Alice Ryhl Date: Sun, 22 Sep 2024 18:28:26 +0200 Subject: [PATCH 5/7] fixi miri compilation --- tokio/src/util/ptr_expose.rs | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/tokio/src/util/ptr_expose.rs b/tokio/src/util/ptr_expose.rs index 63eb33201d4..be06bc7f184 100644 --- a/tokio/src/util/ptr_expose.rs +++ b/tokio/src/util/ptr_expose.rs @@ -6,19 +6,22 @@ use std::marker::PhantomData; #[cfg(miri)] -use {crate::loom::sync::Mutex, std::collections::HashMap}; +use {crate::loom::sync::Mutex, std::collections::BTreeMap}; pub(crate) struct PtrExposeDomain { #[cfg(miri)] - map: Mutex>, + map: Mutex>, _phantom: PhantomData, } +// SAFETY: Actually using the pointers is unsafe, so it's sound to transfer them across threads. +unsafe impl Sync for PtrExposeDomain {} + impl PtrExposeDomain { pub(crate) const fn new() -> Self { Self { #[cfg(miri)] - map: Mutex::new(HashMap::new()), + map: Mutex::const_new(BTreeMap::new()), _phantom: PhantomData, } } @@ -45,7 +48,7 @@ impl PtrExposeDomain { pub(crate) fn from_exposed_addr(&self, addr: usize) -> *const T { #[cfg(miri)] { - self.map + *self.map .lock() .get(&addr) .expect("Provided address is not exposed.") @@ -65,7 +68,7 @@ impl PtrExposeDomain { let addr: usize = unsafe { std::mem::transmute(_ptr) }; self.map .lock() - .remove(addr) + .remove(&addr) .expect("Provided address is not exposed."); } } From 8eb8e0c2799518cf186abc89b3bd2194ab876318 Mon Sep 17 00:00:00 2001 From: Alice Ryhl Date: Sun, 22 Sep 2024 18:29:19 +0200 Subject: [PATCH 6/7] fmt --- tokio/src/util/ptr_expose.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tokio/src/util/ptr_expose.rs b/tokio/src/util/ptr_expose.rs index be06bc7f184..e3ba1f8d467 100644 --- a/tokio/src/util/ptr_expose.rs +++ b/tokio/src/util/ptr_expose.rs @@ -48,7 +48,8 @@ impl PtrExposeDomain { pub(crate) fn from_exposed_addr(&self, addr: usize) -> *const T { #[cfg(miri)] { - *self.map + *self + .map .lock() .get(&addr) .expect("Provided address is not exposed.") From 9b51a3e1dca0d00c1ad06ec6cc3e824e386a8395 Mon Sep 17 00:00:00 2001 From: Alice Ryhl Date: Sun, 22 Sep 2024 18:46:26 +0200 Subject: [PATCH 7/7] trigger UB instead of panics --- tokio/src/util/ptr_expose.rs | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/tokio/src/util/ptr_expose.rs b/tokio/src/util/ptr_expose.rs index e3ba1f8d467..c69722fd1b6 100644 --- a/tokio/src/util/ptr_expose.rs +++ b/tokio/src/util/ptr_expose.rs @@ -48,11 +48,11 @@ impl PtrExposeDomain { pub(crate) fn from_exposed_addr(&self, addr: usize) -> *const T { #[cfg(miri)] { - *self - .map - .lock() - .get(&addr) - .expect("Provided address is not exposed.") + let maybe_ptr = self.map.lock().get(&addr).copied(); + + // SAFETY: Intentionally trigger a miri failure if the provenance we want is not + // exposed. + unsafe { maybe_ptr.unwrap_unchecked() } } #[cfg(not(miri))] @@ -67,10 +67,11 @@ impl PtrExposeDomain { { // SAFETY: Equivalent to `pointer::addr` which is safe. let addr: usize = unsafe { std::mem::transmute(_ptr) }; - self.map - .lock() - .remove(&addr) - .expect("Provided address is not exposed."); + let maybe_ptr = self.map.lock().remove(&addr); + + // SAFETY: Intentionally trigger a miri failure if the provenance we want is not + // exposed. + unsafe { maybe_ptr.unwrap_unchecked() }; } } }