From a3adfbc5529d542e25e8a64ab4e069ed3da17c2d Mon Sep 17 00:00:00 2001 From: SEPURI-SAI-KRISHNA Date: Sun, 19 Jul 2026 15:13:50 +0530 Subject: [PATCH] fix(cypher): bound OPTIONAL fallback in expand_pattern_rels to prevent heap overflow (#627) Signed-off-by: SEPURI-SAI-KRISHNA --- src/cypher/cypher.c | 11 +++++++-- tests/test_cypher.c | 57 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 66 insertions(+), 2 deletions(-) diff --git a/src/cypher/cypher.c b/src/cypher/cypher.c index 98ffb4c97..9dd54aefc 100644 --- a/src/cypher/cypher.c +++ b/src/cypher/cypher.c @@ -3221,7 +3221,14 @@ static void expand_pattern_rels(cbm_store_t *store, cbm_pattern_t *pat, binding_ bool is_variable_length = (rel->min_hops != SKIP_ONE || rel->max_hops != SKIP_ONE); - size_t alloc_n = (size_t)*bind_cap * (size_t)CYP_GROWTH_10 + SKIP_ONE; + /* Room for up to max_new (bind_cap*10) expanded rows PLUS one OPTIONAL + * fallback row per source binding (the match_count==0 case below). The + * two are mutually exclusive per source, but a saturating hub can drive + * the expanded count to max_new while later sources still take the + * OPTIONAL path — so the buffer must hold both. The old `bind_cap*10 + 1` + * left room for a single OPTIONAL row after a saturated expansion and + * overflowed once a second one followed (heap OOB write). */ + size_t alloc_n = (size_t)*bind_cap * (size_t)CYP_GROWTH_10 + (size_t)*bind_count + SKIP_ONE; binding_t *new_bindings = malloc(alloc_n * sizeof(binding_t)); if (!new_bindings) { return; /* OOM: leave existing bindings untouched rather than corrupt */ @@ -3250,7 +3257,7 @@ static void expand_pattern_rels(cbm_store_t *store, cbm_pattern_t *pat, binding_ } /* OPTIONAL MATCH: keep binding with empty target if no matches */ - if (is_optional && match_count == 0) { + if (is_optional && match_count == 0 && (size_t)new_count < alloc_n) { binding_t nb = {0}; binding_copy(&nb, b); /* Don't set to_var — it remains unbound; projection returns "" */ diff --git a/tests/test_cypher.c b/tests/test_cypher.c index fc3a06932..4c4374a68 100644 --- a/tests/test_cypher.c +++ b/tests/test_cypher.c @@ -489,6 +489,62 @@ TEST(cypher_exec_match_all_functions) { PASS(); } +/* Regression: expand_pattern_rels sized its output buffer as bind_cap*10 + 1 — + * room for the bounded expansion (max_new = bind_cap*10) plus a SINGLE OPTIONAL + * fallback row. When one source saturates the expansion to max_new and two or + * more later sources take the OPTIONAL (no-match) path, the second fallback + * write ran past the allocation (ASan: heap-buffer-overflow). Query text is + * agent-controlled via the MCP query tool. */ +TEST(cypher_exec_optional_rel_saturated_no_overflow) { + cbm_store_t *s = cbm_store_open_memory(); + cbm_store_upsert_project(s, "test", "/tmp/test"); + + /* 1 hub + 20 leaf Function nodes → bind_cap = 21, max_new = 210, + * old alloc = 211 slots. The hub is inserted first so it is expanded before + * the leaves; it saturates the expansion, then each leaf adds an OPTIONAL + * fallback row, pushing new_count well past the allocation. */ + cbm_node_t hub = { + .project = "test", .label = "Function", .name = "hub", .qualified_name = "test.hub"}; + int64_t hub_id = cbm_store_upsert_node(s, &hub); + for (int i = 0; i < 20; i++) { + char nm[32]; + char qn[48]; + snprintf(nm, sizeof(nm), "leaf%02d", i); + snprintf(qn, sizeof(qn), "test.leaf%02d", i); + cbm_node_t leaf = { + .project = "test", .label = "Function", .name = nm, .qualified_name = qn}; + cbm_store_upsert_node(s, &leaf); + } + + /* Give the hub 300 CALLS edges (> max_new = 210) so its expansion saturates + * the buffer; targets are non-Function so they don't inflate bind_cap. */ + for (int i = 0; i < 300; i++) { + char nm[32]; + char qn[48]; + snprintf(nm, sizeof(nm), "callee%d", i); + snprintf(qn, sizeof(qn), "test.callee%d", i); + cbm_node_t callee = {.project = "test", .label = "Var", .name = nm, .qualified_name = qn}; + int64_t cid = cbm_store_upsert_node(s, &callee); + cbm_edge_t e = {.project = "test", .source_id = hub_id, .target_id = cid, .type = "CALLS"}; + cbm_store_insert_edge(s, &e); + } + + /* max_rows must be below the Function count (21) so bind_cap tracks + * scan_count (21) rather than the 100000 result ceiling — the same regime a + * large repo (> ceiling functions) or an agent-supplied small limit hits. + * bind_cap = 21 → max_new = 210, old alloc = 211 slots. */ + cbm_cypher_result_t r = {0}; + int rc = cbm_cypher_execute( + s, "MATCH (a:Function) OPTIONAL MATCH (a)-[:CALLS]->(b) RETURN a.name", "test", 5, &r); + ASSERT_EQ(rc, 0); + /* hub expands (capped at max_new), each leaf keeps one unbound row. */ + ASSERT_GT(r.row_count, 0); + + 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 +3136,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_rel_saturated_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);