fix!(core): Make HubSwitchGuard !Send to prevent thread corruption

CHANGELOG.md

@@ -6,6 +6,11 @@
 
 - Added a `Envelope::into_items` method, which returns an iterator over owned [`EnvelopeItem`s](https://docs.rs/sentry/0.46.2/sentry/protocol/enum.EnvelopeItem.html) in the [`Envelope`](https://docs.rs/sentry/0.46.2/sentry/struct.Envelope.html) ([#983](https://github.com/getsentry/sentry-rust/pull/983)).
 
+### 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.2
 
 ### New Features

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<Hub>, bool)>,
+    /// Makes this type `!Send` while keeping it `Sync`.
+    _not_send: PhantomData<MutexGuard<'static, ()>>,
 }
 
 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<Arc<Hub>> {

sentry-tracing/src/layer.rs → sentry-tracing/src/layer/mod.rs

@@ -5,7 +5,7 @@ use std::sync::Arc;
 
 use bitflags::bitflags;
 use sentry_core::protocol::Value;
-use sentry_core::{Breadcrumb, TransactionOrSpan};
+use sentry_core::{Breadcrumb, Hub, HubSwitchGuard, TransactionOrSpan};
 use tracing_core::field::Visit;
 use tracing_core::{span, Event, Field, Level, Metadata, Subscriber};
 use tracing_subscriber::layer::{Context, Layer};
@@ -16,6 +16,9 @@ use crate::SENTRY_NAME_FIELD;
 use crate::SENTRY_OP_FIELD;
 use crate::SENTRY_TRACE_FIELD;
 use crate::TAGS_PREFIX;
+use span_guard_stack::SpanGuardStack;
+
+mod span_guard_stack;
 
 bitflags! {
     /// The action that Sentry should perform for a given [`Event`]
@@ -236,7 +239,6 @@ pub(super) struct SentrySpanData {
     pub(super) sentry_span: TransactionOrSpan,
     parent_sentry_span: Option<TransactionOrSpan>,
     hub: Arc<sentry_core::Hub>,
-    hub_switch_guard: Option<sentry_core::HubSwitchGuard>,
 }
 
 impl<S> Layer<S> for SentryLayer<S>
@@ -338,7 +340,6 @@ where
             sentry_span,
             parent_sentry_span,
             hub,
-            hub_switch_guard: None,
         });
     }
 
@@ -350,12 +351,27 @@ where
             None => return,
         };
 
-        let mut extensions = span.extensions_mut();
-        if let Some(data) = extensions.get_mut::<SentrySpanData>() {
-            data.hub_switch_guard = Some(sentry_core::HubSwitchGuard::new(data.hub.clone()));
-            data.hub.configure_scope(|scope| {
+        let extensions = span.extensions();
+        if let Some(data) = extensions.get::<SentrySpanData>() {
+            // We fork the hub (based on the hub associated with the span)
+            // upon entering the span. This prevents data leakage if the span
+            // is entered and exited multiple times.
+            //
+            // Further, Hubs are meant to manage thread-local state, even
+            // though they can be shared across threads. As the span may being
+            // entered on a different thread than where it was created, we need
+            // to use a new hub to avoid altering state on the original thread.
+            let hub = Arc::new(Hub::new_from_top(&data.hub));
+
+            hub.configure_scope(|scope| {
                 scope.set_span(Some(data.sentry_span.clone()));
-            })
+            });
+
+            let guard = HubSwitchGuard::new(hub);
+
+            SPAN_GUARDS.with(|guards| {
+                guards.borrow_mut().push(id.clone(), guard);
+            });
         }
     }
 
@@ -366,18 +382,29 @@ where
             None => return,
         };
 
-        let mut extensions = span.extensions_mut();
-        if let Some(data) = extensions.get_mut::<SentrySpanData>() {
+        let extensions = span.extensions();
+        if let Some(data) = extensions.get::<SentrySpanData>() {
             data.hub.configure_scope(|scope| {
                 scope.set_span(data.parent_sentry_span.clone());
             });
-            data.hub_switch_guard.take();
         }
+
+        // Drop the guard to switch back to previous hub
+        SPAN_GUARDS.with(|guards| guards.borrow_mut().pop(id.clone()));
     }
 
     /// 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>) {
