sqlite: fix segfault in expandedSQL

[tniessen]

The call to sqlite3_expanded_sql() may return NULL depending on various factors. Handle this case instead of running into a segmentation fault.

[cjihrig]

LGTM. Thanks. I've had this on my TODO list since coverity reported it a while back.

[targos]

Isn't there a way to test it?

[tniessen]

@targos It's easiest with different compile-time options, which is what I did locally. I'm not sure if the current JavaScript API allows configuring SQLite in a way that doesn't require a lot of memory for triggering the segmentation fault. I can try that later.

[codecov]

Codecov Report

Attention: Patch coverage is 57.14286% with 6 lines in your changes missing coverage. Please review.

Project coverage is 87.60%. Comparing base (4c844a2) to head (d4685f0).
Report is 259 commits behind head on main.

Files with missing lines	Patch %	Lines
src/node_sqlite.cc	57.14%	5 Missing and 1 partial ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #54687 +/- ##
==========================================
+ Coverage 87.33% 87.60% +0.27% 
==========================================
Files 650 650 Lines 182832 182843 +11 Branches 35067 35384 +317 ==========================================
+ Hits 159670 160184 +514 + Misses 16421 15937 -484 + Partials 6741 6722 -19 

Files with missing lines	Coverage Δ
src/node_sqlite.cc	83.05% <57.14%> (-0.95%)	⬇️

... and 67 files with indirect coverage changes

[tniessen]

@targos Here is a demonstration using the default options and the official Node.js 22.7.0 binary:

const{ DatabaseSync }=require('node:sqlite');constdb=newDatabaseSync('tmp.sqlite');db.exec('CREATE TABLE a (t TEXT NOT NULL) STRICT;');consttargetStringSize=1024*1024*1024;constnRows=10_000;constvalueLength=targetStringSize/nRows;conststmt=db.prepare(`INSERT INTO a (t) VALUES ${[...Array(nRows).keys()].map(()=>'(?)').join(',')};`);constvalue='A'.repeat(valueLength);constallValues=[...Array(nRows).keys()].map(()=>value);stmt.run(...allValues);console.log(stmt.expandedSQL());

The segmentation fault occurs after a few seconds:

$ node -vv22.7.0
$ time node --experimental-sqlite trigger-it.js Segmentation fault (core dumped)real 0m3.872suser 0m1.444ssys 0m1.590s

With this PR, instead, this happens:

$ ./node --experimental-sqlite trigger-it.js /home/tniessen/dev/node/trigger-it.js:15console.log(stmt.expandedSQL()); ^Error: Expanded SQL text would exceed configured limits at Object.<anonymous> (/home/tniessen/dev/node/trigger-it.js:15:18) at Module._compile (node:internal/modules/cjs/loader:1546:14) at Module._extensions..js (node:internal/modules/cjs/loader:1691:10) at Module.load (node:internal/modules/cjs/loader:1317:32) at Module._load (node:internal/modules/cjs/loader:1127:12) at TracingChannel.traceSync (node:diagnostics_channel:315:14) at wrapModuleLoad (node:internal/modules/cjs/loader:217:24) at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:166:5) at node:internal/main/run_main_module:30:49 { code: 'ERR_SQLITE_ERROR'}Node.js v23.0.0-pre

However, this test is quite slow and takes a lot of memory, so I am not sure if I should add it to our test suite.

[targos]

Thanks for demonstrating it. Let's not add the test.

[jasnell]

I know it's not new to this PR but this should likely return MaybeLocal<Object> and not depend on ToLocalChecked() and Check(), in order to be most consistent.

[nodejs-github-bot]

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

[nodejs-github-bot]

Landed in 17b49bd