src: throw on a malformed localStorage file · nodejs/node@de34ddc · GitHub
Skip to content

Commit de34ddc

Browse files
TrevorBurnhamaduh95
TrevorBurnham
authored and
aduh95
committed
src: throw on a malformed localStorage file
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>
1 parent 8b24247 commit de34ddc

5 files changed

Lines changed: 358 additions & 17 deletions

File tree

‎src/inspector/dom_storage_agent.cc‎

Lines changed: 32 additions & 1 deletion

‎src/node_webstorage.cc‎

Lines changed: 62 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -59,6 +59,19 @@ using v8::Value;
5959
} \
6060
} while (0)
6161

62+
// The backing file is a user-specified path, and the schema below is created
63+
// with IF NOT EXISTS, so a file that already holds tables of those names is
64+
// adopted as-is and its values may have any type. A wrong type is therefore a
65+
// statement about untrusted input, not a broken internal invariant.
66+
#define CHECK_COLUMN_TYPE_OR_THROW(env, stmt, idx, expected, detail, ret) \
67+
do { \
68+
if (sqlite3_column_type((stmt), (idx)) != (expected)) { \
69+
THROW_ERR_INVALID_STATE((env), \
70+
"localStorage database is malformed: " detail); \
71+
return (ret); \
72+
} \
73+
} while (0)
74+
6275
static void ThrowQuotaExceededException(Local<Context> context) {
6376
Isolate* isolate = Isolate::GetCurrent();
6477
auto quota_exceeded_str =
@@ -173,6 +186,12 @@ Maybe<void> Storage::Open() {
173186
}
174187

175188
int r = sqlite3_open(location_.c_str(), &db);
189+
// Adopt the connection before anything below can return early, so that a
190+
// failure does not leak it. sqlite3_open() allocates a connection to be
191+
// closed even when it fails. This is declared ahead of the statement below
192+
// so that the statement is finalized first; sqlite3_close() fails while a
193+
// statement is still open, and conn_deleter treats that as fatal.
194+
auto conn = conn_unique_ptr(db);
176195
CHECK_ERROR_OR_THROW(env(), r, SQLITE_OK, Nothing<void>());
177196
r = sqlite3_exec(db, init_sql_v0.data(), nullptr, nullptr, nullptr);
178197
CHECK_ERROR_OR_THROW(env(), r, SQLITE_OK, Nothing<void>());
@@ -184,12 +203,16 @@ Maybe<void> Storage::Open() {
184203
get_schema_version_sql.size(),
185204
&s,
186205
nullptr);
187-
r = sqlite3_exec(db, init_sql_v0.data(), nullptr, nullptr, nullptr);
188-
CHECK_ERROR_OR_THROW(env(), r, SQLITE_OK, Nothing<void>());
189206
auto stmt = stmt_unique_ptr(s);
207+
CHECK_ERROR_OR_THROW(env(), r, SQLITE_OK, Nothing<void>());
190208
CHECK_ERROR_OR_THROW(
191209
env(), sqlite3_step(stmt.get()), SQLITE_ROW, Nothing<void>());
192-
CHECK(sqlite3_column_type(stmt.get(), 0) == SQLITE_INTEGER);
210+
CHECK_COLUMN_TYPE_OR_THROW(env(),
211+
stmt.get(),
212+
0,
213+
SQLITE_INTEGER,
214+
"expected schema_version to be an integer",
215+
Nothing<void>());
193216
int schema_version = sqlite3_column_int(stmt.get(), 0);
194217
stmt = nullptr; // Force finalization.
195218

@@ -209,7 +232,7 @@ Maybe<void> Storage::Open() {
209232
CHECK_ERROR_OR_THROW(env(), r, SQLITE_OK, Nothing<void>());
210233
}
211234

212-
db_ = conn_unique_ptr(db);
235+
db_ = std::move(conn);
213236
return JustVoid();
214237
}
215238

