From bd47acc920f8229c7157d1e472d4289f8e16c458 Mon Sep 17 00:00:00 2001 From: Mischan Toosarani-Hausberger Date: Tue, 27 Jun 2023 10:06:35 +0200 Subject: [PATCH 1/3] fix: deadlock pattern in dynamic sdk-name assignment --- CHANGELOG.md | 6 ++++++ src/sentry_core.c | 4 +--- src/sentry_scope.c | 17 ++++++++++++----- src/sentry_scope.h | 6 ++++++ tests/unit/test_concurrency.c | 6 +++--- 5 files changed, 28 insertions(+), 11 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ebc2ccb01..df2acbfe6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,11 @@ # Changelog +## Unreleased + +**Fixes**: + +- Remove deadlock pattern in dynamic sdk-name assignment ([#857](https://github.com/getsentry/sentry-native/pull/857)) + ## 0.6.4 **Fixes**: diff --git a/src/sentry_core.c b/src/sentry_core.c index 8cbb39a03..456f8e291 100644 --- a/src/sentry_core.c +++ b/src/sentry_core.c @@ -166,9 +166,7 @@ sentry_init(sentry_options_t *options) // since at least crashpad needs that. // the only way to get a reference to the scope is by locking it, the macro // does all that at once, including invoking the backends scope flush hook - SENTRY_WITH_SCOPE_MUT (scope) { - (void)scope; - } + SENTRY_WITH_SCOPE_MUT_AND_OPTIONS(scope, options) { (void)scope; } if (backend && backend->user_consent_changed_func) { backend->user_consent_changed_func(backend); } diff --git a/src/sentry_scope.c b/src/sentry_scope.c index a98a583ba..46aa24175 100644 --- a/src/sentry_scope.c +++ b/src/sentry_scope.c @@ -25,11 +25,11 @@ static sentry_scope_t g_scope = { 0 }; static sentry_mutex_t g_lock = SENTRY__MUTEX_INIT; static sentry_value_t -get_client_sdk(void) +get_client_sdk(const sentry_options_t *options) { sentry_value_t client_sdk = sentry_value_new_object(); - SENTRY_WITH_OPTIONS (options) { + if (options) { sentry_value_t sdk_name = sentry_value_new_string(options->sdk_name); sentry_value_set_by_key(client_sdk, "name", sdk_name); } @@ -66,7 +66,7 @@ get_client_sdk(void) } static sentry_scope_t * -get_scope(void) +get_scope(const sentry_options_t *options) { if (g_scope_initialized) { return &g_scope; @@ -82,7 +82,7 @@ get_scope(void) sentry_value_set_by_key(g_scope.contexts, "os", sentry__get_os_context()); g_scope.breadcrumbs = sentry_value_new_list(); g_scope.level = SENTRY_LEVEL_ERROR; - g_scope.client_sdk = get_client_sdk(); + g_scope.client_sdk = get_client_sdk(options); g_scope.transaction_object = NULL; g_scope.span = NULL; @@ -115,7 +115,14 @@ sentry_scope_t * sentry__scope_lock(void) { sentry__mutex_lock(&g_lock); - return get_scope(); + return get_scope(NULL); +} + +sentry_scope_t * +sentry__scope_lock_with_options(const sentry_options_t *options) +{ + sentry__mutex_lock(&g_lock); + return get_scope(options); } void diff --git a/src/sentry_scope.h b/src/sentry_scope.h index 5b2ba5fe6..70c7f5131 100644 --- a/src/sentry_scope.h +++ b/src/sentry_scope.h @@ -58,6 +58,9 @@ sentry_scope_t *sentry__scope_lock(void); */ void sentry__scope_unlock(void); +sentry_scope_t *sentry__scope_lock_with_options( + const sentry_options_t *options); + /** * This will free all the data attached to the global scope */ @@ -93,6 +96,9 @@ void sentry__scope_apply_to_event(const sentry_scope_t *scope, #define SENTRY_WITH_SCOPE_MUT_NO_FLUSH(Scope) \ for (sentry_scope_t *Scope = sentry__scope_lock(); Scope; \ sentry__scope_unlock(), Scope = NULL) +#define SENTRY_WITH_SCOPE_MUT_AND_OPTIONS(Scope, Options) \ + for (sentry_scope_t *Scope = sentry__scope_lock_with_options(Options); \ + Scope; sentry__scope_flush_unlock(), Scope = NULL) #endif diff --git a/tests/unit/test_concurrency.c b/tests/unit/test_concurrency.c index 946081dba..47fc691a9 100644 --- a/tests/unit/test_concurrency.c +++ b/tests/unit/test_concurrency.c @@ -43,7 +43,7 @@ SENTRY_TEST(multiple_inits) SENTRY_LEVEL_INFO, "root", "Hello World!")); sentry_value_t obj = sentry_value_new_object(); - // something that is not a uuid, as this will be forcibly changed + // something that is not a UUID, as this will be forcibly changed sentry_value_set_by_key(obj, "event_id", sentry_value_new_int32(1234)); sentry_capture_event(obj); @@ -64,7 +64,7 @@ thread_worker(void *called) SENTRY_LEVEL_INFO, "root", "Hello World!")); sentry_value_t obj = sentry_value_new_object(); - // something that is not a uuid, as this will be forcibly changed + // something that is not a UUID, as this will be forcibly changed sentry_value_set_by_key(obj, "event_id", sentry_value_new_int32(1234)); sentry_capture_event(obj); @@ -75,7 +75,7 @@ SENTRY_TEST(concurrent_init) { long called = 0; -#define THREADS_NUM 10 +#define THREADS_NUM 100 sentry_threadid_t threads[THREADS_NUM]; for (size_t i = 0; i < THREADS_NUM; i++) { From 3b9e94b65ce95c95ac6244754a176fcb8308e235 Mon Sep 17 00:00:00 2001 From: Mischan Toosarani-Hausberger Date: Tue, 27 Jun 2023 13:21:05 +0200 Subject: [PATCH 2/3] test: add integration test for dynamic sdk-name --- examples/example.c | 4 ++++ tests/test_integration_stdout.py | 21 ++++++++++++++++++++- tests/unit/test_options.c | 2 +- 3 files changed, 25 insertions(+), 2 deletions(-) diff --git a/examples/example.c b/examples/example.c index ad9e594f9..7e1b71b0c 100644 --- a/examples/example.c +++ b/examples/example.c @@ -218,6 +218,10 @@ main(int argc, char **argv) options, discarding_on_crash_callback, NULL); } + if (has_arg(argc, argv, "override-sdk-name")) { + sentry_options_set_sdk_name(options, "sentry.native.android.flutter"); + } + sentry_init(options); if (!has_arg(argc, argv, "no-setup")) { diff --git a/tests/test_integration_stdout.py b/tests/test_integration_stdout.py index ecb2c6f95..e58dc25be 100644 --- a/tests/test_integration_stdout.py +++ b/tests/test_integration_stdout.py @@ -14,7 +14,6 @@ assert_event, assert_crash, assert_minidump, - assert_timestamp, assert_before_send, assert_no_before_send, assert_crash_timestamp, @@ -46,6 +45,26 @@ def test_capture_stdout(cmake): assert_event(envelope) +def test_dynamic_sdk_name_override(cmake): + tmp_path = cmake( + ["sentry_example"], + { + "SENTRY_BACKEND": "none", + "SENTRY_TRANSPORT": "none", + }, + ) + + output = check_output( + tmp_path, + "sentry_example", + ["stdout", "override-sdk-name", "capture-event"], + ) + envelope = Envelope.deserialize(output) + + assert_meta(envelope, sdk_override="sentry.native.android.flutter") + assert_event(envelope) + + def test_sdk_name_override(cmake): sdk_name = "cUsToM.SDK" tmp_path = cmake( diff --git a/tests/unit/test_options.c b/tests/unit/test_options.c index 529b91472..c9115a434 100644 --- a/tests/unit/test_options.c +++ b/tests/unit/test_options.c @@ -43,7 +43,7 @@ SENTRY_TEST(options_sdk_name_invalid) const char *sdk_name = NULL; const int result = sentry_options_set_sdk_name(options, sdk_name); - // then the value should should be ignored + // then the value should be ignored TEST_CHECK_INT_EQUAL(result, 1); TEST_CHECK_STRING_EQUAL( sentry_options_get_sdk_name(options), SENTRY_SDK_NAME); From 1b8333aa704756b164f9f5f7605dbada19896b0d Mon Sep 17 00:00:00 2001 From: Mischan Toosarani-Hausberger Date: Tue, 27 Jun 2023 13:23:40 +0200 Subject: [PATCH 3/3] docs: add new example parameter. --- CONTRIBUTING.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 61bc6f297..2af86e966 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -144,6 +144,7 @@ The example currently supports the following commands: - `discarding-before-send`: Installs a `before_send()` callback that discards the event. - `on-crash`: Installs an `on_crash()` callback that retains the crash event. - `discarding-on-crash`: Installs an `on_crash()` callback that discards the crash event. +- `override-sdk-name`: Changes the SDK name via the options at runtime. Only on Windows using crashpad with its WER handler module: