From b07dbe67ad16d62d16a003d4d46bb34cd13cf627 Mon Sep 17 00:00:00 2001 From: SEPURI-SAI-KRISHNA Date: Sun, 19 Jul 2026 14:20:11 +0530 Subject: [PATCH] fix(cypher): bounds-check and error-propagate cross_join_nodes allocation Signed-off-by: SEPURI-SAI-KRISHNA --- src/cypher/cypher.c | 81 ++++++++++++++++++++++++++++++++++++--------- src/cypher/cypher.h | 6 ++++ tests/test_cypher.c | 56 +++++++++++++++++++++++++++++++ 3 files changed, 128 insertions(+), 15 deletions(-) diff --git a/src/cypher/cypher.c b/src/cypher/cypher.c index 98ffb4c97..fe6af3e38 100644 --- a/src/cypher/cypher.c +++ b/src/cypher/cypher.c @@ -35,6 +35,7 @@ enum { #define CYP_DBL_MAX 1e308 #include +#include // INT_MAX #include "foundation/compat_regex.h" #include #include // int64_t @@ -4400,19 +4401,53 @@ static void execute_default_projection(cbm_pattern_t *pat0, binding_t *bindings, } } -/* Cross-join node-only pattern into existing bindings */ -static void cross_join_nodes(binding_t **bindings, int *bind_count, cbm_node_t *extra_nodes, - int extra_count, const char *nvar, bool opt) { - binding_t *new_bindings = malloc(((*bind_count * extra_count) + SKIP_ONE) * sizeof(binding_t)); - int new_count = 0; +/* Worst-case binding_t slot count for a node cross-join: one row per + * (existing binding × extra node), plus — for an OPTIONAL join with no extra + * nodes — one fallback row per existing binding, plus a trailing sentinel slot. + * + * The plain-int product bind_count * extra_count overflows to a negative/garbage + * malloc size on large graphs (the failure mode fixed for cross_join_with_rels), + * so the count is computed in size_t and rejected if it would not fit the int + * binding counter or would overflow the size_t byte size. Returns 0 and writes + * *out_n on success, CBM_NOT_FOUND on overflow. Non-static so the arithmetic + * boundary can be unit-tested directly. */ +int cbm_cypher_cross_join_alloc(int bind_count, int extra_count, bool opt, size_t *out_n) { + size_t per_binding = extra_count > 0 ? (size_t)extra_count : (opt ? (size_t)SKIP_ONE : 0U); + size_t rows = (size_t)bind_count * per_binding; + if (rows > (size_t)INT_MAX) { + return CBM_NOT_FOUND; /* new_count would not fit the int binding counter */ + } + size_t n = rows + SKIP_ONE; + if (n > SIZE_MAX / sizeof(binding_t)) { + return CBM_NOT_FOUND; /* alloc_n * sizeof(binding_t) would overflow size_t */ + } + *out_n = n; + return 0; +} + +/* Cross-join node-only pattern into existing bindings. Returns 0 on success, + * CBM_NOT_FOUND when the allocation is refused (overflow) or fails (OOM); the + * caller propagates that as a query error rather than silently skipping the + * MATCH, which would return a wrong (short) result. */ +static int cross_join_nodes(binding_t **bindings, int *bind_count, cbm_node_t *extra_nodes, + int extra_count, const char *nvar, bool opt) { + size_t alloc_n = 0; + if (cbm_cypher_cross_join_alloc(*bind_count, extra_count, opt, &alloc_n) != 0) { + return CBM_NOT_FOUND; /* product overflows int/size_t: fail the query */ + } + binding_t *new_bindings = malloc(alloc_n * sizeof(binding_t)); + if (!new_bindings) { + return CBM_NOT_FOUND; /* OOM: propagate, don't silently skip the MATCH */ + } + size_t new_count = 0; for (int bi = 0; bi < *bind_count; bi++) { - for (int ni = 0; ni < extra_count; ni++) { + for (int ni = 0; ni < extra_count && new_count < alloc_n; ni++) { binding_t nb = {0}; binding_copy(&nb, &(*bindings)[bi]); binding_set(&nb, nvar, &extra_nodes[ni]); new_bindings[new_count++] = nb; } - if (opt && extra_count == 0) { + if (opt && extra_count == 0 && new_count < alloc_n) { binding_t nb = {0}; binding_copy(&nb, &(*bindings)[bi]); new_bindings[new_count++] = nb; @@ -4423,7 +4458,8 @@ static void cross_join_nodes(binding_t **bindings, int *bind_count, cbm_node_t * } free(*bindings); *bindings = new_bindings; - *bind_count = new_count; + *bind_count = (int)new_count; /* new_count <= INT_MAX, guaranteed above */ + return 0; } /* Cross-join pattern-with-rels into existing bindings */ @@ -4562,9 +4598,9 @@ static void expand_from_bound_terminal(cbm_store_t *store, cbm_pattern_t *patn, } /* Expand additional MATCH patterns (pi >= 1) */ -static void expand_additional_patterns(cbm_store_t *store, cbm_query_t *q, const char *project, - int max_rows, binding_t **bindings, int *bind_count, - int *bind_cap) { +static int expand_additional_patterns(cbm_store_t *store, cbm_query_t *q, const char *project, + int max_rows, binding_t **bindings, int *bind_count, + int *bind_cap) { for (int pi = SKIP_ONE; pi < q->pattern_count; pi++) { cbm_pattern_t *patn = &q->patterns[pi]; bool opt = q->pattern_optional[pi]; @@ -4593,14 +4629,19 @@ static void expand_additional_patterns(cbm_store_t *store, cbm_query_t *q, const cbm_node_t *extra_nodes = NULL; int extra_count = 0; scan_pattern_nodes(store, project, max_rows, &patn->nodes[0], &extra_nodes, &extra_count); + int rc = 0; if (patn->rel_count == 0) { - cross_join_nodes(bindings, bind_count, extra_nodes, extra_count, nvar, opt); + rc = cross_join_nodes(bindings, bind_count, extra_nodes, extra_count, nvar, opt); } else { cross_join_with_rels(store, patn, bindings, bind_count, extra_nodes, extra_count, nvar, opt); } cbm_store_free_nodes(extra_nodes, extra_count); + if (rc != 0) { + return rc; /* allocation refused/failed: propagate as a query error */ + } } + return 0; } /* Project RETURN clause results */ @@ -4667,7 +4708,15 @@ static int execute_single(cbm_store_t *store, cbm_query_t *q, const char *projec q->pattern_optional[0]); /* Step 2b: Additional patterns */ - expand_additional_patterns(store, q, project, max_rows, &bindings, &bind_count, &bind_cap); + if (expand_additional_patterns(store, q, project, max_rows, &bindings, &bind_count, + &bind_cap) != 0) { + for (int bi = 0; bi < bind_count; bi++) { + binding_free(&bindings[bi]); + } + free(bindings); + cbm_store_free_nodes(scanned, scan_count); + return CBM_NOT_FOUND; /* cross-join allocation refused/failed */ + } /* Step 3: Late WHERE */ if (q->where && (pat0->rel_count > 0 || q->pattern_count > SKIP_ONE)) { @@ -4712,9 +4761,10 @@ int cbm_cypher_execute(cbm_store_t *store, const char *query, const char *projec } result_builder_t rb = {0}; - // cppcheck-suppress knownConditionTrueFalse if (execute_single(store, q, project, max_rows, &rb) < 0) { + rb_free(&rb); cbm_query_free(q); + out->error = heap_strdup("query aborted: out of memory or an allocation limit was reached"); return CBM_NOT_FOUND; } @@ -4722,11 +4772,12 @@ int cbm_cypher_execute(cbm_store_t *store, const char *query, const char *projec cbm_query_t *uq = q->union_next; while (uq) { result_builder_t rb2 = {0}; - // cppcheck-suppress knownConditionTrueFalse if (execute_single(store, uq, project, max_rows, &rb2) < 0) { rb_free(&rb); rb_free(&rb2); cbm_query_free(q); + out->error = + heap_strdup("query aborted: out of memory or an allocation limit was reached"); return CBM_NOT_FOUND; } /* Concatenate rows from rb2 into rb */ diff --git a/src/cypher/cypher.h b/src/cypher/cypher.h index 55d187251..6ad140e65 100644 --- a/src/cypher/cypher.h +++ b/src/cypher/cypher.h @@ -342,4 +342,10 @@ void cbm_query_free(cbm_query_t *q); * check; a negative value restores the default budget. */ void cbm_cypher_test_set_deadline_ms(int64_t budget_ms); +/* Worst-case binding slot count for a node cross-join. Computes the count in + * size_t and rejects any that would not fit the int binding counter or would + * overflow the size_t byte size; returns 0 and writes *out_n on success, + * CBM_NOT_FOUND on overflow. Exposed for arithmetic-boundary unit tests. */ +int cbm_cypher_cross_join_alloc(int bind_count, int extra_count, bool opt, size_t *out_n); + #endif /* CBM_CYPHER_H */ diff --git a/tests/test_cypher.c b/tests/test_cypher.c index fc3a06932..4dfc04cf9 100644 --- a/tests/test_cypher.c +++ b/tests/test_cypher.c @@ -489,6 +489,60 @@ TEST(cypher_exec_match_all_functions) { PASS(); } +/* Regression: an OPTIONAL MATCH whose label matches zero nodes drove + * cross_join_nodes with extra_count == 0. The old allocation + * (bind_count * 0 + 1) reserved a single binding slot, but the OPTIONAL + * fallback then wrote one binding per existing row — a heap buffer overflow + * once the first MATCH bound more than one node (ASan: heap-buffer-overflow). + * (The same function also used a plain-int bind_count*extra_count product, + * which wraps to a tiny malloc on large graphs; the count is now computed and + * bounds-checked in size_t by cbm_cypher_cross_join_alloc — exercised at its + * arithmetic boundary by cypher_cross_join_alloc_rejects_overflow below.) + * The query text is agent-controlled via the MCP query tool. */ +TEST(cypher_exec_optional_empty_label_no_overflow) { + cbm_store_t *s = setup_cypher_store(); /* 4 Function nodes */ + cbm_cypher_result_t r = {0}; + + int rc = cbm_cypher_execute( + s, "MATCH (a:Function) OPTIONAL MATCH (b:NoSuchLabel) RETURN a.name", "test", 0, &r); + ASSERT_EQ(rc, 0); + /* One row per Function, each with b left unbound (dead-code semantics). */ + ASSERT_EQ(r.row_count, 4); + + cbm_cypher_result_free(&r); + cbm_store_close(s); + PASS(); +} + +/* Arithmetic-boundary companion to the zero-label overflow above: the node + * cross-join sizes its buffer from bind_count * extra_count. As a plain int that + * product wraps past INT_MAX to a negative/garbage malloc size (the large-graph + * #627 failure mode). cbm_cypher_cross_join_alloc now computes it in size_t and + * rejects a count that would not fit the int binding counter or overflow the + * byte size. Tested directly so the boundary is exercised without allocating + * billions of bindings. */ +TEST(cypher_cross_join_alloc_rejects_overflow) { + size_t n = 0; + + /* 46341 * 46341 = 2147488281 > INT_MAX (2147483647): pre-fix the int product + * wrapped negative -> tiny malloc -> heap OOB. Now rejected. */ + ASSERT_TRUE(cbm_cypher_cross_join_alloc(46341, 46341, false, &n) != 0); + + /* A normal join still succeeds: bind_count * extra_count + 1 slots. */ + ASSERT_EQ(cbm_cypher_cross_join_alloc(4, 3, false, &n), 0); + ASSERT_EQ(n, (size_t)13); + + /* OPTIONAL with no extra nodes reserves one fallback row per binding + 1. */ + ASSERT_EQ(cbm_cypher_cross_join_alloc(4, 0, true, &n), 0); + ASSERT_EQ(n, (size_t)5); + + /* Non-OPTIONAL with no extra nodes: just the sentinel slot. */ + ASSERT_EQ(cbm_cypher_cross_join_alloc(4, 0, false, &n), 0); + ASSERT_EQ(n, (size_t)1); + + PASS(); +} + TEST(cypher_exec_where_eq) { cbm_store_t *s = setup_cypher_store(); cbm_cypher_result_t r = {0}; @@ -3080,6 +3134,8 @@ SUITE(cypher) { RUN_TEST(cypher_exec_deadline_aborts_runaway_query_issue601); RUN_TEST(cypher_exec_deadline_allows_normal_query_issue601); RUN_TEST(cypher_exec_match_all_functions); + RUN_TEST(cypher_exec_optional_empty_label_no_overflow); + RUN_TEST(cypher_cross_join_alloc_rejects_overflow); RUN_TEST(cypher_issue240_labels_function); RUN_TEST(cypher_issue237_distinct_order_limit); RUN_TEST(cypher_issue873_distinct_order_limit_dedupes_before_limit);