src: throw on a malformed localStorage file - #65879
Merged
nodejs-github-bot merged 5 commits intoSep 26, 2026
Merged
nodejs-github-bot merged 5 commits into
nodejs-github-bot merged 5 commits into
Conversation
Collaborator
|
Review requested:
|
nodejs-github-bot
added
c++
TrevorBurnham
changed the title
TrevorBurnham
force-pushed
the
webstorage-throw-on-malformed-file
branch
4 times, most recently
from
September 7, 2026 22:26
2f08679 to
43e1abd
Compare
TrevorBurnham
marked this pull request as ready for review
September 7, 2026 22:41
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #65879 +/- ##
==========================================
+ Coverage 90.27% 90.34% +0.06%
==========================================
Files 790 790
Lines 272810 273853 +1043
Branches 52098 52374 +276
==========================================
+ Hits 246280 247401 +1121
+ Misses 16995 16932 -63
+ Partials 9535 9520 -15
🚀 New features to boost your workflow:
|
trivikr
added
sqlite
cjihrig
approved these changes
Sep 12, 2026
The localStorage backing file is a user-specified path, and the schema is created with CREATE TABLE IF NOT EXISTS, so a file that already contains tables of those names is adopted as-is. Its stored values may then have any SQLite type, but every read asserted the expected type with CHECK, so a wrong-typed value aborted the process. A bad schema_version was the worst case: that assertion is in Storage::Open(), so any access aborted and the application had no chance to inspect or repair the file. Report these as ERR_INVALID_STATE instead, matching the throw four lines below the schema_version assertion for a version that is too new. Storage::GetAll() has no JavaScript caller to throw at, so it returns std::nullopt and the DOM storage inspector agent reports a protocol error. Now that a failed open returns instead of aborting, Open() has to clean up after itself: adopt the sqlite3* into a conn_unique_ptr immediately, so that an error does not leak the connection and leave the next access to open another one. Storage::GetAll() also ignored the result of sqlite3_prepare_v2() and the status its row loop ended on, reporting a malformed file or a mid-scan error as an empty store. Both now return std::nullopt. Also drop a redundant second sqlite3_exec() of the init SQL that clobbered the result of the sqlite3_prepare_v2() above it, hiding prepare failures behind a misleading "bad parameter or other API misuse". Signed-off-by: Trevor Burnham <trevorburnham@gmail.com> Assisted-by: Claude Opus 5
A protocol message from a remote frontend is dispatched from a libuv callback with no HandleScope on the stack, inside the SealHandleScope that MainThreadInterface::DispatchMessages() installs. Opening the localStorage backing file can throw, so allocating the error object was fatal: FATAL ERROR: v8::HandleScope::CreateHandle() Cannot create a handle without a HandleScope Every Storage::Open() failure was affected, including a --localstorage-file that names a directory, so this did not need a malformed file to reach. getWebStorage() already opens a HandleScope and a TryCatch for its own handle use; do the same around the GetAll() call and report the failure as a protocol error. Signed-off-by: Trevor Burnham <trevorburnham@gmail.com> Assisted-by: Claude Opus 5
TrevorBurnham
force-pushed
the
webstorage-throw-on-malformed-file
branch
from
September 22, 2026 14:34
43e1abd to
1812f50
Compare
The test asserted that the reason a store could not be read always reaches the frontend, which the code does not promise. The reason is read off the v8::Message, and V8 only builds one on a best-effort basis: Isolate::Throw() skips it while the bootstrapper is active, and PropagateExceptionToExternalTryCatch() does not hand it to an external TryCatch when a JavaScript handler is the topmost one. The agent already falls back to a message without a reason for that case. The assertion held at -j1 and failed under -j 4 on macOS, in the step that re-runs the suite from a directory with unusual characters. The directory is incidental; that step is the only one that runs tests in parallel. Accept either message, both of which prove that Open() threw and was caught. Reading a store can now fail in four ways that a frontend could not tell apart, so report a different reason for each. This changes the message added in 37305e1 for an unavailable store, which its test matches with a regular expression. Signed-off-by: Trevor Burnham <trevorburnham@gmail.com> Assisted-by: Claude Opus 5
This comment was marked as outdated.
This comment was marked as outdated.
trivikr
approved these changes
Sep 22, 2026
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
Member
|
The AIX and LinuxONE failures in |
Keys are stored as a blob of raw uint16_t memory, so their byte order is the platform's, but the test fixture wrote them with Buffer's utf16le encoding. On AIX and s390x the stored key therefore did not match the one that localStorage.getItem() binds, so the row was not found, no value was type-checked, and the child exited 0 instead of throwing. Encode fixture keys in the platform's byte order. Signed-off-by: Trevor Burnham <trevorburnham@gmail.com> Assisted-by: Claude Opus 5
trivikr
approved these changes
Sep 25, 2026
trivikr
added
request-ci
github-actions
Bot
removed
the
request-ci
Collaborator
Member
|
I'm applying |
panva
added
the
resume-ci
github-actions
Bot
added
resume-ci-failed
Contributor
Failed to resume CIFull Auto Start CI output |
Member
FYI |
The inspector accepts connections before pre-execution defines globalThis.localStorage, and messages reach the main thread through an interrupt that can run during that startup JavaScript. A command that arrived first found no Storage object and reported the store as unavailable. The Storage.getStorageKey round trip usually hid this, but on Windows it failed every time. Wait for the child's script to start before sending commands. This race, not a missing v8::Message, is the likely cause of the macOS failure that the previous fixup loosened the schema_version assertion for: that run reported the message then used for an unavailable store. Restore the exact assertion. Signed-off-by: Trevor Burnham <trevorburnham@gmail.com> Assisted-by: Claude Opus 5.5
Contributor
Author
|
Thanks, both. The AIX/LinuxONE failure was an endianness bug in the test fixture: keys are stored in native byte order, but the fixture always wrote UTF-16LE. That's fixed in e9c2808. The Windows failure was a race in the new inspector test. The inspector accepts connections before pre-execution defines |
Collaborator
trivikr
approved these changes
Sep 26, 2026
trivikr
added
commit-queue-squash
Collaborator
|
Landed in 8812357 |
nodejs-github-bot
removed
the
commit-queue
aduh95
pushed a commit
that referenced
this pull request
Sep 27, 2026
The localStorage backing file is a user-specified path, and the schema is created with CREATE TABLE IF NOT EXISTS, so a file that already contains tables of those names is adopted as-is. Its stored values may then have any SQLite type, but every read asserted the expected type with CHECK, so a wrong-typed value aborted the process. A bad schema_version was the worst case: that assertion is in Storage::Open(), so any access aborted and the application had no chance to inspect or repair the file. Report these as ERR_INVALID_STATE instead, matching the throw four lines below the schema_version assertion for a version that is too new. Storage::GetAll() has no JavaScript caller to throw at, so it returns std::nullopt and the DOM storage inspector agent reports a protocol error. Now that a failed open returns instead of aborting, Open() has to clean up after itself: adopt the sqlite3* into a conn_unique_ptr immediately, so that an error does not leak the connection and leave the next access to open another one. Storage::GetAll() also ignored the result of sqlite3_prepare_v2() and the status its row loop ended on, reporting a malformed file or a mid-scan error as an empty store. Both now return std::nullopt. Also drop a redundant second sqlite3_exec() of the init SQL that clobbered the result of the sqlite3_prepare_v2() above it, hiding prepare failures behind a misleading "bad parameter or other API misuse". Signed-off-by: Trevor Burnham <trevorburnham@gmail.com> Assisted-by: Claude Opus 5 PR-URL: #65879 Fixes: #65878 Fixes: #64640 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
HoonDongKang
pushed a commit
to HoonDongKang/node
that referenced
this pull request
Sep 28, 2026
The localStorage backing file is a user-specified path, and the schema is created with CREATE TABLE IF NOT EXISTS, so a file that already contains tables of those names is adopted as-is. Its stored values may then have any SQLite type, but every read asserted the expected type with CHECK, so a wrong-typed value aborted the process. A bad schema_version was the worst case: that assertion is in Storage::Open(), so any access aborted and the application had no chance to inspect or repair the file. Report these as ERR_INVALID_STATE instead, matching the throw four lines below the schema_version assertion for a version that is too new. Storage::GetAll() has no JavaScript caller to throw at, so it returns std::nullopt and the DOM storage inspector agent reports a protocol error. Now that a failed open returns instead of aborting, Open() has to clean up after itself: adopt the sqlite3* into a conn_unique_ptr immediately, so that an error does not leak the connection and leave the next access to open another one. Storage::GetAll() also ignored the result of sqlite3_prepare_v2() and the status its row loop ended on, reporting a malformed file or a mid-scan error as an empty store. Both now return std::nullopt. Also drop a redundant second sqlite3_exec() of the init SQL that clobbered the result of the sqlite3_prepare_v2() above it, hiding prepare failures behind a misleading "bad parameter or other API misuse". Signed-off-by: Trevor Burnham <trevorburnham@gmail.com> Assisted-by: Claude Opus 5 PR-URL: nodejs#65879 Fixes: nodejs#65878 Fixes: nodejs#64640 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
aduh95
pushed a commit
that referenced
this pull request
Sep 28, 2026
The localStorage backing file is a user-specified path, and the schema is created with CREATE TABLE IF NOT EXISTS, so a file that already contains tables of those names is adopted as-is. Its stored values may then have any SQLite type, but every read asserted the expected type with CHECK, so a wrong-typed value aborted the process. A bad schema_version was the worst case: that assertion is in Storage::Open(), so any access aborted and the application had no chance to inspect or repair the file. Report these as ERR_INVALID_STATE instead, matching the throw four lines below the schema_version assertion for a version that is too new. Storage::GetAll() has no JavaScript caller to throw at, so it returns std::nullopt and the DOM storage inspector agent reports a protocol error. Now that a failed open returns instead of aborting, Open() has to clean up after itself: adopt the sqlite3* into a conn_unique_ptr immediately, so that an error does not leak the connection and leave the next access to open another one. Storage::GetAll() also ignored the result of sqlite3_prepare_v2() and the status its row loop ended on, reporting a malformed file or a mid-scan error as an empty store. Both now return std::nullopt. Also drop a redundant second sqlite3_exec() of the init SQL that clobbered the result of the sqlite3_prepare_v2() above it, hiding prepare failures behind a misleading "bad parameter or other API misuse". Signed-off-by: Trevor Burnham <trevorburnham@gmail.com> Assisted-by: Claude Opus 5 PR-URL: #65879 Fixes: #65878 Fixes: #64640 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes: #65878
Fixes: #64640
src/node_webstorage.ccasserted the SQLite type of every column it read. But the backing file is a user-specified path, and the schema is created withCREATE TABLE IF NOT EXISTS, so a file that already contains tables of those names is adopted as-is and its values may have any type. A wrong-typedschema_versionis the worst case: the assertion is inStorage::Open(), so any access aborts and the application can't inspect or repair the file first.This PR makes
localStoragemethods throwERR_INVALID_STATEin that scenario:Making these paths non-fatal exposed three further problems, all fixed here:
Open()only adopted thesqlite3*intodb_at the very end, so an early return dropped it, and becausedb_stayed null the next access opened another. The oldCHECKaborted on the first attempt, so this never accumulated; now atry { localStorage.length } catch {}loop leaked two descriptors per iteration until the limit was exhausted and the error degraded intounable to open database file. This is Web Storage Maybe leaks SQLite connections when database initialization fails #64640.Storage::GetAll()ignored errors. It checked neither the result ofsqlite3_prepare_v2()nor the status its row loop ended on, so a pre-existing table missing a column, or a corrupt read mid-scan, was reported to the inspector as an empty store while every JavaScript accessor threw for the same file. Both now returnstd::nullopt.HandleScopeon the stack, inside theSealHandleScopethatMainThreadInterface::DispatchMessages()installs, so throwing fromOpen()was fatal:FATAL ERROR: v8::HandleScope::CreateHandle() Cannot create a handle without a HandleScope. EveryOpen()failure was affected, including a--localstorage-filethat names a directory, so this did not need a malformed file to reach.getWebStorage()already opens aHandleScopeand aTryCatchfor its own handle use; theGetAll()call now does the same.Storage::GetAll()reports failure withstd::optionalrather than throwing, because its only caller is the inspector agent and a pending exception there has no JavaScript to propagate to.Storage::Length()keeps itsCHECK: its query isSELECT count(*), which is always an integer.Tests
New tests cover the four JavaScript-reachable assertions and the descriptor leak in
test-webstorage.js, plus both inspector failures in a newtest-inspector-dom-storage-malformed.js.Out of scope
One possible follow-up:
THROW_SQLITE_ERRORusessqlite3_errstr(code), so a prepare failure reportsSQL logic errorrather thanno such column: schema_version. Switching it tosqlite3_errmsg(db)would improve every throw in the file.