From ae982987fc8174225eaa1059874ef7a8e2989680 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Yusufhan=20Sa=C3=A7ak?= Date: Sun, 30 Aug 2026 20:47:40 +0300 Subject: [PATCH] src: fix sqlite connection leak in Web Storage MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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: https://github.com/nodejs/node/issues/64640 Signed-off-by: Yusufhan Saçak --- src/node_webstorage.cc | 12 +- .../test-webstorage-connection-leak.js | 113 ++++++++++++++++++ 2 files changed, 122 insertions(+), 3 deletions(-) create mode 100644 test/parallel/test-webstorage-connection-leak.js diff --git a/src/node_webstorage.cc b/src/node_webstorage.cc index 21f846fbeb62..235115f88428 100644 --- a/src/node_webstorage.cc +++ b/src/node_webstorage.cc @@ -173,6 +173,9 @@ Maybe Storage::Open() { } int r = sqlite3_open(location_.c_str(), &db); + // sqlite3_open() usually returns a database handle even on failure, so + // adopt it immediately to ensure it is closed on every error return path. + auto conn = conn_unique_ptr(db); CHECK_ERROR_OR_THROW(env(), r, SQLITE_OK, Nothing()); r = sqlite3_exec(db, init_sql_v0.data(), nullptr, nullptr, nullptr); CHECK_ERROR_OR_THROW(env(), r, SQLITE_OK, Nothing()); @@ -184,12 +187,15 @@ Maybe Storage::Open() { get_schema_version_sql.size(), &s, nullptr); - r = sqlite3_exec(db, init_sql_v0.data(), nullptr, nullptr, nullptr); CHECK_ERROR_OR_THROW(env(), r, SQLITE_OK, Nothing()); auto stmt = stmt_unique_ptr(s); CHECK_ERROR_OR_THROW( env(), sqlite3_step(stmt.get()), SQLITE_ROW, Nothing()); - CHECK(sqlite3_column_type(stmt.get(), 0) == SQLITE_INTEGER); + if (sqlite3_column_type(stmt.get(), 0) != SQLITE_INTEGER) { + THROW_ERR_INVALID_STATE(env(), + "localStorage schema version is not an integer"); + return Nothing(); + } int schema_version = sqlite3_column_int(stmt.get(), 0); stmt = nullptr; // Force finalization. @@ -209,7 +215,7 @@ Maybe Storage::Open() { CHECK_ERROR_OR_THROW(env(), r, SQLITE_OK, Nothing()); } - db_ = conn_unique_ptr(db); + db_ = std::move(conn); return JustVoid(); } diff --git a/test/parallel/test-webstorage-connection-leak.js b/test/parallel/test-webstorage-connection-leak.js new file mode 100644 index 000000000000..97721a8383aa --- /dev/null +++ b/test/parallel/test-webstorage-connection-leak.js @@ -0,0 +1,113 @@ +'use strict'; + +const common = require('../common'); +common.skipIfSQLiteMissing(); + +if (common.isWindows) { + common.skip('SQLite on Windows uses HANDLEs, not fds'); +} + +const tmpdir = require('../common/tmpdir'); +const assert = require('node:assert'); +const { spawnPromisified } = common; +const { writeFileSync } = require('node:fs'); +const { join } = require('node:path'); +const { DatabaseSync } = require('node:sqlite'); +const { test } = require('node:test'); + +tmpdir.refresh(); + +// Regression tests for https://github.com/nodejs/node/issues/64640. Failed +// localStorage initialisation leaked the SQLite connection opened by +// sqlite3_open(), which usually returns a database handle even on failure. +// Each retry then leaked one more file descriptor. +async function assertFailedInitDoesNotLeak(localStorageFile, message) { + const cp = await spawnPromisified(process.execPath, [ + '--localstorage-file', localStorageFile, + '-e', ` + const assert = require('node:assert'); + const { openSync, closeSync } = require('node:fs'); + // The lowest fd available to a new file. If a failed localStorage + // initialisation leaks its connection, this number grows. + const probeFd = () => { + const fd = openSync(process.execPath, 'r'); + closeSync(fd); + return fd; + }; + const expected = { code: 'ERR_INVALID_STATE', message: ${message} }; + // Warm up lazily initialised resources before sampling the fd space. + assert.throws(() => localStorage.length, expected); + const before = probeFd(); + for (let i = 0; i < 15; i++) { + assert.throws(() => localStorage.length, expected); + } + assert.strictEqual(probeFd(), before); + `, + ]); + + assert.strictEqual(cp.stderr, ''); + assert.strictEqual(cp.code, 0); + assert.strictEqual(cp.signal, null); +} + +test('corrupt non-SQLite file does not leak fds', async () => { + const file = join(tmpdir.path, 'corrupt.localstorage'); + writeFileSync(file, 'not a sqlite database '.repeat(10)); + await assertFailedInitDoesNotLeak(file, '/not a database/'); +}); + +test('unopenable database path does not leak fds', async () => { + const file = join(tmpdir.path, 'missing-dir', 'db.localstorage'); + await assertFailedInitDoesNotLeak(file, '/unable to open database file/'); +}); + +test('newer schema version does not leak fds', async () => { + const file = join(tmpdir.path, 'newer-schema.localstorage'); + const db = new DatabaseSync(file); + db.exec(`CREATE TABLE nodejs_webstorage_state( + max_size INTEGER NOT NULL DEFAULT 10485760, + total_size INTEGER NOT NULL, + schema_version INTEGER NOT NULL DEFAULT 0, + single_row_ INTEGER NOT NULL DEFAULT 1 CHECK(single_row_ = 1), + PRIMARY KEY(single_row_) + ) STRICT; + INSERT INTO nodejs_webstorage_state (total_size, schema_version) + VALUES (0, 99);`); + db.close(); + await assertFailedInitDoesNotLeak(file, '/newer version of Node\\.js/'); +}); + +test('empty state table does not leak fds', async () => { + const file = join(tmpdir.path, 'zero-rows.localstorage'); + const db = new DatabaseSync(file); + // The extra NOT NULL column makes the initialisation script's + // INSERT OR IGNORE skip silently, leaving the state table empty, so + // Open() fails while a prepared statement is still live. + db.exec(`CREATE TABLE nodejs_webstorage_state( + max_size INTEGER NOT NULL DEFAULT 10485760, + total_size INTEGER NOT NULL, + schema_version INTEGER NOT NULL DEFAULT 0, + single_row_ INTEGER NOT NULL DEFAULT 1 CHECK(single_row_ = 1), + extra INTEGER NOT NULL, + PRIMARY KEY(single_row_) + ) STRICT;`); + db.close(); + await assertFailedInitDoesNotLeak(file, '/no more rows available/'); +}); + +test('non-integer schema version throws instead of aborting', async () => { + const file = join(tmpdir.path, 'text-schema.localstorage'); + const db = new DatabaseSync(file); + db.exec(`CREATE TABLE nodejs_webstorage_state( + max_size INTEGER NOT NULL DEFAULT 10485760, + total_size INTEGER NOT NULL, + schema_version TEXT NOT NULL, + single_row_ INTEGER NOT NULL DEFAULT 1 CHECK(single_row_ = 1), + PRIMARY KEY(single_row_) + ) STRICT; + INSERT INTO nodejs_webstorage_state (total_size, schema_version) + VALUES (0, 'pwned');`); + db.close(); + await assertFailedInitDoesNotLeak( + file, '/schema version is not an integer/'); +});