From eb7505abfce5ea20b74f898acda7e1109f036e4f Mon Sep 17 00:00:00 2001 From: Martin Vogel Date: Fri, 2 Oct 2026 05:04:06 +0200 Subject: [PATCH 1/2] fix(cypher): walk operator chains and UNION branches in loops A WHERE chain such as `a OR b OR c ...` was folded into a left-deep tree, so every walker over the expression tree recursed once per operand: the evaluator, the seed-selectivity pass of the planner and, every 128 nodes, the free routine. UNION branches were parsed by a nested cbm_parse call per branch, so the parser recursed once per branch as well. Stack use therefore followed the length of a query, not its nesting, and the daemon evaluates queries on a worker thread with a 256 KiB stack. A long but flat query, such as a few thousand `OR name = ...` terms, needs to work there. Chains of one operator are now linked along the right child at parse time, OP(a, OP(b, OP(c, ...))), and eval_expr and cypher_expr_selectivity follow the links in a loop, recursing only into the operands, whose nesting the existing parse-depth cap bounds. Operand order and the left-to-right short-circuit of AND and OR are the same as in the nested form, XOR keeps its running parity, and a missing value takes part in a chain exactly as before. expr_free already walks with an explicit stack and needs no change for a right-linked chain. UNION branches are read by one parser in a loop in cbm_parse and linked as they come; parse_post_where no longer parses the branch after the keyword, and the ALL flag still sits on the branch before it. Tests: 50,000-operand OR and AND chains and 20,000 UNION ALL branches run on a thread with the worker stack size inside a forked child, with the expected rows computed by the test; a chain with operands over a missing property is compared with its parenthesised forms in both associations; 50 UNION and UNION ALL branches return the right rows. Signed-off-by: Martin Vogel --- src/cypher/cypher.c | 268 +++++++++++------- src/foundation/recursion_whitelist.h | 12 +- tests/test_cypher.c | 406 +++++++++++++++++++++++++++ 3 files changed, 580 insertions(+), 106 deletions(-) diff --git a/src/cypher/cypher.c b/src/cypher/cypher.c index e1fcf3467d..276268e3a9 100644 --- a/src/cypher/cypher.c +++ b/src/cypher/cypher.c @@ -1258,58 +1258,79 @@ static cbm_expr_t *parse_not_expr(parser_t *p) { // NOLINT(misc-no-recursion) return parse_atom_expr(p); } +/* A chain `a OP b OP c ...` of one associative operator is linked along the + * RIGHT child: OP(a, OP(b, OP(c, ...))). The chain's length is bounded only by + * the message size, so a left-deep fold would hand every walker (evaluate, + * seed-selectivity, free) a tree as deep as the chain is long; linked this + * way each walker follows the chain in a loop and only recurses into the + * operands, whose nesting the parse-depth cap bounds. Operand order and the + * left-to-right short-circuit are the same as in the nested form. `next` is + * the operand just parsed; `more` says whether another OP follows it. */ +static void expr_chain_link(cbm_expr_t **tail, cbm_expr_type_t type, cbm_expr_t *next, bool more) { + if (more) { + cbm_expr_t *node = expr_binary(type, next, NULL); + (*tail)->right = node; + *tail = node; + } else { + (*tail)->right = next; + } +} + /* AND: not (AND not)* */ static cbm_expr_t *parse_and_expr(parser_t *p) { // NOLINT(misc-no-recursion) - cbm_expr_t *left = parse_not_expr(p); - if (!left) { - return NULL; - } - while (check(p, TOK_AND)) { - advance(p); - cbm_expr_t *right = parse_not_expr(p); - if (!right) { - expr_free(left); + cbm_expr_t *first = parse_not_expr(p); + if (!first || !check(p, TOK_AND)) { + return first; + } + cbm_expr_t *head = expr_binary(EXPR_AND, first, NULL); + cbm_expr_t *tail = head; + while (match(p, TOK_AND)) { + cbm_expr_t *next = parse_not_expr(p); + if (!next) { + expr_free(head); return NULL; } - left = expr_binary(EXPR_AND, left, right); + expr_chain_link(&tail, EXPR_AND, next, check(p, TOK_AND)); } - return left; + return head; } /* XOR: and (XOR and)* */ static cbm_expr_t *parse_xor_expr(parser_t *p) { // NOLINT(misc-no-recursion) - cbm_expr_t *left = parse_and_expr(p); - if (!left) { - return NULL; - } - while (check(p, TOK_XOR)) { - advance(p); - cbm_expr_t *right = parse_and_expr(p); - if (!right) { - expr_free(left); + cbm_expr_t *first = parse_and_expr(p); + if (!first || !check(p, TOK_XOR)) { + return first; + } + cbm_expr_t *head = expr_binary(EXPR_XOR, first, NULL); + cbm_expr_t *tail = head; + while (match(p, TOK_XOR)) { + cbm_expr_t *next = parse_and_expr(p); + if (!next) { + expr_free(head); return NULL; } - left = expr_binary(EXPR_XOR, left, right); + expr_chain_link(&tail, EXPR_XOR, next, check(p, TOK_XOR)); } - return left; + return head; } /* OR: xor (OR xor)* */ static cbm_expr_t *parse_or_expr(parser_t *p) { // NOLINT(misc-no-recursion) - cbm_expr_t *left = parse_xor_expr(p); - if (!left) { - return NULL; - } - while (check(p, TOK_OR)) { - advance(p); - cbm_expr_t *right = parse_xor_expr(p); - if (!right) { - expr_free(left); + cbm_expr_t *first = parse_xor_expr(p); + if (!first || !check(p, TOK_OR)) { + return first; + } + cbm_expr_t *head = expr_binary(EXPR_OR, first, NULL); + cbm_expr_t *tail = head; + while (match(p, TOK_OR)) { + cbm_expr_t *next = parse_xor_expr(p); + if (!next) { + expr_free(head); return NULL; } - left = expr_binary(EXPR_OR, left, right); + expr_chain_link(&tail, EXPR_OR, next, check(p, TOK_OR)); } - return left; + return head; } /* Parse WHERE clause — builds expression tree */ @@ -1972,9 +1993,9 @@ static int parse_match_chain(parser_t *p, cbm_query_t *q, int *pat_cap) { return 0; } -/* Parse post-WHERE clauses: additional MATCH, WITH, RETURN, UNION */ -static int parse_post_where(parser_t *p, cbm_query_t *q, // NOLINT(misc-no-recursion) - int *pat_cap) { +/* Parse post-WHERE clauses: additional MATCH, WITH, RETURN. A UNION keyword + * is left for cbm_parse, which reads the branches in a loop. */ +static int parse_post_where(parser_t *p, cbm_query_t *q, int *pat_cap) { /* More MATCH / OPTIONAL MATCH after WHERE */ if (parse_match_chain(p, q, pat_cap) < 0) { return CBM_NOT_FOUND; @@ -1999,57 +2020,44 @@ static int parse_post_where(parser_t *p, cbm_query_t *q, // NOLINT(misc-no-recur if (parse_return(p, &q->ret) < 0) { return CBM_NOT_FOUND; } - /* UNION [ALL] */ - if (check(p, TOK_UNION)) { - advance(p); - q->union_all = match(p, TOK_ALL); - cbm_parse_result_t sub = {0}; - if (cbm_parse(&p->tokens[p->pos], p->count - p->pos, &sub) < 0) { - if (sub.error) { - snprintf(p->error, sizeof(p->error), "%s", sub.error); - } - cbm_parse_free(&sub); - return CBM_NOT_FOUND; - } - q->union_next = sub.query; - sub.query = NULL; - cbm_parse_free(&sub); - /* The branch after UNION was parsed by a SEPARATE parser over a slice - * of these tokens, so this parser's cursor never moved past the UNION - * keyword. That sub-parse now refuses to succeed with anything left - * over, so everything from here to the end is accounted for. Move the - * cursor to the end to say so, or cbm_parse's end-of-input check reads - * a fully parsed UNION query as unfinished. */ - p->pos = p->count; - } return 0; } -int cbm_parse(const cbm_token_t *tokens, int token_count, // NOLINT(misc-no-recursion) - cbm_parse_result_t *out) { - memset(out, 0, sizeof(*out)); - parser_t p = {.tokens = tokens, .count = token_count, .pos = 0}; +/* A stage that failed without writing a message gets the stage's own. */ +static void parse_error_default(parser_t *p, const char *msg) { + if (!p->error[0]) { + snprintf(p->error, sizeof(p->error), "%s", msg); + } +} + +/* One MATCH ... RETURN block: the whole query, or one branch of a UNION. On + * failure p->error holds the message and *out stays NULL. */ +static int parse_query_block(parser_t *p, cbm_query_t **out) { + *out = NULL; + /* Each block starts with a clean message, as it did when every UNION + * branch had a parser of its own. */ + p->error[0] = '\0'; /* Check for unsupported leading keywords */ - const char *unsup = unsupported_clause_error(peek(&p)->type); + const char *unsup = unsupported_clause_error(peek(p)->type); if (unsup) { - out->error = heap_strdup(unsup); + snprintf(p->error, sizeof(p->error), "%s", unsup); return CBM_NOT_FOUND; } cbm_query_t *q = calloc(CBM_ALLOC_ONE, sizeof(cbm_query_t)); - if (check(&p, TOK_UNWIND)) { - parse_unwind_clause(&p, q); + if (check(p, TOK_UNWIND)) { + parse_unwind_clause(p, q); } bool first_optional = false; - if (check(&p, TOK_OPTIONAL)) { - advance(&p); + if (check(p, TOK_OPTIONAL)) { + advance(p); first_optional = true; } - if (!expect(&p, TOK_MATCH)) { - out->error = heap_strdup(p.error[0] ? p.error : "expected MATCH"); + if (!expect(p, TOK_MATCH)) { + parse_error_default(p, "expected MATCH"); cbm_query_free(q); return CBM_NOT_FOUND; } @@ -2058,32 +2066,66 @@ int cbm_parse(const cbm_token_t *tokens, int token_count, // NOLINT(misc-no-recu q->patterns = malloc(pat_cap * sizeof(cbm_pattern_t)); q->pattern_optional = malloc(pat_cap * sizeof(bool)); - if (parse_match_pattern(&p, &q->patterns[0]) < 0) { - out->error = heap_strdup(p.error[0] ? p.error : "failed to parse pattern"); + if (parse_match_pattern(p, &q->patterns[0]) < 0) { + parse_error_default(p, "failed to parse pattern"); cbm_query_free(q); return CBM_NOT_FOUND; } q->pattern_optional[0] = first_optional; q->pattern_count = SKIP_ONE; - if (parse_match_chain(&p, q, &pat_cap) < 0) { - out->error = heap_strdup(p.error[0] ? p.error : "failed to parse additional pattern"); + if (parse_match_chain(p, q, &pat_cap) < 0) { + parse_error_default(p, "failed to parse additional pattern"); cbm_query_free(q); return CBM_NOT_FOUND; } - if (parse_where(&p, &q->where) < 0) { - out->error = heap_strdup(p.error[0] ? p.error : "failed to parse WHERE"); + if (parse_where(p, &q->where) < 0) { + parse_error_default(p, "failed to parse WHERE"); cbm_query_free(q); return CBM_NOT_FOUND; } - if (parse_post_where(&p, q, &pat_cap) < 0) { - out->error = heap_strdup(p.error[0] ? p.error : "failed to parse query"); + if (parse_post_where(p, q, &pat_cap) < 0) { + parse_error_default(p, "failed to parse query"); cbm_query_free(q); return CBM_NOT_FOUND; } + *out = q; + return 0; +} + +int cbm_parse(const cbm_token_t *tokens, int token_count, cbm_parse_result_t *out) { + memset(out, 0, sizeof(*out)); + parser_t p = {.tokens = tokens, .count = token_count, .pos = 0}; + + /* The branches of a UNION are read one after another by this one parser + * and linked as they come, so the number of branches is a loop count, not + * a recursion depth, and one depth counter covers the whole query. */ + cbm_query_t *q = NULL; + cbm_query_t *tail = NULL; + for (;;) { + cbm_query_t *block = NULL; + if (parse_query_block(&p, &block) < 0) { + out->error = heap_strdup(p.error); + cbm_query_free(q); + return CBM_NOT_FOUND; + } + if (tail) { + tail->union_next = block; + } else { + q = block; + } + tail = block; + /* UNION [ALL]: the ALL flag sits on the branch before the keyword. */ + if (!check(&p, TOK_UNION)) { + break; + } + advance(&p); + tail->union_all = match(&p, TOK_ALL); + } + /* Every token must be consumed. The grammar accepts at most one WITH and * treats RETURN as optional, so a query with a second WITH stage — or any * typo after RETURN — used to stop parsing there and succeed anyway. The @@ -2732,24 +2774,40 @@ static bool eval_condition(const cbm_condition_t *c, binding_t *b) { return c->negated ? !result : result; } -/* Recursive expression tree evaluator */ +/* Expression tree evaluator. Recurses into NOT and into the left operand of a + * binary node; a chain of one operator is linked along the right child (see + * expr_chain_link) and is walked here in a loop. */ static bool eval_expr(const cbm_expr_t *e, binding_t *b) { // NOLINT(misc-no-recursion) if (!e) { return true; } - switch (e->type) { - case EXPR_CONDITION: + if (e->type == EXPR_CONDITION) { return eval_condition(&e->cond, b); - case EXPR_AND: - return (eval_expr(e->left, b) && eval_expr(e->right, b)) != 0; - case EXPR_OR: - return (eval_expr(e->left, b) || eval_expr(e->right, b)) != 0; - case EXPR_NOT: + } + if (e->type == EXPR_NOT) { return (!eval_expr(e->left, b)) != 0; - case EXPR_XOR: - return eval_expr(e->left, b) != eval_expr(e->right, b); } - return true; + /* AND / OR / XOR. The nested form `a OP (b OP (c ...))` evaluates a, then + * b, then c, and AND/OR stop at the first operand that decides the result; + * the loop does exactly that, one link per iteration. A right child that + * is not a link of the same operator is the chain's last operand. */ + const cbm_expr_type_t type = e->type; + bool acc = eval_expr(e->left, b); + const cbm_expr_t *rest = e->right; + for (;;) { + if ((type == EXPR_AND && !acc) || (type == EXPR_OR && acc)) { + return acc; + } + bool linked = rest && rest->type == type; + bool v = eval_expr(linked ? rest->left : rest, b); + /* AND reaches here only with acc true, OR only with acc false, so the + * combined value is v itself; XOR keeps the running parity. */ + acc = (type == EXPR_XOR) ? (acc != v) : v; + if (!linked) { + return acc; + } + rest = rest->right; + } } /* Evaluate WHERE clause — uses expression tree if available, falls back to legacy */ @@ -5180,19 +5238,27 @@ static int cypher_cond_selectivity(const cbm_condition_t *c, const char *var) { return (strcmp(c->property, "name") == 0 || strcmp(c->property, "qualified_name") == 0) ? 3 : 2; } -static int cypher_expr_selectivity(const cbm_expr_t *e, const char *var) { - if (!e) { - return 0; - } - if (e->type == EXPR_CONDITION) { - return cypher_cond_selectivity(&e->cond, var); - } - if (e->type == EXPR_AND) { +/* Best equality on `var` reachable through AND alone. An AND chain is linked + * along the right child (expr_chain_link), so the links are walked in a loop + * and only each link's left operand recurses. */ +static int cypher_expr_selectivity(const cbm_expr_t *e, // NOLINT(misc-no-recursion) + const char *var) { + int best = 0; + while (e) { + if (e->type == EXPR_CONDITION) { + int s = cypher_cond_selectivity(&e->cond, var); + return s > best ? s : best; + } + if (e->type != EXPR_AND) { + return best; /* OR / NOT / XOR: no single equality to seed from */ + } int l = cypher_expr_selectivity(e->left, var); - int r = cypher_expr_selectivity(e->right, var); - return l > r ? l : r; + if (l > best) { + best = l; + } + e = e->right; } - return 0; /* OR / NOT / XOR: no single equality to seed from */ + return best; } static int cypher_node_selectivity(const cbm_node_pattern_t *n, const cbm_where_clause_t *w) { diff --git a/src/foundation/recursion_whitelist.h b/src/foundation/recursion_whitelist.h index c3e2b5c8c6..642f44074e 100644 --- a/src/foundation/recursion_whitelist.h +++ b/src/foundation/recursion_whitelist.h @@ -4,12 +4,14 @@ * These functions use bounded recursion where iterative conversion would * add complexity with no practical benefit: * - * Cypher recursive descent parser (bounded by query nesting depth ~5): + * Cypher recursive descent parser (bounded by CYPHER_MAX_PARSE_DEPTH; an + * operator chain is linked, not nested, and UNION branches parse in a loop): * - parse_or_expr, parse_xor_expr, parse_and_expr, parse_not_expr - * - parse_atom_expr, parse_post_where, cbm_parse + * - parse_atom_expr * - * Cypher expression evaluator (bounded by WHERE clause depth ~5): - * - eval_expr + * Cypher expression walkers (bounded by the same parse depth; each follows + * an operator chain in a loop and recurses only into the operands): + * - eval_expr, cypher_expr_selectivity * * Glob pattern matcher (bounded by pattern nesting ~3): * - glob_match, glob_match_star, glob_match_doublestar @@ -27,7 +29,7 @@ */ #define CBM_RECURSION_WHITELIST \ "parse_or_expr", "parse_xor_expr", "parse_and_expr", "parse_not_expr", "parse_atom_expr", \ - "parse_post_where", "cbm_parse", "eval_expr", "glob_match", "glob_match_star", \ + "eval_expr", "cypher_expr_selectivity", "glob_match", "glob_match_star", \ "glob_match_doublestar", "glob_match_doublestar_slash", "glob_match_doublestar_any", \ "parse_bool_expr", "parse_bool_atom", "r_collect_imports", \ "find_first_descendant_by_kind", "find_first_descendant_of" diff --git a/tests/test_cypher.c b/tests/test_cypher.c index 75d3a62624..9ad58ba7ed 100644 --- a/tests/test_cypher.c +++ b/tests/test_cypher.c @@ -5010,7 +5010,407 @@ TEST(cypher_exec_max_rows_above_ceiling_is_clamped) { ASSERT_EQ(rc, 0); ASSERT_EQ(r.row_count, 2); cbm_cypher_result_free(&r); + cbm_store_close(s); + PASS(); +} + +/* ══════════════════════════════════════════════════════════════════ + * LONG OPERATOR CHAINS AND MANY UNION BRANCHES + * + * A WHERE chain `a OR b OR c ...` and a UNION chain are bounded only by the + * message size, and an agent that lists a few thousand names in one query is + * ordinary input. The parser used to fold a chain into a tree as deep as the + * chain is long and to parse every UNION branch through a nested call, so each + * walker (evaluation, seed selectivity, parsing) recursed once per operand or + * branch. The daemon runs a query on a worker thread with a 256 KiB stack + * (daemon/runtime.c RUNTIME_WORKER_STACK_SIZE), where a few thousand frames + * already overflow. These tests run their query on a thread of that size, + * inside a forked child on POSIX, so an overflow is reported as the signal + * that killed the child instead of ending the test runner. + * ══════════════════════════════════════════════════════════════════ */ + +enum { CYPHER_WORKER_STACK_BYTES = 256 * 1024 }; /* = RUNTIME_WORKER_STACK_SIZE */ + +typedef struct { + int (*check)(void *); + void *arg; + int result; +} worker_stack_run_t; + +static void *worker_stack_thread(void *opaque) { + worker_stack_run_t *run = opaque; + run->result = run->check(run->arg); + return NULL; +} + +/* Runs check(arg) on a thread with the worker stack size; returns its result. */ +static int run_on_worker_stack_thread(worker_stack_run_t *run) { + cbm_thread_t t; + if (cbm_thread_create(&t, CYPHER_WORKER_STACK_BYTES, worker_stack_thread, run) != 0) { + FAIL("could not create the worker-stack thread"); + } + (void)cbm_thread_join(&t); + return run->result; +} + +/* Runs check(arg) the way the daemon runs a query. POSIX: on the worker-sized + * thread of a forked child, whose exit status is the check's result; a killing + * signal is reported as such. Windows has no fork, and CreateThread's size + * argument sets the committed stack while the reserve stays the image default, + * so the thread runs in-process there. */ +static int run_on_worker_stack(int (*check)(void *), void *arg) { + worker_stack_run_t run = {.check = check, .arg = arg, .result = 1}; +#ifdef _WIN32 + return run_on_worker_stack_thread(&run); +#else + fflush(NULL); + pid_t pid = fork(); + if (pid == 0) { + int code = run_on_worker_stack_thread(&run); + fflush(NULL); + _exit(code == 0 ? 0 : 1); + } + ASSERT_TRUE(pid > 0); + int status = 0; + (void)waitpid(pid, &status, 0); + if (WIFSIGNALED(status)) { + char m[96]; + snprintf(m, sizeof(m), "query thread killed by signal %d on a %d KiB stack", + WTERMSIG(status), CYPHER_WORKER_STACK_BYTES / 1024); + FAIL(m); + } + ASSERT_TRUE(WIFEXITED(status)); + ASSERT_EQ(WEXITSTATUS(status), 0); + return 0; +#endif +} + +/* The fixture graph of setup_cypher_store, restated here so the expected rows + * are computed by the test and not read back from the engine. */ +static const char *const cypher_fixture_functions[] = {"HandleOrder", "ValidateOrder", + "SubmitOrder", "LogError"}; +enum { CYPHER_FIXTURE_FUNCTION_COUNT = 4 }; +static const char *const cypher_fixture_calls[][2] = {{"HandleOrder", "ValidateOrder"}, + {"ValidateOrder", "SubmitOrder"}, + {"HandleOrder", "LogError"}}; +enum { CYPHER_FIXTURE_CALL_COUNT = 3 }; +static const char *const cypher_fixture_nodes[] = {"HandleOrder", "ValidateOrder", "SubmitOrder", + "main", "LogError"}; +enum { CYPHER_FIXTURE_NODE_COUNT = 5 }; + +enum { CYPHER_CHAIN_OPERANDS = 50000 }; + +/* Operand i of a generated chain compares f.name with this value. Two fixture + * names sit deep in the chain (the middle and the very last operand); every + * other operand names a function that does not exist. */ +static const char *chain_operand_value(int i, char *buf, size_t n) { + if (i == CYPHER_CHAIN_OPERANDS / 2) { + return "ValidateOrder"; + } + if (i == CYPHER_CHAIN_OPERANDS - 1) { + return "LogError"; + } + snprintf(buf, n, "no_such_function_%d", i); + return buf; +} + +/* The test's own reading of the chain: does any operand name `name`? */ +static bool chain_names(const char *name) { + for (int i = 0; i < CYPHER_CHAIN_OPERANDS; i++) { + char buf[48]; + if (strcmp(chain_operand_value(i, buf, sizeof(buf)), name) == 0) { + return true; + } + } + return false; +} + +/* head + "f.name \"v0\"" + join + "f.name \"v1\"" + ... + tail */ +static char *chain_query(const char *head, const char *op, const char *join, const char *tail) { + size_t cap = 256 + (size_t)CYPHER_CHAIN_OPERANDS * 64; + char *q = malloc(cap); + if (!q) { + return NULL; + } + size_t len = (size_t)snprintf(q, cap, "%s", head); + for (int i = 0; i < CYPHER_CHAIN_OPERANDS && len < cap; i++) { + char buf[48]; + const char *v = chain_operand_value(i, buf, sizeof(buf)); + len += (size_t)snprintf(q + len, cap - len, "%sf.name %s \"%s\"", i ? join : "", op, v); + } + if (len >= cap) { + free(q); + return NULL; + } + snprintf(q + len, cap - len, "%s", tail); + return q; +} +typedef struct { + const char *head; + const char *op; + const char *join; + const char *tail; + bool admits_named; /* true: a row passes when the chain names f; false: when it does not */ + int expected_rows; +} chain_case_t; + +/* Runs the chain query and checks every row against the test's own reading of + * the chain, then checks that the engine still answers afterwards. */ +static int chain_check(void *arg) { + const chain_case_t *c = arg; + char *query = chain_query(c->head, c->op, c->join, c->tail); + ASSERT_NOT_NULL(query); + + cbm_store_t *s = setup_cypher_store(); + cbm_cypher_result_t r = {0}; + int rc = cbm_cypher_execute(s, query, "test", 0, &r); + free(query); + if (rc != 0) { + printf(" query error: %s\n", r.error ? r.error : "(none)"); + } + ASSERT_EQ(rc, 0); + ASSERT_EQ(r.row_count, c->expected_rows); + for (int i = 0; i < r.row_count; i++) { + ASSERT_EQ(chain_names(r.rows[i][0]), c->admits_named); + } + cbm_cypher_result_free(&r); + + rc = cbm_cypher_execute(s, "MATCH (f:Function) RETURN f.name", "test", 0, &r); + ASSERT_EQ(rc, 0); + ASSERT_EQ(r.row_count, CYPHER_FIXTURE_FUNCTION_COUNT); + cbm_cypher_result_free(&r); + cbm_store_close(s); + return 0; +} + +/* 50,000 operands joined by OR over a single node pattern: the rows are the + * Functions the chain names. The expected count comes from the fixture list + * and chain_names, not from the engine. */ +TEST(cypher_long_or_chain_evaluates) { + chain_case_t c = {.head = "MATCH (f:Function) WHERE ", + .op = "=", + .join = " OR ", + .tail = " RETURN f.name", + .admits_named = true, + .expected_rows = 0}; + for (int i = 0; i < CYPHER_FIXTURE_FUNCTION_COUNT; i++) { + c.expected_rows += chain_names(cypher_fixture_functions[i]) ? 1 : 0; + } + ASSERT_EQ(c.expected_rows, 2); /* the generator plants ValidateOrder and LogError */ + int rc = run_on_worker_stack(chain_check, &c); + if (rc != 0) { + return rc; + } + PASS(); +} + +/* 50,000 operands joined by AND over a single-hop pattern, so the planner's + * seed-selectivity walk sees the chain as well as the evaluator: the rows are + * the CALLS edges whose caller the chain does not name. */ +TEST(cypher_long_and_chain_evaluates) { + chain_case_t c = {.head = "MATCH (f:Function)-[:CALLS]->(g:Function) WHERE ", + .op = "<>", + .join = " AND ", + .tail = " RETURN f.name, g.name", + .admits_named = false, + .expected_rows = 0}; + for (int i = 0; i < CYPHER_FIXTURE_CALL_COUNT; i++) { + c.expected_rows += chain_names(cypher_fixture_calls[i][0]) ? 0 : 1; + } + ASSERT_EQ(c.expected_rows, 2); /* HandleOrder's two calls; ValidateOrder's is named */ + int rc = run_on_worker_stack(chain_check, &c); + if (rc != 0) { + return rc; + } + PASS(); +} + +static int cmp_cstr(const void *a, const void *b) { + return strcmp(*(const char *const *)a, *(const char *const *)b); +} + +/* Runs `query` and checks that its first column holds exactly the names in + * `expected`, given in strcmp order; the rows are sorted the same way so the + * engine's row order does not matter. */ +static int assert_name_set(cbm_store_t *s, const char *query, const char *const *expected, int n) { + enum { NAME_SET_MAX = 8 }; + ASSERT_TRUE(n <= NAME_SET_MAX); + cbm_cypher_result_t r = {0}; + int rc = cbm_cypher_execute(s, query, "test", 0, &r); + if (rc != 0) { + printf(" query error: %s\n", r.error ? r.error : "(none)"); + } + ASSERT_EQ(rc, 0); + ASSERT_EQ(r.row_count, n); + const char *names[NAME_SET_MAX]; + for (int i = 0; i < n; i++) { + names[i] = r.rows[i][0]; + } + qsort(names, (size_t)n, sizeof(names[0]), cmp_cstr); + for (int i = 0; i < n; i++) { + ASSERT_STR_EQ(names[i], expected[i]); + } + cbm_cypher_result_free(&r); + return 0; +} + +/* Every spelling in `forms` is one WHERE expression; all must yield `expected`. */ +static int where_forms_agree(cbm_store_t *s, const char *const *forms, int form_count, + const char *const *expected, int n) { + for (int f = 0; f < form_count; f++) { + char query[512]; + snprintf(query, sizeof(query), "MATCH (n) WHERE %s RETURN n.name", forms[f]); + int rc = assert_name_set(s, query, expected, n); + if (rc != 0) { + printf(" form: %s\n", forms[f]); + return rc; + } + } + return 0; +} + +/* A chain and the same expression written out with parentheses, in both + * associations, must agree row for row. The operands include a property the + * `main` node does not have: file_path reads as the empty string there, so + * `IS NULL` holds and `<>` holds, and a missing value takes part in AND, OR and + * XOR like any other operand instead of voiding the chain. */ +TEST(cypher_mixed_chain_matches_nested_form) { + cbm_store_t *s = setup_cypher_store(); + + static const char *const or_forms[] = { + "n.file_path = \"handler.go\" OR n.file_path IS NULL OR n.name = \"LogError\"", + "(n.file_path = \"handler.go\" OR n.file_path IS NULL) OR n.name = \"LogError\"", + "n.file_path = \"handler.go\" OR (n.file_path IS NULL OR n.name = \"LogError\")"}; + static const char *const or_rows[] = {"HandleOrder", "LogError", "main"}; + ASSERT_EQ(where_forms_agree(s, or_forms, 3, or_rows, 3), 0); + + static const char *const and_forms[] = { + "n.file_path <> \"handler.go\" AND n.file_path <> \"log.go\" AND n.name <> \"SubmitOrder\"", + "(n.file_path <> \"handler.go\" AND n.file_path <> \"log.go\") AND n.name <> " + "\"SubmitOrder\"", + "n.file_path <> \"handler.go\" AND (n.file_path <> \"log.go\" AND n.name <> " + "\"SubmitOrder\")"}; + static const char *const and_rows[] = {"ValidateOrder", "main"}; + ASSERT_EQ(where_forms_agree(s, and_forms, 3, and_rows, 2), 0); + + /* Parity: main is T^T^F^F, LogError F^F^T^F, HandleOrder F^F^F^T. */ + static const char *const xor_forms[] = { + "n.name = \"main\" XOR n.file_path IS NULL XOR n.name = \"LogError\" XOR " + "n.file_path = \"handler.go\"", + "((n.name = \"main\" XOR n.file_path IS NULL) XOR n.name = \"LogError\") XOR " + "n.file_path = \"handler.go\"", + "n.name = \"main\" XOR (n.file_path IS NULL XOR (n.name = \"LogError\" XOR " + "n.file_path = \"handler.go\"))"}; + static const char *const xor_rows[] = {"HandleOrder", "LogError"}; + ASSERT_EQ(where_forms_agree(s, xor_forms, 3, xor_rows, 2), 0); + + /* Mixed precedence (NOT > AND > XOR > OR) with a NOT over a missing value: + * main by name, LogError by (F AND T) XOR T, SubmitOrder by NOT (F). */ + static const char *const mixed_forms[] = { + "n.name = \"main\" OR n.file_path IS NULL AND n.name = \"LogError\" XOR " + "n.file_path = \"log.go\" OR NOT n.file_path <> \"submit.go\"", + "n.name = \"main\" OR ((n.file_path IS NULL AND n.name = \"LogError\") XOR " + "n.file_path = \"log.go\") OR (NOT (n.file_path <> \"submit.go\"))", + "(n.name = \"main\" OR ((n.file_path IS NULL AND n.name = \"LogError\") XOR " + "n.file_path = \"log.go\")) OR (NOT (n.file_path <> \"submit.go\"))"}; + static const char *const mixed_rows[] = {"LogError", "SubmitOrder", "main"}; + ASSERT_EQ(where_forms_agree(s, mixed_forms, 3, mixed_rows, 3), 0); + + cbm_store_close(s); + PASS(); +} + +/* Branch i selects fixture node i % 5 by name; `join` is " UNION " or + * " UNION ALL ". */ +static char *union_query(int branches, const char *join) { + size_t cap = 64 + (size_t)branches * 80; + char *q = malloc(cap); + if (!q) { + return NULL; + } + size_t len = 0; + for (int i = 0; i < branches && len < cap; i++) { + len += + (size_t)snprintf(q + len, cap - len, "%sMATCH (n) WHERE n.name = \"%s\" RETURN n.name", + i ? join : "", cypher_fixture_nodes[i % CYPHER_FIXTURE_NODE_COUNT]); + } + if (len >= cap) { + free(q); + return NULL; + } + return q; +} + +enum { CYPHER_UNION_MANY_BRANCHES = 20000 }; + +/* UNION ALL keeps every branch's row in branch order. */ +static int union_many_check(void *arg) { + (void)arg; + char *query = union_query(CYPHER_UNION_MANY_BRANCHES, " UNION ALL "); + ASSERT_NOT_NULL(query); + + cbm_store_t *s = setup_cypher_store(); + cbm_cypher_result_t r = {0}; + int rc = cbm_cypher_execute(s, query, "test", 0, &r); + free(query); + if (rc != 0) { + printf(" query error: %s\n", r.error ? r.error : "(none)"); + } + ASSERT_EQ(rc, 0); + ASSERT_EQ(r.row_count, CYPHER_UNION_MANY_BRANCHES); + for (int i = 0; i < r.row_count; i++) { + ASSERT_STR_EQ(r.rows[i][0], cypher_fixture_nodes[i % CYPHER_FIXTURE_NODE_COUNT]); + } + cbm_cypher_result_free(&r); + cbm_store_close(s); + return 0; +} + +/* 20,000 UNION ALL branches parse, run and free on the worker stack, and + * every branch contributes its row. */ +TEST(cypher_many_union_branches) { + int rc = run_on_worker_stack(union_many_check, NULL); + if (rc != 0) { + return rc; + } + PASS(); +} + +/* 50 branches: UNION ALL yields one row per branch in branch order, UNION + * keeps the first row of each name, and a UNION with nothing after it is still + * a clean parse error. */ +TEST(cypher_fifty_union_branches) { + enum { BRANCHES = 50 }; + cbm_store_t *s = setup_cypher_store(); + char *all = union_query(BRANCHES, " UNION ALL "); + char *distinct = union_query(BRANCHES, " UNION "); + ASSERT_NOT_NULL(all); + ASSERT_NOT_NULL(distinct); + + cbm_cypher_result_t r = {0}; + int rc = cbm_cypher_execute(s, all, "test", 0, &r); + ASSERT_EQ(rc, 0); + ASSERT_EQ(r.row_count, BRANCHES); + for (int i = 0; i < r.row_count; i++) { + ASSERT_STR_EQ(r.rows[i][0], cypher_fixture_nodes[i % CYPHER_FIXTURE_NODE_COUNT]); + } + cbm_cypher_result_free(&r); + + rc = cbm_cypher_execute(s, distinct, "test", 0, &r); + ASSERT_EQ(rc, 0); + ASSERT_EQ(r.row_count, CYPHER_FIXTURE_NODE_COUNT); + for (int i = 0; i < r.row_count; i++) { + ASSERT_STR_EQ(r.rows[i][0], cypher_fixture_nodes[i]); + } + cbm_cypher_result_free(&r); + free(all); + free(distinct); + + rc = cbm_cypher_execute(s, "MATCH (n) RETURN n.name UNION", "test", 0, &r); + ASSERT_EQ(rc, -1); + ASSERT_NOT_NULL(r.error); + cbm_cypher_result_free(&r); cbm_store_close(s); PASS(); } @@ -5245,4 +5645,10 @@ SUITE(cypher) { RUN_TEST(cypher_exec_prop_string_with_escaped_quote); RUN_TEST(cypher_single_hop_seeds_from_selective_far_node); RUN_TEST(cypher_exec_max_rows_above_ceiling_is_clamped); + /* Long operator chains and many UNION branches */ + RUN_TEST(cypher_long_or_chain_evaluates); + RUN_TEST(cypher_long_and_chain_evaluates); + RUN_TEST(cypher_mixed_chain_matches_nested_form); + RUN_TEST(cypher_many_union_branches); + RUN_TEST(cypher_fifty_union_branches); } From c4d738b830c178f2c138654e2cfc605ec0a174a5 Mon Sep 17 00:00:00 2001 From: Martin Vogel Date: Sat, 3 Oct 2026 17:56:00 +0200 Subject: [PATCH 2/2] test(cypher): isolate UNION stack checks from elapsed time Use a thread-local test clock for the large UNION structural regression so sanitizer overhead cannot consume its execution deadline. Preserve all row, ordering, and constrained-stack checks. Add an exact positive budget expiry case alongside the existing deadline tests. Request the fixture's exact 20000-row budget to avoid reserving the default 100000 binding slots for every branch, and assert no truncation. Signed-off-by: Martin Vogel --- src/cypher/cypher.c | 21 +++++++++++++++++++-- src/cypher/cypher.h | 6 ++++++ tests/test_cypher.c | 39 ++++++++++++++++++++++++++++++++++++++- 3 files changed, 63 insertions(+), 3 deletions(-) diff --git a/src/cypher/cypher.c b/src/cypher/cypher.c index 276268e3a9..42cee6df52 100644 --- a/src/cypher/cypher.c +++ b/src/cypher/cypher.c @@ -3032,11 +3032,28 @@ static _Thread_local bool g_cypher_timed_out = false; static _Thread_local bool g_cypher_truncated = false; static _Thread_local int64_t g_cypher_deadline_override_ms = -1; /* test hook; <0 = default */ +#ifdef CBM_ENABLE_TEST_SEAMS +static _Thread_local cbm_cypher_test_clock_fn g_cypher_deadline_clock = NULL; + +void cbm_cypher_test_set_deadline_clock(cbm_cypher_test_clock_fn clock_fn) { + g_cypher_deadline_clock = clock_fn; +} +#endif + +static uint64_t cypher_deadline_now(void) { +#ifdef CBM_ENABLE_TEST_SEAMS + if (g_cypher_deadline_clock) { + return g_cypher_deadline_clock(); + } +#endif + return cbm_now_ms(); +} + static void cypher_deadline_arm(void) { g_cypher_timed_out = false; int64_t budget = g_cypher_deadline_override_ms >= 0 ? g_cypher_deadline_override_ms : CYPHER_DEADLINE_BUDGET_MS; - g_cypher_deadline_ms = cbm_now_ms() + (uint64_t)budget; + g_cypher_deadline_ms = cypher_deadline_now() + (uint64_t)budget; } /* True once the query has run past its wall-clock budget. Sticky: after the @@ -3048,7 +3065,7 @@ static bool cypher_deadline_exceeded(void) { if (g_cypher_deadline_ms == 0) { return false; } - if (cbm_now_ms() >= g_cypher_deadline_ms) { + if (cypher_deadline_now() >= g_cypher_deadline_ms) { g_cypher_timed_out = true; return true; } diff --git a/src/cypher/cypher.h b/src/cypher/cypher.h index e9bd5f74c2..dda0ecbe3d 100644 --- a/src/cypher/cypher.h +++ b/src/cypher/cypher.h @@ -355,6 +355,12 @@ void cbm_query_free(cbm_query_t *q); * check; a negative value restores the default budget. */ void cbm_cypher_test_set_deadline_ms(int64_t budget_ms); +#ifdef CBM_ENABLE_TEST_SEAMS +/* Override only the deadline clock on this thread; NULL restores real time. */ +typedef uint64_t (*cbm_cypher_test_clock_fn)(void); +void cbm_cypher_test_set_deadline_clock(cbm_cypher_test_clock_fn clock_fn); +#endif + /* Worst-case binding slot count for a node cross-join. Computes the count in * size_t and rejects any that would not fit the int binding counter or would * overflow the size_t byte size; returns 0 and writes *out_n on success, diff --git a/tests/test_cypher.c b/tests/test_cypher.c index 9ad58ba7ed..b9b49b7179 100644 --- a/tests/test_cypher.c +++ b/tests/test_cypher.c @@ -4974,6 +4974,35 @@ TEST(cypher_exec_deadline_aborts_runaway_query_issue601) { PASS(); } +static uint64_t cypher_frozen_deadline_clock(void) { + return 1000; +} + +static _Thread_local unsigned cypher_deadline_clock_reads = 0; + +static uint64_t cypher_expiring_deadline_clock(void) { + /* Default budget is 30 seconds; expire exactly at the first checkpoint. */ + return cypher_deadline_clock_reads++ == 0 ? 1000 : 31000; +} + +TEST(cypher_exec_deadline_expires_at_positive_budget_boundary) { + cbm_store_t *s = setup_cypher_store(); + cbm_cypher_result_t r = {0}; + cypher_deadline_clock_reads = 0; + cbm_cypher_test_set_deadline_clock(cypher_expiring_deadline_clock); + int rc = cbm_cypher_execute(s, "MATCH (n) RETURN n.name", "test", 0, &r); + cbm_cypher_test_set_deadline_clock(NULL); + + ASSERT_TRUE(cypher_deadline_clock_reads >= 2); + ASSERT_TRUE(rc != 0); + ASSERT_NOT_NULL(r.error); + ASSERT_NOT_NULL(strstr(r.error, "time limit")); + ASSERT_EQ(r.row_count, 0); + cbm_cypher_result_free(&r); + cbm_store_close(s); + PASS(); +} + /* #601 companion: the default (ample) budget must NOT false-positive on a * normal small query — it still returns its rows. */ TEST(cypher_exec_deadline_allows_normal_query_issue601) { @@ -5352,13 +5381,20 @@ static int union_many_check(void *arg) { cbm_store_t *s = setup_cypher_store(); cbm_cypher_result_t r = {0}; - int rc = cbm_cypher_execute(s, query, "test", 0, &r); + /* This exercises stack depth and every result, independently of how + * slowly instrumentation runs. Dedicated deadline tests retain expiry. */ + cbm_cypher_test_set_deadline_clock(cypher_frozen_deadline_clock); + /* Request exactly the output this fixture needs: the default 100k ceiling + * also reserves 100k binding slots separately for every UNION branch. */ + int rc = cbm_cypher_execute(s, query, "test", CYPHER_UNION_MANY_BRANCHES, &r); + cbm_cypher_test_set_deadline_clock(NULL); free(query); if (rc != 0) { printf(" query error: %s\n", r.error ? r.error : "(none)"); } ASSERT_EQ(rc, 0); ASSERT_EQ(r.row_count, CYPHER_UNION_MANY_BRANCHES); + ASSERT_TRUE(!r.truncated); for (int i = 0; i < r.row_count; i++) { ASSERT_STR_EQ(r.rows[i][0], cypher_fixture_nodes[i % CYPHER_FIXTURE_NODE_COUNT]); } @@ -5457,6 +5493,7 @@ SUITE(cypher) { RUN_TEST(cypher_parse_error); /* Execution */ RUN_TEST(cypher_exec_deadline_aborts_runaway_query_issue601); + RUN_TEST(cypher_exec_deadline_expires_at_positive_budget_boundary); RUN_TEST(cypher_exec_deadline_allows_normal_query_issue601); RUN_TEST(cypher_deep_nesting_rejected_not_crash); RUN_TEST(cypher_exec_match_all_functions);