From 22809b20ef90bd3d56da2df5e1830d0d9387fba1 Mon Sep 17 00:00:00 2001 From: Betty Da Date: Fri, 17 Dec 2021 20:06:14 -0500 Subject: [PATCH] feat(tracing): Don't allow missing orphan spans to be parents or be finished --- src/sentry_core.c | 47 ++++++++++++++++++++++++++++++--------- tests/unit/test_tracing.c | 16 ++++++++++++- 2 files changed, 52 insertions(+), 11 deletions(-) diff --git a/src/sentry_core.c b/src/sentry_core.c index 4054fe0f7..01f4702b7 100644 --- a/src/sentry_core.c +++ b/src/sentry_core.c @@ -845,29 +845,41 @@ sentry_span_start_child( // There isn't an active transaction. This span has nothing to attach // to. if (sentry_value_is_null(scope->span)) { - return sentry_value_new_null(); + goto fail; } // Aggressively discard spans if a transaction is unsampled to avoid // wasting memory sentry_value_t sampled = sentry_value_get_by_key(scope->span, "sampled"); if (!sentry_value_is_true(sampled)) { - return sentry_value_new_null(); + goto fail; } sentry_value_t spans = sentry_value_get_by_key(scope->span, "spans"); span_count = sentry_value_get_length(spans); if (span_count >= max_spans) { - return sentry_value_new_null(); + goto fail; } - // TODO: if the parent span can't be found in the current active - // transaction, take ownership of the parent span context and return - // null. sentry_value_t parent; if (sentry_value_is_null(parent_span_context)) { parent = scope->span; } else { parent = parent_span_context; + + // TODO: Man there's gotta be a more terse way to do all of this + // Make sure the parent span actually exists in the transaction + const char *parent_span_id = sentry_value_as_string( + sentry_value_get_by_key(parent_span_context, "span_id")); + int32_t index = sentry_value_as_int32( + sentry_value_get_by_key(parent_span_context, "index")); + + sentry_value_t maybe_span = sentry_value_get_by_index(spans, index); + const char *maybe_span_id = sentry_value_as_string( + sentry_value_get_by_key(maybe_span, "span_id")); + + if (!sentry__string_eq(parent_span_id, maybe_span_id)) { + goto fail; + } } sentry_value_t child = sentry__value_new_span(parent, operation); @@ -891,6 +903,10 @@ sentry_span_start_child( child_span_context, "index", sentry_value_new_int32((int)span_count)); return child_span_context; + +fail: + sentry_value_decref(parent_span_context); + return sentry_value_new_null(); } void @@ -905,11 +921,22 @@ sentry_span_finish(sentry_value_t span_context) SENTRY_WITH_SCOPE_MUT (scope) { sentry_value_t spans = sentry_value_get_by_key(scope->span, "spans"); - // TODO: maybe validate that to_update.span_id == span.span_id sentry_value_t to_update = sentry_value_get_by_index(spans, index); - sentry_value_set_by_key(to_update, "timestamp", - sentry__value_new_string_owned( - sentry__msec_time_to_iso8601(sentry__msec_time()))); + + const char *expected_span_id = sentry_value_as_string( + sentry_value_get_by_key(span_context, "span_id")); + const char *found_span_id = sentry_value_as_string( + sentry_value_get_by_key(to_update, "span_id")); + if (!sentry__string_eq(expected_span_id, found_span_id)) { + SENTRY_DEBUGF("Attempted to finish a span expecting span ID %s but " + "found %s instead. Maybe its parent transaction " + "already finished?", + expected_span_id, found_span_id); + } else { + sentry_value_set_by_key(to_update, "timestamp", + sentry__value_new_string_owned( + sentry__msec_time_to_iso8601(sentry__msec_time()))); + } } sentry_value_decref(span_context); diff --git a/tests/unit/test_tracing.c b/tests/unit/test_tracing.c index f11e25cc0..9eb1d755a 100644 --- a/tests/unit/test_tracing.c +++ b/tests/unit/test_tracing.c @@ -358,6 +358,21 @@ SENTRY_TEST(basic_spans) // Should be finished TEST_CHECK(!IS_NULL(stored_child, "timestamp")); + // Make sure you can't create a grandchild of a span that isn't on the + // current transaction any more + sentry_value_t sibling + = sentry_span_start_child(sentry_value_new_null(), "beep", "car"); + sentry_uuid_t event_id = sentry_transaction_finish(); + TEST_CHECK(!sentry_uuid_is_nil(&event_id)); + + tx_cxt = sentry_value_new_transaction("wowee!", NULL); + sentry_transaction_start(tx_cxt); + + sentry_value_t orphan = sentry_span_start_child(sibling, "ding", "bicycle"); + TEST_CHECK(sentry_value_is_null(orphan)); + + sentry_value_decref(orphan); + sentry_close(); } @@ -449,7 +464,6 @@ SENTRY_TEST(overflow_spans) sentry_value_get_by_key(scope_tx, "spans"), 0); CHECK_STRING_PROPERTY(stored_child, "span_id", child_span_id); - sentry_value_decref(child); sentry_value_decref(overflow_child); sentry_close();