From 38fd3741bc71d57421d37a2a0c3f56272f17284a Mon Sep 17 00:00:00 2001 From: Arpad Borsos Date: Tue, 21 Dec 2021 14:53:17 +0100 Subject: [PATCH] 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. --- src/sentry_core.c | 27 +++++++++++++-------------- src/sentry_core.h | 4 ---- src/sentry_session.c | 16 ++++++++++------ tests/unit/test_concurrency.c | 27 +++++++++++++++++++++++++++ tests/unit/tests.inc | 1 + 5 files changed, 51 insertions(+), 24 deletions(-) diff --git a/src/sentry_core.c b/src/sentry_core.c index 6eb2e8418..ad7ea8882 100644 --- 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 index bec05ef5a..d7fc9e4e2 100644 --- 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 index f58a88e88..1aa7dc796 100644 --- 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 index e2c1446bf..83f780240 100644 --- 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 index 781361644..ef572129a 100644 --- 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)