diff --git a/src/cypher/cypher.c b/src/cypher/cypher.c index 7c394c289..eba190777 100644 --- a/src/cypher/cypher.c +++ b/src/cypher/cypher.c @@ -3233,7 +3233,15 @@ 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; + /* Size this hop's output for BOTH writers without dropping any row: the + * expansion helpers emit at most max_new = bind_cap*10 rows (they stop at + * max_new), and the OPTIONAL fallback emits at most one row per source + * (<= *bind_count). A source either matches (feeds the expansion) or takes + * the fallback, never both, so the two counts are additive and bounded by + * max_new + *bind_count. Computed in size_t so the product cannot overflow. + * The previous "+ 1" sizing fit only a SINGLE fallback row after a + * saturated expansion; a second one ran off the end (heap OOB, CWE-787). */ + size_t alloc_n = (size_t)*bind_cap * (size_t)CYP_GROWTH_10 + (size_t)*bind_count; binding_t *new_bindings = malloc(alloc_n * sizeof(binding_t)); if (!new_bindings) { return; /* OOM: leave existing bindings untouched rather than corrupt */ @@ -3261,7 +3269,10 @@ static void expand_pattern_rels(cbm_store_t *store, cbm_pattern_t *pat, binding_ &new_count, max_new, &match_count); } - /* OPTIONAL MATCH: keep binding with empty target if no matches */ + /* OPTIONAL MATCH: no expansion for this source, so keep the binding + * with the target unbound (projection renders it ""). The buffer is + * sized max_new + *bind_count precisely so every such fallback row has + * a slot — no guard needed, and no OPTIONAL no-match row is dropped. */ if (is_optional && match_count == 0) { binding_t nb = {0}; binding_copy(&nb, b); diff --git a/tests/test_cypher.c b/tests/test_cypher.c index d25f498dd..658ee5199 100644 --- a/tests/test_cypher.c +++ b/tests/test_cypher.c @@ -517,6 +517,69 @@ TEST(cypher_exec_optional_empty_label_no_overflow) { PASS(); } +/* Regression: expand_pattern_rels sized its OPTIONAL-expansion 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 saturated the expansion to + * max_new and two or more later sources took the OPTIONAL (no-match) path, the + * second fallback write ran past the allocation (ASan: heap-buffer-overflow). + * + * The fix sizes the buffer for both writers losslessly (max_new + *bind_count), + * so every OPTIONAL no-match row keeps its slot — no overflow AND no dropped + * rows. Here the hub saturates the expansion to max_new while all 20 leaves take + * the fallback; the buffer holds them all, and the max_rows LIMIT bounds only the + * OUTPUT. 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, so the hop + * buffer holds max_new + *bind_count = 231 rows. The hub is inserted first so + * it is expanded before the leaves; it saturates the expansion to max_new, + * then each of the 20 leaves adds one OPTIONAL fallback row — under the old + * alloc (211 slots) the 2nd such write overflowed; now all 230 fit. */ + 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 + * max_new; 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 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. */ + 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); + /* Bounded success, no overflow (ASan proves the buffer holds every row); the + * LIMIT caps the output rows rather than the query crashing. */ + ASSERT_EQ(rc, 0); + ASSERT_GT(r.row_count, 0); + ASSERT_TRUE(r.row_count <= 5); + + 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 @@ -546,6 +609,72 @@ TEST(cypher_cross_join_alloc_rejects_overflow) { PASS(); } +/* Companion to the truncation regression: when the expansion does NOT saturate + * the ceiling, every leaf's OPTIONAL fallback row must survive with its target + * unbound. A `row_count > 0` check is too weak — it can pass on hub rows alone — + * so this asserts a specific leaf appears with an empty b.name, and that a real + * expanded hub row is present too. */ +TEST(cypher_exec_optional_rel_leaf_fallback_survives) { + cbm_store_t *s = cbm_store_open_memory(); + cbm_store_upsert_project(s, "test", "/tmp/test"); + + /* 1 hub (2 CALLS edges) + 3 leaves (no edges). max_rows 0 is defaulted to + * CYPHER_RESULT_CEILING (100000) in cbm_cypher_execute before bind_cap is + * computed, so bind_cap = max(scan_count, 100000) = 100000 and the buffer is + * far larger than needed here — this exercises the fallback rows, not the + * saturation edge. */ + 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 < 3; i++) { + char nm[32]; + char qn[48]; + snprintf(nm, sizeof(nm), "leaf%d", i); + snprintf(qn, sizeof(qn), "test.leaf%d", i); + cbm_node_t leaf = { + .project = "test", .label = "Function", .name = nm, .qualified_name = qn}; + cbm_store_upsert_node(s, &leaf); + } + for (int i = 0; i < 2; 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); + } + + cbm_cypher_result_t r = {0}; + int rc = cbm_cypher_execute( + s, "MATCH (a:Function) OPTIONAL MATCH (a)-[:CALLS]->(b) RETURN a.name, b.name", "test", 0, + &r); + ASSERT_EQ(rc, 0); + ASSERT_EQ(r.col_count, 2); + + /* Scan for a leaf fallback row (a.name = "leaf0", b.name unbound = "") and a + * real expanded hub row (a.name = "hub", b.name non-empty). */ + bool leaf_fallback = false; + bool hub_expanded = false; + for (int i = 0; i < r.row_count; i++) { + const char *a = r.rows[i][0]; + const char *b = r.rows[i][1]; + if (strcmp(a, "leaf0") == 0 && b[0] == '\0') { + leaf_fallback = true; + } + if (strcmp(a, "hub") == 0 && b[0] != '\0') { + hub_expanded = true; + } + } + ASSERT_TRUE(leaf_fallback); /* the OPTIONAL no-match row survived */ + ASSERT_TRUE(hub_expanded); /* the expansion still produced bound rows */ + + 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}; @@ -3294,6 +3423,8 @@ SUITE(cypher) { 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_exec_optional_rel_saturated_no_overflow); + RUN_TEST(cypher_exec_optional_rel_leaf_fallback_survives); RUN_TEST(cypher_issue240_labels_function); RUN_TEST(cypher_issue237_distinct_order_limit); RUN_TEST(cypher_issue873_distinct_order_limit_dedupes_before_limit);