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

[SEPURI-SAI-KRISHNA]

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)

[DeusData]

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]

@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.

[DeusData]

Thanks for the revision and for replacing the weak row-count assertion. I see current head 8763aa928b921a02a7d7b7d38054b1e79a86fa92. The single ceiling, defined truncation behavior, explicit unbound-leaf assertion, and removal of the unproven #627 association address the prior triage blockers. No further contributor action is requested now; this is back in exact-head maintainer review.

[DeusData]

Reviewed and approved in substance — I just cannot land it as-is, and the reason is our merge order rather than anything about your patch.

The bug is real and I verified the index arithmetic by hand: the allocation is bind_cap * 10 + 1 slots, the expansion helpers stop at bind_cap * 10, but the OPTIONAL fallback was unguarded — so after a saturating hub the first fallback lands in the spare slot and the second writes one past the end. Your guard is the minimal correct fix and matches how expand_from_bound_terminal already bounds its fallback. I also liked that the second test is an explicit anti-vacuity companion: it asserts a real leaf-fallback row and a genuine hub-expanded row, so it cannot pass by accidentally testing nothing.

I merged your #1176 a few minutes ago, and both PRs insert their new tests at the same point in tests/test_cypher.c — so this one is now conflicting. The production hunks are in completely separate regions, so the rebase should be mechanical: only the test-registration area needs reconciling.

Could you rebase onto current main? It goes in as soon as it applies cleanly.

Worth saying plainly: four PRs, four genuine memory-safety bugs, each with a test that actually fails without the fix. That is a real contribution to this codebase and I am glad to have them. One consequence to note for the record — once a hop saturates, subsequent OPTIONAL fallback rows are now silently dropped. That regime was undefined behaviour before, so this is strictly better; it just means results truncate rather than error on very dense graphs.

[SEPURI-SAI-KRISHNA]

Rebased onto current main, the conflict is resolved and the branch is now a clean fast-forward of main (git merge-base --is-ancestor upstream/main HEAD passes), so it applies cleanly.

As you noted, the production hunks never conflicted; the only overlap was the test-registration area. I kept all four tests, #1176's cypher_exec_optional_empty_label_no_overflow and cypher_cross_join_alloc_rejects_overflow (now on main) alongside this PR's cypher_exec_optional_rel_ceiling_truncates_no_overflow and cypher_exec_optional_rel_leaf_fallback_survives each with its own registration.

Verified locally: full cypher suite passes (166/166) under ASan+UBSan. Diff vs main is cypher.c +13/−2 (only the && new_count < max_new fallback guard + comments) and test_cypher.c +127 (just this PR's two tests, no duplication of #1176's).

Head is now de4aec9. Ready when you are, thanks for the quick reviews across the set.

[DeusData]

Excellent find, and an exemplary report. This is a real heap out-of-bounds write in the agent-reachable query engine — I verified it byte-for-byte on current main, not just against your branch.

The bug. origin/main:src/cypher/cypher.c:3237 allocates alloc_n = bind_cap*10 + 1. The expansion helpers stop at max_new = bind_cap*10, but the OPTIONAL-fallback append at cypher.c:3265-3270 writes new_bindings[new_count++] = nb with no ceiling check. One saturating hub plus two or more later no-match sources puts the second fallback write one slot past the allocation. You identified the asymmetry precisely, including why the spare +1 slot masks it until the second fallback after saturation.

Your reachability analysis matches the code exactly: execute_single sets bind_cap = max(scan_count, max_rows), so the trigger regime is "matched start nodes exceed the row limit" — forceable by an agent supplying a small limit to query_graph, or reached naturally on a large repo. That makes this attacker-influenced heap corruption, not merely a crash. Your sibling-precedent argument also checks out: expand_from_bound_terminal already guards its own fallback with the identical ceiling.

The fix is a pure guard with no buffer arithmetic touched. Post-fix the maximum index written is max_new - 1, inside the allocation with a slot spare, and rows skipped by the guard are still freed by the source cleanup loop — no leak on the rejected path.

Special credit for the companion test. Rejecting a bare row-count assertion as too weak, and instead pinning a specific leaf's unbound-target row alongside a real expanded hub row, is exactly the discipline we want — it would catch an overcorrected fix that dropped all fallbacks. And reproducing the overflow under ASan first is the right venue, since the sanitizer legs are what actually bind this.

This is your third overflow fix in this engine after #1173 and #1176. Genuinely appreciated — that engine is agent-reachable and these are the bugs that matter most there.

One design question before merge, and I want to be clear it is a question rather than a blocker.

Your guard converts the overflow into silently dropping OPTIONAL left-rows once a hop saturates. Under OPTIONAL MATCH semantics those are the worst rows to lose — a source with no match is precisely what OPTIONAL MATCH ... WHERE b IS NULL exists to find.

There is a lossless alternative: fallback rows are bounded at one per source, so sizing the hop buffer as max_new + bind_count and letting only the fallback use the extra region closes the overflow with zero dropped rows, for the cost of bind_cap extra binding_t slots per hop.

I am not asking you to redo it on that basis alone — the status quo is heap corruption, your version is a strict improvement, and ceiling-consistency with the sibling function is a defensible choice. But the trade is worth making deliberately rather than inheriting. Would you prefer the max_new + bind_count variant, or to keep this guard as the invariant backstop and treat the lossless sizing as a follow-up? Either is fine by me.

