From 595bc0e7c0b4a92e6936862a0e1a6aa0d49ca5a0 Mon Sep 17 00:00:00 2001
From: Arpad Borsos <arpad.borsos@sentry.io>
Date: Fri, 13 Jan 2023 13:32:04 +0100
Subject: [PATCH] fix: Avoid Deadlock popping ScopeGuards out of order

The `ScopeGuard` panics when it is being dropped out of order.
Previously it would do so while holding the Scope/stack `RwLock`.
The `PanicIntegration` would also try to acquire that same lock in
order to capture the resulting panic, resulting in a deadlock or a
double-panic in case the `RwLock` implementation detected the deadlock.
---
 sentry-core/src/performance.rs |  1 +
 sentry-core/src/scope/real.rs  | 21 +++++++++++++---
 sentry/tests/test_basic.rs     | 46 ++++++++++++++++++++++++++++++++++
 3 files changed, 64 insertions(+), 4 deletions(-)

diff --git a/sentry-core/src/performance.rs b/sentry-core/src/performance.rs
index 313006847..701dbf7f9 100644
--- a/sentry-core/src/performance.rs
+++ b/sentry-core/src/performance.rs
@@ -531,6 +531,7 @@ impl Transaction {
                     let sample_profile = inner.profiler_guard.take().and_then(|profiler_guard| {
                         profiling::finish_profiling(&transaction, profiler_guard, inner.context.trace_id)
                     });
+                    drop(inner);
 
                     let mut envelope = protocol::Envelope::new();
                     envelope.add_item(transaction);
diff --git a/sentry-core/src/scope/real.rs b/sentry-core/src/scope/real.rs
index eb92621e5..0b22940a7 100644
--- a/sentry-core/src/scope/real.rs
+++ b/sentry-core/src/scope/real.rs
@@ -125,11 +125,24 @@ impl fmt::Debug for ScopeGuard {
 impl Drop for ScopeGuard {
     fn drop(&mut self) {
         if let Some((stack, depth)) = self.0.take() {
-            let mut stack = stack.write().unwrap_or_else(PoisonError::into_inner);
-            if stack.depth() != depth {
-                panic!("Tried to pop guards out of order");
+            let popped_depth = {
+                let mut stack = stack.write().unwrap_or_else(PoisonError::into_inner);
+                let popped_depth = stack.depth();
+                stack.pop();
+                popped_depth
+            };
+            // NOTE: We need to drop the `stack` lock before panicing, as the
+            // `PanicIntegration` will want to lock the `stack` itself
+            // (through `capture_event` -> `HubImpl::with`), and would thus
+            // result in a deadlock.
+            // Though that deadlock itself is detected by the `RwLock` (on macOS)
+            // and results in its own panic: `rwlock read lock would result in deadlock`.
+            // However that panic happens in the panic handler and will thus
+            // ultimately result in a `thread panicked while processing panic. aborting.`
+            // Long story short, we should not panic while holding the lock :-)
+            if popped_depth != depth {
+                panic!("Popped scope guard out of order");
             }
-            stack.pop();
         }
     }
 }
diff --git a/sentry/tests/test_basic.rs b/sentry/tests/test_basic.rs
index e266b632f..3813b14ea 100644
--- a/sentry/tests/test_basic.rs
+++ b/sentry/tests/test_basic.rs
@@ -199,3 +199,49 @@ fn test_attachment_sent_from_scope() {
         && attachment.buffer == vec![1, 2, 3, 4, 5, 6, 7, 8, 9]
     ));
 }
+
+#[cfg(feature = "panic")]
+#[test]
+fn test_panic_scope_pop() {
+    let options = sentry::ClientOptions::new()
+        .add_integration(sentry::integrations::panic::PanicIntegration::new());
+
+    let events = sentry::test::with_captured_events_options(
+        || {
+            // in case someone wants to log the original panics:
+            // let next = std::panic::take_hook();
+            // std::panic::set_hook(Box::new(move |info| {
+            //     dbg!(&info);
+            //     println!("{}", std::backtrace::Backtrace::force_capture());
+            //     next(info);
+            // }));
+
+            let hub = sentry::Hub::current();
+            let scope1 = hub.push_scope();
+            let scope2 = hub.push_scope();
+
+            let panic = std::panic::catch_unwind(|| {
+                drop(scope1);
+            });
+            assert!(panic.is_err());
+
+            let panic = std::panic::catch_unwind(|| {
+                drop(scope2);
+            });
+            assert!(panic.is_err());
+        },
+        options,
+    );
+
+    assert_eq!(events.len(), 2);
+    assert_eq!(&events[0].exception[0].ty, "panic");
+    assert_eq!(
+        events[0].exception[0].value,
+        Some("Popped scope guard out of order".into())
+    );
+    assert_eq!(&events[1].exception[0].ty, "panic");
+    assert_eq!(
+        events[1].exception[0].value,
+        Some("Popped scope guard out of order".into())
+    );
+}