diff --git a/src/cypher/cypher.c b/src/cypher/cypher.c index 98ffb4c97..fdbfbc73d 100644 --- a/src/cypher/cypher.c +++ b/src/cypher/cypher.c @@ -4403,16 +4403,28 @@ 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; + /* size_t arithmetic: bind_count * extra_count can exceed INT_MAX on large + * graphs, wrapping the plain-int product negative and yielding a tiny/garbage + * malloc → heap OOB write (the same failure mode fixed for cross_join_with_rels + * in #627). Additionally, an OPTIONAL join with extra_count == 0 still emits one + * row per existing binding, so the buffer must hold *bind_count entries even + * though the product is 0 — the old `bind_count * 0 + 1` allocated a single slot + * and then wrote *bind_count of them. */ + size_t per_binding = extra_count > 0 ? (size_t)extra_count : (opt ? (size_t)SKIP_ONE : 0U); + size_t alloc_n = (size_t)*bind_count * per_binding + SKIP_ONE; + binding_t *new_bindings = malloc(alloc_n * sizeof(binding_t)); + if (!new_bindings) { + return; /* OOM: leave existing bindings untouched rather than corrupt */ + } + 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 +4435,7 @@ 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; } /* Cross-join pattern-with-rels into existing bindings */ diff --git a/tests/test_cypher.c b/tests/test_cypher.c index fc3a06932..252bce033 100644 --- a/tests/test_cypher.c +++ b/tests/test_cypher.c @@ -489,6 +489,29 @@ 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 plain-int bind_count*extra_count, which wraps + * to a tiny malloc on large graphs — the #627 failure mode, unfixed here.) + * 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(); +} + TEST(cypher_exec_where_eq) { cbm_store_t *s = setup_cypher_store(); cbm_cypher_result_t r = {0}; @@ -3080,6 +3103,7 @@ 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_issue240_labels_function); RUN_TEST(cypher_issue237_distinct_order_limit); RUN_TEST(cypher_issue873_distinct_order_limit_dedupes_before_limit);