Also needed: a trivial rebase. The conflict looks like context drift in tests/test_cypher.c from your own merged #1173 and #1176 — tests added near the same suite-registration region — rather than anything semantic.

Tiny nit while you are in there: the second test's comment says bind_cap = 100000, but with max_rows 0 the code yields bind_cap = scan_count = 4. Cosmetic; nothing truncates either way.

[SEPURI-SAI-KRISHNA]

Went with the lossless variant, you're right that silently dropping the no-match rows is exactly backwards for OPTIONAL MATCH semantics. The hop buffer is now sized max_new + *bind_count (in size_t), and the fallback guard is gone. Since a source either feeds the expansion (≤ max_new total) or takes the fallback (≤ one per source, ≤ *bind_count) but never both, the two are additive and bounded by the allocation, no overflow, and no OPTIONAL row is ever dropped.

The reproducer still bites with the old sizing: reverting the allocation to + 1 faults at the fallback write (ASan: heap-buffer-overflow, WRITE of size 1624) from the saturated-hub test; the lossless sizing clears it. I renamed that test to ..._saturated_no_overflow since the hop no longer truncates, the max_rows LIMIT caps output instead.

Good catch on the bind_cap comment to, the value is right (max_rows 0 is defaulted to CYPHER_RESULT_CEILING in cbm_cypher_execute before bind_cap is computed, so it's 100000), but the defaulting was non-obvious; I've spelled that out in the comment.

Rebased onto current main (head c2a19b8); cypher suite is 166/166 green under ASan+UBSan.

[DeusData]

Re-reviewed in full, and this is exactly the lossless variant we hoped for. Approved — it goes in as-is. The verification below is independent of your description, because a sizing proof is worth checking rather than accepting.

The bound holds, and it holds more strongly than your argument claims. The two writers into new_bindings are the expansion helpers (process_edges at cypher.c:3085→3109, expand_var_length at 3144→3158) and the OPTIONAL fallback (3276–3281). Both expansion paths test *new_count < max_new immediately before every write, so expansion rows can only ever occupy indices below max_new. The fallback writes at most one row per source-loop iteration, and *bind_count is not mutated inside that loop — it is written at 3289, after it. So the worst-case fallback write lands at index max_new + (*bind_count − 1) = alloc_n − 1. The maximum index written is exactly the last valid slot, under any interleaving.

That last part is the nice property: the bound does not actually depend on your "a source feeds either the expansion or the fallback, never both" exclusivity. That exclusivity is also true — match_count resets per source at 3261 and increments in lockstep with emitted rows at 3109/3158, so match_count == 0 is exactly "this source emitted nothing" — but the allocation is safe even without it. Safety that does not rest on a semantic subtlety is worth more than safety that does.

Your saturated-hub arithmetic checks out too. 21 Function nodes give bind_cap = 21 and max_new = 210; the hub's 300 CALLS edges saturate that; the 20 leaf fallbacks then want indices 210–229, while the old bind_cap*10 + 1 = 211 allocation runs out at the second one. And the 1624-byte ASan write size matches a single binding_t assignment (16 node vars + 8 edge vars, cypher.c:2160–2168). The max_rows == 0 → CYPHER_RESULT_CEILING defaulting is correct as well: it happens in cbm_cypher_execute at 4774 before bind_cap is computed at 4708, so the comment now says the right thing.

The companion test earns its place. …_leaf_fallback_survives asserts a specific leaf row with b="" and a real expanded hub row, rather than just row_count > 0 — which matters, because the saturated test's output is LIMIT-capped at 5 and so could not by itself catch someone re-introducing a lossy guard later. One is the regression discriminator, the other is the semantics keeper. That is the right pair.

One observation for a follow-up, with no action needed here: the sibling expand_from_bound_terminal (cypher.c:4552) still uses the old +1 sizing with a guarded fallback. It is safe against overflow, but it is lossy under saturation in precisely the way you have just fixed — the same OPTIONAL-drop question, different door. Worth its own issue.

This is your third overflow fix in this engine after #1173 and #1176, and the quality has gone up each time — this one arrived with the proof already written. Thank you.

The merge itself is queued behind a maintainer-side gate on my end rather than anything to do with your branch, which is CLEAN with 28/28 green. Nothing further is needed from you.

[DeusData]

Merged as 7814e1bf9 — the maintainer-side hold I mentioned has cleared, so this is in.

Thank you again for taking the lossless route. Closing an attacker-influenced heap write is worth doing carefully, and you did it in the shape that gives up nothing: no OPTIONAL MATCH row is dropped, and the bound does not depend on a semantic invariant that a future refactor could quietly break.

The follow-up I mentioned stands on its own and is entirely optional — expand_from_bound_terminal (cypher.c:4552) still carries the old +1 sizing with a guarded fallback, so the bound-terminal path can still drop no-match rows under saturation. Same question, different door. If you would rather leave it, that is completely fine; I will open an issue either way so it does not get lost.

[SEPURI-SAI-KRISHNA]

Thanks for merging, and for the pointer. I'll take the expand_from_bound_terminal (cypher.c:4552) follow-up as a separate PR, same lossless shape: size the hop bind_count*10 + bind_count and drop the new_count < max_new guard on the OPTIONAL fallback so no-match rows survive saturation, keeping the expansion itself capped at the ceiling. Will open it against current main with a RED→GREEN regression test that shows a dropped no-match row.