Skip to content

sqlite: keep source database alive during backup#62673

Open
mcollina wants to merge 1 commit intonodejs:mainfrom
mcollina:fix-sqlite-backup-uaf
Open

sqlite: keep source database alive during backup#62673
mcollina wants to merge 1 commit intonodejs:mainfrom
mcollina:fix-sqlite-backup-uaf

Conversation

@mcollina
Copy link
Copy Markdown
Member

BackupJob held a raw DatabaseSync* to the source. If the source lost its last JS reference during an in-flight backup(), GC could destroy it while the job was still pending, and Finalize() would later dereference a dangling pointer via source_->RemoveBackup(this).

Switch BackupJob::source_ to BaseObjectPtr<DatabaseSync> so the source stays alive for the lifetime of the job, and delete the job at its terminal states (previously leaked).

@nodejs-github-bot
Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/sqlite

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. sqlite Issues and PRs related to the SQLite subsystem. labels Apr 10, 2026
Comment on lines +344 to +349
if (typeof global.gc === 'function') {
for (let i = 0; i < 5; i++) {
global.gc();
await new Promise((resolve) => setImmediate(resolve));
}
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can add the flag at the top

// Flags: --expose_gc

Then do

Suggested change
if (typeof global.gc === 'function') {
for (let i = 0; i < 5; i++) {
global.gc();
await new Promise((resolve) => setImmediate(resolve));
}
}
for (let i = 0; i < 5; i++) {
global.gc();
await new Promise((resolve) => setImmediate(resolve));
}

@codecov
Copy link
Copy Markdown

codecov bot commented Apr 10, 2026

Codecov Report

❌ Patch coverage is 37.50000% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.81%. Comparing base (449a93a) to head (a30f3ca).
⚠️ Report is 51 commits behind head on main.

Files with missing lines Patch % Lines
src/node_sqlite.cc 37.50% 1 Missing and 4 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #62673      +/-   ##
==========================================
+ Coverage   89.78%   89.81%   +0.02%     
==========================================
  Files         697      699       +2     
  Lines      215749   216373     +624     
  Branches    41294    41364      +70     
==========================================
+ Hits       193705   194327     +622     
- Misses      14124    14145      +21     
+ Partials     7920     7901      -19     
Files with missing lines Coverage Δ
src/node_sqlite.cc 80.54% <37.50%> (-0.27%) ⬇️

... and 61 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@mcollina mcollina force-pushed the fix-sqlite-backup-uaf branch from b9c80c9 to a30f3ca Compare April 11, 2026 17:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. sqlite Issues and PRs related to the SQLite subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants