Skip to content

fix(cypher): bound OPTIONAL fallback in expand_pattern_rels to a single ceiling#1177

Open
SEPURI-SAI-KRISHNA wants to merge 1 commit into
DeusData:mainfrom
SEPURI-SAI-KRISHNA:fix/cypher-expand-optional-overflow
Open

fix(cypher): bound OPTIONAL fallback in expand_pattern_rels to a single ceiling#1177
SEPURI-SAI-KRISHNA wants to merge 1 commit into
DeusData:mainfrom
SEPURI-SAI-KRISHNA:fix/cypher-expand-optional-overflow

Conversation

@SEPURI-SAI-KRISHNA

@SEPURI-SAI-KRISHNA SEPURI-SAI-KRISHNA commented Jul 19, 2026

Copy link
Copy Markdown

What does this PR do?

Fixes a heap buffer overflow (CWE-787) in expand_pattern_rels
(src/cypher/cypher.c) when expanding an OPTIONAL MATCH relationship pattern.
The query text is agent-controlled via the MCP query tool.

The bug

The per-hop output buffer was sized bind_cap * 10 + 1:

size_t alloc_n = (size_t)*bind_cap * (size_t)CYP_GROWTH_10 + SKIP_ONE;

The expansion helpers (expand_fixed_length / expand_var_length, via
process_edges) correctly stop at max_new = bind_cap * 10. But the OPTIONAL
fallback that keeps a row with the target left unbound was not bounded:

if (is_optional && match_count == 0) {
    ...
    new_bindings[new_count++] = nb;   // no ceiling check
}

The + 1 left room for a single fallback row after a saturated expansion.
When one source binding saturates the expansion to max_new and two or more
later sources take the OPTIONAL (no-match) path, the second fallback write runs
past the allocation.

Reachable whenever bind_cap tracks the scanned node count — i.e. the number of
matched start nodes exceeds the result limit — with e.g.:

MATCH (a:Function) OPTIONAL MATCH (a)-[:CALLS]->(b) RETURN a.name

That regime is hit on a repo with more start nodes than the supplied max_rows
(or, at the default limit, more than the 100000 result ceiling). A dense hub
(out-degree ≥ bind_cap * 10) saturates the buffer; the leaf nodes that follow
on the OPTIONAL path then overflow it.

The fix

Hold the OPTIONAL fallback to the same single global ceiling (max_new = bind_cap * 10) the expansion helpers already respect, instead of growing the
allocation. The buffer is sized for the ceiling, and every writer — expansion
and fallback alike — stops there:

int max_new = *bind_cap * CYP_GROWTH_10;
size_t alloc_n = (size_t)max_new + SKIP_ONE;
...
if (is_optional && match_count == 0 && new_count < max_new) { ... }

Behavior at the ceiling is now defined: the hop truncates at max_new
(bounded success, no overflow, no error), matching how the sibling
expand_from_bound_terminal already bounds its own fallback.

Reproduced first (ASan, before the fix):

==ERROR: AddressSanitizer: heap-buffer-overflow ... WRITE of size ...
    #0 expand_pattern_rels src/cypher/cypher.c   (OPTIONAL fallback write)
       allocated at src/cypher/cypher.c          (bind_cap*10 + 1 malloc)
    #5 test_cypher_exec_optional_rel_ceiling_truncates_no_overflow
SUMMARY: AddressSanitizer: heap-buffer-overflow ... in expand_pattern_rels

After the fix the full cypher suite passes with no sanitizer errors.

Tests

Two regression tests:

  • cypher_exec_optional_rel_ceiling_truncates_no_overflow — a hub that
    saturates the expansion followed by leaf functions on the OPTIONAL path;
    asserts the query still succeeds with a bounded result (defined truncation, no
    overflow).
  • cypher_exec_optional_rel_leaf_fallback_survives — a non-saturating graph;
    asserts a specific leaf's OPTIONAL row survives with its target unbound and
    that a real expanded hub row is also present (a row_count > 0 check would be
    too weak — it can pass on hub rows alone).

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)
  • New behavior is covered by a test (reproduce-first for bug fixes)

@SEPURI-SAI-KRISHNA
SEPURI-SAI-KRISHNA force-pushed the fix/cypher-expand-optional-overflow branch from 7891465 to a3adfbc Compare July 19, 2026 12:22
@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 isolating a genuine start-bound OPTIONAL expansion overflow. The current fix needs revision before further review:

  1. Keep OPTIONAL fallback rows inside one explicit global output/memory ceiling rather than growing the already-large allocation by up to one binding_t per source row.
  2. Define and test the truncation or error behavior when that ceiling is reached.
  3. Strengthen the regression to prove the expected leaf fallback rows survive; row_count > 0 can pass using only hub results.
  4. Remove the Crash when calling query_graph #627 claim unless its exact terminal-bound reproduction is shown RED then GREEN. This PR currently exercises the opposite, start-bound route.

This feedback is for reviewed head a3adfbc5529d542e25e8a64ab4e069ed3da17c2d.

@SEPURI-SAI-KRISHNA
SEPURI-SAI-KRISHNA force-pushed the fix/cypher-expand-optional-overflow branch from a3adfbc to fe548db Compare July 22, 2026 16:34
@SEPURI-SAI-KRISHNA SEPURI-SAI-KRISHNA changed the title fix(cypher): bound OPTIONAL fallback in expand_pattern_rels to prevent heap overflow (#627) fix(cypher): bound OPTIONAL fallback in expand_pattern_rels to prevent heap overflow Jul 22, 2026
@SEPURI-SAI-KRISHNA SEPURI-SAI-KRISHNA changed the title fix(cypher): bound OPTIONAL fallback in expand_pattern_rels to prevent heap overflow fix(cypher): bound OPTIONAL fallback in expand_pattern_rels to a single ceiling Jul 22, 2026
@SEPURI-SAI-KRISHNA

Copy link
Copy Markdown
Author

@DeusData

Thanks revised to address all four points:

  1. One explicit global ceiling. The OPTIONAL fallback is now held to the same max_new = bind_cap * 10 ceiling the expansion helpers already enforce (new_count < max_new), and the buffer is sized to that ceiling — no more growing the allocation by a binding_t per source row. This matches the sibling expand_from_bound_terminal, which already bounds its fallback the same way.

  2. Defined ceiling behavior. At the ceiling the hop truncates: bounded success, no overflow, no error. cypher_exec_optional_rel_ceiling_truncates_no_overflow asserts the saturating case returns rc == 0 with a bounded row count.

  3. Stronger regression. Replaced the row_count > 0 check with two tests. The truncation test above, plus cypher_exec_optional_rel_leaf_fallback_survives, which scans the result rows for a specific leaf appearing with an unbound b.name and for a real expanded hub row — so it can no longer pass on hub rows alone. Confirmed RED→GREEN: with the ceiling guard removed the truncation test aborts under ASan (heap-buffer-overflow, WRITE of size 1624 at cypher.c:3266); with it in place the full cypher suite is clean (161 passed).

  4. Crash when calling query_graph #627 claim removed from the title and description. This is the start-bound OPTIONAL expansion route, not the terminal-bound reproduction, so I've dropped the association rather than assert an unproven link.

…le ceiling

Signed-off-by: SEPURI-SAI-KRISHNA <saik20533@gmail.com>
@SEPURI-SAI-KRISHNA
SEPURI-SAI-KRISHNA force-pushed the fix/cypher-expand-optional-overflow branch from fe548db to 8763aa9 Compare July 22, 2026 16:57
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