From efabaf845447ca9ebaa9b85e8c1285b6a74514db Mon Sep 17 00:00:00 2001 From: Daniel Szoke Date: Tue, 14 Jul 2026 10:10:51 +0200 Subject: [PATCH] fix(core): Prevent `HubSwitchGuard` panic at thread teardown When a `HubSwitchGuard` is dropped, the `THREAD_HUB` is meant to be restored to its previous value. In the past, we performed this swap using `THREAD_HUB.with`, which panics if the `THREAD_HUB` thread-local has been destroyed, which can be the case during thread destruction. Essentially, if a `HubSwitchGuard` is stored in a thread-local until the thread terminates, it can happen that the `THREAD_HUB` is destroyed before the `HubSwitchGuard` is dropped, leading to a panic when the `HubSwitchGuard` is dropped. TLS drop order is not guaranteed by Rust, but, at least on macOS platforms, this behavior reliably occurs if the thread-local storing the `HubSwitchGuard` gets initialized before the `THREAD_HUB` thread-local variable. The solution here is simple: instead of using `THREAD_HUB.with`, we use `THREAD_HUB.try_with`, which returns an error rather than panicking in the case that the `THREAD_HUB` has been destroyed. We ignore any resulting errors because, if the `THREAD_HUB` thread-local slot has been destroyed, there is nothing to swap back to the original value, and this would anyways typically occur only when the thread is shutting down. We also add two regression tests, at least one of which should fail if the bug is reintroduced. As the TLS drop order is not guaranteed by the language, we test both initializing the guard slot before the `THREAD_HUB` (this reliably fails on macOS when the bug is introduced), and we also test initializing `THREAD_HUB` before the `HubSwitchGuard` slot (this reliably passes on macOS, even without the fix, but may fail on other platforms depending on implementation). Fixes [#1237](https://github.com/getsentry/sentry-rust/issues/1237) Fixes [RUST-264](https://linear.app/getsentry/issue/RUST-264) --- CHANGELOG.md | 6 ++ sentry-core/src/hub_impl.rs | 19 ++++-- .../tests/switch_guard_tls_teardown.rs | 68 +++++++++++++++++++ 3 files changed, 87 insertions(+), 6 deletions(-) create mode 100644 sentry-core/tests/switch_guard_tls_teardown.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index ada5ba4f4..455bd3a8b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,11 @@ # Changelog +## Unreleased + +### Fixes + +- Fixed a bug that could cause the SDK to panic and, in some cases, panic while handling the panic, aborting the process ([#1241](https://github.com/getsentry/sentry-rust/pull/1241)). + ## 0.48.4 ### New Features diff --git a/sentry-core/src/hub_impl.rs b/sentry-core/src/hub_impl.rs index aa1dce975..e08b009e2 100644 --- a/sentry-core/src/hub_impl.rs +++ b/sentry-core/src/hub_impl.rs @@ -70,12 +70,19 @@ impl SwitchGuard { } fn swap(&mut self) -> Option> { - self.inner.take().map(|mut hub| { - THREAD_HUB.with(|thread_hub| { - let mut thread_hub = thread_hub.borrow_mut(); - std::mem::swap(&mut *thread_hub, &mut hub); - hub - }) + self.inner.take().and_then(|mut hub| { + // We use `try_with` to access the `THREAD_HUB`, intentionally ignoring any errors that + // result. If `try_with` errors, this is because the `THREAD_HUB` local key has been + // destroyed, which means that there is nothing to swap with, making this operation + // pointless. Further, the destruction of the thread-local indicates that the thread + // is likely shutting down. + THREAD_HUB + .try_with(|thread_hub| { + let mut thread_hub = thread_hub.borrow_mut(); + std::mem::swap(&mut *thread_hub, &mut hub); + hub + }) + .ok() }) } } diff --git a/sentry-core/tests/switch_guard_tls_teardown.rs b/sentry-core/tests/switch_guard_tls_teardown.rs new file mode 100644 index 000000000..d5b4968df --- /dev/null +++ b/sentry-core/tests/switch_guard_tls_teardown.rs @@ -0,0 +1,68 @@ +//! Regression tests for [issue #1237]. +//! +//! The tests cover both possible destruction orders for the thread-local +//! storage containing `HubSwitchGuard` values and `THREAD_HUB`. TLS destruction +//! order is not guaranteed, so testing both orders increases the likelihood +//! that a reintroduced bug is detected on a given platform. +//! +//! [issue #1237]: https://github.com/getsentry/sentry-rust/issues/1237 + +use std::cell::RefCell; +use std::sync::Arc; +use std::thread; + +use sentry::{Hub, HubSwitchGuard}; + +/// Initializes the guard TLS before `THREAD_HUB`, reliably reproducing the +/// reported failure on macOS targets. The test passes once the bug is fixed. +#[test] +fn switch_guard_tolerates_destroyed_thread_hub() { + thread_local! { + static STORED_GUARD: RefCell> = + const { RefCell::new(None) }; + } + + thread::spawn(|| { + // Initialize this TLS value before THREAD_HUB. + STORED_GUARD.with(|_| {}); + + // Hub::current initializes THREAD_HUB. + let current = Hub::current(); + let replacement = Arc::new(Hub::new_from_top(current)); + let guard = HubSwitchGuard::new(replacement); + + // Keep the guard alive until TLS teardown. + STORED_GUARD.with_borrow_mut(|slot| { + *slot = Some(guard); + }); + }) + .join() + .expect("worker should exit without panicking"); +} + +/// Initializes the guard TLS after `THREAD_HUB` and covers the other possible +/// TLS destruction order. TLS destruction order is not guaranteed, so this test +/// helps detect a reintroduced bug on platforms where the first order does not +/// reliably occur. +#[test] +fn switch_guard_drops_before_thread_hub() { + thread_local! { + static STORED_GUARD: RefCell> = + const { RefCell::new(None) }; + } + + thread::spawn(|| { + // Hub::current initializes THREAD_HUB before STORED_GUARD. + let current = Hub::current(); + let replacement = Arc::new(Hub::new_from_top(current)); + let guard = HubSwitchGuard::new(replacement); + + // Keep the guard alive until TLS teardown, but initialize its TLS + // after THREAD_HUB. + STORED_GUARD.with_borrow_mut(|slot| { + *slot = Some(guard); + }); + }) + .join() + .expect("worker should exit without panicking"); +}