fix: Possible race conditions in init/close and sessions (#545)

.clang-format

@@ -4,4 +4,4 @@ IndentPPDirectives: AfterHash
 ColumnLimit: 80
 AlwaysBreakAfterDefinitionReturnType: All
 PointerAlignment: Right
-ForEachMacros: ['SENTRY_WITH_SCOPE', 'SENTRY_WITH_SCOPE_MUT', 'SENTRY_WITH_SCOPE_MUT_NO_FLUSH', 'SENTRY_WITH_OPTIONS']
+ForEachMacros: ['SENTRY_WITH_SCOPE', 'SENTRY_WITH_SCOPE_MUT', 'SENTRY_WITH_SCOPE_MUT_NO_FLUSH', 'SENTRY_WITH_OPTIONS', 'SENTRY_WITH_OPTIONS_MUT']

src/sentry_core.c

@@ -36,6 +36,19 @@ sentry__options_getref(void)
     return options;
 }
 
+sentry_options_t *
+sentry__options_lock(void)
+{
+    sentry__mutex_lock(&g_options_lock);
+    return g_options;
+}
+
+void
+sentry__options_unlock(void)
+{
+    sentry__mutex_unlock(&g_options_lock);
+}
+
 static void
 load_user_consent(sentry_options_t *opts)
 {
@@ -72,6 +85,12 @@ sentry__should_skip_upload(void)
 int
 sentry_init(sentry_options_t *options)
 {
+    // this function is to be called only once, so we do not allow more than one
+    // caller
+    sentry__mutex_lock(&g_options_lock);
+    // pre-init here, so we can consistently use bailing out to :fail
+    sentry_transport_t *transport = NULL;
+
     sentry_close();
 
     sentry_logger_t logger = { NULL, NULL };
@@ -84,11 +103,10 @@ sentry_init(sentry_options_t *options)
     if (sentry__path_create_dir_all(options->database_path)) {
         SENTRY_WARN("failed to create database directory or there is no write "
                     "access to this directory");
-        sentry_options_free(options);
-        return 1;
+        goto fail;
     }
-    sentry_transport_t *transport = options->transport;
 
+    transport = options->transport;
     sentry_path_t *database_path = options->database_path;
     options->database_path = sentry__path_absolute(database_path);
     if (options->database_path) {
@@ -139,9 +157,7 @@ sentry_init(sentry_options_t *options)
         last_crash = backend->get_last_crash_func(backend);
     }
 
-    sentry__mutex_lock(&g_options_lock);
     g_options = options;
-    sentry__mutex_unlock(&g_options_lock);
 
     // *after* setting the global options, trigger a scope and consent flush,
     // since at least crashpad needs that.
@@ -167,6 +183,7 @@ sentry_init(sentry_options_t *options)
         sentry_start_session();
     }
 
+    sentry__mutex_unlock(&g_options_lock);
     return 0;
 
 fail:
@@ -175,21 +192,21 @@ sentry_init(sentry_options_t *options)
         sentry__transport_shutdown(transport, 0);
     }
     sentry_options_free(options);
+    sentry__mutex_unlock(&g_options_lock);
     return 1;
 }
 
 int
 sentry_close(void)
 {
-    sentry_end_session();
-
+    // this function is to be called only once, so we do not allow more than one
+    // caller
     sentry__mutex_lock(&g_options_lock);
     sentry_options_t *options = g_options;
-    g_options = NULL;
-    sentry__mutex_unlock(&g_options_lock);
 
     size_t dumped_envelopes = 0;
     if (options) {
+        sentry_end_session();
         if (options->backend && options->backend->shutdown_func) {
             SENTRY_TRACE("shutting down backend");
             options->backend->shutdown_func(options->backend);
@@ -210,12 +227,17 @@ sentry_close(void)
                 || !options->backend->can_capture_after_shutdown)) {
             sentry__run_clean(options->run);
         }
-
         sentry_options_free(options);
+    } else {
+        SENTRY_DEBUG("sentry_close() called, but options was empty");
     }
 
+    g_options = NULL;
+    sentry__mutex_unlock(&g_options_lock);
+
     sentry__scope_cleanup();
     sentry_clear_modulecache();
+
     return (int)dumped_envelopes;
 }
 
@@ -343,7 +365,18 @@ sentry_capture_event(sentry_value_t event)
         was_captured = true;
         envelope = sentry__prepare_event(options, event, &event_id);
         if (envelope) {
-            sentry__add_current_session_to_envelope(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__capture_envelope(options->transport, envelope);
         }
     }
@@ -460,10 +493,18 @@ sentry__ensure_event_id(sentry_value_t event, sentry_uuid_t *uuid_out)
 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_WITH_SCOPE_MUT (scope) {
         sentry_value_decref(scope->user);
         scope->user = user;
-        sentry__scope_session_sync(scope);
     }
 }
 

src/sentry_core.h

@@ -70,8 +70,22 @@ sentry_value_t sentry__ensure_event_id(
  */
 const sentry_options_t *sentry__options_getref(void);
 
+/**
+ * This will acquire a lock on the global options.
+ */
+sentry_options_t *sentry__options_lock(void);
+
+/**
+ * Release the lock on the global options.
+ */
+void sentry__options_unlock(void);
+
 #define SENTRY_WITH_OPTIONS(Options)                                           \
     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)
