diff --git a/CHANGELOG.md b/CHANGELOG.md index 33c7493bc..586cee5d2 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**: - Native: add opt-in async crash upload mode so crashed apps can exit early after crash data is captured, while the crash daemon finishes potentially large uploads in the background. ([#1739](https://github.com/getsentry/sentry-native/pull/1739)) @@ -31,6 +35,7 @@ - Native: clamp `module_count` from the shared crash context. ([#1770](https://github.com/getsentry/sentry-native/pull/1770)) - Prevent database cleanup from following symlinks in run and cache directories. ([#1751](https://github.com/getsentry/sentry-native/pull/1751)) - 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)) - Fix a potential out-of-bounds read when parsing non-NUL-terminated `sentry-trace` headers. ([#1749](https://github.com/getsentry/sentry-native/pull/1749)) - Harden ELF note parsing against overflow and OOB reads. ([#1773](https://github.com/getsentry/sentry-native/pull/1773)) - Fix division by zero when breadcrumbs are disabled. ([#1767](https://github.com/getsentry/sentry-native/pull/1767)) diff --git a/include/sentry.h b/include/sentry.h index 25416813e..75d5ebd88 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 c1fce886c..79da3abe0 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); } } @@ -479,13 +476,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); } } @@ -520,13 +514,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 2565fb71c..6a1ea6d06 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 ab5e6a38b..ba0ef57c2 100644 --- a/tests/unit/test_value.c +++ b/tests/unit/test_value.c @@ -1804,3 +1804,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 93d434916..e7ad60ab9 100644 --- a/tests/unit/tests.inc +++ b/tests/unit/tests.inc @@ -391,6 +391,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)