fixup! sqlite: add virtual table support via createModule()

Address review feedback from trivikr. createModule() re-checks that the database is still open after reading the options bag and the column definitions. A property getter there runs user JavaScript, so close() could release the connection before sqlite3_create_module_v2() received it, and SQLite dereferenced the null handle. Seven other entry points in this file already carry the same re-check, and test-sqlite-options-getter-reentry.js covers the hazard for nine APIs; createModule() was the one that skipped it. xClose no longer calls PropagateJSError when the iterator's return() throws. SQLite discards xClose's return value, so no SQLite error is generated for the suppression flag to consume; it stayed set until the next unrelated statement, where prepare() returned undefined instead of throwing ERR_SQLITE_ERROR. A SQLite error that does follow the cleanup throw now surfaces instead of being suppressed in favor of the JavaScript error. Signed-off-by: Trevor Burnham <trevorburnham@gmail.com> Assisted-by: Claude Opus 5
nodejs-github-bot · Sep 22, 2026 · Sep 4, 2026 · Sep 8, 2026 · Sep 8, 2026 · Sep 8, 2026
commit f15a71d81fb58fc9273669f1abf372782e38830e
diff --git a/src/node_sqlite.cc b/src/node_sqlite.cc
@@ -1236,11 +1236,11 @@ int VirtualTableModule::xClose(sqlite3_vtab_cursor* pCursor) {
           env->context(), iterator, 0, nullptr));
     }
 
-    // Re-throw so that an error already pending when SQLite unwound into
-    // xClose still reaches the caller, and so that a throwing `finally` is not
-    // silently discarded.
+    // Re-throw so that a throwing `finally` is not silently discarded. SQLite
+    // discards xClose's return value, so there is no SQLite error here to
+    // suppress; calling PropagateJSError would leave the suppression flag set
+    // and swallow the next unrelated SQLite error.
     if (try_catch.HasCaught() && !try_catch.HasTerminated()) {
-      mod->PropagateJSError();
       try_catch.ReThrow();
     }
   }
@@ -3020,6 +3020,11 @@ void DatabaseSync::CreateModule(const FunctionCallbackInfo<Value>& args) {
 
   schema_sql += ")";
 
+  // Reading the options bag and the column definitions above can run user
+  // JavaScript through a property getter, which may have closed the database
+  // since it was checked.
+  THROW_AND_RETURN_ON_BAD_STATE(env, !db->IsOpen(), "database is not open");
+
   VirtualTableModule* vtab_mod =
       new VirtualTableModule(env,
                              BaseObjectWeakPtr<DatabaseSync>(db),

diff --git a/test/parallel/test-sqlite-options-getter-reentry.js b/test/parallel/test-sqlite-options-getter-reentry.js
@@ -84,6 +84,40 @@ suite('closing the database from an options getter', () => {
     }, invalidState);
   });
 
+  test('createModule() throws instead of using a closed connection', (t) => {
+    const db = new DatabaseSync(':memory:');
+    t.assert.throws(() => {
+      db.createModule('mod', {
+        columns: [{ name: 'value', type: 'INTEGER' }],
+        *rows() {
+          yield [1];
+        },
+        get directOnly() {
+          db.close();
+          return false;
+        },
+      });
+    }, invalidState);
+  });
+
+  test('createModule() throws when a column getter closes the database', (t) => {
+    const db = new DatabaseSync(':memory:');
+    t.assert.throws(() => {
+      db.createModule('mod', {
+        columns: [{
+          name: 'value',
+          get type() {
+            db.close();
+            return 'INTEGER';
+          },
+        }],
+        *rows() {
+          yield [1];
+        },
+      });
+    }, invalidState);
+  });
+
   test('deserialize() throws instead of using a closed connection', (t) => {
     const source = new DatabaseSync(':memory:');
     source.exec('CREATE TABLE data(value TEXT)');

diff --git a/test/parallel/test-sqlite-virtual-table.js b/test/parallel/test-sqlite-virtual-table.js
@@ -587,6 +587,33 @@ suite('DatabaseSync.prototype.createModule()', () => {
       }
       assert.strictEqual(cleanedUp, false);
     });
+
+    test('a throwing cleanup does not swallow the next SQLite error', () => {
+      // SQLite discards xClose's return value, so a cleanup error there has no
+      // SQLite error to pair with. Suppressing one would leave the suppression
+      // flag set for the next unrelated statement.
+      const db = new DatabaseSync(':memory:');
+
+      db.createModule('cleanup_leak', {
+        columns: [{ name: 'value', type: 'INTEGER' }],
+        rows() {
+          let i = 0;
+          return {
+            [Symbol.iterator]() { return this; },
+            next() { return { value: [i++], done: i > 50 }; },
+            return() { throw new Error('cleanup boom'); },
+          };
+        },
+      });
+
+      assert.throws(() => {
+        db.prepare('SELECT value FROM cleanup_leak LIMIT 1').all();
+      }, /cleanup boom/);
+
+      assert.throws(() => {
+        db.prepare('SELECT * FROM no_such_table');
+      }, { code: 'ERR_SQLITE_ERROR', message: /no such table: no_such_table/ });
+    });
   });
 
   suite('iteration protocol violations', () => {