bun:sqlite: finalize outstanding prepared statements on close via sqlite3_next_stmt - #36573
Code review found 1 important issue
Found 4 candidates, confirmed 3. See review comments for details.
Details
| Severity | Count |
|---|---|
| 🔴 Important | 1 |
| 🟡 Nit | 2 |
| 🟣 Pre-existing | 0 |
| Severity | File:Line | Issue |
|---|---|---|
| 🔴 Important | src/jsc/bindings/sqlite/JSSQLStatement.cpp:2557-2563 |
raw() exception arm calls sqlite3_reset on a stmt freed by close() during push() |
| 🟡 Nit | src/jsc/bindings/sqlite/JSSQLStatement.cpp:1873-1880 |
#13082 test becomes vacuous under new close() semantics (rejections swallowed by allSettled) |
| 🟡 Nit | src/jsc/bindings/sqlite/JSSQLStatement.cpp:1882-1889 |
close(true) throw path leaves finalizedStatementsOnClose=true with db handle still non-null |
Annotations
Check failure on line 2563 in src/jsc/bindings/sqlite/JSSQLStatement.cpp
claude / Claude Code Review
raw() exception arm calls sqlite3_reset on a stmt freed by close() during push()
The exception arm after `resultArray->push()` calls `sqlite3_reset(stmt)` *before* either liveness check, so an `Array.prototype[0]` setter that calls `db.close()` (which now `sqlite3_finalize`s every statement via `sqlite3_next_stmt`) and then throws hits `sqlite3_reset` on freed memory. The sibling `all()`/`values()` loops use a bare `RETURN_IF_EXCEPTION(scope, {})` after `push()` with no reset — matching them here (or moving both guards above the reset) fixes it.
Check warning on line 1880 in src/jsc/bindings/sqlite/JSSQLStatement.cpp
claude / Claude Code Review
#13082 test becomes vacuous under new close() semantics (rejections swallowed by allSettled)
The existing `#13082` test (test/js/bun/sqlite/sqlite.test.js) becomes vacuous under the new close() semantics: `stmt.all()/get()/run()` now throw "Database has closed" at `CHECK_PREPARED`, and the 100 rejections are silently swallowed by `Promise.allSettled` with no assertion on the settled values. Since the original UAF-under-GC path is now unreachable by construction, consider updating the test to assert the new throw (or removing it with a stated reason) rather than leaving it green while co
Check warning on line 1889 in src/jsc/bindings/sqlite/JSSQLStatement.cpp
claude / Claude Code Review
close(true) throw path leaves finalizedStatementsOnClose=true with db handle still non-null
nit: on the `close(true)` throw path, `versionDB->db` stays non-null (and `sqlite3_close_v2` is skipped) even though `finalizedStatementsOnClose` was already set — so a caught error leaves the `VersionSqlite3` in `{db != nullptr, finalizedStatementsOnClose = true}`, where a subsequent `prepare()` creates a stmt whose wrapper is dead-on-arrival and whose destructor skips `sqlite3_finalize`. After the `sqlite3_next_stmt` loop this can only trigger over an extension-created `sqlite3_blob`/`sqlite3_