From 1087dfd8466136942aff4ad480d139b2a65fe2aa Mon Sep 17 00:00:00 2001 From: Andrew Hundt Date: Fri, 26 Jun 2026 19:39:26 -0400 Subject: [PATCH] fix(pagerank): reject NaN damping/epsilon in the range clamp cbm_pagerank_compute's clamp used `damping < 0 || damping > 1` and `epsilon <= 0`, both of which IEEE-754 NaN bypasses (every NaN comparison is false). Since damping/epsilon became config-tunable and cbm_config_get_double uses strtod (which parses "nan"), a `config set pagerank_damping nan` would let NaN propagate through the power iteration: every rank becomes NaN, the convergence check `delta < epsilon` is always false (runs full max_iter), and the pagerank table is silently corrupted, poisoning the search relevance sort. Switch to the inverted-range form `!(damping >= 0 && damping <= 1)` and `!(epsilon > 0)`, which reject NaN because the inner comparison is false. Found by the silent-failure review agent; verified independently (strtod parses "nan"; NaN bypassed the old clamp). test_pagerank.c pagerank_nan_damping_is_clamped: a NaN damping must yield finite ranks summing to ~1. Green: pagerank 44 passed. Signed-off-by: Andrew Hundt --- src/pagerank/pagerank.c | 10 ++++++++-- tests/test_pagerank.c | 28 ++++++++++++++++++++++++++++ 2 files changed, 36 insertions(+), 2 deletions(-) diff --git a/src/pagerank/pagerank.c b/src/pagerank/pagerank.c index 10081942c..9d9a05ce3 100644 --- a/src/pagerank/pagerank.c +++ b/src/pagerank/pagerank.c @@ -150,9 +150,15 @@ int cbm_pagerank_compute(cbm_store_t *store, const char *project, cbm_rank_scope_t scope) { if (!store || !project || !project[0]) return -1; if (!weights) weights = &CBM_DEFAULT_EDGE_WEIGHTS; - if (damping < 0.0 || damping > 1.0) damping = CBM_PAGERANK_DAMPING; + /* Reject out-of-range AND NaN. IEEE-754 makes every NaN comparison false, + * so the naive `damping < 0 || damping > 1` form lets NaN through (it then + * poisons every rank and prevents convergence). The inverted-range form + * `!(x >= lo && x <= hi)` rejects NaN because the inner >= is false. + * NaN is reachable via config (strtod parses "nan") since damping/epsilon + * became user-tunable. */ + if (!(damping >= 0.0 && damping <= 1.0)) damping = CBM_PAGERANK_DAMPING; if (max_iter <= 0) max_iter = CBM_PAGERANK_MAX_ITER; - if (epsilon <= 0.0) epsilon = CBM_PAGERANK_EPSILON; + if (!(epsilon > 0.0)) epsilon = CBM_PAGERANK_EPSILON; sqlite3 *db = cbm_store_get_db(store); if (!db) return -1; diff --git a/tests/test_pagerank.c b/tests/test_pagerank.c index 9fed29d96..20333206d 100644 --- a/tests/test_pagerank.c +++ b/tests/test_pagerank.c @@ -840,6 +840,33 @@ TEST(pagerank_damping_epsilon_config_tunable) { PASS(); } +/* Robustness (review finding #44): a NaN damping must NOT corrupt PageRank. + * Since #21 made damping/epsilon config-tunable and cbm_config_get_double uses + * strtod (which parses "nan"), a user can `config set pagerank_damping nan`. + * The range clamp must reject NaN — IEEE-754 makes all NaN comparisons false, + * so the naive `damping < 0 || damping > 1` form lets NaN through, poisoning + * every rank and preventing convergence. */ +TEST(pagerank_nan_damping_is_clamped) { + cbm_store_t *s = cbm_store_open_memory(); + cbm_store_upsert_project(s, "nan", "/tmp/nan"); + int64_t a = add_node(s, "nan", "a"); + int64_t b = add_node(s, "nan", "b"); + add_edge(s, "nan", a, b, "CALLS"); + add_edge(s, "nan", b, a, "CALLS"); + + int rc = cbm_pagerank_compute(s, "nan", NAN, CBM_PAGERANK_EPSILON, + CBM_PAGERANK_MAX_ITER, NULL, CBM_DEFAULT_RANK_SCOPE); + ASSERT_EQ(rc, 2); + double ra = get_pr(s, a); + double rb = get_pr(s, b); + ASSERT_TRUE(isfinite(ra)); /* NaN damping must be clamped, not propagated */ + ASSERT_TRUE(isfinite(rb)); + ASSERT_TRUE(fabs((ra + rb) - 1.0) < 0.05); + + cbm_store_close(s); + PASS(); +} + /* ── Suite registration ──────────────────────────────────── */ SUITE(pagerank) { @@ -852,6 +879,7 @@ SUITE(pagerank) { RUN_TEST(pagerank_edge_weights); RUN_TEST(pagerank_convergence); RUN_TEST(pagerank_damping_epsilon_config_tunable); + RUN_TEST(pagerank_nan_damping_is_clamped); RUN_TEST(pagerank_sum_to_one); RUN_TEST(pagerank_stored_in_db); RUN_TEST(pagerank_recompute_replaces);