GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | |||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | |||||||||||||||||||||||||||||||
GeezerVison is showing github.com — sqlite: fix segfault in expandedSQL by tniessen · Pull Request #54687 · nodejs/node Conversation
Member
The call to
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. All reactions
The call to sqlite3_expanded_sql() may return NULL depending on various factors. Handle this case instead of running into a segmentation fault.
cjihrig
approved these changes
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. LGTM. Thanks. I've had this on my TODO list since coverity reported it a while back.
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. All reactions
Member
All reactions
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page.
Member
Author
All reactions
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page.
|
There was an error while loading. Please reload this page.
Codecov ReportAttention: Patch coverage is
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
|
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
@targos Here is a demonstration using the default options and the official Node.js 22.7.0 binary: const { DatabaseSync } = require('node:sqlite'); const db = new DatabaseSync('tmp.sqlite'); db.exec('CREATE TABLE a (t TEXT NOT NULL) STRICT;'); const targetStringSize = 1024 * 1024 * 1024; const nRows = 10_000; const valueLength = targetStringSize / nRows; const stmt = db.prepare(`INSERT INTO a (t) VALUES ${[...Array(nRows).keys()].map(() => '(?)').join(',')};`); const value = 'A'.repeat(valueLength); const allValues = [...Array(nRows).keys()].map(() => value); stmt.run(...allValues); console.log(stmt.expandedSQL()); The segmentation fault occurs after a few seconds: $ node -v v22.7.0 $ time node --experimental-sqlite trigger-it.js Segmentation fault (core dumped) real 0m3.872s user 0m1.444s sys 0m1.590s With this PR, instead, this happens: $ ./node --experimental-sqlite trigger-it.js /home/tniessen/dev/node/trigger-it.js:15 console.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. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Thanks for demonstrating it. Let's not add the test. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| Local<Object> e = Exception::Error(js_msg) | ||
| ->ToObject(isolate->GetCurrentContext()) | ||
| .ToLocalChecked(); | ||
| e->Set(isolate->GetCurrentContext(), |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
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.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Landed in 17b49bd |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
The call to sqlite3_expanded_sql() may return NULL depending on various factors. Handle this case instead of running into a segmentation fault. PR-URL: #54687 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
As per James' suggestion, consistently use MaybeLocal and avoid Check() and ToLocalChecked() entirely. Refs: nodejs#54687
As per James' suggestion, consistently use MaybeLocal and avoid Check() and ToLocalChecked() entirely. Refs: #54687 PR-URL: #55571 Reviewed-By: Colin Ihrig <[email protected]>
As per James' suggestion, consistently use MaybeLocal and avoid Check() and ToLocalChecked() entirely. Refs: #54687 PR-URL: #55571 Reviewed-By: Colin Ihrig <[email protected]>
As per James' suggestion, consistently use MaybeLocal and avoid Check() and ToLocalChecked() entirely. Refs: #54687 PR-URL: #55571 Reviewed-By: Colin Ihrig <[email protected]>
As per James' suggestion, consistently use MaybeLocal and avoid Check() and ToLocalChecked() entirely. Refs: #54687 PR-URL: #55571 Reviewed-By: Colin Ihrig <[email protected]>
As per James' suggestion, consistently use MaybeLocal and avoid Check() and ToLocalChecked() entirely. Refs: #54687 PR-URL: #55571 Reviewed-By: Colin Ihrig <[email protected]>
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.