fix: Be more defensive around transactions

CHANGELOG.md

@@ -9,32 +9,21 @@
 **Internal**:
 
 - Updated Breakpad and Crashpad backends to 2022-09-14. ([#735](https://github.com/getsentry/sentry-native/pull/735))
+- Be more defensive around transactions ([#757](https://github.com/getsentry/sentry-native/pull/757))
 
 ## 0.5.0
 
 **Features**
 
 - Provide `on_crash()` callback to allow clients to act on detected crashes.
-  Users often inquired about distinguishing between crashes and "normal" events in the `before_send()` hook.
-  `on_crash()` can be considered a replacement for `before_send()` for crash events, where the goal is to use
-  `before_send()` only for normal events, while `on_crash()` is only invoked for crashes. This change is backward
-  compatible for current users of `before_send()` and allows gradual migration to `on_crash()`
-  ([see the docs for details](https://docs.sentry.io/platforms/native/configuration/filtering/)).
-  ([#724](https://github.com/getsentry/sentry-native/pull/724),
-  [#734](https://github.com/getsentry/sentry-native/pull/734))
+  Users often inquired about distinguishing between crashes and "normal" events in the `before_send()` hook. `on_crash()` can be considered a replacement for `before_send()` for crash events, where the goal is to use `before_send()` only for normal events, while `on_crash()` is only invoked for crashes. This change is backward compatible for current users of `before_send()` and allows gradual migration to `on_crash()` ([see the docs for details](https://docs.sentry.io/platforms/native/configuration/filtering/)). ([#724](https://github.com/getsentry/sentry-native/pull/724), [#734](https://github.com/getsentry/sentry-native/pull/734))
 
 **Fixes**
 
-- Make Windows ModuleFinder more resilient to missing Debug Info
-  ([#732](https://github.com/getsentry/sentry-native/pull/732))
-- Aligned pre-send event processing in `sentry_capture_event()` with the
-  [cross-SDK session filter order](https://develop.sentry.dev/sdk/sessions/#filter-order)
-  ([#729](https://github.com/getsentry/sentry-native/pull/729))
-- Align the default value initialization for the `environment` payload attribute with the
-  [developer documentation](https://develop.sentry.dev/sdk/event-payloads/#optional-attribute)
-  ([#739](https://github.com/getsentry/sentry-native/pull/739))
-- Iterate all debug directory entries when parsing PE modules for a valid CodeView record
-  ([#740](https://github.com/getsentry/sentry-native/pull/740))
+- Make Windows ModuleFinder more resilient to missing Debug Info ([#732](https://github.com/getsentry/sentry-native/pull/732))
+- Aligned pre-send event processing in `sentry_capture_event()` with the [cross-SDK session filter order](https://develop.sentry.dev/sdk/sessions/#filter-order) ([#729](https://github.com/getsentry/sentry-native/pull/729))
+- Align the default value initialization for the `environment` payload attribute with the [developer documentation](https://develop.sentry.dev/sdk/event-payloads/#optional-attribute) ([#739](https://github.com/getsentry/sentry-native/pull/739))
+- Iterate all debug directory entries when parsing PE modules for a valid CodeView record ([#740](https://github.com/getsentry/sentry-native/pull/740))
 
 **Thank you**:
 

src/sentry_core.c

@@ -737,6 +737,10 @@ sentry_transaction_start(
     // traces_sampler.
     sentry_value_decref(sampling_ctx);
 
+    if (!opaque_tx_cxt) {
+        return NULL;
+    }
+
     sentry_value_t tx_cxt = opaque_tx_cxt->inner;
 
     // If the parent span ID is some empty-ish value, just remove it
@@ -1001,11 +1005,11 @@ sentry_span_finish(sentry_span_t *opaque_span)
         sentry_value_set_by_key(root_transaction, "spans", spans);
     }
     sentry_value_append(spans, span);
-    sentry__span_free(opaque_span);
+    sentry__span_decref(opaque_span);
     return;
 
 fail:
-    sentry__span_free(opaque_span);
+    sentry__span_decref(opaque_span);
     return;
 }
 

src/sentry_tracing.c

@@ -55,17 +55,13 @@ sentry_transaction_context_new(const char *name, const char *operation)
     if (!tx_cxt) {
         return NULL;
     }
-    memset(tx_cxt, 0, sizeof(sentry_transaction_context_t));
+    tx_cxt->inner = sentry__value_transaction_context_new(name, operation);
 
-    sentry_value_t inner
-        = sentry__value_transaction_context_new(name, operation);
-
-    if (sentry_value_is_null(inner)) {
+    if (sentry_value_is_null(tx_cxt->inner)) {
+        sentry_free(tx_cxt);
         return NULL;
     }
 
-    tx_cxt->inner = inner;
-
     return tx_cxt;
 }
 
@@ -87,36 +83,48 @@ void
 sentry_transaction_context_set_name(
     sentry_transaction_context_t *tx_cxt, const char *name)
 {
-    sentry_value_set_by_key(
-        tx_cxt->inner, "transaction", sentry_value_new_string(name));
+    if (tx_cxt) {
+        sentry_value_set_by_key(
+            tx_cxt->inner, "transaction", sentry_value_new_string(name));
+    }
 }
 
 void
 sentry_transaction_context_set_operation(
     sentry_transaction_context_t *tx_cxt, const char *operation)
 {
-    sentry_value_set_by_key(
-        tx_cxt->inner, "op", sentry_value_new_string(operation));
+    if (tx_cxt) {
+        sentry_value_set_by_key(
+            tx_cxt->inner, "op", sentry_value_new_string(operation));
+    }
 }
 
 void
 sentry_transaction_context_set_sampled(
     sentry_transaction_context_t *tx_cxt, int sampled)
 {
-    sentry_value_set_by_key(
-        tx_cxt->inner, "sampled", sentry_value_new_bool(sampled));
+    if (tx_cxt) {
+        sentry_value_set_by_key(
+            tx_cxt->inner, "sampled", sentry_value_new_bool(sampled));
+    }
 }
 
 void
 sentry_transaction_context_remove_sampled(sentry_transaction_context_t *tx_cxt)
 {
-    sentry_value_remove_by_key(tx_cxt->inner, "sampled");
+    if (tx_cxt) {
+        sentry_value_remove_by_key(tx_cxt->inner, "sampled");
+    }
 }
 
 void
 sentry_transaction_context_update_from_header(
     sentry_transaction_context_t *tx_cxt, const char *key, const char *value)
 {
+    if (!tx_cxt) {
+        return;
+    }
+
     // do case-insensitive header key comparison
     const char sentry_trace[] = "sentry-trace";
     for (size_t i = 0; i < sizeof(sentry_trace); i++) {
@@ -169,7 +177,6 @@ sentry__transaction_new(sentry_value_t inner)
     if (!tx) {
         return NULL;
     }
-    memset(tx, 0, sizeof(sentry_transaction_t));
 
     tx->inner = inner;
 
@@ -179,7 +186,9 @@ sentry__transaction_new(sentry_value_t inner)
 void
 sentry__transaction_incref(sentry_transaction_t *tx)
 {
-    sentry_value_incref(tx->inner);
+    if (tx) {
+        sentry_value_incref(tx->inner);
+    }
 }
 
 void
@@ -200,7 +209,9 @@ sentry__transaction_decref(sentry_transaction_t *tx)
 void
 sentry__span_incref(sentry_span_t *span)
 {
-    sentry_value_incref(span->inner);
+    if (span) {
+        sentry_value_incref(span->inner);
+    }
 }
 
 void
@@ -212,6 +223,7 @@ sentry__span_decref(sentry_span_t *span)
 
     if (sentry_value_refcount(span->inner) <= 1) {
         sentry_value_decref(span->inner);
+        sentry__transaction_decref(span->transaction);
         sentry_free(span);
     } else {
         sentry_value_decref(span->inner);
@@ -229,7 +241,6 @@ sentry__span_new(sentry_transaction_t *tx, sentry_value_t inner)
     if (!span) {
         return NULL;
     }
-    memset(span, 0, sizeof(sentry_span_t));
 
     span->inner = inner;
 
@@ -270,20 +281,6 @@ sentry__value_span_new(
     return sentry_value_new_null();
 }
 
-// TODO: for now, don't allow multiple references to spans. this should be
-// revisited when sentry_transaction_t stores a list of sentry_span_t's
-// instead of a list of sentry_value_t's.
-void
-sentry__span_free(sentry_span_t *span)
-{
-    if (!span) {
-        return;
-    }
-    sentry_value_decref(span->inner);
-    sentry__transaction_decref(span->transaction);
-    sentry_free(span);
-}
-
 sentry_value_t
 sentry__value_get_trace_context(sentry_value_t span)
 {
@@ -323,8 +320,10 @@ sentry__value_get_trace_context(sentry_value_t span)
 void
 sentry_transaction_set_name(sentry_transaction_t *tx, const char *name)
 {
-    sentry_value_set_by_key(
-        tx->inner, "transaction", sentry_value_new_string(name));
+    if (tx) {
+        sentry_value_set_by_key(
+            tx->inner, "transaction", sentry_value_new_string(name));
+    }
 }
 
 static void
@@ -348,13 +347,17 @@ void
 sentry_transaction_set_tag(
     sentry_transaction_t *tx, const char *tag, const char *value)
 {
-    set_tag(tx->inner, tag, value);
+    if (tx) {
+        set_tag(tx->inner, tag, value);
+    }
 }
 
 void
 sentry_span_set_tag(sentry_span_t *span, const char *tag, const char *value)
 {
-    set_tag(span->inner, tag, value);
+    if (span) {
+        set_tag(span->inner, tag, value);
+    }
 }
 
 static void
@@ -369,13 +372,17 @@ remove_tag(sentry_value_t item, const char *tag)
 void
 sentry_transaction_remove_tag(sentry_transaction_t *tx, const char *tag)
 {
-    remove_tag(tx->inner, tag);
+    if (tx) {
+        remove_tag(tx->inner, tag);
+    }
 }
 
 void
 sentry_span_remove_tag(sentry_span_t *span, const char *tag)
 {
-    remove_tag(span->inner, tag);
+    if (span) {
+        remove_tag(span->inner, tag);
+    }
 }
 
 static void
@@ -393,13 +400,17 @@ void
 sentry_transaction_set_data(
     sentry_transaction_t *tx, const char *key, sentry_value_t value)
 {
-    set_data(tx->inner, key, value);
+    if (tx) {
+        set_data(tx->inner, key, value);
+    }
 }
 
 void
 sentry_span_set_data(sentry_span_t *span, const char *key, sentry_value_t value)
 {
-    set_data(span->inner, key, value);
+    if (span) {
+        set_data(span->inner, key, value);
+    }
 }
 
 static void
@@ -414,13 +425,17 @@ remove_data(sentry_value_t item, const char *key)
 void
 sentry_transaction_remove_data(sentry_transaction_t *tx, const char *key)
 {
-    remove_data(tx->inner, key);
+    if (tx) {
+        remove_data(tx->inner, key);
+    }
 }
 
 void
 sentry_span_remove_data(sentry_span_t *span, const char *key)
 {
-    remove_data(span->inner, key);
+    if (span) {
+        remove_data(span->inner, key);
+    }
 }
 
 sentry_value_t
@@ -475,14 +490,18 @@ set_status(sentry_value_t item, sentry_span_status_t status)
 void
 sentry_span_set_status(sentry_span_t *span, sentry_span_status_t status)
 {
-    set_status(span->inner, status);
+    if (span) {
+        set_status(span->inner, status);
+    }
 }
 
 void
 sentry_transaction_set_status(
     sentry_transaction_t *tx, sentry_span_status_t status)
 {
-    set_status(tx->inner, status);
+    if (tx) {
+        set_status(tx->inner, status);
+    }
 }
 
 static void
@@ -509,12 +528,16 @@ void
 sentry_span_iter_headers(sentry_span_t *span,
     sentry_iter_headers_function_t callback, void *userdata)
 {
-    sentry__span_iter_headers(span->inner, callback, userdata);
+    if (span) {
+        sentry__span_iter_headers(span->inner, callback, userdata);
+    }
 }
 
 void
 sentry_transaction_iter_headers(sentry_transaction_t *tx,
     sentry_iter_headers_function_t callback, void *userdata)
 {
-    sentry__span_iter_headers(tx->inner, callback, userdata);
+    if (tx) {
+        sentry__span_iter_headers(tx->inner, callback, userdata);
+    }
 }

src/sentry_tracing.h

@@ -39,7 +39,6 @@ sentry_value_t sentry__value_span_new(size_t max_spans, sentry_value_t parent,
     char *operation, char *description);
 sentry_span_t *sentry__span_new(
     sentry_transaction_t *parent_tx, sentry_value_t inner);
-void sentry__span_free(sentry_span_t *span);
 
 /**
  * Returns an object containing tracing information extracted from a

tests/unit/test_tracing.c

@@ -773,7 +773,7 @@ SENTRY_TEST(distributed_headers)
         sentry_value_get_by_key(dist_tx->inner, "sampled")));
 
     sentry__transaction_decref(dist_tx);
-    sentry__span_free(child);
+    sentry__span_decref(child);
     sentry__transaction_decref(tx);
 
     // check sampled flag
@@ -801,7 +801,7 @@ SENTRY_TEST(distributed_headers)
         sentry_value_get_by_key(dist_tx->inner, "sampled")));
 
     sentry__transaction_decref(dist_tx);
-    sentry__span_free(child);
+    sentry__span_decref(child);
     sentry__transaction_decref(tx);
 
     sentry_close();