Skip to content

src: fix sqlite connection leak in Web Storage - #65673

Open
JosephDoUrden wants to merge 1 commit into
nodejs:mainfrom
JosephDoUrden:fix/webstorage-connection-leak
Open

src: fix sqlite connection leak in Web Storage#65673
JosephDoUrden wants to merge 1 commit into
nodejs:mainfrom
JosephDoUrden:fix/webstorage-connection-leak

Conversation

@JosephDoUrden

Copy link
Copy Markdown

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, but Storage::Open() only adopts it into the RAII conn_unique_ptr on the last line of the function, so every early error return in between leaves the connection open. And since db_ 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 second sqlite3_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 (compared sqlite_master dumps), 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-existing nodejs_webstorage_state table has that column typed differently (CREATE TABLE IF NOT EXISTS preserves whatever is there). A crafted or corrupted localStorage file should not be able to take down the process, so it now throws ERR_INVALID_STATE like 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 (openSync on process.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-time CHECK_EQ(sqlite3_close, SQLITE_OK) depends on.

One thing deliberately left out: GetAll() also fails to check its sqlite3_prepare_v2() result, but the consequence there is an empty result rather than a leak or crash (sqlite3_step(nullptr) returns SQLITE_MISUSE and 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.

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels Aug 30, 2026
@MikeMcC399

Copy link
Copy Markdown
Contributor

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 main branch, every PR change requires an explicit approval, as explained in Q: How do I trigger the CI runs? which from your point of view means additional delays, and from the collaborator / triager point of view means extra effort to monitor the PR and approve each run.

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>
@JosephDoUrden
JosephDoUrden force-pushed the fix/webstorage-connection-leak branch from 0788665 to ae98298 Compare August 30, 2026 19:18
@JosephDoUrden

Copy link
Copy Markdown
Author

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

@codecov

codecov Bot commented Aug 30, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.04%. Comparing base (01c1300) to head (ae98298).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #65673      +/-   ##
==========================================
- Coverage   90.04%   90.04%   -0.01%     
==========================================
  Files         754      754              
  Lines      255747   255750       +3     
  Branches    48323    48326       +3     
==========================================
- Hits       230299   230295       -4     
- Misses      16567    16587      +20     
+ Partials     8881     8868      -13     
Files with missing lines Coverage Δ
src/node_webstorage.cc 78.01% <100.00%> (+3.08%) ⬆️

... and 33 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Web Storage Maybe leaks SQLite connections when database initialization fails

3 participants