+        // Ensure all remaining Hub guards are dropped, to restore the original
+        // Hub.
+        //
+        // By this point, the span probably should be fully executed, but we should
+        // still ensure the guard is dropped in case this expectation is violated.
+        SPAN_GUARDS.with(|guards| {
+            guards.borrow_mut().remove(&id);
+        });
+
         let span = match ctx.span(&id) {
             Some(span) => span,
             None => return,
@@ -503,6 +530,9 @@ fn extract_span_data(
 
 thread_local! {
     static VISITOR_BUFFER: RefCell<String> = 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<SpanGuardStack> = RefCell::new(SpanGuardStack::new());
 }
 
 /// Records all span fields into a `BTreeMap`, reusing a mutable `String` as buffer.

sentry-tracing/src/layer/span_guard_stack.rs

@@ -0,0 +1,118 @@
+//! This module contains code for stack-like storage for `HubSwitchGuard`s keyed
+//! by tracing span ID.
+
+use std::collections::hash_map::Entry;
+use std::collections::HashMap;
+
+use sentry_core::HubSwitchGuard;
+use tracing_core::span::Id as SpanId;
+
+/// Holds per-span stacks of `HubSwitchGuard`s to handle span re-entrancy.
+///
+/// Each time a span is entered, we should push a new guard onto the stack.
+/// When the span exits, we should pop the guard from the stack.
+pub(super) struct SpanGuardStack {
+    /// The map of span IDs to their respective guard stacks.
+    guards: HashMap<SpanId, Vec<HubSwitchGuard>>,
+}
+
+impl SpanGuardStack {
+    /// Creates an empty guard stack map.
+    pub(super) fn new() -> Self {
+        Self {
+            guards: HashMap::new(),
+        }
+    }
+
+    /// Pushes a guard for the given span ID, creating the stack if needed.
+    pub(super) fn push(&mut self, id: SpanId, guard: HubSwitchGuard) {
+        self.guards.entry(id).or_default().push(guard);
+    }
+
+    /// Pops the most recent guard for the span ID, removing the stack when empty.
+    pub(super) fn pop(&mut self, id: SpanId) -> Option<HubSwitchGuard> {
+        match self.guards.entry(id) {
+            Entry::Occupied(mut entry) => {
+                let stack = entry.get_mut();
+                let guard = stack.pop();
+                if stack.is_empty() {
+                    entry.remove();
+                }
+                guard
+            }
+            Entry::Vacant(_) => None,
+        }
+    }
+
+    /// Removes all guards for the span ID without returning them.
+    ///
+    /// This function guarantees that the guards are dropped in LIFO order.
+    /// That way, the hub which was active when the span was first entered
+    /// will be the one active after this function returns.
+    ///
+    /// Typically, remove should only get called once the span is fully
+    /// exited, so this removal order guarantee is mostly just defensive.
+    pub(super) fn remove(&mut self, id: &SpanId) {
+        self.guards
+            .remove(id)
+            .into_iter()
+            .flatten()
+            .rev() // <- we drop in reverse order
+            .for_each(drop);
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    use super::SpanGuardStack;
+    use sentry_core::{Hub, HubSwitchGuard};
+    use std::sync::Arc;
+    use tracing_core::span::Id as SpanId;
+
+    #[test]
+    fn pop_is_lifo() {
+        let initial = Hub::current();
+        let hub_a = Arc::new(Hub::new_from_top(initial.clone()));
+        let hub_b = Arc::new(Hub::new_from_top(hub_a.clone()));
+
+        let mut stack = SpanGuardStack::new();
+        let id = SpanId::from_u64(1);
+
+        stack.push(id.clone(), HubSwitchGuard::new(hub_a.clone()));
+        assert!(Arc::ptr_eq(&Hub::current(), &hub_a));
+
+        stack.push(id.clone(), HubSwitchGuard::new(hub_b.clone()));
+        assert!(Arc::ptr_eq(&Hub::current(), &hub_b));
+
+        drop(stack.pop(id.clone()).expect("guard for hub_b"));
+        assert!(Arc::ptr_eq(&Hub::current(), &hub_a));
+
+        drop(stack.pop(id.clone()).expect("guard for hub_a"));
+        assert!(Arc::ptr_eq(&Hub::current(), &initial));
+
+        assert!(stack.pop(id).is_none());
+    }
+
+    #[test]
+    fn remove_drops_all_guards_in_lifo_order() {
+        let initial = Hub::current();
+        let hub_a = Arc::new(Hub::new_from_top(initial.clone()));
+        let hub_b = Arc::new(Hub::new_from_top(hub_a.clone()));
+
+        assert!(!Arc::ptr_eq(&hub_b, &initial));
+        assert!(!Arc::ptr_eq(&hub_a, &initial));
+        assert!(!Arc::ptr_eq(&hub_a, &hub_b));
+
+        let mut stack = SpanGuardStack::new();
+        let id = SpanId::from_u64(2);
+
+        stack.push(id.clone(), HubSwitchGuard::new(hub_a.clone()));
+        assert!(Arc::ptr_eq(&Hub::current(), &hub_a));
+
+        stack.push(id.clone(), HubSwitchGuard::new(hub_b.clone()));
+        assert!(Arc::ptr_eq(&Hub::current(), &hub_b));
+
+        stack.remove(&id);
+        assert!(Arc::ptr_eq(&Hub::current(), &initial));
+    }
+}

sentry-tracing/tests/span_cross_thread.rs

@@ -0,0 +1,77 @@
+mod shared;
+
+use sentry::protocol::Context;
+use std::thread;
+use std::time::Duration;
+
+#[test]
+fn cross_thread_span_entries_share_transaction() {
+    let transport = shared::init_sentry(1.0);
+
+    let span = tracing::info_span!("foo");
+    let span2 = span.clone();
+
+    let handle1 = thread::spawn(move || {
+        let _guard = span.enter();
+        let _bar_span = tracing::info_span!("bar").entered();
+        thread::sleep(Duration::from_millis(100));
+    });
+
+    let handle2 = thread::spawn(move || {
+        thread::sleep(Duration::from_millis(10));
+        let _guard = span2.enter();
+        let _baz_span = tracing::info_span!("baz").entered();
+        thread::sleep(Duration::from_millis(50));
+    });
+
+    handle1.join().unwrap();
+    handle2.join().unwrap();
+
+    let data = transport.fetch_and_clear_envelopes();
+    let transactions: Vec<_> = data
+        .into_iter()
+        .flat_map(|envelope| {
+            envelope
+                .items()
+                .filter_map(|item| match item {
+                    sentry::protocol::EnvelopeItem::Transaction(transaction) => {
+                        Some(transaction.clone())
+                    }
+                    _ => None,
+                })
+                .collect::<Vec<_>>()
+        })
+        .collect();
+
+    assert_eq!(
+        transactions.len(),
+        1,
+        "expected a single transaction for cross-thread span entries"
+    );
+
+    let transaction = &transactions[0];
+    assert_eq!(transaction.name.as_deref(), Some("foo"));
+
+    let trace = match transaction
+        .contexts
+        .get("trace")
+        .expect("transaction should include trace context")
+    {
+        Context::Trace(trace) => trace,
+        unexpected => panic!("expected trace context but got {unexpected:?}"),
+    };
+
+    let bar_span = transaction
+        .spans
+        .iter()
+        .find(|span| span.description.as_deref() == Some("bar"))
+        .expect("expected span \"bar\" to be recorded in the transaction");
+    let baz_span = transaction
+        .spans
+        .iter()
+        .find(|span| span.description.as_deref() == Some("baz"))
+        .expect("expected span \"baz\" to be recorded in the transaction");
+
+    assert_eq!(bar_span.parent_span_id, Some(trace.span_id));
+    assert_eq!(baz_span.parent_span_id, Some(trace.span_id));
+}