+
 #endif

src/sentry_logger.h

@@ -32,4 +32,6 @@ void sentry__logger_log(sentry_level_t level, const char *message, ...);
 
 #define SENTRY_WARN(message) sentry__logger_log(SENTRY_LEVEL_WARNING, message)
 
+#define SENTRY_ERROR(message) sentry__logger_log(SENTRY_LEVEL_ERROR, message)
+
 #endif

src/sentry_options.h

@@ -4,6 +4,7 @@
 #include "sentry_boot.h"
 
 #include "sentry_logger.h"
+#include "sentry_session.h"
 #include "sentry_utils.h"
 
 // Defaults to 2s as per
@@ -57,6 +58,7 @@ typedef struct sentry_options_s {
     /* everything from here on down are options which are stored here but
        not exposed through the options API */
     struct sentry_backend_s *backend;
+    sentry_session_t *session;
 
     long user_consent;
     long refcount;

src/sentry_scope.c

@@ -73,7 +73,6 @@ get_scope(void)
     g_scope.breadcrumbs = sentry_value_new_list();
     g_scope.level = SENTRY_LEVEL_ERROR;
     g_scope.client_sdk = get_client_sdk();
-    g_scope.session = NULL;
 
     g_scope_initialized = true;
 
@@ -112,27 +111,16 @@ sentry__scope_unlock(void)
 }
 
 void
-sentry__scope_flush_unlock(const sentry_scope_t *scope)
+sentry__scope_flush_unlock()
 {
-    bool did_unlock = false;
+    sentry__scope_unlock();
     SENTRY_WITH_OPTIONS (options) {
-        if (scope->session) {
-            sentry__run_write_session(options->run, scope->session);
-            sentry__scope_unlock();
-        } else {
-            sentry__scope_unlock();
-            sentry__run_clear_session(options->run);
-        }
-        did_unlock = true;
         // we try to unlock the scope/session lock as soon as possible. The
         // backend will do its own `WITH_SCOPE` internally.
         if (options->backend && options->backend->flush_scope_func) {
             options->backend->flush_scope_func(options->backend, options);
         }
     }
-    if (!did_unlock) {
-        sentry__scope_unlock();
-    }
 }
 
 static void
@@ -304,24 +292,3 @@ sentry__scope_apply_to_event(const sentry_scope_t *scope,
 #undef IS_NULL
 #undef SET
 }
-
-void
-sentry__scope_session_sync(sentry_scope_t *scope)
-{
-    if (!scope->session) {
-        return;
-    }
-
-    if (!sentry_value_is_null(scope->user)) {
-        sentry_value_t did = sentry_value_get_by_key(scope->user, "id");
-        if (sentry_value_is_null(did)) {
-            did = sentry_value_get_by_key(scope->user, "email");
-        }
-        if (sentry_value_is_null(did)) {
-            did = sentry_value_get_by_key(scope->user, "username");
-        }
-        sentry_value_decref(scope->session->distinct_id);
-        sentry_value_incref(did);
-        scope->session->distinct_id = did;
-    }
-}

src/sentry_scope.h

@@ -19,7 +19,6 @@ typedef struct sentry_scope_s {
     sentry_value_t breadcrumbs;
     sentry_level_t level;
     sentry_value_t client_sdk;
-    sentry_session_t *session;
 } sentry_scope_t;
 
 /**
@@ -54,11 +53,11 @@ void sentry__scope_unlock(void);
 void sentry__scope_cleanup(void);
 
 /**
- * This will notify any backend of scope changes, and persist session
- * information to disk. This function must be called while holding the scope
- * lock, and it will be unlocked internally.
+ * This will notify any backend of scope changes.
+ * This function must be called while holding the scope lock, and it will be
+ * unlocked internally.
  */
-void sentry__scope_flush_unlock(const sentry_scope_t *scope);
+void sentry__scope_flush_unlock();
 
 /**
  * This will merge the requested data which is in the given `scope` to the given
@@ -70,12 +69,6 @@ void sentry__scope_apply_to_event(const sentry_scope_t *scope,
     const sentry_options_t *options, sentry_value_t event,
     sentry_scope_mode_t mode);
 
-/**
- * This will update a sessions `distinct_id`, which is generated out of other
- * scope data.
- */
-void sentry__scope_session_sync(sentry_scope_t *scope);
-
 /**
  * These are convenience macros to automatically lock/unlock a scope inside a
  * code block.
@@ -85,7 +78,7 @@ void sentry__scope_session_sync(sentry_scope_t *scope);
          sentry__scope_unlock(), Scope = NULL)
 #define SENTRY_WITH_SCOPE_MUT(Scope)                                           \
     for (sentry_scope_t *Scope = sentry__scope_lock(); Scope;                  \
-         sentry__scope_flush_unlock(Scope), Scope = NULL)
+         sentry__scope_flush_unlock(), Scope = NULL)
 #define SENTRY_WITH_SCOPE_MUT_NO_FLUSH(Scope)                                  \
     for (sentry_scope_t *Scope = sentry__scope_lock(); Scope;                  \
          sentry__scope_unlock(), Scope = NULL)