src: fix sqlite connection leak in Web Storage - #65673
Conversation
|
Please make sure you have read and understood the following documents:
The guide for first-time contributors says that you should not open any new PRs until your first PR has been approved. Your first PR in this repo, #65634, is awaiting review, so it has not reached the stage where it could be approved. So long as you don't have a completed PR merged into the Waiting until your first PR has cleared also makes sure that you have experienced the complete process from start to finish. Before you submit any further changes, you should test these locally. See https://github.com/nodejs/node/blob/main/doc/contributing/pull-requests.md#step-6-test. |
Storage::Open() only adopted the sqlite3 handle into its RAII holder after initialisation succeeded, so every early error return leaked the connection opened by sqlite3_open(), which usually returns a handle even on failure. Repeated failed initialisations then leaked one file descriptor each, since db_ is never set and every operation retries. Adopt the handle immediately after sqlite3_open() instead. Also check the sqlite3_prepare_v2() return value in Open(), which was overwritten before being checked by a duplicated sqlite3_exec() of the initialisation SQL that also ran the script a second time on every open. Validate the stored schema version's column type instead of asserting it, so a crafted localStorage file surfaces ERR_INVALID_STATE rather than aborting the process. Fixes: nodejs#64640 Signed-off-by: Yusufhan Saçak <yusufhansacak@icloud.com>
0788665 to
ae98298
Compare
|
Thanks @MikeMcC399, understood. I'll hold any new PRs until #65634 clears. The format-cpp failure is fixed, ran make format-cpp locally and it produces no diff now |
Fixes #64640.
Every failed Web Storage initialisation leaks the SQLite connection. Root cause is in the issue thread:
sqlite3_open()usually returns a handle even on failure, butStorage::Open()only adopts it into the RAIIconn_unique_ptron the last line of the function, so every early error return in between leaves the connection open. And sincedb_never gets set on failure, the next localStorage operation retries the whole init and leaks another one. Measured before the fix: 50 failed inits on a corrupt file leaked exactly 50 fds; the later error paths (schema version check, empty state table) leaked 2 per attempt because WAL is established by then. After the fix all paths are flat.The change adopts the handle immediately after
sqlite3_open(), so RAII covers every return path. Two adjacent defects in the same function are included because a reviewer would trip over them anyway:The
sqlite3_prepare_v2()return value was overwritten before being checked by what looks like a copy-pasted secondsqlite3_exec()of the init SQL, which also ran the whole script twice on every open. The stray exec is deleted and the prepare result is now checked. Schema output of a fresh db is byte-identical before and after (comparedsqlite_masterdumps), so running the script once is behaviourally inert.The schema version read used
CHECK(sqlite3_column_type(...) == SQLITE_INTEGER), which aborts the whole process if a pre-existingnodejs_webstorage_statetable has that column typed differently (CREATE TABLE IF NOT EXISTSpreserves whatever is there). A crafted or corrupted localStorage file should not be able to take down the process, so it now throwsERR_INVALID_STATElike every other failure in this function.The test covers five failure paths (corrupt file, unopenable path, newer schema version, empty state table, non-integer schema version), each asserting the right error and a flat fd probe. The probe uses the lowest-available-fd trick (
openSynconprocess.execPath) rather than listing/dev/fd, so it works anywhere with POSIX fd semantics; Windows is skipped because SQLite uses raw HANDLEs there and no fd probe can see the leak. The empty-state-table case doubles as a guard on the destruction order of the connection and statement holders, which the close-timeCHECK_EQ(sqlite3_close, SQLITE_OK)depends on.One thing deliberately left out:
GetAll()also fails to check itssqlite3_prepare_v2()result, but the consequence there is an empty result rather than a leak or crash (sqlite3_step(nullptr)returnsSQLITE_MISUSEand the loop never runs), and fixing it properly needs an error channel the current return type does not have. Separate PR.AI disclosure: I used an AI coding agent for parts of the investigation and drafting. I verified the root cause against the SQLite and Node sources myself, and every number above comes from runs on my own machine.