FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

sqlite: fix undefined behaviour in `Session::Changeset()` by ndossche · Pull Request #63637 · nodejs/node · GitHub

/ node Public

sqlite: fix undefined behaviour in Session::Changeset() - #63637

Merged
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
ndossche:sqlite-ubsan-1
Aug 6, 2026
ConversationCommits (1)ChecksFiles changed
Merged

sqlite: fix undefined behaviour in Session::Changeset()#63637
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
ndossche:sqlite-ubsan-1

Conversation

ndossche commented May 29, 2026

Copy link
Copy Markdown
Contributor

If nChangeset == 0, the pointer pChangeset may be nullptr. Passing a nullptr to memcpy() is undefined behaviour. This can be triggered by running the test suite under UBSAN.

../src/node_sqlite.cc:3342:42: runtime error: null pointer passed as argument 1, which is declared to never be null
/usr/include/string.h:44:28: note: nonnull attribute specified here
    #0 0x64b18916cde5 in void node::sqlite::Session::Changeset<&sqlite3session_changeset>(v8::FunctionCallbackInfo<v8::Value> const&) /work/node/out/../src/node_sqlite.cc:3342:3
    #1 0x7c81ae412f8c in Builtins_CallApiCallbackGeneric embedded.o
    #2 0x7c818e751ff0  (<unknown module>)

Note: this was found by a static-dynamic analyser I'm developing.

sqlite: fix undefined behaviour in Session::Changeset()
6600a2e 
If `nChangeset == 0`, the pointer `pChangeset` may be nullptr.
Passing a nullptr to `memcpy()` is undefined behaviour.
This can be triggered by running the test suite under UBSAN.

Signed-off-by: ndossche <nora.dossche@ugent.be>

nodejs-github-bot commented May 29, 2026

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/sqlite

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 May 29, 2026

louwers commented May 29, 2026

Copy link
Copy Markdown
Contributor

If sqlite3session_changeset returns SQLITE_OK the returned changeset is not nullptr. https://sqlite.org/session/sqlite3session_changeset.html

If successful, set *ppChangeset to point to a buffer containing the changeset and *pnChangeset to the size of the changeset in bytes before returning SQLITE_OK

codecov Bot commented May 29, 2026 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 66.66667% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 90.29%. Comparing base (40dc5a1) to head (6600a2e).
⚠️ Report is 743 commits behind head on main.

Files with missing lines Patch % Lines
src/node_sqlite.cc 66.66% 0 Missing and 1 partial ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main   #63637      +/-   ##
==========================================
- Coverage   90.30%   90.29%   -0.02%     
==========================================
  Files         730      730              
  Lines      234802   234804       +2     
  Branches    43957    43951       -6     
==========================================
- Hits       212041   212017      -24     
- Misses      14485    14493       +8     
- Partials     8276     8294      +18
Files with missing lines Coverage Δ
src/node_sqlite.cc 80.60% <66.66%> (-0.03%) ⬇️

... and 31 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.

ndossche commented May 29, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

If sqlite3session_changeset returns SQLITE_OK the returned changeset is not nullptr. https://sqlite.org/session/sqlite3session_changeset.html

If successful, set *ppChangeset to point to a buffer containing the changeset and *pnChangeset to the size of the changeset in bytes before returning SQLITE_OK

Well, I tested this without error injection, so perhaps the documentation is wrong.

https://github.com/sqlite/sqlite/blob/6a8e887a64798221b633068f882b2ac2c48a3cbb/ext/session/sqlite3session.c#L3191-L3194

This success return path takes the size and pointer from the buf object, which is initialized to all zero bytes at the start of the function. If the loop does not run then both the pointer and size will be nullptr/0.

Renegade334 approved these changes May 29, 2026
View reviewed changes

Renegade334 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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

This success return path takes the size and pointer from the buf object, which is initialized to all zero bytes at the start of the function. If the loop does not run then both the pointer and size will be nullptr/0.

This analysis appears correct to me, although libc's are realistically going to handle this gracefully, even if technically UB.

(It's becoming defined behaviour in c2y 😃)

ndossche commented May 29, 2026

Copy link
Copy Markdown
Contributor Author

although libc's are realistically going to handle this gracefully, even if technically UB.

Right, although the main concern is what a smart compiler may do ;p

(It's becoming defined behaviour in c2y 😃)

Fortunately

louwers commented May 29, 2026

Copy link
Copy Markdown
Contributor

OK maybe we should report this upstream, the SQLite folks are amazingly responsive.

trivikr added request-ci Add this label to start a Jenkins CI on a PR. author ready PRs that have at least one approval, no outstanding review comments, and a CI started. labels Jul 25, 2026
github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 26, 2026

This comment was marked as outdated.

Sign in to view
TrevorBurnham mentioned this pull request Aug 4, 2026
Open

This comment was marked as outdated.

Sign in to view

nodejs-github-bot commented Aug 6, 2026

Copy link
Copy Markdown
Collaborator

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

trivikr added the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 6, 2026
nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 6, 2026
nodejs-github-bot merged commit c18fd90 into nodejs:main Aug 6, 2026
89 checks passed

nodejs-github-bot commented Aug 6, 2026

Copy link
Copy Markdown
Collaborator

Landed in c18fd90

aduh95 pushed a commit that referenced this pull request Aug 13, 2026
sqlite: fix undefined behaviour in Session::Changeset()
5754b49 
If `nChangeset == 0`, the pointer `pChangeset` may be nullptr.
Passing a nullptr to `memcpy()` is undefined behaviour.
This can be triggered by running the test suite under UBSAN.

Signed-off-by: ndossche <nora.dossche@ugent.be>
PR-URL: #63637
Reviewed-By: René <contact.9a5d6388@renegade334.me.uk>
aduh95 pushed a commit that referenced this pull request Aug 25, 2026
sqlite: fix undefined behaviour in Session::Changeset()
09d0da9 
If `nChangeset == 0`, the pointer `pChangeset` may be nullptr.
Passing a nullptr to `memcpy()` is undefined behaviour.
This can be triggered by running the test suite under UBSAN.

Signed-off-by: ndossche <nora.dossche@ugent.be>
PR-URL: #63637
Reviewed-By: René <contact.9a5d6388@renegade334.me.uk>
github-actions Bot mentioned this pull request Aug 25, 2026
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Renegade334 Renegade334 approved these changes

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, and a CI started. 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.

5 participants

Back | FazBrowse Home | New Git URL