fix(cypher): bound OPTIONAL fallback in expand_pattern_rels to prevent heap overflow (#627)

Signed-off-by: SEPURI-SAI-KRISHNA <saik20533@gmail.com>

Author: SEPURI-SAI-KRISHNA

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 "" */

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);