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

All modified and coverable lines are covered by tests ✅

Project coverage is 89.21%. Comparing base (c4fb331) to head (597b2c2).

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #56840   +/-   ##
=======================================
  Coverage   89.21%   89.21%           
=======================================
  Files         665      665           
  Lines      192514   192513    -1     
  Branches    37040    37041    +1     
=======================================
+ Hits       171744   171753    +9     
+ Misses      13613    13606    -7     
+ Partials     7157     7154    -3     

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

... and 36 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); });