fix: Avoid deadlocks with uninitialized options (#639)

The `SENTRY_WITH_OPTIONS_MUT` was a footgun since it never unlocked when the options were NULL (uninitialized). This removes the macro and replaces its uses with explicit lock/unlock calls.
getsentry · Dec 21, 2021 · 38fd374 · 38fd374
1 parent 737a654
commit 38fd374
Show file tree

Hide file tree

Showing 5 changed files with 51 additions and 24 deletions.
diff --git a/src/sentry_core.c b/src/sentry_core.c
@@ -386,15 +386,14 @@ sentry__capture_event(sentry_value_t event)
         }
         if (envelope) {
             if (options->session) {
-                SENTRY_WITH_OPTIONS_MUT (mut_options) {
-                    sentry__envelope_add_session(
-                        envelope, mut_options->session);
-                    // we're assuming that if a session is added to an envelope
-                    // it will be sent onwards.  This means we now need to set
-                    // the init flag to false because we're no longer the
-                    // initial session update.
-                    mut_options->session->init = false;
-                }
+                sentry_options_t *mut_options = sentry__options_lock();
+                sentry__envelope_add_session(envelope, mut_options->session);
+                // we're assuming that if a session is added to an envelope
+                // it will be sent onwards.  This means we now need to set
+                // the init flag to false because we're no longer the
+                // initial session update.
+                mut_options->session->init = false;
+                sentry__options_unlock();
             }
 
             sentry__capture_envelope(options->transport, envelope);
@@ -549,12 +548,12 @@ void
 sentry_set_user(sentry_value_t user)
 {
     if (!sentry_value_is_null(user)) {
-        SENTRY_WITH_OPTIONS_MUT (options) {
-            if (options->session) {
-                sentry__session_sync_user(options->session, user);
-                sentry__run_write_session(options->run, options->session);
-            }
+        sentry_options_t *options = sentry__options_lock();
+        if (options && options->session) {
+            sentry__session_sync_user(options->session, user);
+            sentry__run_write_session(options->run, options->session);
         }
+        sentry__options_unlock();
     }
 
     SENTRY_WITH_SCOPE_MUT (scope) {

diff --git a/src/sentry_core.h b/src/sentry_core.h
@@ -95,10 +95,6 @@ void sentry__options_unlock(void);
     for (const sentry_options_t *Options = sentry__options_getref(); Options;  \
          sentry_options_free((sentry_options_t *)Options), Options = NULL)
 
-#define SENTRY_WITH_OPTIONS_MUT(Options)                                       \
-    for (sentry_options_t *Options = sentry__options_lock(); Options;          \
-         sentry__options_unlock(), Options = NULL)
-
 // these for now are only needed for tests
 #ifdef SENTRY_UNITTEST
 bool sentry__roll_dice(double probability);

diff --git a/src/sentry_session.c b/src/sentry_session.c
@@ -215,35 +215,39 @@ sentry_start_session(void)
 {
     sentry_end_session();
     SENTRY_WITH_SCOPE (scope) {
-        SENTRY_WITH_OPTIONS_MUT (options) {
+        sentry_options_t *options = sentry__options_lock();
+        if (options) {
             options->session = sentry__session_new();
             if (options->session) {
                 sentry__session_sync_user(options->session, scope->user);
                 sentry__run_write_session(options->run, options->session);
             }
         }
+        sentry__options_unlock();
     }
 }
 
 void
 sentry__record_errors_on_current_session(uint32_t error_count)
 {
-    SENTRY_WITH_OPTIONS_MUT (options) {
-        if (options->session) {
-            options->session->errors += error_count;
-        }
+    sentry_options_t *options = sentry__options_lock();
+    if (options && options->session) {
+        options->session->errors += error_count;
     }
+    sentry__options_unlock();
 }
 
 static sentry_session_t *
 sentry__end_session_internal(void)
 {
     sentry_session_t *session = NULL;
-    SENTRY_WITH_OPTIONS_MUT (options) {
+    sentry_options_t *options = sentry__options_lock();
+    if (options) {
         session = options->session;
         options->session = NULL;
         sentry__run_clear_session(options->run);
     }
+    sentry__options_unlock();
 
     if (session && session->status == SENTRY_SESSION_STATUS_OK) {
         session->status = SENTRY_SESSION_STATUS_EXITED;

diff --git a/tests/unit/test_concurrency.c b/tests/unit/test_concurrency.c
@@ -95,3 +95,30 @@ SENTRY_TEST(concurrent_init)
     TEST_CHECK(called >= THREADS_NUM * 1);
     TEST_CHECK(called <= THREADS_NUM * 3);
 }
+
+SENTRY_THREAD_FN
+thread_breadcrumb(void *UNUSED(arg))
+{
+    sentry_value_t breadcrumb = sentry_value_new_breadcrumb("foo", "bar");
+    sentry_add_breadcrumb(breadcrumb);
+
+    return 0;
+}
+
+SENTRY_TEST(concurrent_uninit)
+{
+    sentry_value_t user = sentry_value_new_object();
+    sentry_set_user(user);
+
+    sentry_threadid_t thread;
+    sentry__thread_init(&thread);
+    sentry__thread_spawn(&thread, &thread_breadcrumb, NULL);
+
+    sentry_value_t breadcrumb = sentry_value_new_breadcrumb("foo", "bar");
+    sentry_add_breadcrumb(breadcrumb);
+
+    sentry__thread_join(thread);
+    sentry__thread_free(&thread);
+
+    sentry_close();
+}
diff --git a/tests/unit/tests.inc b/tests/unit/tests.inc
@@ -8,6 +8,7 @@ XX(basic_tracing_context)
 XX(basic_transaction)
 XX(buildid_fallback)
 XX(concurrent_init)
+XX(concurrent_uninit)
 XX(count_sampled_events)
 XX(custom_logger)
 XX(dsn_parsing_complete)