From 41e649281bb9b27ead5d6ec223a0f0c78924c8f8 Mon Sep 17 00:00:00 2001 From: Joshua Richter Date: Tue, 1 Sep 2026 17:21:55 -0400 Subject: [PATCH] fix(cypher): refuse a query naming more variables than a binding holds MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit check_projection_scope models declared names in a fixed 32-entry array and skipped the check entirely when a query declared more. Nothing bounded how many variables a pattern could declare, so the out-of-scope refusal added by #1922 switched itself off on a wide query: the same undeclared name was refused at 10 declared names and quietly accepted at 35, answering a column of empty strings. Skipping was deliberate — a wrong refusal costs the caller a working query, which is worse than the silence. The premise was wrong. A binding holds CYP_MAX_VARS node variables and CYP_MAX_EDGE_VARS edge variables in plain arrays, and binding_set and binding_set_edge drop anything past those without a word, so a query naming more cannot be answered at all. Its extra names bind to nothing and project as blanks, which reads as "the graph holds no such data". The choice was never refuse-or-stay-quiet; it was refuse, or answer wrong. So bound the input. check_pattern_var_capacity counts distinct node and edge variables across every pattern — they share one binding, and an OPTIONAL MATCH pattern sits in the same array — and refuses beyond what a binding holds, naming the limit and how to get under it. Leaving the name off a node frees its slot, because an unnamed node takes none. Bounding the input rather than each consumer also makes three other silent drops unreachable: binding_set past 16, binding_set_edge past 8, and the column collection in execute_default_projection. Those would otherwise need an error path threaded up through every caller, since both binding setters return void. The two `< 0` branches in check_projection_scope stay, with comments saying they can no longer fire. They keep the guard standing if either bound ever moves. Fixes #1995 Signed-off-by: Joshua Richter --- src/cypher/cypher.c | 79 ++++++++++++++++++++++++++++++++++++++-- tests/test_cypher.c | 87 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 164 insertions(+), 2 deletions(-) diff --git a/src/cypher/cypher.c b/src/cypher/cypher.c index 17840b04c..e23fabde2 100644 --- a/src/cypher/cypher.c +++ b/src/cypher/cypher.c @@ -5043,13 +5043,88 @@ static const char *scope_checkable_var(const cbm_return_item_t *item) { return item->variable; } +/* Says which limit the query passed and how to get under it. An unnamed node + * takes no slot, so dropping a name the query never uses is the cheap way out. + * Splitting the MATCH is NOT — every pattern of one query shares one binding, + * which is why the caller counts across all of them. Separate queries do work, + * because each gets a binding of its own. */ +static char *var_capacity_error(const char *kind, int limit) { + char buf[CBM_SZ_256]; + snprintf(buf, sizeof(buf), + "too many %s variables: a query can name at most %d — " + "leave the name off the ones you do not use, or run separate queries", + kind, limit); + return heap_strdup(buf); +} + +/* A binding holds a fixed number of variables: CYP_MAX_VARS node variables and + * CYP_MAX_EDGE_VARS edge variables, both in plain arrays (see binding_t). + * binding_set and binding_set_edge drop anything past those without a word, so + * a query naming more variables than a binding holds cannot be answered — the + * extra names bind to nothing and project as empty strings, which reads as + * "the graph holds no such data" rather than "this query is too wide". + * + * Refuse such a query instead, before any row is touched. Bounding the input + * here is also what stops collect_declared_names below overflowing its array: + * the patterns contribute at most CYP_MAX_VARS + CYP_MAX_EDGE_VARS names, plus + * one UNWIND alias, which is well inside CYP_SCOPE_MAX_NAMES. + * + * Counts DISTINCT variables across every pattern, because they all land in the + * same binding: a multi-MATCH query shares one, and an OPTIONAL MATCH pattern + * sits in this same array (q->pattern_optional marks which). A node variable + * and an edge variable may share a name and each take a slot, because the + * binding keeps the two in separate arrays. */ +static char *check_pattern_var_capacity(const cbm_query_t *q) { + /* Initialized because cppcheck cannot see that scope_holds reads only the + * node_n / edge_n entries already written, and reports the first call as a + * read of an uninitialized array. */ + const char *node_vars[CYP_MAX_VARS] = {NULL}; + const char *edge_vars[CYP_MAX_EDGE_VARS] = {NULL}; + int node_n = 0; + int edge_n = 0; + for (int pi = 0; pi < q->pattern_count; pi++) { + const cbm_pattern_t *pat = &q->patterns[pi]; + for (int ni = 0; ni < pat->node_count; ni++) { + const char *var = pat->nodes[ni].variable; + if (!var || scope_holds(node_vars, node_n, var)) { + continue; + } + if (node_n >= CYP_MAX_VARS) { + return var_capacity_error("node", CYP_MAX_VARS); + } + node_vars[node_n++] = var; + } + for (int ri = 0; ri < pat->rel_count; ri++) { + const char *var = pat->rels[ri].variable; + if (!var || scope_holds(edge_vars, edge_n, var)) { + continue; + } + if (edge_n >= CYP_MAX_EDGE_VARS) { + return var_capacity_error("edge", CYP_MAX_EDGE_VARS); + } + edge_vars[edge_n++] = var; + } + } + return NULL; +} + /* Answers NULL when the query is fine, or a heap message naming the first * variable that is not in scope. Checks one query; the caller walks a UNION. */ static char *check_projection_scope(const cbm_query_t *q) { + /* Runs first, so the rest of this function can trust that the query names + * no more variables than the arrays below can model. */ + char *capacity_err = check_pattern_var_capacity(q); + if (capacity_err) { + return capacity_err; + } + const char *declared[CYP_SCOPE_MAX_NAMES]; int declared_n = collect_declared_names(q, declared, CYP_SCOPE_MAX_NAMES); if (declared_n < 0) { - return NULL; /* too many names to model — stay quiet rather than guess */ + /* Unreachable while the capacity check above holds. Kept so the guard + * still stands if either bound ever moves. Skipping the check was the + * old behaviour, and it let an out-of-scope name through in silence. */ + return NULL; } /* A WITH still reads the pattern variables. */ @@ -5073,7 +5148,7 @@ static char *check_projection_scope(const cbm_query_t *q) { if (q->with_clause) { scope_n = collect_with_names(q->with_clause, after_with, CYP_SCOPE_MAX_NAMES); if (scope_n < 0) { - return NULL; + return NULL; /* unreachable: a WITH holds at most CYP_SCOPE_MAX_NAMES items */ } scope = after_with; } diff --git a/tests/test_cypher.c b/tests/test_cypher.c index b674da15d..d7732aee6 100644 --- a/tests/test_cypher.c +++ b/tests/test_cypher.c @@ -2982,6 +2982,90 @@ TEST(cypher_exec_return_star) { PASS(); } +/* Build "MATCH (a0:NoSuchLabelXYZ)-[:CALLS]->(a1)-…->(aN-1)" into buf. The label + * matches nothing, so any query built on it is instant and needs no fixture. */ +static void build_node_chain(char *buf, size_t buf_sz, int nodes) { + int off = snprintf(buf, buf_sz, "MATCH (a0:NoSuchLabelXYZ)"); + for (int i = 1; i < nodes; i++) { + off += snprintf(buf + off, buf_sz - (size_t)off, "-[:CALLS]->(a%d)", i); + } +} + +TEST(cypher_wide_pattern_refused) { + /* A binding holds CYP_MAX_VARS (16) node variables, and binding_set drops + * the 17th without a word. The query then answers a column of empty strings + * for every name it could not bind, which reads as "the graph holds no such + * data". Refuse the query instead of answering it wrong. */ + cbm_store_t *s = setup_cypher_store(); + char query[2048]; + + build_node_chain(query, sizeof(query), 20); /* 20 > CYP_MAX_VARS */ + strncat(query, " RETURN a0.name", sizeof(query) - strlen(query) - 1); + cbm_cypher_result_t wide = {0}; + ASSERT_TRUE(cbm_cypher_execute(s, query, "test", 0, &wide) != 0); + ASSERT_NOT_NULL(wide.error); + ASSERT_TRUE(strstr(wide.error, "node") != NULL); /* says which limit was passed */ + cbm_cypher_result_free(&wide); + + /* The width right at the bound still runs, so the guard refuses only what a + * binding genuinely cannot hold. */ + build_node_chain(query, sizeof(query), 16); + strncat(query, " RETURN a0.name", sizeof(query) - strlen(query) - 1); + cbm_cypher_result_t ok = {0}; + ASSERT_EQ(cbm_cypher_execute(s, query, "test", 0, &ok), 0); + cbm_cypher_result_free(&ok); + + cbm_store_close(s); + PASS(); +} + +TEST(cypher_wide_edge_pattern_refused) { + /* Same shape on the edge table, where binding_set_edge stops at + * CYP_MAX_EDGE_VARS (8). Only NAMED relationships take a slot. */ + cbm_store_t *s = setup_cypher_store(); + char query[2048]; + int off = snprintf(query, sizeof(query), "MATCH (a0:NoSuchLabelXYZ)"); + for (int i = 1; i <= 9; i++) { /* 9 > CYP_MAX_EDGE_VARS */ + off += snprintf(query + off, sizeof(query) - (size_t)off, "-[r%d:CALLS]->(a%d)", i, i); + } + snprintf(query + off, sizeof(query) - (size_t)off, " RETURN a0.name"); + cbm_cypher_result_t wide = {0}; + ASSERT_TRUE(cbm_cypher_execute(s, query, "test", 0, &wide) != 0); + ASSERT_NOT_NULL(wide.error); + ASSERT_TRUE(strstr(wide.error, "edge") != NULL); + cbm_cypher_result_free(&wide); + cbm_store_close(s); + PASS(); +} + +TEST(cypher_scope_check_survives_wide_pattern) { + /* Regression test for #1995. check_projection_scope models declared names in + * a fixed array and used to skip the check entirely when a query declared + * more than it held. So the same out-of-scope name was refused on a narrow + * query and quietly accepted on a wide one. Both must now be refused. */ + cbm_store_t *s = setup_cypher_store(); + char query[4096]; + + build_node_chain(query, sizeof(query), 10); + strncat(query, " RETURN zzz.name", sizeof(query) - strlen(query) - 1); + cbm_cypher_result_t narrow = {0}; + ASSERT_TRUE(cbm_cypher_execute(s, query, "test", 0, &narrow) != 0); + ASSERT_NOT_NULL(narrow.error); + ASSERT_TRUE(strstr(narrow.error, "zzz") != NULL); + cbm_cypher_result_free(&narrow); + + /* 35 declared names — this one used to answer a zzz.name column of nothing. */ + build_node_chain(query, sizeof(query), 35); + strncat(query, " RETURN zzz.name", sizeof(query) - strlen(query) - 1); + cbm_cypher_result_t wide = {0}; + ASSERT_TRUE(cbm_cypher_execute(s, query, "test", 0, &wide) != 0); + ASSERT_NOT_NULL(wide.error); + cbm_cypher_result_free(&wide); + + cbm_store_close(s); + PASS(); +} + TEST(cypher_parse_neq) { cbm_query_t *q = NULL; char *err = NULL; @@ -4290,6 +4374,9 @@ SUITE(cypher) { RUN_TEST(cypher_exec_where_is_null); RUN_TEST(cypher_exec_where_is_not_null); RUN_TEST(cypher_exec_return_star); + RUN_TEST(cypher_wide_pattern_refused); + RUN_TEST(cypher_wide_edge_pattern_refused); + RUN_TEST(cypher_scope_check_survives_wide_pattern); RUN_TEST(cypher_parse_neq); RUN_TEST(cypher_parse_in); RUN_TEST(cypher_parse_is_null);