diff --git a/CHANGELOG.md b/CHANGELOG.md index e564de85..ca9d82c3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,12 @@ # Changelog +## Unreleased + +### Fixes + +- Fixed thread corruption bug where `HubSwitchGuard` could be dropped on wrong thread ([#957](https://github.com/getsentry/sentry-rust/pull/957)) + - **Breaking change**: `sentry_core::HubSwitchGuard` is now `!Send`, preventing it from being moved across threads. Code that previously sent the guard to another thread will no longer compile. + ## 0.46.1 ### Improvements diff --git a/sentry-core/src/hub_impl.rs b/sentry-core/src/hub_impl.rs index 5786daa3..36994fa1 100644 --- a/sentry-core/src/hub_impl.rs +++ b/sentry-core/src/hub_impl.rs @@ -1,5 +1,6 @@ use std::cell::{Cell, UnsafeCell}; -use std::sync::{Arc, LazyLock, PoisonError, RwLock}; +use std::marker::PhantomData; +use std::sync::{Arc, LazyLock, MutexGuard, PoisonError, RwLock}; use std::thread; use crate::Scope; @@ -19,10 +20,14 @@ thread_local! { ); } -/// A Hub switch guard used to temporarily swap -/// active hub in thread local storage. +/// A guard that temporarily swaps the active hub in thread-local storage. +/// +/// This type is `!Send` because it manages thread-local state and must be +/// dropped on the same thread where it was created. pub struct SwitchGuard { inner: Option<(Arc, bool)>, + /// Makes this type `!Send` while keeping it `Sync`. + _not_send: PhantomData>, } impl SwitchGuard { @@ -41,7 +46,10 @@ impl SwitchGuard { let was_process_hub = is_process_hub.replace(false); Some((hub, was_process_hub)) }); - SwitchGuard { inner } + SwitchGuard { + inner, + _not_send: PhantomData, + } } fn swap(&mut self) -> Option> { diff --git a/sentry-tracing/src/layer.rs b/sentry-tracing/src/layer.rs index d02c0496..72178001 100644 --- a/sentry-tracing/src/layer.rs +++ b/sentry-tracing/src/layer.rs @@ -1,6 +1,6 @@ use std::borrow::Cow; use std::cell::RefCell; -use std::collections::BTreeMap; +use std::collections::{BTreeMap, HashMap}; use std::sync::Arc; use bitflags::bitflags; @@ -236,7 +236,6 @@ pub(super) struct SentrySpanData { pub(super) sentry_span: TransactionOrSpan, parent_sentry_span: Option, hub: Arc, - hub_switch_guard: Option, } impl Layer for SentryLayer @@ -338,7 +337,6 @@ where sentry_span, parent_sentry_span, hub, - hub_switch_guard: None, }); } @@ -350,12 +348,15 @@ where None => return, }; - let mut extensions = span.extensions_mut(); - if let Some(data) = extensions.get_mut::() { - data.hub_switch_guard = Some(sentry_core::HubSwitchGuard::new(data.hub.clone())); + let extensions = span.extensions(); + if let Some(data) = extensions.get::() { + let guard = sentry_core::HubSwitchGuard::new(data.hub.clone()); + SPAN_GUARDS.with(|guards| { + guards.borrow_mut().insert(id.clone(), guard); + }); data.hub.configure_scope(|scope| { scope.set_span(Some(data.sentry_span.clone())); - }) + }); } } @@ -366,18 +367,24 @@ where None => return, }; - let mut extensions = span.extensions_mut(); - if let Some(data) = extensions.get_mut::() { + let extensions = span.extensions(); + if let Some(data) = extensions.get::() { data.hub.configure_scope(|scope| { scope.set_span(data.parent_sentry_span.clone()); }); - data.hub_switch_guard.take(); } + SPAN_GUARDS.with(|guards| { + guards.borrow_mut().remove(id); + }); } /// When a span gets closed, finish the underlying sentry span, and set back /// its parent as the *current* sentry span. fn on_close(&self, id: span::Id, ctx: Context<'_, S>) { + SPAN_GUARDS.with(|guards| { + guards.borrow_mut().remove(&id); + }); + let span = match ctx.span(&id) { Some(span) => span, None => return, @@ -503,6 +510,10 @@ fn extract_span_data( thread_local! { static VISITOR_BUFFER: RefCell = const { RefCell::new(String::new()) }; + /// Hub switch guards keyed by span ID. Stored in thread-local so guards are + /// always dropped on the same thread where they were created. + static SPAN_GUARDS: RefCell> = + RefCell::new(HashMap::new()); } /// Records all span fields into a `BTreeMap`, reusing a mutable `String` as buffer.