diff --git a/CHANGELOG.md b/CHANGELOG.md index 686d33c59..6a2c1cf77 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,10 @@ ## Unreleased +**Breaking**: + +- `sentry_value_incref` now returns `sentry_value_t` and `sentry_value_decref` returns `int` (0 if freed). ([#1763](https://github.com/getsentry/sentry-native/pull/1763)) + **Features**: - Add a `transfer_timeout` option for SDK-managed HTTP transports. ([#1741](https://github.com/getsentry/sentry-native/pull/1741)) @@ -21,6 +25,7 @@ - Cap rate-limit retry-after values at 24 hours to prevent a MITM-provided response from disabling event delivery for the process lifetime. ([#1744](https://github.com/getsentry/sentry-native/pull/1744)) - Native: validate ELF header entry sizes. ([#1746](https://github.com/getsentry/sentry-native/pull/1746)) - Structured logs: respect printf argument widths when extracting log parameters to avoid stack-data disclosure and corrupted attributes on 32-bit platforms. ([#1752](https://github.com/getsentry/sentry-native/pull/1752)) +- Fix TOCTOU races in transaction/span refcounting by switching to the atomic decref return value. ([#1763](https://github.com/getsentry/sentry-native/pull/1763)) ## 0.14.2 diff --git a/include/sentry.h b/include/sentry.h index 7f7dcecdf..055013f89 100644 --- a/include/sentry.h +++ b/include/sentry.h @@ -245,12 +245,14 @@ typedef union sentry_value_u sentry_value_t; /** * Increments the reference count on the value. */ -SENTRY_API void sentry_value_incref(sentry_value_t value); +SENTRY_API sentry_value_t sentry_value_incref(sentry_value_t value); /** * Decrements the reference count on the value. + * Returns 0 if the value was freed or is a primitive (no tracking needed), + * or non-zero if it still has references. */ -SENTRY_API void sentry_value_decref(sentry_value_t value); +SENTRY_API int sentry_value_decref(sentry_value_t value); /** * Returns the refcount of a value. diff --git a/src/sentry_tracing.c b/src/sentry_tracing.c index 8c32a97c9..b7c4e135a 100644 --- a/src/sentry_tracing.c +++ b/src/sentry_tracing.c @@ -152,11 +152,8 @@ sentry__transaction_context_free(sentry_transaction_context_t *tx_ctx) if (!tx_ctx) { return; } - if (sentry_value_refcount(tx_ctx->inner) <= 1) { - sentry_value_decref(tx_ctx->inner); + if (!sentry_value_decref(tx_ctx->inner)) { sentry_free(tx_ctx); - } else { - sentry_value_decref(tx_ctx->inner); } } @@ -475,13 +472,10 @@ sentry__transaction_decref(sentry_transaction_t *tx) return; } - if (sentry_value_refcount(tx->inner) <= 1) { - sentry_value_decref(tx->inner); + if (!sentry_value_decref(tx->inner)) { sentry_free(tx->children); sentry__mutex_free(&tx->children_mutex); sentry_free(tx); - } else { - sentry_value_decref(tx->inner); } } @@ -516,13 +510,10 @@ sentry__span_decref(sentry_span_t *span) return; } - if (sentry_value_refcount(span->inner) <= 1) { + if (!sentry_value_decref(span->inner)) { sentry__transaction_remove_child(span->transaction, span); - sentry_value_decref(span->inner); sentry__transaction_decref(span->transaction); sentry_free(span); - } else { - sentry_value_decref(span->inner); } } diff --git a/src/sentry_value.c b/src/sentry_value.c index e79d34a11..9034f7838 100644 --- a/src/sentry_value.c +++ b/src/sentry_value.c @@ -257,22 +257,25 @@ value_as_unfrozen_thing(sentry_value_t value) /* public api implementations */ -void +sentry_value_t sentry_value_incref(sentry_value_t value) { thing_t *thing = value_as_thing(value); if (thing) { sentry__atomic_fetch_and_add(&thing->refcount, 1); } + return value; } -void +int sentry_value_decref(sentry_value_t value) { thing_t *thing = value_as_thing(value); if (thing && sentry__atomic_fetch_and_add(&thing->refcount, -1) == 1) { thing_free(thing); + return 0; } + return thing ? 1 : 0; } size_t diff --git a/tests/unit/test_value.c b/tests/unit/test_value.c index b9e0f81f2..cca5d7f36 100644 --- a/tests/unit/test_value.c +++ b/tests/unit/test_value.c @@ -1762,3 +1762,33 @@ SENTRY_TEST(value_merge_breadcrumbs_max_limit) sentry_value_decref(list_a); sentry_value_decref(list_b); } + +SENTRY_TEST(value_refcount) +{ + // primitive values always report refcount 1, and decref is a no-op + sentry_value_t null_val = sentry_value_new_null(); + TEST_CHECK_INT_EQUAL(1, sentry_value_refcount(null_val)); + TEST_CHECK(!sentry_value_decref(null_val)); + TEST_CHECK_INT_EQUAL(1, sentry_value_refcount(null_val)); + + sentry_value_t bool_val = sentry_value_new_bool(true); + TEST_CHECK_INT_EQUAL(1, sentry_value_refcount(bool_val)); + TEST_CHECK(!sentry_value_decref(bool_val)); + TEST_CHECK_INT_EQUAL(1, sentry_value_refcount(bool_val)); + + sentry_value_t int_val = sentry_value_new_int32(42); + TEST_CHECK_INT_EQUAL(1, sentry_value_refcount(int_val)); + TEST_CHECK(!sentry_value_decref(int_val)); + TEST_CHECK_INT_EQUAL(1, sentry_value_refcount(int_val)); + + // thing-allocated values track refcount through incref/decref + sentry_value_t obj = sentry_value_new_object(); + TEST_CHECK_INT_EQUAL(1, sentry_value_refcount(obj)); + TEST_CHECK_INT_EQUAL(2, sentry_value_refcount(sentry_value_incref(obj))); + TEST_CHECK_INT_EQUAL(3, sentry_value_refcount(sentry_value_incref(obj))); + TEST_CHECK(!!sentry_value_decref(obj)); + TEST_CHECK_INT_EQUAL(2, sentry_value_refcount(obj)); + TEST_CHECK(!!sentry_value_decref(obj)); + TEST_CHECK_INT_EQUAL(1, sentry_value_refcount(obj)); + TEST_CHECK(!sentry_value_decref(obj)); +} diff --git a/tests/unit/tests.inc b/tests/unit/tests.inc index c49562c55..3d9e01264 100644 --- a/tests/unit/tests.inc +++ b/tests/unit/tests.inc @@ -383,6 +383,7 @@ XX(value_null) XX(value_object) XX(value_object_merge) XX(value_object_merge_nested) +XX(value_refcount) XX(value_remove_by_null_key) XX(value_set_by_null_key) XX(value_set_stacktrace)