fix(ext/node): segfault on calling StatementSync methods after connection has closed
#31331
Conversation
WalkthroughRefactors SQLite prepared statement management by replacing raw C-style pointers with an Changes
Sequence DiagramsequenceDiagram
participant App as Application
participant Stmt as StatementSync
participant DB as DatabaseSync
participant FFI as SQLite FFI
rect rgb(240, 248, 255)
Note over Stmt,FFI: Normal Operation Flow
App->>Stmt: Call operation (e.g., step())
Stmt->>Stmt: stmt_ptr() → get raw ptr from Rc<Cell<>>
alt Pointer Valid
Stmt->>FFI: Call sqlite3_step(ptr)
FFI-->>Stmt: Result
Stmt-->>App: Success
else Pointer is None
Stmt-->>App: Error: StatementFinalized
end
end
rect rgb(255, 240, 240)
Note over App,FFI: Finalization Flow (on DB close)
App->>DB: close()
DB->>DB: Iterate statements vector
DB->>Stmt: Access InnerStatementPtr
Stmt->>FFI: sqlite3_finalize(ptr)
Stmt->>Stmt: set(None) in Rc<Cell<>>
Note over Stmt: Future access returns StatementFinalized
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. Comment |
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a segmentation fault that occurs when calling StatementSync methods after the database connection has been closed. The fix wraps the raw SQLite statement pointer in Rc<Cell<Option<*mut ffi::sqlite3_stmt>>> to track finalization state.
Key changes:
- Introduction of
InnerStatementPtrtype alias to wrap statement pointers with finalization tracking - Addition of
StatementFinalizederror variant for proper error handling when statements are used after finalization - Updated all
StatementSyncmethods to check finalization state before accessing the underlying pointer
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| ext/node/ops/sqlite/statement.rs | Core logic changes: wraps statement pointer, adds finalization checks to all methods |
| ext/node/ops/sqlite/database.rs | Updates prepare and close methods to work with wrapped statement pointers |
| ext/node/ops/sqlite/mod.rs | Adds StatementFinalized error variant |
| tests/unit_node/sqlite_test.ts | Adds comprehensive test for post-close method calls |
| tests/node_compat/config.toml | Enables Node.js compatibility test for statement columns |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
ext/node/ops/sqlite/statement.rs (3)
80-101: Drop logic safely avoids double finalization; minor simplification possibleThe
Dropimplementation correctly:
- Removes this statement’s
InnerStatementPtrfrom the sharedstatementsvec.- Finalizes only if the cell still contains
Some(ptr).- Clears the cell to
Noneafterward so future uses seeStatementFinalized.You could simplify slightly and avoid the temporary
finalized_stmtby usingCell::take/replace:- let mut finalized_stmt = None; - - if let Some(pos) = statements - .iter() - .position(|stmt| Rc::ptr_eq(stmt, &self.inner)) - { - let stmt = statements.remove(pos); - finalized_stmt = stmt.get(); - stmt.set(None); - } - - if let Some(ptr) = finalized_stmt { + if let Some(pos) = statements + .iter() + .position(|stmt| Rc::ptr_eq(stmt, &self.inner)) + { + let stmt = statements.remove(pos); + if let Some(ptr) = stmt.replace(None) { // SAFETY: `ptr` is a valid pointer to a sqlite3_stmt. unsafe { ffi::sqlite3_finalize(ptr); } } }Purely a readability/ergonomics win; behavior is equivalent.
147-185: Column iterator error handling is functionally correct but slightly lossy
ColumnIterator::newand the iterator implementation now propagateSqliteErrorviaResult, which letsread_rowcleanly surfaceStatementFinalizedwhen the underlying statement is gone. Innext, though, any error fromcolumn_nameis coerced toStatementFinalized:let name = match self.stmt.column_name(index) { Ok(name) => name, Err(_) => return Some(Err(SqliteError::StatementFinalized)), };Given that
column_namecurrently only fails due tostmt_ptr(i.e. finalized statements), this is correct, but if new error cases are added later you might want to preserve the original error instead of hard‑codingStatementFinalized.
196-211:assert_statement_finalizednaming is inverted relative to its behaviorThe helper:
fn assert_statement_finalized(&self) -> Result<(), SqliteError> { if self.inner.get().is_none() { return Err(SqliteError::StatementFinalized); } Ok(()) }actually asserts that the statement has not been finalized and is used as such in the setters. The behavior is correct, but the name is misleading and could cause confusion for future changes.
Consider renaming to something like
assert_not_finalizedorensure_not_finalizedfor clarity, and updating call sites accordingly.Also applies to: 793-824
ext/node/ops/sqlite/database.rs (1)
533-581:prepare()wiring toInnerStatementPtrlooks correct; consider future GC behaviorThe new
prepare()implementation:
- Prepares
raw_stmtas before.- Wraps it in
Rc<Cell<Option<*mut _>>>and pushes a clone into the sharedstatementsvec.- Returns a
StatementSyncwhoseinnerandstatementsfields point at the same cell/vec as the database.This ensures the DB and statements share finalization state and that both
close()andStatementSync::Dropcan coordinate safely; no issues with the immediate design.One broader (pre‑existing) lifetime consideration: if the
DatabaseSyncobject is garbage‑collected without an explicitclose(), the underlyingrusqlite::Connectionwill be dropped whileStatementSyncinstances may still holdInnerStatementPtrcells containingSome(ptr). That scenario is outside the explicitclose()path this PR addresses, but might still permit use‑after‑close if callers keep statements alive longer than the database.If you want to harden this in the future, a separate follow‑up could:
- Either implement
DropforDatabaseSyncthat finalizes any remaining statements and clears their cells, or- Make
StatementSynchold a strong reference to the connection so the DB outlives all statements.Not a blocker for this change set, but worth tracking.
Also applies to: 569-571
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
ext/node/ops/sqlite/database.rs(4 hunks)ext/node/ops/sqlite/mod.rs(2 hunks)ext/node/ops/sqlite/statement.rs(24 hunks)tests/node_compat/config.toml(1 hunks)tests/unit_node/sqlite_test.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
ext/node/ops/sqlite/mod.rs (2)
ext/node/ops/sqlite/validators.rs (1)
code(17-22)ext/node/polyfills/internal/errors.ts (1)
ERR_INVALID_STATE(2692-2696)
ext/node/ops/sqlite/statement.rs (1)
ext/node/ops/sqlite/database.rs (1)
new(404-442)
tests/unit_node/sqlite_test.ts (1)
tests/unit/test_util.ts (1)
assertThrows(19-19)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: CodeQL analysis (javascript-typescript)
- GitHub Check: Agent
- GitHub Check: test debug windows-x86_64
- GitHub Check: test release linux-x86_64
- GitHub Check: test debug linux-x86_64
- GitHub Check: test debug linux-aarch64
- GitHub Check: test release linux-aarch64
- GitHub Check: test debug macos-aarch64
- GitHub Check: bench release linux-x86_64
- GitHub Check: lint debug linux-x86_64
- GitHub Check: build libs
- GitHub Check: lint debug windows-x86_64
🔇 Additional comments (7)
ext/node/ops/sqlite/statement.rs (2)
65-72: SharedInnerStatementPtrdesign correctly centralizes statement lifetimeUsing
InnerStatementPtr = Rc<Cell<Option<*mut sqlite3_stmt>>>for bothStatementSync.innerand entries inDatabaseSync.statementsnicely guarantees a single source of truth for the statement handle and its finalized state. This is exactly the right foundation to prevent use‑after‑finalize across connection close and statement drop paths.
212-246: All statement operations now defensively gate onstmt_ptr()
reset,step,column_count,column_name,column_value,bind_value,bind_params,source_sql,expanded_sql, andcolumnsare all correctly routed throughstmt_ptr()so that any use after the connection has closed (and the underlyingInnerStatementPtrhas been cleared toNone) surfacesSqliteError::StatementFinalizedinstead of touching freed memory. This directly addresses the segfault scenario described in the PR and looks solid.Also applies to: 248-313, 349-425, 443-559, 826-987
tests/node_compat/config.toml (1)
978-979: Node compat test inclusion looks correctAdding
"parallel/test-sqlite-statement-sync-columns.js" = {}under[tests]is consistent with the existing configuration and ensures the Node 24 columns test runs against the new statement‑finalization logic.ext/node/ops/sqlite/mod.rs (1)
126-130: NewStatementFinalizederror integrates cleanly with existing error mappingThe
SqliteError::StatementFinalizedvariant and itscode()mapping:
- Provide a clear, dedicated error for finalized statements with message
"statement has been finalized", matching the TS test expectations.- Correctly classify it as
ERR_INVALID_STATEalongsideAlreadyOpen/AlreadyClosed/InUse, which is consistent with Node’s error taxonomy.No issues from the API or error‑surface perspective.
Also applies to: 178-193
tests/unit_node/sqlite_test.ts (1)
442-463: Post‑close StatementSync test thoroughly exercises the new error pathThe new test:
- Mirrors the reported repro (calling
StatementSyncmethods afterdb.close()).- Verifies a consistent
"statement has been finalized"message across read APIs (all,get,iterate,expandedSQL,sourceSQL) and configuration setters.This is a solid regression test for the segfault fix and should guard future changes to statement lifecycle handling.
ext/node/ops/sqlite/database.rs (2)
33-35: Database tracks statement cells instead of raw pointers in a consistent waySwitching
DatabaseSync.statementstoRc<RefCell<Vec<InnerStatementPtr>>>and importingInnerStatementPtraligns the database’s tracking structure with the new shared pointer abstraction used byStatementSync. This keeps ownership/lifetime information centralized and consistent.Also applies to: 232-237
488-505:close()correctly finalizes all tracked statements and clears their cellsThe revised
close():
- Guards on
AlreadyClosedas before.- Drains
self.statements, finalizing each underlying sqlite3_stmt only if the cell currently holdsSome(ptr), then sets the cell toNone.This ensures:
- No double‑finalization, even if
StatementSync::Dropruns later.- All live
StatementSyncinstances observeStatementFinalizedviastmt_ptr()afterdb.close().Behavior matches the PR’s goal and looks safe.
2 participants
Previously, running this code causes segfault:
Changes in this PR also allows the https://github.com/nodejs/node/blob/v24.2.0/test/parallel/test-sqlite-statement-sync-columns.js test to pass.