From e3cf7c1ac0612a9cd4dd28cfc078f152230dc333 Mon Sep 17 00:00:00 2001 From: "Daniel Szoke (via Pi Coding Agent)" Date: Thu, 7 May 2026 14:51:55 +0200 Subject: [PATCH 1/2] ref(core): Reduce unsafe access in hub switching Use RefCell for the thread-local hub slot. --- sentry-core/src/hub_impl.rs | 40 +++++++++++++++++++++++++++---------- 1 file changed, 30 insertions(+), 10 deletions(-) diff --git a/sentry-core/src/hub_impl.rs b/sentry-core/src/hub_impl.rs index 146caa87..f897cd40 100644 --- a/sentry-core/src/hub_impl.rs +++ b/sentry-core/src/hub_impl.rs @@ -1,4 +1,4 @@ -use std::cell::{Cell, UnsafeCell}; +use std::cell::{Cell, RefCell}; use std::marker::PhantomData; use std::sync::{Arc, LazyLock, MutexGuard, PoisonError, RwLock}; use std::thread; @@ -14,8 +14,8 @@ static PROCESS_HUB: LazyLock<(Arc, thread::ThreadId)> = LazyLock::new(|| { }); thread_local! { - static THREAD_HUB: (UnsafeCell>, Cell) = ( - UnsafeCell::new(Arc::new(Hub::new_from_top(&PROCESS_HUB.0))), + static THREAD_HUB: (RefCell>, Cell) = ( + RefCell::new(Arc::new(Hub::new_from_top(&PROCESS_HUB.0))), Cell::new(PROCESS_HUB.1 == thread::current().id()) ); } @@ -50,13 +50,11 @@ impl SwitchGuard { /// to the previous one. pub fn new(mut hub: Arc) -> Self { let inner = THREAD_HUB.with(|(thread_hub, is_process_hub)| { - // SAFETY: `thread_hub` will always be a valid thread local hub, - // by definition not shared between threads. - let thread_hub = unsafe { &mut *thread_hub.get() }; + let mut thread_hub = thread_hub.borrow_mut(); if std::ptr::eq(thread_hub.as_ref(), hub.as_ref()) { return None; } - std::mem::swap(thread_hub, &mut hub); + std::mem::swap(&mut *thread_hub, &mut hub); let was_process_hub = is_process_hub.replace(false); Some((hub, was_process_hub)) }); @@ -69,8 +67,8 @@ impl SwitchGuard { fn swap(&mut self) -> Option> { if let Some((mut hub, was_process_hub)) = self.inner.take() { Some(THREAD_HUB.with(|(thread_hub, is_process_hub)| { - let thread_hub = unsafe { &mut *thread_hub.get() }; - std::mem::swap(thread_hub, &mut hub); + let mut thread_hub = thread_hub.borrow_mut(); + std::mem::swap(&mut *thread_hub, &mut hub); if was_process_hub { is_process_hub.set(true); } @@ -171,7 +169,11 @@ impl Hub { if is_process_hub.get() { f(&PROCESS_HUB.0) } else { - f(unsafe { &*hub.get() }) + // Bind `hub` as `hub.borrow().clone()`. + // It is essential we drop `hub.borrow()` before the callback, otherwise we will + // panic if the callback calls `Hub::run`, so we need the binding. + let hub = hub.borrow().clone(); + f(&hub) } }) } @@ -214,3 +216,21 @@ impl Hub { .with_mut(|stack| f(Arc::make_mut(&mut stack.top_mut().scope))) } } + +#[cfg(test)] +mod tests { + use super::*; + + /// Regression test for [`Hub::with`], ensuring that the `RefCell` borrow is not held during the callback. + /// + /// If we hold the `RefCell` borrow during the callback, this would panic. + #[test] + fn hub_run_inside_with_scope() { + let outer_hub = Arc::new(Hub::new(None, Default::default())); + let inner_hub = Arc::new(Hub::new(None, Default::default())); + + Hub::run(outer_hub, || { + Hub::with(|_| Hub::run(inner_hub, || {})); + }); + } +} From abb5eb328b07fdd9905c809848aa04f7e19a8f9c Mon Sep 17 00:00:00 2001 From: Daniel Szoke Date: Thu, 7 May 2026 18:27:26 +0200 Subject: [PATCH 2/2] ref(hub): Add ProcessHub struct Add a new struct with named fields. The previous tuple made it hard to reason about the purpose of each field. --- sentry-core/src/hub_impl.rs | 25 ++++++++++++++++--------- 1 file changed, 16 insertions(+), 9 deletions(-) diff --git a/sentry-core/src/hub_impl.rs b/sentry-core/src/hub_impl.rs index f897cd40..299c4ecf 100644 --- a/sentry-core/src/hub_impl.rs +++ b/sentry-core/src/hub_impl.rs @@ -2,21 +2,20 @@ use std::cell::{Cell, RefCell}; use std::marker::PhantomData; use std::sync::{Arc, LazyLock, MutexGuard, PoisonError, RwLock}; use std::thread; +use std::thread::ThreadId; use crate::Scope; use crate::{scope::Stack, Client, Hub}; -static PROCESS_HUB: LazyLock<(Arc, thread::ThreadId)> = LazyLock::new(|| { - ( - Arc::new(Hub::new(None, Arc::new(Default::default()))), - thread::current().id(), - ) +static PROCESS_HUB: LazyLock = LazyLock::new(|| ProcessHub { + hub: Arc::new(Hub::new(None, Arc::new(Default::default()))), + thread: thread::current().id(), }); thread_local! { static THREAD_HUB: (RefCell>, Cell) = ( - RefCell::new(Arc::new(Hub::new_from_top(&PROCESS_HUB.0))), - Cell::new(PROCESS_HUB.1 == thread::current().id()) + RefCell::new(Arc::new(Hub::new_from_top(&PROCESS_HUB.hub))), + Cell::new(PROCESS_HUB.thread == thread::current().id()) ); } @@ -154,7 +153,7 @@ impl Hub { /// This is similar to [`Hub::current`] but instead of picking the /// current thread's hub it returns the main thread's hub instead. pub fn main() -> Arc { - PROCESS_HUB.0.clone() + PROCESS_HUB.hub.clone() } /// Invokes the callback with the default hub. @@ -167,7 +166,7 @@ impl Hub { { THREAD_HUB.with(|(hub, is_process_hub)| { if is_process_hub.get() { - f(&PROCESS_HUB.0) + f(&PROCESS_HUB.hub) } else { // Bind `hub` as `hub.borrow().clone()`. // It is essential we drop `hub.borrow()` before the callback, otherwise we will @@ -217,6 +216,14 @@ impl Hub { } } +/// Helper struct for storing the [`PROCESS_HUB`]. +struct ProcessHub { + /// The process's main hub. + hub: Arc, + /// The thread on which the main hub was initialized. + thread: ThreadId, +} + #[cfg(test)] mod tests { use super::*;