sqlite: fix undefined behaviour in Session::Changeset()

[ndossche]

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.

[nodejs-github-bot]

Review requested:

• @nodejs/sqlite

[louwers]

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]

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 1 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]

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]

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]

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]

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