sqlite: keep source database alive during backup

Author: mcollina

src/node_sqlite.cc

@@ -546,8 +546,10 @@ class BackupJob : public ThreadPoolWork {
         TryCatch try_catch(env()->isolate());
         USE(fn->Call(env()->context(), Null(env()->isolate()), 1, argv));
         if (try_catch.HasCaught()) {
+          Local<Value> exception = try_catch.Exception();
           Finalize();
-          resolver->Reject(env()->context(), try_catch.Exception()).ToChecked();
+          resolver->Reject(env()->context(), exception).ToChecked();
+          delete this;
           return;
         }
       }
@@ -566,11 +568,15 @@ class BackupJob : public ThreadPoolWork {
     resolver
         ->Resolve(env()->context(), Integer::New(env()->isolate(), total_pages))
         .ToChecked();
+    delete this;
   }
 
   void Finalize() {
     Cleanup();
-    source_->RemoveBackup(this);
+    if (source_) {
+      source_->RemoveBackup(this);
+      source_.reset();
+    }
   }
 
   void Cleanup() {
@@ -591,28 +597,32 @@ class BackupJob : public ThreadPoolWork {
     Local<Object> e;
     if (!CreateSQLiteError(env()->isolate(), dest_).ToLocal(&e)) {
       Finalize();
+      delete this;
       return;
     }
 
     Finalize();
     resolver->Reject(env()->context(), e).ToChecked();
+    delete this;
   }
 
   void HandleBackupError(Local<Promise::Resolver> resolver, int errcode) {
     Local<Object> e;
     if (!CreateSQLiteError(env()->isolate(), errcode).ToLocal(&e)) {
       Finalize();
+      delete this;
       return;
     }
 
     Finalize();
     resolver->Reject(env()->context(), e).ToChecked();
+    delete this;
   }
 
   Environment* env() const { return env_; }
 
   Environment* env_;
-  DatabaseSync* source_;
+  BaseObjectPtr<DatabaseSync> source_;
   Global<Promise::Resolver> resolver_;
   Global<Function> progressFunc_;
   sqlite3* dest_ = nullptr;

test/parallel/test-sqlite-backup.mjs

@@ -314,3 +314,45 @@ test('backup has correct name and length', (t) => {
   t.assert.strictEqual(backup.name, 'backup');
   t.assert.strictEqual(backup.length, 2);
 });
+
+test('source database is kept alive while a backup is in flight', async (t) => {
+  // Regression test: previously, BackupJob stored a raw DatabaseSync* and the
+  // source could be garbage-collected while the backup was still running,
+  // leading to a use-after-free when BackupJob::Finalize() dereferenced the
+  // stale pointer via source_->RemoveBackup(this).
+  const destDb = nextDb();
+
+  let database = makeSourceDb();
+  // Insert enough rows to ensure the backup takes multiple steps.
+  const insert = database.prepare('INSERT INTO data (key, value) VALUES (?, ?)');
+  for (let i = 3; i <= 500; i++) {
+    insert.run(i, 'A'.repeat(1024) + i);
+  }
+
+  const p = backup(database, destDb, {
+    rate: 1,
+    progress() {},
+  });
+  // Drop the last strong JS reference to the source database. With the bug,
+  // the DatabaseSync could be collected here and the in-flight backup would
+  // later crash while accessing the freed source.
+  database = null;
+
+  // Nudge the GC aggressively, but the backup must keep the source alive
+  // regardless. Without the fix, the source DatabaseSync would be collected
+  // and BackupJob::Finalize() would crash the process.
+  if (typeof global.gc === 'function') {
+    for (let i = 0; i < 5; i++) {
+      global.gc();
+      await new Promise((resolve) => setImmediate(resolve));
+    }
+  }
+
+  const totalPages = await p;
+  t.assert.ok(totalPages > 0);
+
+  const backupDb = new DatabaseSync(destDb);
+  t.after(() => { backupDb.close(); });
+  const rows = backupDb.prepare('SELECT COUNT(*) AS n FROM data').get();
+  t.assert.strictEqual(rows.n, 500);
+});