sqlite: fix use-after-free in StatementSync due to premature GC

[littledivy]

This patch updates StatementSync to store a strong reference to the database base object.

DatabaseSync may be garbage collected and freed while StatementSync is using it (due to MakeWeak()). The following code crashes with a segmentation fault:

import { DatabaseSync } from "node:sqlite";

const db = new DatabaseSync(':memory:');
db.exec('CREATE TABLE test (value INTEGER)');

const stmt = db.prepare('INSERT INTO test VALUES (?)');

for(;;) stmt.run(0);

* thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x38)
    frame #0: 0x000000010036e358 node`node::sqlite::StatementSync::Run(v8::FunctionCallbackInfo<v8::Value> const&) + 432
node`node::sqlite::StatementSync::Run:
->  0x10036e358 <+432>: ldr    x1, [x8, #0x38]
    0x10036e35c <+436>: ldr    x22, [x8, #0x78]
    0x10036e360 <+440>: ldrb   w8, [x19, #0x40]
    0x10036e364 <+444>: ldr    x24, [x25, #0xa8]

[cjihrig]

LGTM, thanks!

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/64875/

[codecov]

Codecov Report

Attention: Patch coverage is 50.00000% with 7 lines in your changes missing coverage. Please review.

Project coverage is 89.20%. Comparing base (316ac8c) to head (e91a418).
Report is 9 commits behind head on main.

Files with missing lines	Patch %	Lines
src/node_sqlite.cc	50.00%	0 Missing and 7 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #56840      +/-   ##
==========================================
+ Coverage   89.18%   89.20%   +0.01%     
==========================================
  Files         665      665              
  Lines      192543   192542       -1     
  Branches    37045    37051       +6     
==========================================
+ Hits       171726   171749      +23     
- Misses      13636    13639       +3     
+ Partials     7181     7154      -27     

Files with missing lines	Coverage Δ
src/node_sqlite.h	70.00% <ø> (ø)
src/node_sqlite.cc	79.82% <50.00%> (-0.02%)	⬇️

... and 21 files with indirect coverage changes

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/64885/

[cjihrig]

Hm, it looks like there may be related failures on Windows while trying to clean up:

16:58:48     (node:12696) ExperimentalWarning: SQLite is an experimental feature and might change at any time
16:58:48     (Use `node --trace-warnings ...` to show where the warning was created)
16:58:48     Can't clean tmpdir: d:\workspace\node-test-binary-windows-js-suites\node\test\.tmp.729
16:58:48     Files blocking: [
16:58:48       'database-5.db',
16:58:48       'database-6.db',
16:58:48       'database-7.db',
16:58:48       'database-8.db',
16:58:48       'database-9.db'
16:58:48     ]
16:58:48     
16:58:48     d:\workspace\node-test-binary-windows-js-suites\node\test\common\tmpdir.js:69
16:58:48         throw e;
16:58:48         ^
16:58:48     
16:58:48     Error: EPERM, Permission denied: \\?\d:\workspace\node-test-binary-windows-js-suites\node\test\.tmp.729 '\\?\d:\workspace\node-test-binary-windows-js-suites\node\test\.tmp.729'
16:58:48         at Object.rmSync (node:fs:1250:18)
16:58:48         at rmSync (d:\workspace\node-test-binary-windows-js-suites\node\test\common\tmpdir.js:20:8)
16:58:48         at onexit (d:\workspace\node-test-binary-windows-js-suites\node\test\common\tmpdir.js:54:5)
16:58:48         at process.<anonymous> (d:\workspace\node-test-binary-windows-js-suites\node\test\common\tmpdir.js:43:14)
16:58:48         at process.emit (node:events:519:35) {
16:58:48       errno: 1,
16:58:48       code: 'EPERM',
16:58:48       path: '\\\\?\\d:\\workspace\\node-test-binary-windows-js-suites\\node\\test\\.tmp.729',
16:58:48       syscall: 'rm'
16:58:48     }
16:58:48     
16:58:48     Node.js v24.0.0-pre
16:58:48   ...

This happened a number of times on Windows. I'll restart the CI to see if that was a one off issue, but I have a feeling it's legit.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/64898/

[cjihrig]

The latest run had the same file files blocking cleanup. They appear to correspond to these five tests. I'm not sure off the top of my head why those ones would be blocking. I do see that the two iterate() tests never explicitly close the database, so maybe that is related. Using in memory databases might be an option too, but that seems like a less correct solution.

[littledivy]

I updated the two iterate() tests to close the db. The tests seem to pass locally on Windows.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/64949/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/64957/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/64976/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/64984/

[cjihrig]

@littledivy can you apply these changes. These should resolve the test failures now due to a conflicting change that landed.

diff --git a/src/node_sqlite.cc b/src/node_sqlite.cc
index 39e1be871d..1639c196ce 100644
--- a/src/node_sqlite.cc
+++ b/src/node_sqlite.cc
@@ -977,7 +977,7 @@ inline bool StatementSync::IsFinalized() {
 
 bool StatementSync::BindParams(const FunctionCallbackInfo<Value>& args) {
   int r = sqlite3_clear_bindings(statement_);
-  CHECK_ERROR_OR_THROW(env()->isolate(), db_, r, SQLITE_OK, false);
+  CHECK_ERROR_OR_THROW(env()->isolate(), db_.get(), r, SQLITE_OK, false);
 
   int anon_idx = 1;
   int anon_start = 0;
@@ -1107,7 +1107,7 @@ bool StatementSync::BindValue(const Local<Value>& value, const int index) {
     return false;
   }
 
-  CHECK_ERROR_OR_THROW(env()->isolate(), db_, r, SQLITE_OK, false);
+  CHECK_ERROR_OR_THROW(env()->isolate(), db_.get(), r, SQLITE_OK, false);
   return true;
 }
 
@@ -1173,7 +1173,7 @@ void StatementSync::All(const FunctionCallbackInfo<Value>& args) {
       env, stmt->IsFinalized(), "statement has been finalized");
   Isolate* isolate = env->isolate();
   int r = sqlite3_reset(stmt->statement_);
-  CHECK_ERROR_OR_THROW(isolate, stmt->db_, r, SQLITE_OK, void());
+  CHECK_ERROR_OR_THROW(isolate, stmt->db_.get(), r, SQLITE_OK, void());
 
   if (!stmt->BindParams(args)) {
     return;
@@ -1202,7 +1202,7 @@ void StatementSync::All(const FunctionCallbackInfo<Value>& args) {
     rows.emplace_back(row);
   }
 
-  CHECK_ERROR_OR_THROW(isolate, stmt->db_, r, SQLITE_DONE, void());
+  CHECK_ERROR_OR_THROW(isolate, stmt->db_.get(), r, SQLITE_DONE, void());
   args.GetReturnValue().Set(Array::New(isolate, rows.data(), rows.size()));
 }
 
@@ -1270,7 +1270,8 @@ void StatementSync::IterateNextCallback(
 
   int r = sqlite3_step(stmt->statement_);
   if (r != SQLITE_ROW) {
-    CHECK_ERROR_OR_THROW(env->isolate(), stmt->db_, r, SQLITE_DONE, void());
+    CHECK_ERROR_OR_THROW(
+        env->isolate(), stmt->db_.get(), r, SQLITE_DONE, void());
 
     // cleanup when no more rows to fetch
     sqlite3_reset(stmt->statement_);
@@ -1322,7 +1323,7 @@ void StatementSync::Iterate(const FunctionCallbackInfo<Value>& args) {
   auto isolate = env->isolate();
   auto context = env->context();
   int r = sqlite3_reset(stmt->statement_);
-  CHECK_ERROR_OR_THROW(env->isolate(), stmt->db_, r, SQLITE_OK, void());
+  CHECK_ERROR_OR_THROW(env->isolate(), stmt->db_.get(), r, SQLITE_OK, void());
 
   if (!stmt->BindParams(args)) {
     return;
@@ -1386,7 +1387,7 @@ void StatementSync::Get(const FunctionCallbackInfo<Value>& args) {
       env, stmt->IsFinalized(), "statement has been finalized");
   Isolate* isolate = env->isolate();
   int r = sqlite3_reset(stmt->statement_);
-  CHECK_ERROR_OR_THROW(isolate, stmt->db_, r, SQLITE_OK, void());
+  CHECK_ERROR_OR_THROW(isolate, stmt->db_.get(), r, SQLITE_OK, void());
 
   if (!stmt->BindParams(args)) {
     return;
@@ -1396,7 +1397,7 @@ void StatementSync::Get(const FunctionCallbackInfo<Value>& args) {
   r = sqlite3_step(stmt->statement_);
   if (r == SQLITE_DONE) return;
   if (r != SQLITE_ROW) {
-    THROW_ERR_SQLITE_ERROR(isolate, stmt->db_);
+    THROW_ERR_SQLITE_ERROR(isolate, stmt->db_.get());
     return;
   }
 
@@ -1432,7 +1433,7 @@ void StatementSync::Run(const FunctionCallbackInfo<Value>& args) {
   THROW_AND_RETURN_ON_BAD_STATE(
       env, stmt->IsFinalized(), "statement has been finalized");
   int r = sqlite3_reset(stmt->statement_);
-  CHECK_ERROR_OR_THROW(env->isolate(), stmt->db_, r, SQLITE_OK, void());
+  CHECK_ERROR_OR_THROW(env->isolate(), stmt->db_.get(), r, SQLITE_OK, void());
 
   if (!stmt->BindParams(args)) {
     return;
@@ -1441,7 +1442,7 @@ void StatementSync::Run(const FunctionCallbackInfo<Value>& args) {
   auto reset = OnScopeLeave([&]() { sqlite3_reset(stmt->statement_); });
   r = sqlite3_step(stmt->statement_);
   if (r != SQLITE_ROW && r != SQLITE_DONE) {
-    THROW_ERR_SQLITE_ERROR(env->isolate(), stmt->db_);
+    THROW_ERR_SQLITE_ERROR(env->isolate(), stmt->db_.get());
     return;
   }
 
@@ -1674,7 +1675,7 @@ void Session::Changeset(const FunctionCallbackInfo<Value>& args) {
   void* pChangeset;
   int r = sqliteChangesetFunc(session->session_, &nChangeset, &pChangeset);
   CHECK_ERROR_OR_THROW(
-      env->isolate(), session->database_, r, SQLITE_OK, void());
+      env->isolate(), session->database_.get(), r, SQLITE_OK, void());
 
   auto freeChangeset = OnScopeLeave([&] { sqlite3_free(pChangeset); });
 

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/64997/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/65003/

[cjihrig]

LGTM. Thanks again.

[nodejs-github-bot]

Landed in cebf4c8