@@ -266,7 +289,12 @@ MaybeLocal<Array> Storage::Enumerate() {
266289
LocalVector<Value> values(env()->isolate());
267290
Local<Value> value;
268291
while ((r = sqlite3_step(stmt.get())) == SQLITE_ROW) {
269-
CHECK(sqlite3_column_type(stmt.get(), 0) == SQLITE_BLOB);
292+
CHECK_COLUMN_TYPE_OR_THROW(env(),
293+
stmt.get(),
294+
0,
295+
SQLITE_BLOB,
296+
"expected key to be a blob",
297+
Local<Array>());
270298
auto size = sqlite3_column_bytes(stmt.get(), 0) / sizeof(uint16_t);
271299
if (!String::NewFromTwoByte(env()->isolate(),
272300
reinterpret_cast<const uint16_t*>(
@@ -282,20 +310,28 @@ MaybeLocal<Array> Storage::Enumerate() {
282310
return Array::New(env()->isolate(), values.data(), values.size());
283311
}
284312

285-
std::unordered_map<std::u16string, std::u16string> Storage::GetAll() {
313+
std::optional<std::unordered_map<std::u16string, std::u16string>>
314+
Storage::GetAll() {
286315
if (!Open().IsJust()) {
287-
return {};
316+
return std::nullopt;
288317
}
289318

290319
static constexpr std::string_view sql =
291320
"SELECT key, value FROM nodejs_webstorage";
292321
sqlite3_stmt* s = nullptr;
293322
int r = sqlite3_prepare_v2(db_.get(), sql.data(), sql.size(), &s, nullptr);
294323
auto stmt = stmt_unique_ptr(s);
324+
// Unlike the other accessors, this one has no JavaScript caller to throw at,
325+
// so every failure below is reported to the inspector agent instead.
326+
if (r != SQLITE_OK) {
327+
return std::nullopt;
328+
}
295329
std::unordered_map<std::u16string, std::u16string> result;
296330
while ((r = sqlite3_step(stmt.get())) == SQLITE_ROW) {
297-
CHECK(sqlite3_column_type(stmt.get(), 0) == SQLITE_BLOB);
298-
CHECK(sqlite3_column_type(stmt.get(), 1) == SQLITE_BLOB);
331+
if (sqlite3_column_type(stmt.get(), 0) != SQLITE_BLOB ||
332+
sqlite3_column_type(stmt.get(), 1) != SQLITE_BLOB) {
333+
return std::nullopt;
334+
}
299335
auto key_size = sqlite3_column_bytes(stmt.get(), 0) / sizeof(uint16_t);
300336
auto value_size = sqlite3_column_bytes(stmt.get(), 1) / sizeof(uint16_t);
301337
auto key_uint16(
@@ -308,6 +344,9 @@ std::unordered_map<std::u16string, std::u16string> Storage::GetAll() {
308344

309345
result.emplace(std::move(key), std::move(value));
310346
}
347+
if (r != SQLITE_DONE) {
348+
return std::nullopt;
349+
}
311350
return result;
312351
}
313352

@@ -324,6 +363,8 @@ MaybeLocal<Value> Storage::Length() {
324363
auto stmt = stmt_unique_ptr(s);
325364
CHECK_ERROR_OR_THROW(
326365
env(), sqlite3_step(stmt.get()), SQLITE_ROW, Local<Value>());
366+
// Unlike the reads above, this one is not a claim about the file's contents:
367+
// count(*) is an integer whatever the table holds.
327368
CHECK(sqlite3_column_type(stmt.get(), 0) == SQLITE_INTEGER);
328369
int result = sqlite3_column_int(stmt.get(), 0);
329370
return Integer::New(env()->isolate(), result);
@@ -351,7 +392,12 @@ MaybeLocal<Value> Storage::Load(Local<Name> key) {
351392
CHECK_ERROR_OR_THROW(env(), r, SQLITE_OK, Local<Value>());
352393
r = sqlite3_step(stmt.get());
353394
if (r == SQLITE_ROW) {
354-
CHECK(sqlite3_column_type(stmt.get(), 0) == SQLITE_BLOB);
395+
CHECK_COLUMN_TYPE_OR_THROW(env(),
396+
stmt.get(),
397+
0,
398+
SQLITE_BLOB,
399+
"expected value to be a blob",
400+
Local<Value>());
355401
auto size = sqlite3_column_bytes(stmt.get(), 0) / sizeof(uint16_t);
356402
return String::NewFromTwoByte(env()->isolate(),
357403
reinterpret_cast<const uint16_t*>(
@@ -383,7 +429,12 @@ MaybeLocal<Value> Storage::LoadKey(const int index) {
383429

384430
r = sqlite3_step(stmt.get());
385431
if (r == SQLITE_ROW) {
386-
CHECK(sqlite3_column_type(stmt.get(), 0) == SQLITE_BLOB);
432+
CHECK_COLUMN_TYPE_OR_THROW(env(),
433+
stmt.get(),
434+
0,
435+
SQLITE_BLOB,
436+
"expected key to be a blob",
437+
Local<Value>());
387438
auto size = sqlite3_column_bytes(stmt.get(), 0) / sizeof(uint16_t);
388439
return String::NewFromTwoByte(env()->isolate(),
389440
reinterpret_cast<const uint16_t*>(

‎src/node_webstorage.h‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33

44
#if defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS
55

6+
#include <optional>
67
#include <unordered_map>
78
#include "base_object.h"
89
#include "node_mem.h"
@@ -41,7 +42,12 @@ class Storage : public BaseObject {
4142
v8::MaybeLocal<v8::Value> LoadKey(const int index);
4243
v8::Maybe<void> Remove(v8::Local<v8::Name> key);
4344
v8::Maybe<void> Store(v8::Local<v8::Name> key, v8::Local<v8::Value> value);
44-
std::unordered_map<std::u16string, std::u16string> GetAll();
45+
// Returns nothing if the backing store could not be read, e.g. because it
46+
// holds values of an unexpected type. Opening the store can also throw, so
47+
// the caller must hold a v8::TryCatch: an empty return does not say which of
48+
// the two happened, and a pending exception is left for the caller to
49+
// handle.
50+
std::optional<std::unordered_map<std::u16string, std::u16string>> GetAll();
4551

4652
SET_MEMORY_INFO_NAME(Storage)
4753
SET_SELF_SIZE(Storage)

‎test/parallel/test-inspector-dom-storage-malformed.js‎

Lines changed: 101 additions & 0 deletions

0 commit comments

Comments
 (0)