| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
1 parent c7d27c8 commit 20f40c2
2 files changed
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
@@ -565,8 +565,10 @@ class BackupJob : public ThreadPoolWork { | |||
| 565 | 565 | TryCatch try_catch(env()->isolate()); | |
| 566 | 566 | USE(fn->Call(env()->context(), Null(env()->isolate()), 1, argv)); | |
| 567 | 567 | if (try_catch.HasCaught()) { | |
| 568 | + Local<Value> exception = try_catch.Exception(); | ||
| 568 | 569 | Finalize(); | |
| 569 | - resolver->Reject(env()->context(), try_catch.Exception()).ToChecked(); | ||
| 570 | + resolver->Reject(env()->context(), exception).ToChecked(); | ||
| 571 | + delete this; | ||
| 570 | 572 | return; | |
| 571 | 573 | } | |
| 572 | 574 | } | |
@@ -585,11 +587,15 @@ class BackupJob : public ThreadPoolWork { | |||
| 585 | 587 | resolver | |
| 586 | 588 | ->Resolve(env()->context(), Integer::New(env()->isolate(), total_pages)) | |
| 587 | 589 | .ToChecked(); | |
| 590 | + delete this; | ||
| 588 | 591 | } | |
| 589 | 592 | ||
| 590 | 593 | void Finalize() { | |
| 591 | 594 | Cleanup(); | |
| 592 | - source_->RemoveBackup(this); | ||
| 595 | + if (source_) { | ||
| 596 | + source_->RemoveBackup(this); | ||
| 597 | + source_.reset(); | ||
| 598 | + } | ||
| 593 | 599 | } | |
| 594 | 600 | ||
| 595 | 601 | void Cleanup() { | |
@@ -610,28 +616,32 @@ class BackupJob : public ThreadPoolWork { | |||
| 610 | 616 | Local<Object> e; | |
| 611 | 617 | if (!CreateSQLiteError(env()->isolate(), dest_).ToLocal(&e)) { | |
| 612 | 618 | Finalize(); | |
| 619 | + delete this; | ||
| 613 | 620 | return; | |
| 614 | 621 | } | |
| 615 | 622 | ||
| 616 | 623 | Finalize(); | |
| 617 | 624 | resolver->Reject(env()->context(), e).ToChecked(); | |
| 625 | + delete this; | ||
| 618 | 626 | } | |
| 619 | 627 | ||
| 620 | 628 | void HandleBackupError(Local<Promise::Resolver> resolver, int errcode) { | |
| 621 | 629 | Local<Object> e; | |
| 622 | 630 | if (!CreateSQLiteError(env()->isolate(), errcode).ToLocal(&e)) { | |
| 623 | 631 | Finalize(); | |
| 632 | + delete this; | ||
| 624 | 633 | return; | |
| 625 | 634 | } | |
| 626 | 635 | ||
| 627 | 636 | Finalize(); | |
| 628 | 637 | resolver->Reject(env()->context(), e).ToChecked(); | |
| 638 | + delete this; | ||
| 629 | 639 | } | |
| 630 | 640 | ||
| 631 | 641 | Environment* env() const { return env_; } | |
| 632 | 642 | ||
| 633 | 643 | Environment* env_; | |
| 634 | - DatabaseSync* source_; | ||
| 644 | + BaseObjectPtr<DatabaseSync> source_; | ||
| 635 | 645 | Global<Promise::Resolver> resolver_; | |
| 636 | 646 | Global<Function> progressFunc_; | |
| 637 | 647 | sqlite3* dest_ = nullptr; | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
@@ -1,3 +1,4 @@ | |||
| 1 | + // Flags: --expose-gc | ||
| 1 | 2 | import { isWindows, skipIfSQLiteMissing } from '../common/index.mjs'; | |
| 2 | 3 | import tmpdir from '../common/tmpdir.js'; | |
| 3 | 4 | import { join } from 'node:path'; | |
@@ -314,3 +315,43 @@ test('backup has correct name and length', (t) => { | |||
| 314 | 315 | t.assert.strictEqual(backup.name, 'backup'); | |
| 315 | 316 | t.assert.strictEqual(backup.length, 2); | |
| 316 | 317 | }); | |
| 318 | + | ||
| 319 | + test('source database is kept alive while a backup is in flight', async (t) => { | ||
| 320 | + // Regression test: previously, BackupJob stored a raw DatabaseSync* and the | ||
| 321 | + // source could be garbage-collected while the backup was still running, | ||
| 322 | + // leading to a use-after-free when BackupJob::Finalize() dereferenced the | ||
| 323 | + // stale pointer via source_->RemoveBackup(this). | ||
| 324 | + const destDb = nextDb(); | ||
| 325 | + | ||
| 326 | + let database = makeSourceDb(); | ||
| 327 | + // Insert enough rows to ensure the backup takes multiple steps. | ||
| 328 | + const insert = database.prepare('INSERT INTO data (key, value) VALUES (?, ?)'); | ||
| 329 | + for (let i = 3; i <= 500; i++) { | ||
| 330 | + insert.run(i, 'A'.repeat(1024) + i); | ||
| 331 | + } | ||
| 332 | + | ||
| 333 | + const p = backup(database, destDb, { | ||
| 334 | + rate: 1, | ||
| 335 | + progress() {}, | ||
| 336 | + }); | ||
| 337 | + // Drop the last strong JS reference to the source database. With the bug, | ||
| 338 | + // the DatabaseSync could be collected here and the in-flight backup would | ||
| 339 | + // later crash while accessing the freed source. | ||
| 340 | + database = null; | ||
| 341 | + | ||
| 342 | + // Nudge the GC aggressively, but the backup must keep the source alive | ||
| 343 | + // regardless. Without the fix, the source DatabaseSync would be collected | ||
| 344 | + // and BackupJob::Finalize() would crash the process. | ||
| 345 | + for (let i = 0; i < 5; i++) { | ||
| 346 | + global.gc(); | ||
| 347 | + await new Promise((resolve) => setImmediate(resolve)); | ||
| 348 | + } | ||
| 349 | + | ||
| 350 | + const totalPages = await p; | ||
| 351 | + t.assert.ok(totalPages > 0); | ||
| 352 | + | ||
| 353 | + const backupDb = new DatabaseSync(destDb); | ||
| 354 | + t.after(() => { backupDb.close(); }); | ||
| 355 | + const rows = backupDb.prepare('SELECT COUNT(*) AS n FROM data').get(); | ||
| 356 | + t.assert.strictEqual(rows.n, 500); | ||
| 357 | + }); | ||
| Back | FazBrowse Home | New Git URL |
0 commit comments