From e8ea485c54bbb72feb509d017c7167647bc06fad Mon Sep 17 00:00:00 2001 From: Yagiz Nizipli Date: Thu, 11 Jun 2026 12:05:11 -0400 Subject: [PATCH] sqlite: do not leave database open after failed open When sqlite3_open_v2() fails, SQLite still assigns a database handle in a "sick" state. Such a handle may only be used to retrieve the error and must then be released with sqlite3_close(). Keeping it in connection_ meant that after a failed open(): - isOpen incorrectly reported true, - every method passed the "database is not open" check and called SQLite APIs on the sick handle, which is an API misuse, - registering a user-defined function leaked its user data in builds with SQLITE_ENABLE_API_ARMOR, because SQLite rejects the call before taking ownership of the user data, - open() could not be retried since the database appeared open. Close and reset the connection handle when open() fails at any point so that the database remains closed and open() can be retried. Fixes: https://github.com/nodejs/node/issues/63831 Signed-off-by: Yagiz Nizipli --- src/node_sqlite.cc | 14 ++++++++ test/parallel/test-sqlite-database-sync.js | 38 +++++++++++++++++++++- 2 files changed, 51 insertions(+), 1 deletion(-) diff --git a/src/node_sqlite.cc b/src/node_sqlite.cc index 87cd52d7283f71..049fd786126fce 100644 --- a/src/node_sqlite.cc +++ b/src/node_sqlite.cc @@ -941,6 +941,19 @@ bool DatabaseSync::Open() { return false; } + // sqlite3_open_v2() assigns a database handle even when it fails. Such a + // handle is in a "sick" state and may only be used to retrieve the error + // and must then be released with sqlite3_close(). Close and reset the + // handle on any failure below so that a failed open() does not leave the + // database in an open state. + bool opened = false; + auto reset_connection_on_failure = OnScopeLeave([&]() { + if (!opened && connection_ != nullptr) { + sqlite3_close_v2(connection_); + connection_ = nullptr; + } + }); + // TODO(cjihrig): Support additional flags. int default_flags = SQLITE_OPEN_URI; int flags = open_config_.get_read_only() @@ -1003,6 +1016,7 @@ bool DatabaseSync::Open() { env()->isolate(), this, load_extension_ret, SQLITE_OK, false); } + opened = true; return true; } diff --git a/test/parallel/test-sqlite-database-sync.js b/test/parallel/test-sqlite-database-sync.js index ac3a3c66d6469a..af7677a3cfc037 100644 --- a/test/parallel/test-sqlite-database-sync.js +++ b/test/parallel/test-sqlite-database-sync.js @@ -2,7 +2,7 @@ const { skipIfSQLiteMissing } = require('../common'); skipIfSQLiteMissing(); const tmpdir = require('../common/tmpdir'); -const { existsSync } = require('node:fs'); +const { existsSync, mkdirSync } = require('node:fs'); const { join } = require('node:path'); const { DatabaseSync, StatementSync } = require('node:sqlite'); const { suite, test } = require('node:test'); @@ -308,6 +308,42 @@ suite('DatabaseSync.prototype.open()', () => { }); t.assert.strictEqual(db.isOpen, true); }); + + test('does not leave the database open after a failed open', (t) => { + // Regression test for https://github.com/nodejs/node/issues/63831 + const dbDir = join(tmpdir.path, `database-dir-${cnt++}`); + const dbPath = join(dbDir, 'failed-open.db'); + using db = new DatabaseSync(dbPath, { open: false }); + + // The directory does not exist, so opening the database fails. + t.assert.throws(() => { + db.open(); + }, { + code: 'ERR_SQLITE_ERROR', + message: /unable to open database file/, + }); + t.assert.strictEqual(db.isOpen, false); + + // The connection must not be usable after a failed open. + t.assert.throws(() => { + db.exec('SELECT 1'); + }, { + code: 'ERR_INVALID_STATE', + message: /database is not open/, + }); + t.assert.throws(() => { + db.function('fn', () => {}); + }, { + code: 'ERR_INVALID_STATE', + message: /database is not open/, + }); + + // The database can be opened once the underlying problem is resolved. + mkdirSync(dbDir); + t.assert.strictEqual(db.open(), undefined); + t.assert.strictEqual(db.isOpen, true); + db.exec('CREATE TABLE foo (id INTEGER PRIMARY KEY)'); + }); }); suite('DatabaseSync.prototype.close()', () => {