src: throw on a malformed localStorage file - #65879
Open
TrevorBurnham wants to merge 2 commits into
Open
Conversation
Collaborator
|
Review requested:
|
TrevorBurnham
force-pushed
the
webstorage-throw-on-malformed-file
branch
3 times, most recently
from
September 7, 2026 21:26
881d7fc to
2f08679
Compare
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 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.19% 90.18% -0.02%
==========================================
Files 771 771
Lines 264911 265116 +205
Branches 50323 50358 +35
==========================================
+ Hits 238939 239087 +148
- Misses 16922 16978 +56
- Partials 9050 9051 +1
🚀 New features to boost your workflow:
|
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
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.