diff --git a/src/cypher/cypher.c b/src/cypher/cypher.c index 2b2efc110..7936eff39 100644 --- a/src/cypher/cypher.c +++ b/src/cypher/cypher.c @@ -25,6 +25,7 @@ enum { CYP_MAX_VARS = 16, /* max Cypher variables in a query */ CYP_MAX_EDGE_VARS = 8, /* max edge variables */ CYP_GROWTH_10 = 10, /* binding growth factor */ + CYP_GROWTH_2 = 2, /* geometric buffer growth (#1196) */ CYP_CHAR_IDX1 = 1, /* second character index (e.g. op[1]) */ CYP_EBUF_MASK = 7, CYP_NODE_COLS = 4, /* columns per node var: name, qn, label, file */ @@ -3101,10 +3102,33 @@ static void scan_pattern_nodes(cbm_store_t *store, const char *project, cbm_node /* Process edges: look up target node, filter by label/props, add binding. * `inbound` controls which end of the edge is the target id. */ +/* #1196 (second mechanism): a hop's output buffer must hold EVERY matched + * row. max_rows is an OUTPUT-row limit — projection already enforces it — and + * WHERE/aggregation run after expansion, so any cap here silently falsifies + * results: field-measured, count() reported 9,360 of 13,691 DEFINES because + * the old bind_cap*10 ceiling dropped edges before aggregation ever saw them, + * and a labeled source did not save you. The buffer now grows geometrically; + * only allocation failure stops materialisation (the #601 deadline still + * bounds time), and match_count stays truthful either way (#627 contract). */ +static bool binding_out_append(binding_t **rows, int *count, int *cap, binding_t *nb) { + if (*count == *cap) { + int next = *cap > 0 ? *cap * CYP_GROWTH_2 : CYP_GROWTH_10; + binding_t *grown = realloc(*rows, (size_t)next * sizeof(binding_t)); + if (!grown) { + binding_free(nb); + return false; + } + *rows = grown; + *cap = next; + } + (*rows)[(*count)++] = *nb; + return true; +} + static void process_edges(cbm_store_t *store, cbm_edge_t *edges, int edge_count, bool inbound, const cbm_node_pattern_t *target_node, binding_t *b, const char *to_var, - const char *rel_var, binding_t *new_bindings, int *new_count, int max_new, - int *match_count) { + const char *rel_var, binding_t **new_bindings, int *new_count, + int *new_cap, int *match_count) { /* When the terminal node variable is ALREADY bound (e.g. the second pattern * `(c)-[:CALLS]->(f)` where `f` came from an earlier MATCH), we must FILTER * to edges that actually reach the bound node — not overwrite the caller's @@ -3137,16 +3161,14 @@ static void process_edges(cbm_store_t *store, cbm_edge_t *edges, int edge_count, node_fields_free(&found); continue; } - (*match_count)++; /* a real neighbour exists, budget or not */ - if (*new_count < max_new) { - binding_t nb = {0}; - binding_copy(&nb, b); - binding_set(&nb, to_var, &found); - if (rel_var) { - binding_set_edge(&nb, rel_var, &edges[ei]); - } - new_bindings[(*new_count)++] = nb; + (*match_count)++; /* a real neighbour exists, OOM or not */ + binding_t nb = {0}; + binding_copy(&nb, b); + binding_set(&nb, to_var, &found); + if (rel_var) { + binding_set_edge(&nb, rel_var, &edges[ei]); } + (void)binding_out_append(new_bindings, new_count, new_cap, &nb); node_fields_free(&found); } } @@ -3161,8 +3183,8 @@ static _Thread_local int g_cypher_depth_clamped = 0; static void expand_var_length(cbm_store_t *store, cbm_rel_pattern_t *rel, cbm_node_pattern_t *target_node, binding_t *b, cbm_node_t *src, - const char *to_var, binding_t *new_bindings, int *new_count, - int max_new, int *match_count) { + const char *to_var, binding_t **new_bindings, int *new_count, + int *new_cap, int *match_count) { /* Clamp BOTH the explicit (`*1..N`) and unbounded (`*`, `*..m`) forms to the * engine ceiling: an explicit N above the cap was previously honoured * verbatim, driving cbm_store_bfs to an unbounded hop count (#887). WARN on @@ -3201,12 +3223,10 @@ static void expand_var_length(cbm_store_t *store, cbm_rel_pattern_t *rel, continue; } (*match_count)++; - if (*new_count < max_new) { - binding_t nb = {0}; - binding_copy(&nb, b); - binding_set(&nb, to_var, &hop->node); - new_bindings[(*new_count)++] = nb; - } + binding_t nb = {0}; + binding_copy(&nb, b); + binding_set(&nb, to_var, &hop->node); + (void)binding_out_append(new_bindings, new_count, new_cap, &nb); } cbm_store_traverse_free(&tr); } @@ -3214,8 +3234,8 @@ static void expand_var_length(cbm_store_t *store, cbm_rel_pattern_t *rel, /* Expand fixed-length (1-hop) relationship edges */ static void expand_fixed_length(cbm_store_t *store, cbm_rel_pattern_t *rel, cbm_node_pattern_t *target_node, binding_t *b, cbm_node_t *src, - const char *to_var, binding_t *new_bindings, int *new_count, - int max_new, int *match_count) { + const char *to_var, binding_t **new_bindings, int *new_count, + int *new_cap, int *match_count) { bool is_inbound = rel->direction && strcmp(rel->direction, "inbound") == 0; bool is_any = rel->direction && strcmp(rel->direction, "any") == 0; const char *rel_var = rel->variable; @@ -3232,7 +3252,7 @@ static void expand_fixed_length(cbm_store_t *store, cbm_rel_pattern_t *rel, &edge_count); } process_edges(store, edges, edge_count, is_inbound, target_node, b, to_var, rel_var, - new_bindings, new_count, max_new, match_count); + new_bindings, new_count, new_cap, match_count); cbm_store_free_edges(edges, edge_count); } if (is_any) { @@ -3242,7 +3262,7 @@ static void expand_fixed_length(cbm_store_t *store, cbm_rel_pattern_t *rel, cbm_store_find_edges_by_target_type(store, src->id, rel->types[ti], &edges, &edge_count); process_edges(store, edges, edge_count, true, target_node, b, to_var, rel_var, - new_bindings, new_count, max_new, match_count); + new_bindings, new_count, new_cap, match_count); cbm_store_free_edges(edges, edge_count); } } @@ -3255,21 +3275,21 @@ static void expand_fixed_length(cbm_store_t *store, cbm_rel_pattern_t *rel, cbm_store_find_edges_by_source(store, src->id, &edges, &edge_count); } process_edges(store, edges, edge_count, is_inbound, target_node, b, to_var, rel_var, - new_bindings, new_count, max_new, match_count); + new_bindings, new_count, new_cap, match_count); cbm_store_free_edges(edges, edge_count); if (is_any) { edges = NULL; edge_count = 0; cbm_store_find_edges_by_target(store, src->id, &edges, &edge_count); process_edges(store, edges, edge_count, true, target_node, b, to_var, rel_var, - new_bindings, new_count, max_new, match_count); + new_bindings, new_count, new_cap, match_count); cbm_store_free_edges(edges, edge_count); } } } static void expand_pattern_rels(cbm_store_t *store, cbm_pattern_t *pat, binding_t **bindings, - int *bind_count, const int *bind_cap, const char **var_name, + int *bind_count, int *bind_cap, const char **var_name, bool is_optional) { for (int ri = 0; ri < pat->rel_count; ri++) { /* #601: stop expanding further hops once the wall-clock budget is spent @@ -3283,16 +3303,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 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)); + /* #1196: the hop's output buffer GROWS to hold every matched row — + * the old bind_cap*10 ceiling silently dropped edges before WHERE and + * aggregation, falsifying counts. binding_out_append handles growth + * and the OPTIONAL fallback shares it, so no writer can run off the + * end and no row class is dropped (the old fixed sizing had exactly + * those two failure modes, CWE-787 and the OPTIONAL data loss). */ + int new_cap = *bind_count > 0 ? *bind_count : CYP_GROWTH_10; + binding_t *new_bindings = malloc((size_t)new_cap * sizeof(binding_t)); if (!new_bindings) { return; /* OOM: leave existing bindings untouched rather than corrupt */ } @@ -3310,24 +3328,22 @@ static void expand_pattern_rels(cbm_store_t *store, cbm_pattern_t *pat, binding_ int match_count = 0; - int max_new = *bind_cap * CYP_GROWTH_10; if (is_variable_length) { - expand_var_length(store, rel, target_node, b, src, to_var, new_bindings, &new_count, - max_new, &match_count); + expand_var_length(store, rel, target_node, b, src, to_var, &new_bindings, + &new_count, &new_cap, &match_count); } else { - expand_fixed_length(store, rel, target_node, b, src, to_var, new_bindings, - &new_count, max_new, &match_count); + expand_fixed_length(store, rel, target_node, b, src, to_var, &new_bindings, + &new_count, &new_cap, &match_count); } /* 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. */ + * with the target unbound (projection renders it ""). The shared + * growable append gives every fallback row a slot. */ if (is_optional && match_count == 0) { binding_t nb = {0}; binding_copy(&nb, b); /* Don't set to_var — it remains unbound; projection returns "" */ - new_bindings[new_count++] = nb; + (void)binding_out_append(&new_bindings, &new_count, &new_cap, &nb); } } @@ -3337,6 +3353,7 @@ static void expand_pattern_rels(cbm_store_t *store, cbm_pattern_t *pat, binding_ free(*bindings); *bindings = new_bindings; *bind_count = new_count; + *bind_cap = new_cap; *var_name = to_var; } } @@ -4577,8 +4594,8 @@ static void cross_join_with_rels(cbm_store_t *store, cbm_pattern_t *patn, bindin * graphs (e.g. an unbound `c` scanned against ~29 K `f` bindings), wrapping * the int product negative and yielding a tiny/garbage malloc → heap OOB * write → SIGSEGV/SIGABRT (#627). */ - size_t alloc_n = (size_t)*bind_count * (size_t)extra_count * (size_t)CYP_GROWTH_10 + SKIP_ONE; - binding_t *new_bindings = malloc(alloc_n * sizeof(binding_t)); + int new_cap = *bind_count > 0 && extra_count > 0 ? *bind_count : SKIP_ONE; + binding_t *new_bindings = malloc((size_t)new_cap * sizeof(binding_t)); if (!new_bindings) { return; /* OOM: leave existing bindings untouched rather than corrupt */ } @@ -4595,7 +4612,7 @@ static void cross_join_with_rels(cbm_store_t *store, cbm_pattern_t *patn, bindin const char *tv = nvar; expand_pattern_rels(store, patn, &tmp, &tc, &tcap, &tv, opt); for (int ti = 0; ti < tc; ti++) { - new_bindings[new_count++] = tmp[ti]; + (void)binding_out_append(&new_bindings, &new_count, &new_cap, &tmp[ti]); } free(tmp); } @@ -4647,13 +4664,12 @@ static void expand_from_bound_terminal(cbm_store_t *store, cbm_pattern_t *patn, * OPTIONAL no-match row (a data-loss bug, not an OOB — the fallback stayed * in-bounds behind that guard) — exactly the rows * `OPTIONAL MATCH ... WHERE IS NULL` is meant to surface. */ - size_t alloc_n = (size_t)*bind_count * (size_t)CYP_GROWTH_10 + (size_t)*bind_count; - binding_t *new_bindings = malloc(alloc_n * sizeof(binding_t)); + int new_cap = *bind_count > 0 ? *bind_count : CYP_GROWTH_10; + binding_t *new_bindings = malloc((size_t)new_cap * sizeof(binding_t)); if (!new_bindings) { return; } int new_count = 0; - int max_new = *bind_count * CYP_GROWTH_10; for (int bi = 0; bi < *bind_count; bi++) { binding_t *b = &(*bindings)[bi]; @@ -4694,15 +4710,13 @@ static void expand_from_bound_terminal(cbm_store_t *store, cbm_pattern_t *patn, continue; } match_count++; - if (new_count < max_new) { - binding_t nb = {0}; - binding_copy(&nb, b); - binding_set(&nb, start_var, &found); - if (rel->variable) { - binding_set_edge(&nb, rel->variable, &edges[ei]); - } - new_bindings[new_count++] = nb; + binding_t nb = {0}; + binding_copy(&nb, b); + binding_set(&nb, start_var, &found); + if (rel->variable) { + binding_set_edge(&nb, rel->variable, &edges[ei]); } + (void)binding_out_append(&new_bindings, &new_count, &new_cap, &nb); node_fields_free(&found); } cbm_store_free_edges(edges, edge_count); @@ -4711,15 +4725,12 @@ static void expand_from_bound_terminal(cbm_store_t *store, cbm_pattern_t *patn, if (opt && match_count == 0) { /* GENUINELY no matching neighbour: the scan above runs unconditionally, * so match_count is the true neighbour count and match_count == 0 here - * means the terminal really has none — not merely that the buffer filled - * first. Keep the row with start_var left UNBOUND - * so `WHERE IS NULL` correctly identifies the no-edge case. The - * buffer is sized max_new + *bind_count precisely so every fallback row - * has a slot — no guard needed, and no OPTIONAL no-match row is dropped - * even after the expansion has saturated max_new. */ + * means the terminal really has none. Keep the row with start_var left + * UNBOUND so `WHERE IS NULL` correctly identifies the no-edge + * case; the shared growable append gives every fallback row a slot. */ binding_t nb = {0}; binding_copy(&nb, b); - new_bindings[new_count++] = nb; + (void)binding_out_append(&new_bindings, &new_count, &new_cap, &nb); } } diff --git a/tests/test_cypher.c b/tests/test_cypher.c index 4e9132cf9..ff5250c48 100644 --- a/tests/test_cypher.c +++ b/tests/test_cypher.c @@ -688,9 +688,12 @@ TEST(cypher_exec_optional_saturated_does_not_fabricate_no_match) { /* 3 Function nodes → scan_count = 3; max_rows = 3 → bind_cap = 3 and * max_new = 30. A alone exceeds that, so B and C are reached with the * budget already spent — the regime that produced the fabrication. */ - cbm_node_t a = {.project = "test", .label = "Function", .name = "A", .qualified_name = "test.A"}; - cbm_node_t b = {.project = "test", .label = "Function", .name = "B", .qualified_name = "test.B"}; - cbm_node_t c = {.project = "test", .label = "Function", .name = "C", .qualified_name = "test.C"}; + cbm_node_t a = { + .project = "test", .label = "Function", .name = "A", .qualified_name = "test.A"}; + cbm_node_t b = { + .project = "test", .label = "Function", .name = "B", .qualified_name = "test.B"}; + cbm_node_t c = { + .project = "test", .label = "Function", .name = "C", .qualified_name = "test.C"}; int64_t a_id = cbm_store_upsert_node(s, &a); (void)cbm_store_upsert_node(s, &b); /* B: no outgoing CALLS at all */ int64_t c_id = cbm_store_upsert_node(s, &c); @@ -965,9 +968,9 @@ TEST(cypher_exec_bound_terminal_saturation_no_false_deadcode) { ASSERT_EQ(rc, 0); ASSERT_EQ(r.col_count, 2); - bool hub_expanded = false; /* sanity: the buffer really did fill from a hub */ + bool hub_expanded = false; /* sanity: the buffer really did fill from a hub */ bool hub_false_deadcode = false; /* the bug: a hub with callers invented as dead */ - bool leaf_deadcode = false; /* the lossless property: genuine dead code kept */ + bool leaf_deadcode = false; /* the lossless property: genuine dead code kept */ for (int i = 0; i < r.row_count; i++) { const char *f = r.rows[i][0]; const char *c = r.rows[i][1]; @@ -1034,8 +1037,8 @@ TEST(cypher_exec_unlabeled_where_beyond_result_limit_issue1196) { ASSERT_GT(cbm_store_upsert_node(s, &late), 0); cbm_cypher_result_t r = {0}; - int rc = cbm_cypher_execute( - s, "MATCH (n) WHERE n.name = \"zz_late_match\" RETURN n.name", "test", 1, &r); + int rc = cbm_cypher_execute(s, "MATCH (n) WHERE n.name = \"zz_late_match\" RETURN n.name", + "test", 1, &r); ASSERT_EQ(rc, 0); ASSERT_EQ(r.row_count, 1); ASSERT_STR_EQ(r.rows[0][0], "zz_late_match"); @@ -1045,6 +1048,73 @@ TEST(cypher_exec_unlabeled_where_beyond_result_limit_issue1196) { PASS(); } +/* #1196 (second mechanism): relationship expansion capped this hop's TOTAL + * output at bind_cap*10, so edges past the cap were silently dropped BEFORE + * WHERE and aggregation — a count() then reported the scanned prefix as if + * it were a fact (field-measured: 9,360 of 13,691 DEFINES with a labeled + * source and --max-rows 1000). max_rows is an OUTPUT-row limit (projection + * already enforces it); expansion must see every matched edge. Fixture: 2 + * labeled sources with 30 edges each; max_rows=2 makes the old cap 20. */ +TEST(cypher_exec_aggregate_sees_all_edges_beyond_expansion_cap_issue1196) { + cbm_store_t *s = cbm_store_open_memory(); + ASSERT_NOT_NULL(s); + ASSERT_EQ(cbm_store_upsert_project(s, "test", "/tmp/test"), CBM_STORE_OK); + + int64_t src_ids[2]; + for (int i = 0; i < 2; i++) { + char name[32]; + char qn[64]; + snprintf(name, sizeof(name), "file_%d", i); + snprintf(qn, sizeof(qn), "test.%s", name); + cbm_node_t src = {.project = "test", + .label = "File", + .name = name, + .qualified_name = qn, + .file_path = name}; + src_ids[i] = cbm_store_upsert_node(s, &src); + ASSERT_GT(src_ids[i], 0); + } + for (int i = 0; i < 2; i++) { + for (int j = 0; j < 30; j++) { + char name[32]; + char qn[64]; + snprintf(name, sizeof(name), "def_%d_%02d", i, j); + snprintf(qn, sizeof(qn), "test.%s", name); + cbm_node_t target = {.project = "test", + .label = "Function", + .name = name, + .qualified_name = qn, + .file_path = "defs.py"}; + int64_t tid = cbm_store_upsert_node(s, &target); + ASSERT_GT(tid, 0); + cbm_edge_t e = { + .project = "test", .source_id = src_ids[i], .target_id = tid, .type = "DEFINES"}; + cbm_store_insert_edge(s, &e); + } + } + + /* Aggregate: one output row, so max_rows=2 never limits the OUTPUT — + * only the (buggy) expansion. Ground truth: 60 edges. */ + cbm_cypher_result_t r = {0}; + int rc = cbm_cypher_execute(s, "MATCH (a:File)-[rel]->(b) RETURN count(rel)", "test", 2, &r); + ASSERT_EQ(rc, 0); + ASSERT_EQ(r.row_count, 1); + ASSERT_STR_EQ(r.rows[0][0], "60"); + cbm_cypher_result_free(&r); + + /* The list form must saturate at the output limit, not at the scan: + * max_rows=25 returns exactly 25 rows (old cap: bind_cap=25 -> 250, + * fine here — but max_rows=2 must return 2 rows, not 2-of-20-scanned). */ + cbm_cypher_result_t r2 = {0}; + rc = cbm_cypher_execute(s, "MATCH (a:File)-[rel]->(b) RETURN b.name", "test", 2, &r2); + ASSERT_EQ(rc, 0); + ASSERT_EQ(r2.row_count, 2); + cbm_cypher_result_free(&r2); + + cbm_store_close(s); + PASS(); +} + /* #874: coalesce(var.prop, literal) in WHERE — null-safe numeric filters * for audit queries over OPTIONAL graph properties. The parser rejected the * call outright ("unexpected operator"); RETURN-side coalesce already @@ -2946,11 +3016,10 @@ TEST(cypher_exec_multikey_order_by_keeps_limit_issue1334) { cbm_store_t *s = setup_cypher_store(); cbm_cypher_result_t r = {0}; /* start_lines: HandleOrder=10, ValidateOrder=5, SubmitOrder=0, LogError=0 */ - int rc = cbm_cypher_execute( - s, - "MATCH (f:Function) RETURN f.name, f.start_line " - "ORDER BY f.start_line DESC, f.name ASC LIMIT 2", - "test", 0, &r); + int rc = cbm_cypher_execute(s, + "MATCH (f:Function) RETURN f.name, f.start_line " + "ORDER BY f.start_line DESC, f.name ASC LIMIT 2", + "test", 0, &r); ASSERT_EQ(rc, 0); ASSERT_EQ(r.row_count, 2); ASSERT_STR_EQ(r.rows[0][0], "HandleOrder"); @@ -2966,11 +3035,10 @@ TEST(cypher_exec_multikey_order_by_tiebreak_issue1334) { cbm_cypher_result_t r = {0}; /* start_line ASC puts the two 0-line functions first; name DESC breaks the * tie: SubmitOrder before LogError. */ - int rc = cbm_cypher_execute( - s, - "MATCH (f:Function) RETURN f.name, f.start_line " - "ORDER BY f.start_line ASC, f.name DESC LIMIT 2", - "test", 0, &r); + int rc = cbm_cypher_execute(s, + "MATCH (f:Function) RETURN f.name, f.start_line " + "ORDER BY f.start_line ASC, f.name DESC LIMIT 2", + "test", 0, &r); ASSERT_EQ(rc, 0); ASSERT_EQ(r.row_count, 2); ASSERT_STR_EQ(r.rows[0][0], "SubmitOrder"); @@ -3295,8 +3363,7 @@ TEST(cypher_issue1111_with_type_count_group) { cbm_store_t *s = setup_cypher_store(); cbm_cypher_result_t r = {0}; int rc = cbm_cypher_execute( - s, - "MATCH (a)-[r]->(b) WITH type(r) AS t, count(*) AS n RETURN t, n ORDER BY n DESC", + s, "MATCH (a)-[r]->(b) WITH type(r) AS t, count(*) AS n RETURN t, n ORDER BY n DESC", "test", 0, &r); ASSERT_EQ(rc, 0); ASSERT_EQ(r.row_count, 2); @@ -3938,6 +4005,7 @@ SUITE(cypher) { RUN_TEST(cypher_issue305_count_star_alias); RUN_TEST(cypher_exec_where_eq); RUN_TEST(cypher_exec_unlabeled_where_beyond_result_limit_issue1196); + RUN_TEST(cypher_exec_aggregate_sees_all_edges_beyond_expansion_cap_issue1196); RUN_TEST(cypher_exec_varlength_path_semantics_issue797); RUN_TEST(cypher_exec_where_coalesce_issue874); RUN_TEST(cypher_exec_where_regex);