Skip to content

fix(cypher): bound UNWIND literal-list buffer to prevent stack overflow - #1173

Merged
DeusData merged 1 commit into
DeusData:mainfrom
SEPURI-SAI-KRISHNA:fix/cypher-unwind-stack-overflow
Jul 30, 2026
Merged

fix(cypher): bound UNWIND literal-list buffer to prevent stack overflow#1173
DeusData merged 1 commit into
DeusData:mainfrom
SEPURI-SAI-KRISHNA:fix/cypher-unwind-stack-overflow

Conversation

@SEPURI-SAI-KRISHNA

Copy link
Copy Markdown
Contributor

What does this PR do?

Fixes a stack out-of-bounds write (CWE-787) in the Cypher UNWIND literal-list
parser (parse_unwind_clause, src/cypher/cypher.c).

When assembling a literal list into the fixed 2 KB stack buffer
char buf[CBM_SZ_2K], the code advances the write cursor by the return value of
snprintf:

blen += snprintf(buf + blen, sizeof(buf) - blen, "\"%s\"", peek(p)->text);

snprintf returns the number of bytes it would have written, so a single
oversized element pushes blen past sizeof(buf). The trailing writes are then
unguarded:

buf[blen++] = ']';
buf[blen] = '\0';

and on multi-element lists the next iteration's sizeof(buf) - blen underflows
to a huge size_t, letting snprintf write far past the buffer.

The query string is agent-controlled through the MCP query tool, and Cypher
string literals lex up to CBM_SZ_4K - 1 (4095) chars with no length cap before
parsing, so UNWIND ["<~3000 chars>"] AS x MATCH (n) RETURN n reaches the
overflow.

Reproduced first (UBSan, before the fix):

src/cypher/cypher.c:1855:12: runtime error: index 3002 out of bounds for type 'char [2048]'
src/cypher/cypher.c:1855:21: runtime error: store to address 0x... with insufficient space for an object of type 'char'
src/cypher/cypher.c:1856:12: runtime error: index 3003 out of bounds for type 'char [2048]'

The fix clamps blen after every snprintf and guards the raw
buf[blen++] stores — the same pattern already used by the sibling function
format_collect_list(), which builds an identical JSON array into a
CBM_SZ_2K buffer and was already hardened. After the fix all 161 cypher tests
pass with zero sanitizer errors.

Two regression tests are added (both fail under UBSan without the fix):

  • cypher_parse_unwind_oversized_literal_no_overflow — single oversized element
  • cypher_parse_unwind_many_elements_no_overflow — many elements accumulating past 2 KB

Checklist

  • Every commit is signed off (git commit -s) — required, CI rejects
    unsigned commits (DCO, see CONTRIBUTING.md)
  • Tests pass locally (make -f Makefile.cbm test)
  • Lint passes (make -f Makefile.cbm lint-ci)
  • New behavior is covered by a test (reproduce-first for bug fixes)

…ck overflow

Signed-off-by: SEPURI-SAI-KRISHNA <saik20533@gmail.com>
@SEPURI-SAI-KRISHNA
SEPURI-SAI-KRISHNA force-pushed the fix/cypher-unwind-stack-overflow branch from ffc88cc to d0457f2 Compare July 19, 2026 06:27
@DeusData DeusData added bug Something isn't working stability/performance Server crashes, OOM, hangs, high CPU/memory cypher Cypher query language parser/executor bugs security Security vulnerabilities, hardening labels Jul 19, 2026
@DeusData DeusData added this to the 0.9.1-rc milestone Jul 19, 2026
@DeusData DeusData added the priority/high Needs near-term maintainer attention; high-impact bug, regression, safety issue, or release blocker. label Jul 19, 2026
@DeusData

Copy link
Copy Markdown
Owner

Thanks for the reproduce-first memory-safety fix. Triage is complete: this is a high-priority 0.9.1-rc Cypher stability/security item. Exact-head review at d0457f2ba95a521fb53e433394aaa7a511cec3ee found the change scoped to the parser and sanitizer regressions, with no dependency, workflow, installer, network, or vendored-code changes. It remains in the maintainer review queue; this triage pass does not merge PRs.

@DeusData

Copy link
Copy Markdown
Owner

Thanks for this, and sorry for the slow first response. Queued for review.

MERGEABLE with all 20 checks green — nothing blocking on your side.

Useful context you may not have: #1192 reports that UNWIND is parsed but never executed at all. I confirmed that on mainq->unwind_expr and q->unwind_alias are assigned during parsing and thereafter only freed, with no executor reads. Your PR bounds the literal-list buffer on the parse path, which is a genuine memory-safety fix and stands on its own merits regardless of how the executor gap is eventually closed. Just flagging the adjacency so neither of us is surprised.

@DeusData

Copy link
Copy Markdown
Owner

Thank you — this is a real, agent-reachable stack overflow and the fix is the right shape.

Verified independently rather than taking the description on trust: the parser advances blen by snprintf's would-be length into a 2 KB buffer while string literals lex up to 4095 bytes, then writes the closing bracket unguarded — and with multiple elements sizeof(buf) - blen underflows into a huge size_t. Reachable through the MCP query tool, which makes it worth fixing carefully. I traced the corrected control flow: every write stays within cap - 1 and NUL-termination is preserved on each path. It matches the already-hardened format_collect_list, so the codebase now treats both list builders the same way.

One consequence worth stating out loud for whoever reads this later: an oversized literal is now silently truncated, so unwind_expr may lack its closing bracket. That is the safe behaviour and consistent with the sibling function; if we ever decide a parse error is better, that is a deliberate follow-up rather than something this PR got wrong.

Merging.

@DeusData
DeusData merged commit 2f51672 into DeusData:main Jul 30, 2026
20 checks passed
DeusData added a commit that referenced this pull request Jul 31, 2026
…return

CodeQL flagged two high-severity alerts in the previous commit: the help-list
builder accumulated snprintf's return value, which is the length it WOULD have
written rather than what it did. Once len passes cap, the next `cap - len`
underflows to a huge size_t and the following write lands outside the buffer.

That is the same class the Cypher UNWIND list builder was just fixed for in
#1173, where blen advanced past a 2KB buffer the same way.

The capacity computed here is in fact sufficient for the current registry, so
nothing overflows today -- which makes this the more dangerous shape, not the
safer one. The code was correct only by an argument made ten lines away in the
cap loop, so renaming a tool or changing the wrap rule would have turned it into
an out-of-bounds write with nothing in the function to notice.

help_append() returns the bytes ACTUALLY written, clamped to the space left, so
`len <= cap - 1` becomes a local invariant that holds no matter what the
registry or the wrap column later become. Also adds the <stdarg.h> the variadic
helper needs.

Verified: builds clean under -Werror; --help still renders all 15 tools in
registry order with the same wrapping; mcp + cli suites 437 passed, 0 failed;
make -f Makefile.cbm lint-ci clean.

Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working cypher Cypher query language parser/executor bugs priority/high Needs near-term maintainer attention; high-impact bug, regression, safety issue, or release blocker. security Security vulnerabilities, hardening stability/performance Server crashes, OOM, hangs, high CPU/memory

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants