diff --git a/CHANGELOG.md b/CHANGELOG.md index c2621cf3d..50045878e 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/sentry-core/src/hub_impl.rs b/sentry-core/src/hub_impl.rs index 5786daa36..36994fa18 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/mod.rs similarity index 90% rename from sentry-tracing/src/layer.rs rename to sentry-tracing/src/layer/mod.rs index d02c0496d..63b5e7242 100644 --- a/sentry-tracing/src/layer.rs +++ b/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, hub: Arc, - hub_switch_guard: Option, } impl Layer for SentryLayer @@ -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::() { - 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::() { + // 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::() { + 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(); } + + // 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 = 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(SpanGuardStack::new()); } /// Records all span fields into a `BTreeMap`, reusing a mutable `String` as buffer. diff --git a/sentry-tracing/src/layer/span_guard_stack.rs b/sentry-tracing/src/layer/span_guard_stack.rs new file mode 100644 index 000000000..e18435290 --- /dev/null +++ b/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>, +} + +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 { + 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)); + } +} diff --git a/sentry-tracing/tests/span_cross_thread.rs b/sentry-tracing/tests/span_cross_thread.rs new file mode 100644 index 000000000..5eaa2cdd3 --- /dev/null +++ b/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::>() + }) + .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)); +} diff --git a/sentry-tracing/tests/span_reentrancy.rs b/sentry-tracing/tests/span_reentrancy.rs new file mode 100644 index 000000000..8fc55aad4 --- /dev/null +++ b/sentry-tracing/tests/span_reentrancy.rs @@ -0,0 +1,70 @@ +use sentry::protocol::EnvelopeItem; + +mod shared; + +/// Ensures re-entering the same span does not corrupt the current tracing state, +/// so subsequent spans are still recorded under a single transaction. +#[test] +fn reentering_span_preserves_parent() { + let transport = shared::init_sentry(1.0); + + { + // Create a span and enter it, then re-enter the same span to simulate + // common async polling behavior where a span can be entered multiple times. + let span_a = tracing::info_span!("a"); + let _enter_a = span_a.enter(); + { + let _reenter_a = span_a.enter(); + } + + // Create another span while the original span is still entered to ensure + // it is recorded on the same transaction rather than starting a new one. + let span_b = tracing::info_span!("b"); + { + let _enter_b = span_b.enter(); + } + } + + let transactions: Vec<_> = transport + .fetch_and_clear_envelopes() + .into_iter() + .flat_map(|envelope| envelope.into_items()) + .filter_map(|item| match item { + EnvelopeItem::Transaction(transaction) => Some(transaction), + _ => None, + }) + .collect(); + + assert_eq!( + transactions.len(), + 1, + "expected a single transaction when reentering a span" + ); + + let transaction = &transactions[0]; + assert_eq!(transaction.name.as_deref(), Some("a")); + + let trace = match transaction + .contexts + .get("trace") + .expect("transaction should include trace context") + { + sentry::protocol::Context::Trace(trace) => trace, + unexpected => panic!("expected trace context but got {unexpected:?}"), + }; + + let b_span = transaction + .spans + .iter() + .find(|span| span.description.as_deref() == Some("b")) + .expect("expected span \"b\" to be recorded in the transaction"); + + assert_eq!(b_span.parent_span_id, Some(trace.span_id)); + assert!( + !transaction + .spans + .iter() + .any(|span| span.description.as_deref() == Some("a")), + "expected the transaction root span not to be duplicated in span list" + ); +}