fix(cypher,store): prevent crashes from buffer overflow, OOM, and NULL stmts#2
Merged
fix(cypher,store): prevent crashes from buffer overflow, OOM, and NULL stmts#2
Conversation
Review Summary by QodoFix crash safety issues: buffer overflow, OOM, and NULL statement dereference
WalkthroughsDescription• Prevent stack buffer overflow in Cypher lexer by bounds-checking string literals • Add NULL checks for malloc/calloc in Cypher parser to prevent OOM crashes • Add sqlite3_prepare_v2 return code validation to prevent NULL statement dereference • Add comprehensive test for large string literal handling Diagramflowchart LR
A["Cypher Lexer<br/>lex_string_literal"] -->|"Add max_blen<br/>bounds check"| B["Prevent Stack<br/>Overflow"]
C["Cypher Parser<br/>parse_props/rel_types<br/>parse_in_condition<br/>parse_case_expr"] -->|"Add malloc/calloc<br/>NULL checks"| D["Prevent OOM<br/>Crashes"]
E["SQLite Store<br/>cbm_store_get_schema<br/>collect_pkg_names"] -->|"Add prepare_v2<br/>return validation"| F["Prevent NULL<br/>Dereference"]
B --> G["Improved Crash Safety"]
D --> G
F --> G
File Changes1. src/cypher/cypher.c
|
Code Review by Qodo
1. Escaped quote truncated wrong
|
There was a problem hiding this comment.
Pull request overview
This PR hardens the Cypher lexer/parser and store schema queries to avoid crash conditions under oversized inputs, OOM, and SQLite statement preparation failures.
Changes:
- Add bounds checking to Cypher string literal lexing to prevent stack buffer overflow (with a regression test).
- Add allocation-failure guards in several Cypher parser routines.
- Add SQLite
sqlite3_prepare_v2return-code checks to avoid NULL statement dereferences.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
src/cypher/cypher.c |
Adds string-literal truncation guard and OOM checks in parser allocations. |
src/store/store.c |
Adds sqlite3_prepare_v2 error handling to prevent NULL sqlite3_stmt* usage. |
tests/test_cypher.c |
Adds a test ensuring long string literals don’t crash the lexer and are truncated. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| int blen = 0; | ||
| const int max_blen = CBM_SZ_4K - 1; | ||
| while (*pos < len && input[*pos] != quote) { | ||
| if (blen >= max_blen) { (*pos)++; continue; } |
There was a problem hiding this comment.
The truncation guard increments *pos by 1 and continues without preserving escape semantics. If the buffer is full and the next input is a backslash escape (e.g. "), skipping only the backslash allows the following quote to be seen as the terminator, which can prematurely end the string and desync the lexer. When truncating, still consume escape sequences as a unit (skip both the backslash and the escaped char when present), rather than skipping a single byte unconditionally.
Suggested change
| if (blen >= max_blen) { (*pos)++; continue; } | |
| if (blen >= max_blen) { | |
| if (input[*pos] == '\\' && *pos + SKIP_ONE < len) { | |
| *pos += PAIR_LEN; | |
| } else { | |
| (*pos)++; | |
| } | |
| continue; | |
| } |
Comment on lines
1077
to
+1086
| cbm_case_expr_t *kase = calloc(CBM_ALLOC_ONE, sizeof(cbm_case_expr_t)); | ||
| if (!kase) { | ||
| return NULL; | ||
| } | ||
| int bcap = CYP_INIT_CAP4; | ||
| kase->branches = malloc(bcap * sizeof(cbm_case_branch_t)); | ||
| if (!kase->branches) { | ||
| free(kase); | ||
| return NULL; | ||
| } |
There was a problem hiding this comment.
parse_case_expr can now return NULL on allocation failure, but callers need a reliable way to detect that as a parse error. Today parse_return_item assigns item->variable = "CASE" unconditionally after calling parse_case_expr, and does not check for NULL, which can allow an OOM to be treated as a successful parse and lead to incorrect execution or downstream NULL handling issues. Consider setting a parser error / returning an error code when parse_case_expr fails, and ensure the caller propagates that failure.
| "BY COUNT(*) DESC;"; | ||
| sqlite3_stmt *stmt = NULL; | ||
| sqlite3_prepare_v2(s->db, sql, CBM_NOT_FOUND, &stmt, NULL); | ||
| if (sqlite3_prepare_v2(s->db, sql, CBM_NOT_FOUND, &stmt, NULL) != SQLITE_OK || !stmt) { |
There was a problem hiding this comment.
If the first (node labels) query succeeds and allocates out->node_labels, then the edge-types prepare failure returns immediately without freeing the already-populated fields in *out. Since callers typically only call cbm_store_schema_free() on CBM_STORE_OK, this leaks memory on prepare failures. Before returning here, free any partially populated schema (e.g., call cbm_store_schema_free(out) or manually clean up fields already set).
Suggested change
| if (sqlite3_prepare_v2(s->db, sql, CBM_NOT_FOUND, &stmt, NULL) != SQLITE_OK || !stmt) { | |
| if (sqlite3_prepare_v2(s->db, sql, CBM_NOT_FOUND, &stmt, NULL) != SQLITE_OK || !stmt) { | |
| cbm_store_schema_free(out); |
Comment on lines
3289
to
+3292
| sqlite3_stmt *stmt = NULL; | ||
| sqlite3_prepare_v2(s->db, sql, CBM_NOT_FOUND, &stmt, NULL); | ||
| if (sqlite3_prepare_v2(s->db, sql, CBM_NOT_FOUND, &stmt, NULL) != SQLITE_OK || !stmt) { | ||
| return 0; | ||
| } |
There was a problem hiding this comment.
On sqlite3_prepare_v2 failure this now returns 0, which is indistinguishable from “query succeeded but returned 0 rows”. That masks database/SQL errors and can cause silent misclassification in arch_layers. Consider returning an error code (or a negative sentinel) and letting the caller decide whether to treat it as fatal vs. optional, and set the store error buffer for diagnostics.
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+95
to
99
| const int max_blen = CBM_SZ_4K - 1; | ||
| while (*pos < len && input[*pos] != quote) { | ||
| if (blen >= max_blen) { (*pos)++; continue; } | ||
| if (input[*pos] == '\\' && *pos + SKIP_ONE < len) { | ||
| (*pos)++; |
There was a problem hiding this comment.
1. Escaped quote truncated wrong 🐞 Bug ≡ Correctness
lex_string_literal() now skips escape handling once the buffer is full, so a backslash at/after the truncation boundary can be ignored and an escaped quote (\") can incorrectly terminate the string literal early. This can produce wrong tokens / parse failures for long strings with escapes near byte 4095.
Agent Prompt
## Issue description
`lex_string_literal()` truncates output once `blen` reaches `CBM_SZ_4K - 1`, but the new truncation path skips escape handling. If a long literal contains an escaped quote (e.g. `\"`) after the truncation boundary, the lexer can incorrectly treat that quote as the terminator.
## Issue Context
The lexer must keep *parsing* the literal correctly even when it stops *buffering* characters.
## Fix Focus Areas
- src/cypher/cypher.c[88-124]
## Suggested fix
Refactor the loop so position advancement always respects escapes, and buffering is conditional:
- Always detect `\\` and advance `*pos` by 2 (when possible) regardless of `blen`.
- Only write into `buf` / increment `blen` when `blen < max_blen`.
- Keep the current `buf[blen] = '\0'` termination behavior.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
1077
to
+1086
| cbm_case_expr_t *kase = calloc(CBM_ALLOC_ONE, sizeof(cbm_case_expr_t)); | ||
| if (!kase) { | ||
| return NULL; | ||
| } | ||
| int bcap = CYP_INIT_CAP4; | ||
| kase->branches = malloc(bcap * sizeof(cbm_case_branch_t)); | ||
| if (!kase->branches) { | ||
| free(kase); | ||
| return NULL; | ||
| } |
There was a problem hiding this comment.
2. Oom still crashes on realloc 🐞 Bug ☼ Reliability
The new NULL checks cover initial malloc/calloc, but parse_case_expr/parse_props/parse_rel_types/parse_in_list still call safe_realloc() without checking for NULL. On realloc OOM, safe_realloc() frees the old buffer and returns NULL, and the next write dereferences NULL causing a crash.
Agent Prompt
## Issue description
Several parser functions still crash on OOM during array growth because they use `safe_realloc()` without checking for NULL. `safe_realloc()` frees the old pointer on failure, so a failed growth loses the only reference and the next dereference crashes.
## Issue Context
This PR aims to prevent OOM crashes, so growth paths must be hardened too (not just initial allocations).
## Fix Focus Areas
- src/cypher/cypher.c[463-509]
- src/cypher/cypher.c[572-607]
- src/cypher/cypher.c[760-797]
- src/cypher/cypher.c[1074-1109]
- src/foundation/platform.h[18-32]
## Suggested fix
For each growth site:
1. Use a temporary pointer:
- `void *tmp = realloc(ptr, new_size);`
- if `!tmp`: perform local cleanup (free any elements already duplicated), set `p->error` if available, and return an error.
- else assign `ptr = tmp`.
2. Alternatively, change `safe_realloc()` semantics to *not free* on failure (so callers can handle errors without losing the old buffer), then audit call sites accordingly.
3. Ensure callers propagate an error code distinct enough that higher layers can abort cleanly.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+2582
to
+2584
| if (sqlite3_prepare_v2(s->db, sql, CBM_NOT_FOUND, &stmt, NULL) != SQLITE_OK || !stmt) { | ||
| return CBM_NOT_FOUND; | ||
| } |
There was a problem hiding this comment.
3. Schema error path leaks 🐞 Bug ☼ Reliability
cbm_store_get_schema() now returns early if the second sqlite3_prepare_v2 fails, but it may already have allocated out->node_labels from the first query. Callers like ui/layout3d.c only free schema when cbm_store_get_schema returns OK, so this introduces a leak on that failure path.
Agent Prompt
## Issue description
`cbm_store_get_schema()` can now return early after partially filling `out` (e.g., after allocating `out->node_labels`). Some callers only free schema on success, so this introduces a leak.
## Issue Context
The function currently has two independent query blocks and returns directly inside the second block.
## Fix Focus Areas
- src/store/store.c[2544-2605]
- src/ui/layout3d.c[441-492]
## Suggested fix
Option A (preferred): make `cbm_store_get_schema()` self-cleaning on failure:
- Replace early `return CBM_NOT_FOUND;` with `goto cleanup;`.
- In `cleanup:`, call `sqlite3_finalize(stmt)` if non-NULL, and call `cbm_store_schema_free(out)` (or manual frees) before returning an error.
Option B: update callers to always call `cbm_store_schema_free(&schema)` regardless of return code (safe because it zeros and checks fields), but still consider Option A for robustness.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
…L stmts - cypher: Add bounds check in lex_string_literal to prevent stack buffer overflow on string literals >4096 bytes. Escape sequences are always parsed correctly even past the truncation boundary. - cypher: Add malloc/calloc NULL checks in parse_props, parse_rel_types, parse_in_condition, and parse_case_expr. Growth paths use non-destructive realloc (temp pointer) so accumulated elements can be freed on OOM instead of leaking through safe_realloc's free-on-failure semantics. - store: Add sqlite3_prepare_v2 return code checks at 3 sites in cbm_store_schema_info and collect_pkg_names. Partially prepared statements are finalized before returning. Schema function cleans up partially populated output on failure. collect_pkg_names returns CBM_NOT_FOUND (not 0) to distinguish errors from empty results.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
lex_string_literalwrote to a 4KB stack buffer with no bounds check. String literals >4096 bytes caused stack overflow. Addedmax_blenguard to safely truncate.malloc/calloccalls inparse_props,parse_rel_types,parse_in_condition, andparse_case_exprhad no NULL checks. Added guards with proper cleanup on allocation failure.cbm_store_schema_infoandcollect_pkg_namescalledsqlite3_prepare_v2without checking the return code. If prepare fails,bind_textandsqlite3_stepcrash on NULLstmt. Added error checks.Test plan
cypher_lex_string_overflowverifies 5000-char string literal doesn't crash