diff --git a/src/tdigest.c b/src/tdigest.c index 1268c15..aebc7ff 100644 --- a/src/tdigest.c +++ b/src/tdigest.c @@ -104,6 +104,26 @@ static inline uint64_t cap_from_compression(uint64_t compression) { return (UINT64_C(6) * compression) + UINT64_C(10); } +// Validate `compression` and compute the node-array capacity (cap = 6*compression + 10) +// entirely in 64-bit width, WITHOUT allocating. Factored out of td_init() so the +// accepted/rejected boundary can be probed in a test without committing tens of GiB of +// backing storage (at cap ~ INT_MAX the two 8-byte node arrays total ~34 GB / ~32 GiB). +// Returns 0 and writes *capacity on success; returns 1 on rejection and leaves *capacity +// untouched. Rejections: non-finite, <= 0, > INT_MAX, or a capacity that would overflow int +// or a size_t element count for either node array. +static inline int capacity_from_compression(double compression, size_t *capacity) { + if (!isfinite(compression) || compression <= 0 || compression > INT_MAX) { + return 1; + } + const uint64_t capacity64 = cap_from_compression((uint64_t)compression); + if (capacity64 > INT_MAX || capacity64 > SIZE_MAX / sizeof(double) || + capacity64 > SIZE_MAX / sizeof(long long)) { + return 1; + } + *capacity = (size_t)capacity64; + return 0; +} + static inline bool should_td_compress(td_histogram_t *h) { return ((h->merged_nodes + h->unmerged_nodes) >= (h->cap - 1)); } @@ -154,17 +174,12 @@ void td_reset(td_histogram_t *h) { int td_init(double compression, td_histogram_t **result) { - // Compute capacity in an explicitly 64-bit type so 6 * compression + 10 cannot wrap at the - // width of int or size_t before it is validated. cap and the node indexes are stored as int. - if (!isfinite(compression) || compression <= 0 || compression > INT_MAX) { - return 1; - } - const uint64_t capacity64 = cap_from_compression((uint64_t)compression); - if (capacity64 > INT_MAX || capacity64 > SIZE_MAX / sizeof(double) || - capacity64 > SIZE_MAX / sizeof(long long)) { + // Validate compression and size the node arrays in 64-bit width before narrowing to int + // (see capacity_from_compression). On rejection *result is left untouched. + size_t capacity; + if (capacity_from_compression(compression, &capacity) != 0) { return 1; } - const size_t capacity = (size_t)capacity64; td_histogram_t *histogram; histogram = (td_histogram_t *)td_malloc_(sizeof(td_histogram_t)); if (!histogram) { @@ -197,6 +212,12 @@ td_histogram_t *td_new(double compression) { } void td_free(td_histogram_t *histogram) { + // NULL guard: td_new() returns NULL for invalid compression (non-finite / <= 0 / + // cap > INT_MAX) or allocation failure, so the idiomatic td_free(td_new(bad)) cleanup + // would otherwise dereference NULL. (td_new()'s validation landed in #41.) + if (!histogram) { + return; + } if (histogram->nodes_mean) { td_free_((void *)(histogram->nodes_mean)); } diff --git a/src/tdigest.h b/src/tdigest.h index c07436c..71d330e 100644 --- a/src/tdigest.h +++ b/src/tdigest.h @@ -62,7 +62,9 @@ extern "C" { * 1000 is extremely large. * The number of centroids retained will be a smallish (usually less than 10) multiple of this * number. - * @return the histogram on success, NULL if allocation failed. + * @return the histogram on success, NULL if `compression` is invalid (non-finite, <= 0, or so + * large that the centroid capacity would overflow) or if allocation failed. Because NULL is a + * valid return, `td_free(td_new(...))` is a safe cleanup idiom (td_free() tolerates NULL). */ td_histogram_t *td_new(double compression); @@ -74,15 +76,19 @@ td_histogram_t *td_new(double compression); * 1000 is extremely large. * The number of centroids retained will be a smallish (usually less than 10) multiple of this * number. - * @param result Output parameter to capture allocated histogram. - * @return 0 on success, 1 if allocation failed. + * @param result Output parameter to capture allocated histogram. On success `*result` is set to + * the new histogram; on failure `*result` is left untouched (never written), so a caller may seed + * it with a sentinel to distinguish "not written" from NULL. + * @return 0 on success, 1 if `compression` is invalid (non-finite, <= 0, or so large that the + * centroid capacity would overflow) or if allocation failed. */ int td_init(double compression, td_histogram_t **result); /** * Frees the memory associated with the t-digest. * - * @param h The histogram you want to free. + * @param h The histogram you want to free. Passing NULL is allowed and is a no-op, so the + * `td_free(td_new(...))` idiom is safe even when td_new() returns NULL. */ void td_free(td_histogram_t *h); diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 85da305..b18a12d 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -23,6 +23,16 @@ if (BUILD_TESTS) target_link_libraries(td_test tdigest m) enable_testing() add_test(td_test td_test) + + # Capacity-boundary regression: includes tdigest.c to drive the allocation-free + # capacity_from_compression() helper at the exact accepted/rejected boundary without + # allocating the ~34 GB (~32 GiB) that the largest accepted compression would need. + # Use CMAKE_CURRENT_LIST_DIR so the src path stays correct when t-digest-c is consumed + # via add_subdirectory() (CMAKE_SOURCE_DIR would point at the outermost project). + add_executable(td_capacity_test unit/td_capacity_test.c) + target_include_directories(td_capacity_test PRIVATE ${CMAKE_CURRENT_LIST_DIR}/../src) + target_link_libraries(td_capacity_test m) + add_test(td_capacity_test td_capacity_test) endif() diff --git a/tests/unit/td_capacity_test.c b/tests/unit/td_capacity_test.c new file mode 100644 index 0000000..1740a4b --- /dev/null +++ b/tests/unit/td_capacity_test.c @@ -0,0 +1,86 @@ +/* + * Capacity-boundary regression for td_init (follow-up to #41 / #44). + * + * The accepted side of the capacity guard is the largest compression whose node capacity + * (cap = 6*compression + 10) still fits within every limit the helper enforces: INT_MAX and the + * per-array element counts SIZE_MAX/sizeof(double) and SIZE_MAX/sizeof(long long). On a 64-bit + * size_t target INT_MAX binds; on a 32-bit size_t target the SIZE_MAX/8 element limit is smaller + * and binds instead, so the boundary is derived from the minimum rather than assuming INT_MAX. + * Verifying it through td_new()/td_init() is impractical because at cap ~ INT_MAX the two 8-byte + * node arrays total ~34 GB (~32 GiB). This test instead includes the translation unit and drives + * the factored, allocation-free helper capacity_from_compression() directly, so the exact + * boundary (and one past it) is checked without committing tens of GiB. + */ +#include +#include +#include +#include + +#include "tdigest.c" /* brings in the static capacity_from_compression + cap_from_compression */ + +static int failures = 0; + +#define CHECK(cond, msg) \ + do { \ + if (!(cond)) { \ + fprintf(stderr, "FAIL: %s\n", (msg)); \ + failures++; \ + } \ + } while (0) + +int main(void) { + /* The binding capacity limit is the minimum of every check the helper enforces: + * INT_MAX and the per-array element counts SIZE_MAX/sizeof(double|long long). On 64-bit + * size_t this is INT_MAX; on 32-bit size_t it is the (smaller) SIZE_MAX/8. */ + uint64_t max_capacity = (uint64_t)INT_MAX; + if (SIZE_MAX / sizeof(double) < max_capacity) { + max_capacity = (uint64_t)(SIZE_MAX / sizeof(double)); + } + if (SIZE_MAX / sizeof(long long) < max_capacity) { + max_capacity = (uint64_t)(SIZE_MAX / sizeof(long long)); + } + + /* The largest integer compression whose capacity still fits within max_capacity. */ + const long long max_ok = (long long)((max_capacity - 10) / 6); + + /* Sanity on the arithmetic: this capacity must be within the limit, the next one must not. */ + const uint64_t cap_ok = cap_from_compression((uint64_t)max_ok); + const uint64_t cap_over = cap_from_compression((uint64_t)(max_ok + 1)); + CHECK(cap_ok <= max_capacity, "boundary capacity should be within the binding limit"); + CHECK(cap_over > max_capacity, "boundary+1 capacity should exceed the binding limit"); + + const size_t SENTINEL = (size_t)0xA5A5A5A5A5A5A5A5ULL; + size_t cap; + + /* Accepted boundary: returns 0 and writes a capacity that matches the formula. */ + cap = SENTINEL; + CHECK(capacity_from_compression((double)max_ok, &cap) == 0, + "largest in-range compression must be accepted"); + CHECK(cap == (size_t)cap_ok, "accepted boundary capacity must match 6*c+10"); + + /* One past the boundary: rejected, and *capacity is left untouched. */ + cap = SENTINEL; + CHECK(capacity_from_compression((double)(max_ok + 1), &cap) == 1, + "compression just past the boundary must be rejected"); + CHECK(cap == SENTINEL, "rejected input must leave *capacity untouched"); + + /* Invalid inputs are rejected and never touch *capacity. */ + const double bad[] = {NAN, INFINITY, -INFINITY, 0.0, -1.0, (double)INT_MAX}; + for (unsigned i = 0; i < sizeof(bad) / sizeof(bad[0]); ++i) { + cap = SENTINEL; + CHECK(capacity_from_compression(bad[i], &cap) == 1, "invalid compression must be rejected"); + CHECK(cap == SENTINEL, "rejected input must leave *capacity untouched"); + } + + /* A small valid compression still computes the documented capacity. */ + cap = SENTINEL; + CHECK(capacity_from_compression(100.0, &cap) == 0, "compression 100 must be accepted"); + CHECK(cap == 610, "cap(100) must be 6*100 + 10"); + + if (failures == 0) { + printf("OK: capacity boundary max_ok=%lld cap=%llu\n", max_ok, (unsigned long long)cap_ok); + return 0; + } + fprintf(stderr, "%d capacity check(s) failed\n", failures); + return 1; +} diff --git a/tests/unit/td_test.c b/tests/unit/td_test.c index 5803e72..707d3d8 100644 --- a/tests/unit/td_test.c +++ b/tests/unit/td_test.c @@ -521,8 +521,9 @@ MU_TEST(test_td_init) { mu_assert_long_eq(0, td_init(1000000, &t)); td_free(t); - mu_assert_long_eq(0, td_init(100000000, &t)); - td_free(t); + // The exact accepted/rejected capacity boundary is covered by td_capacity_test, which + // exercises the no-alloc capacity helper directly instead of committing the ~34 GB (~32 GiB) + // the largest accepted compression would allocate here. } MU_TEST(test_quantiles) { @@ -594,9 +595,102 @@ MU_TEST(test_quantiles_multiple) { td_free(t); } +// td_free must tolerate NULL: this PR makes td_new() return NULL for invalid +// compression, so td_free(td_new(bad)) is a natural cleanup pattern. +MU_TEST(test_td_free_null) { + td_free(NULL); // must not crash + td_free(td_new(-1.0)); // td_new returns NULL for invalid compression + td_free(td_new(NAN)); +} + +// The td_new() convenience wrapper must reject the same inputs as td_init(). +MU_TEST(test_td_new_rejects_bad_compression) { + mu_assert(td_new(NAN) == NULL, "td_new(NaN) must return NULL"); + mu_assert(td_new(INFINITY) == NULL, "td_new(INF) must return NULL"); + mu_assert(td_new(-1) == NULL, "td_new(-1) must return NULL"); + mu_assert(td_new(0) == NULL, "td_new(0) must return NULL"); + mu_assert(td_new((double)(((INT_MAX - 10) / 6) + 1)) == NULL, + "td_new above the capacity boundary must return NULL"); +} + +// td_init must leave *result untouched on failure (stronger than "stays NULL": +// a sentinel proves td_init never writes the out-param on a rejected input). +MU_TEST(test_td_init_result_untouched_on_failure) { + td_histogram_t sentinel_obj; + td_histogram_t *const sentinel = &sentinel_obj; + td_histogram_t *t; + t = sentinel; + mu_assert_long_eq(1, td_init(NAN, &t)); + mu_assert(t == sentinel, "NaN: *result must be left untouched"); + t = sentinel; + mu_assert_long_eq(1, td_init(0, &t)); + mu_assert(t == sentinel, "zero: *result must be left untouched"); + t = sentinel; + mu_assert_long_eq(1, td_init((double)(((INT_MAX - 10) / 6) + 1), &t)); + mu_assert(t == sentinel, "overflow: *result must be left untouched"); +} + +// Capacity formula (cap = 6*compression + 10) and determinism at safe sizes, +// plus the current behavior for sub-1 / fractional compression (accepted, floored). +MU_TEST(test_td_init_cap_and_determinism) { + td_histogram_t *a = NULL, *b = NULL; + mu_assert_long_eq(0, td_init(1, &a)); + mu_assert_long_eq(16, a->cap); // 6*1 + 10 + td_free(a); + a = NULL; + mu_assert_long_eq(0, td_init(2, &a)); + mu_assert_long_eq(22, a->cap); // 6*2 + 10 + td_free(a); + a = NULL; + // determinism: same compression -> same cap + mu_assert_long_eq(0, td_init(500, &a)); + mu_assert_long_eq(0, td_init(500, &b)); + mu_assert_long_eq(3010, a->cap); // 6*500 + 10 + mu_assert_long_eq(a->cap, b->cap); + td_free(a); + td_free(b); + // Sub-1 / fractional compression is currently ACCEPTED and floored to 0, + // yielding cap 10 (6*0 + 10); td_compression() then reports (int)0.5 == 0. + // Pins today's behavior (see PR discussion on whether to reject compression < 1). + a = NULL; + mu_assert_long_eq(0, td_init(0.5, &a)); + mu_assert_long_eq(10, a->cap); + mu_assert_int_eq(0, td_compression(a)); + td_free(a); +} + +// A large but valid digest (compression 100000 -> cap 600010, ~9.6 MB) must not +// just allocate but actually work end to end. +MU_TEST(test_td_init_large_success_is_usable) { + td_histogram_t *t = NULL; + mu_assert_long_eq(0, td_init(100000, &t)); + mu_assert(t != NULL, "large valid compression should allocate"); + mu_assert_long_eq(600010, t->cap); // 6*100000 + 10 + for (int i = 1; i <= 10000; ++i) { + mu_assert(td_add(t, (double)i, 1) == 0, "Insertion"); + } + mu_assert(td_compress(t) == 0, "compress large digest"); + mu_assert_double_eq(1.0, td_min(t)); + mu_assert_double_eq(10000.0, td_max(t)); + mu_assert_long_eq(10000, td_size(t)); + mu_assert(td_centroid_count(t) <= t->cap, "centroid count must stay within cap"); + // Store the result and assert it is finite first: mu_assert_double_eq_epsilon does NOT fail + // for NaN (fabs(expected - NaN) is NaN, and NaN > epsilon is false), so the epsilon check + // alone would pass even if td_quantile() returned NaN. + const double median = td_quantile(t, 0.5); + mu_assert(isfinite(median), "median must be finite"); + mu_assert_double_eq_epsilon(5000.5, median, 50.0); + td_free(t); +} + MU_TEST_SUITE(test_suite) { MU_RUN_TEST(test_basic); MU_RUN_TEST(test_td_init); + MU_RUN_TEST(test_td_free_null); + MU_RUN_TEST(test_td_new_rejects_bad_compression); + MU_RUN_TEST(test_td_init_result_untouched_on_failure); + MU_RUN_TEST(test_td_init_cap_and_determinism); + MU_RUN_TEST(test_td_init_large_success_is_usable); MU_RUN_TEST(test_compress_small); MU_RUN_TEST(test_compress_large); MU_RUN_TEST(test_nans);