Skip to content

sqlite: keep source database alive during backup#62673

Merged
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
mcollina:fix-sqlite-backup-uaf
May 6, 2026
Merged

sqlite: keep source database alive during backup#62673
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
mcollina:fix-sqlite-backup-uaf

Conversation

@mcollina

@mcollina mcollina commented Apr 10, 2026

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

nodejs-github-bot commented Apr 10, 2026

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
geeksilva97
geeksilva97 reviewed Apr 10, 2026
View reviewed changes
Comment thread test/parallel/test-sqlite-backup.mjs Outdated
@codecov

codecov Bot commented Apr 10, 2026 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 37.50000% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.64%. Comparing base (449a93a) to head (4c42f91).
⚠️ Report is 319 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.64%   -0.14%     
==========================================
  Files         697      708      +11     
  Lines      215749   220412    +4663     
  Branches    41294    42276     +982     
==========================================
+ Hits       193705   197596    +3891     
- Misses      14124    14663     +539     
- Partials     7920     8153     +233
Files with missing lines Coverage Δ
src/node_sqlite.cc 80.54% <37.50%> (-0.27%) ⬇️

... and 146 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
@targos

targos commented Apr 13, 2026

Copy link
Copy Markdown
Member

@nodejs/cpp-reviewers

lemire
lemire approved these changes Apr 13, 2026
View reviewed changes

@lemire lemire left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM.

This class could probably be refactored and made simpler.

geeksilva97
geeksilva97 reviewed Apr 14, 2026
View reviewed changes
Comment thread test/parallel/test-sqlite-backup.mjs

@geeksilva97 geeksilva97 Apr 14, 2026 •
edited
Loading

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.

Need to add

// Flags: --expose_gc

tniessen
tniessen approved these changes Apr 14, 2026
View reviewed changes

@tniessen tniessen left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM besides adding the required flag to the test case.

geeksilva97
geeksilva97 approved these changes Apr 14, 2026
View reviewed changes

@geeksilva97 geeksilva97 left a comment

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.

When flag is added I reapprove and we get CI running

geeksilva97
geeksilva97 reviewed Apr 14, 2026
View reviewed changes
Comment thread AGENTS.md Outdated
@mcollina mcollina force-pushed the fix-sqlite-backup-uaf branch from ce50849 to 53dc8bc Compare April 29, 2026 21:36
@geeksilva97

geeksilva97 commented Apr 29, 2026

Copy link
Copy Markdown
Contributor

Only linter complaining now.

@mcollina
sqlite: keep source database alive during backup
4c42f91 
Signed-off-by: Matteo Collina <hello@matteocollina.com>
@mcollina mcollina force-pushed the fix-sqlite-backup-uaf branch from 53dc8bc to 4c42f91 Compare April 30, 2026 07:46
@mcollina mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label May 5, 2026
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 5, 2026
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@geeksilva97

geeksilva97 commented May 6, 2026

Copy link
Copy Markdown
Contributor

Retried CI

@nodejs-github-bot

nodejs-github-bot commented May 6, 2026

Copy link
Copy Markdown
Collaborator

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

@mcollina mcollina added the commit-queue Add this label to land a pull request using GitHub Actions. label May 6, 2026
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label May 6, 2026
@nodejs-github-bot nodejs-github-bot merged commit b5da751 into nodejs:main May 6, 2026
73 checks passed
@nodejs-github-bot

nodejs-github-bot commented May 6, 2026

Copy link
Copy Markdown
Collaborator

Landed in b5da751

aduh95 pushed a commit that referenced this pull request May 19, 2026
@mcollina @aduh95
sqlite: keep source database alive during backup
fe2bc01 
Signed-off-by: Matteo Collina <hello@matteocollina.com>
PR-URL: #62673
Reviewed-By: Daniel Lemire <daniel@lemire.me>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Edy Silva <edigleyssonsilva@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
aduh95 pushed a commit that referenced this pull request May 19, 2026
@mcollina @aduh95
sqlite: keep source database alive during backup
20f40c2 
Signed-off-by: Matteo Collina <hello@matteocollina.com>
PR-URL: #62673
Reviewed-By: Daniel Lemire <daniel@lemire.me>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Edy Silva <edigleyssonsilva@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@github-actions github-actions Bot mentioned this pull request May 19, 2026
Merged
aduh95 pushed a commit that referenced this pull request May 23, 2026
@mcollina @aduh95
sqlite: keep source database alive during backup
75143da 
Signed-off-by: Matteo Collina <hello@matteocollina.com>
PR-URL: #62673
Reviewed-By: Daniel Lemire <daniel@lemire.me>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Edy Silva <edigleyssonsilva@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
araujogui pushed a commit to araujogui/node that referenced this pull request May 26, 2026
@mcollina @araujogui
sqlite: keep source database alive during backup
92f0619 
Signed-off-by: Matteo Collina <hello@matteocollina.com>
PR-URL: nodejs#62673
Reviewed-By: Daniel Lemire <daniel@lemire.me>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Edy Silva <edigleyssonsilva@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lemire lemire lemire approved these changes

@jasnell jasnell jasnell approved these changes

@tniessen tniessen tniessen approved these changes

@geeksilva97 geeksilva97 geeksilva97 approved these changes

Assignees

No one assigned

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@mcollina @nodejs-github-bot @targos @geeksilva97 @lemire @jasnell @tniessen