From 3090adef8b791db9429ac0b44c5f2651e2145ef1 Mon Sep 17 00:00:00 2001 From: deepak1556 Date: Wed, 19 Aug 2026 14:16:35 +0900 Subject: [PATCH 1/3] inspector: defer JS execution from V8 interrupts Inspector messages for a main thread connection are dispatched from a V8 interrupt handler. Defer frontend messages in this case using a microtask. This preserves the existing timing contract for main thread inspector connections, where notifications are observable after a microtask checkpoint. Starting the inspector I/O thread and toggling network tracking can also call into JS from an interrupt. Signed-off-by: deepak1556 Signed-off-by: Christian Aurich Zanettini Martins --- src/inspector_agent.cc | 183 ++++++++++++++++++++++------------------ src/inspector_agent.h | 18 ++-- src/inspector_js_api.cc | 46 +++++++--- 3 files changed, 145 insertions(+), 102 deletions(-) diff --git a/src/inspector_agent.cc b/src/inspector_agent.cc index 22506e26b32c..5f1a3090e0af 100644 --- a/src/inspector_agent.cc +++ b/src/inspector_agent.cc @@ -80,6 +80,52 @@ static Mutex start_io_thread_agents_mutex; static std::vector start_io_thread_agents; static bool debug_signal_handler_started = false; +template +void SyncJavaScriptHookState(Environment* env, + JavaScriptHookState* state, + Local enable_function, + Local disable_function, + bool require_bootstrapping, + Defer defer, + OnFailure on_failure) { + // The debugger can request an interrupt from within a hook's JS toggle + // function. A nested sync only records the new requested state; the + // outermost sync sees it when re-checking the loop after each toggle. + if (state->syncing) return; + state->syncing = true; + auto on_exit = OnScopeLeave([state]() { state->syncing = false; }); + + Isolate* isolate = env->isolate(); + while (state->wanted != state->enabled) { + // No hook events will be emitted during cleanup, and calling into JS is no + // longer possible. + // Refs: https://github.com/nodejs/node/pull/34362#discussion_r456006039 + if (!env->can_call_into_js()) return; + + bool enable = state->wanted; + Local function = enable ? enable_function : disable_function; + if (function.IsEmpty()) return; + + if (env->is_processing_v8_interrupt()) { + defer(); + return; + } + + if (require_bootstrapping) CHECK(env->has_run_bootstrapping_code()); + Local context = env->context(); + v8::TryCatch try_catch(isolate); + USE(function->Call(context, Undefined(isolate), 0, nullptr)); + if (try_catch.HasCaught()) { + // Termination may abort the toggle invocation. Do not record a state that + // may not have taken effect, so a later sync can retry it. + if (try_catch.HasTerminated()) return; + PrintCaughtException(isolate, context, try_catch); + on_failure(); + } + state->enabled = enable; + } +} + static void RequestIoThreadStartOnAgents() { Mutex::ScopedLock lock(start_io_thread_agents_mutex); for (Agent* agent : start_io_thread_agents) agent->RequestIoThreadStart(); @@ -483,6 +529,16 @@ class SameThreadInspectorSession : public InspectorSession { void NotifyClusterWorkersDebugEnabled(Environment* env) { Isolate* isolate = env->isolate(); + + // Starting the inspector from an interrupt is safe, but emitting the cluster + // notification calls into JS. + if (env->is_processing_v8_interrupt()) { + env->SetImmediate( + [](Environment* env) { NotifyClusterWorkersDebugEnabled(env); }, + CallbackFlags::kUnrefed); + return; + } + HandleScope handle_scope(isolate); // Send message to enable debug in cluster workers @@ -1000,54 +1056,40 @@ void Agent::SetupNetworkTracking(Local enable_function, Local disable_function) { parent_env_->set_inspector_enable_network_tracking(enable_function); parent_env_->set_inspector_disable_network_tracking(disable_function); - if (pending_enable_network_tracking) { - pending_enable_network_tracking = false; - EnableNetworkTracking(); - } else if (pending_disable_network_tracking) { - pending_disable_network_tracking = false; - DisableNetworkTracking(); - } + SyncNetworkTrackingState(); } void Agent::EnableNetworkTracking() { - if (network_tracking_enabled_) { - return; - } - HandleScope scope(parent_env_->isolate()); - Local enable = parent_env_->inspector_enable_network_tracking(); - if (enable.IsEmpty()) { - pending_enable_network_tracking = true; - } else { - ToggleNetworkTracking(parent_env_->isolate(), enable); - network_tracking_enabled_ = true; - } + network_tracking_state_.wanted = true; + SyncNetworkTrackingState(); } void Agent::DisableNetworkTracking() { - if (!network_tracking_enabled_) { - return; - } - HandleScope scope(parent_env_->isolate()); - Local disable = parent_env_->inspector_disable_network_tracking(); - if (disable.IsEmpty()) { - pending_disable_network_tracking = true; - } else if (!client_->hasConnectedSessions()) { - ToggleNetworkTracking(parent_env_->isolate(), disable); - network_tracking_enabled_ = false; - } + if (client_->hasConnectedSessions()) return; + network_tracking_state_.wanted = false; + SyncNetworkTrackingState(); } -void Agent::ToggleNetworkTracking(Isolate* isolate, Local fn) { - if (!parent_env_->can_call_into_js()) return; - auto context = parent_env_->context(); +void Agent::SyncNetworkTrackingState() { + Isolate* isolate = parent_env_->isolate(); HandleScope scope(isolate); - CHECK(!fn.IsEmpty()); - v8::TryCatch try_catch(isolate); - USE(fn->Call(context, Undefined(isolate), 0, nullptr)); - if (try_catch.HasCaught() && !try_catch.HasTerminated()) { - PrintCaughtException(isolate, context, try_catch); - UNREACHABLE("Cannot toggle network tracking, please report this."); - } + SyncJavaScriptHookState( + parent_env_, + &network_tracking_state_, + parent_env_->inspector_enable_network_tracking(), + parent_env_->inspector_disable_network_tracking(), + false, + [this]() { + parent_env_->SetImmediate( + [](Environment* env) { + Agent* agent = env->inspector_agent(); + if (agent != nullptr) agent->SyncNetworkTrackingState(); + }, + CallbackFlags::kUnrefed); + }, + []() { + UNREACHABLE("Cannot toggle network tracking, please report this."); + }); } void Agent::WaitForDisconnect() { @@ -1098,7 +1140,7 @@ void Agent::RegisterAsyncHook(Isolate* isolate, } void Agent::SetAsyncHookTrackingEnabled(bool enabled) { - async_hook_wanted_ = enabled; + async_hook_state_.wanted = enabled; SyncAsyncHookState(); } @@ -1112,52 +1154,25 @@ void Agent::SetAsyncHookTrackingEnabled(bool enabled) { // When it's not safe to call into JS, this is a no-op and we'll try again in // RegisterAsyncHook() (for 1) or from a scheduled immediate (for 2). void Agent::SyncAsyncHookState() { - // The debugger can request an interrupt within the toggle JS function itself, - // A nested call only records the new requested state, the outermost call sees - // it when re-checking the loop condition after each toggle. - if (syncing_async_hook_state_) return; - syncing_async_hook_state_ = true; - auto on_exit = OnScopeLeave([this]() { syncing_async_hook_state_ = false; }); - Isolate* isolate = parent_env_->isolate(); HandleScope scope(isolate); - while (async_hook_wanted_ != async_hook_enabled_) { - // Guard against running this during cleanup -- no async events will be - // emitted anyway at that point anymore, and calling into JS is not - // possible. This should probably not be something we're attempting in the - // first place, - // Refs: https://github.com/nodejs/node/pull/34362#discussion_r456006039 - if (!parent_env_->can_call_into_js()) return; - - bool enable = async_hook_wanted_; - Local fn = enable ? parent_env_->inspector_enable_async_hooks() - : parent_env_->inspector_disable_async_hooks(); - if (fn.IsEmpty()) return; - - if (parent_env_->is_processing_v8_interrupt()) { - parent_env_->SetImmediate( - [](Environment* env) { - Agent* agent = env->inspector_agent(); - if (agent != nullptr) agent->SyncAsyncHookState(); - }, - CallbackFlags::kUnrefed); - return; - } - - CHECK(parent_env_->has_run_bootstrapping_code()); - Local context = parent_env_->context(); - v8::TryCatch try_catch(isolate); - USE(fn->Call(context, Undefined(isolate), 0, nullptr)); - if (try_catch.HasCaught()) { - // Termination may abort the toggle invocation, retrying now would just - // be terminated again. Instead of recording the toggle that may not have - // taken effect, leave the states as-is so that a later sync retries. - if (try_catch.HasTerminated()) return; - PrintCaughtException(isolate, context, try_catch); - UNREACHABLE("Cannot toggle Inspector's AsyncHook, please report this."); - } - async_hook_enabled_ = enable; - } + SyncJavaScriptHookState( + parent_env_, + &async_hook_state_, + parent_env_->inspector_enable_async_hooks(), + parent_env_->inspector_disable_async_hooks(), + true, + [this]() { + parent_env_->SetImmediate( + [](Environment* env) { + Agent* agent = env->inspector_agent(); + if (agent != nullptr) agent->SyncAsyncHookState(); + }, + CallbackFlags::kUnrefed); + }, + []() { + UNREACHABLE("Cannot toggle Inspector's AsyncHook, please report this."); + }); } void Agent::AsyncTaskScheduled(const StringView& task_name, void* task, diff --git a/src/inspector_agent.h b/src/inspector_agent.h index 5a1d1de26547..6b92ef2a08fe 100644 --- a/src/inspector_agent.h +++ b/src/inspector_agent.h @@ -29,6 +29,12 @@ class ParentInspectorHandle; class NodeInspectorClient; class WorkerManager; +struct JavaScriptHookState { + bool wanted = false; + bool enabled = false; + bool syncing = false; +}; + class InspectorSession { public: virtual ~InspectorSession() = default; @@ -134,7 +140,7 @@ class Agent { private: void SyncAsyncHookState(); - void ToggleNetworkTracking(v8::Isolate* isolate, v8::Local fn); + void SyncNetworkTrackingState(); node::Environment* parent_env_; // Encapsulates majority of the Inspector functionality @@ -154,9 +160,7 @@ class Agent { // The state of the async hook used for async stack traces that the protocol // last requested, and the state JS currently has. SyncAsyncHookState() // reconciles the two when it is possible and safe to call into JS. - bool async_hook_wanted_ = false; - bool async_hook_enabled_ = false; - bool syncing_async_hook_state_ = false; + JavaScriptHookState async_hook_state_; // Woken by the SIGUSR1 watchdog; closed by the cleanup hook or ~Agent(), // whichever runs first, and freed by its close callback. @@ -164,9 +168,9 @@ class Agent { void StopAcceptingIoThreadStarts(); static void StopAcceptingIoThreadStartsHook(void* agent); - bool network_tracking_enabled_ = false; - bool pending_enable_network_tracking = false; - bool pending_disable_network_tracking = false; + // Network tracking uses JS hooks. Reconcile the protocol requested and + // applied states after leaving a V8 interrupt. + JavaScriptHookState network_tracking_state_; std::shared_ptr network_resource_manager_; }; diff --git a/src/inspector_js_api.cc b/src/inspector_js_api.cc index 336930f18679..98dc03b2d6ad 100644 --- a/src/inspector_js_api.cc +++ b/src/inspector_js_api.cc @@ -62,13 +62,31 @@ struct MainThreadConnection { template class JSBindingsConnection : public BaseObject { public: + struct PendingMicrotask { + explicit PendingMicrotask(JSBindingsConnection* connection) + : env(connection->env()), connection(connection) { + env->AddCleanupHook(Delete, this); + } + + static void Delete(void* data) { + delete static_cast(data); + } + + static void Run(void* data) { + std::unique_ptr pending( + static_cast(data)); + pending->env->RemoveCleanupHook(Delete, pending.get()); + pending->connection->FlushPendingMessages(); + } + + Environment* env; + BaseObjectPtr connection; + }; + class JSBindingsSessionDelegate : public InspectorSessionDelegate { public: - JSBindingsSessionDelegate(Environment* env, - JSBindingsConnection* connection) - : env_(env), - connection_(connection) { - } + explicit JSBindingsSessionDelegate(JSBindingsConnection* connection) + : connection_(connection) {} void SendMessageToFrontend(const v8_inspector::StringView& message) override { @@ -76,7 +94,6 @@ class JSBindingsConnection : public BaseObject { } private: - Environment* env_; BaseObjectPtr connection_; }; @@ -86,10 +103,10 @@ class JSBindingsConnection : public BaseObject { : BaseObject(env, wrap), callback_(env->isolate(), callback) { Agent* inspector = env->inspector_agent(); session_ = ConnectionType::Connect( - inspector, std::make_unique(env, this)); - // Inspector responses may be produced from a GC weak callback, where - // invoking the JavaScript session callback is forbidden. Defer delivery - // until the GC has completed. + inspector, std::make_unique(this)); + // Inspector responses may be produced from a GC weak callback or a V8 + // interrupt, where invoking the JavaScript session callback is forbidden. + // Defer delivery until it is safe to call into JavaScript. env->isolate()->AddGCPrologueCallback(GCPrologueCallback, this); env->isolate()->AddGCEpilogueCallback(GCEpilogueCallback, this); } @@ -100,7 +117,8 @@ class JSBindingsConnection : public BaseObject { } void SendMessageToFrontend(const v8_inspector::StringView& message) { - if (in_gc_ || delivering_ || !pending_messages_.empty()) { + if (in_gc_ || env()->is_processing_v8_interrupt() || delivering_ || + !pending_messages_.empty()) { pending_messages_.emplace_back( message.is8Bit() ? std::u16string(message.characters8(), @@ -128,6 +146,12 @@ class JSBindingsConnection : public BaseObject { void ScheduleFlush() { if (flush_scheduled_ || delivering_) return; flush_scheduled_ = true; + if (!in_gc_ && env()->is_processing_v8_interrupt()) { + auto* pending = new PendingMicrotask(this); + env()->context()->GetMicrotaskQueue()->EnqueueMicrotask( + env()->isolate(), PendingMicrotask::Run, pending); + return; + } BaseObjectPtr strong_ref{this}; env()->SetImmediate( [strong_ref](Environment*) { strong_ref->FlushPendingMessages(); }, From 14a57e62d7084e285dd38d4253b7962e638111e6 Mon Sep 17 00:00:00 2001 From: deepak1556 Date: Thu, 15 Oct 2026 13:25:31 +0900 Subject: [PATCH 2/3] inspector: avoid JS execution in DOMStorage.getDOMStorageItems Protocol messages from a remote frontend are dispatched from a V8 API interrupt, where executing JavaScript is disallowed. DOMStorageAgent::getDOMStorageItems() previously looked up globalThis.localStorage and globalThis.sessionStorage via v8::Object::Get(), invoking their lazy JS getters on globalThis. In addition, Storage::GetAll() opened the backing store via Storage::Open(), which threw a JS Error on failure only for the inspector agent to catch it with v8::TryCatch. Track live Storage instances in webstorage::BindingData when the inspector subclasses construct them in Storage::New(), using the flag they pass to tell localStorage from sessionStorage, since both can be in memory. Check HasRealNamedCallbackProperty() before reading the global property so the lazy JS getter is not invoked during an interrupt. When the store has not yet been initialized by JS during an interrupt, read the SQLite backing file directly for localStorage and return an empty map for sessionStorage. Factor Storage::OpenConnection() to return open errors as a C++ string rather than throwing a JS exception in Storage::GetAll(). Co-authored-by: Christian Aurich Zanettini Martins Signed-off-by: Christian Aurich Zanettini Martins --- src/base_object_types.h | 3 +- src/inspector/dom_storage_agent.cc | 108 +++++++++++++++--------- src/inspector/dom_storage_agent.h | 2 +- src/node_webstorage.cc | 129 +++++++++++++++++++++-------- src/node_webstorage.h | 30 +++++-- src/path.cc | 40 ++++----- src/path.h | 1 + 7 files changed, 214 insertions(+), 99 deletions(-) diff --git a/src/base_object_types.h b/src/base_object_types.h index 39d7deb8ac26..af9c03f7fd55 100644 --- a/src/base_object_types.h +++ b/src/base_object_types.h @@ -26,7 +26,8 @@ namespace node { V(http2_binding_data, http2::BindingData) \ V(http_parser_binding_data, http_parser::BindingData) \ V(quic_binding_data, quic::BindingData) \ - V(sqlite_binding_data, sqlite::BindingData) + V(sqlite_binding_data, sqlite::BindingData) \ + V(webstorage_binding_data, webstorage::BindingData) // List of (non-binding) BaseObjects that are serializable in the snapshot. // The first argument should match what the type passes to diff --git a/src/inspector/dom_storage_agent.cc b/src/inspector/dom_storage_agent.cc index 7d3ce9dc5d06..a317c9559caf 100644 --- a/src/inspector/dom_storage_agent.cc +++ b/src/inspector/dom_storage_agent.cc @@ -1,7 +1,10 @@ #include "dom_storage_agent.h" #include +#include "base_object-inl.h" #include "env-inl.h" #include "inspector/inspector_object_utils.h" +#include "node_realm-inl.h" +#include "path.h" #include "util.h" #include "v8-exception.h" #include "v8-isolate.h" @@ -100,38 +103,35 @@ protocol::DispatchResponse DOMStorageAgent::getDOMStorageItems( is_local_storage ? &local_storage_map_ : &session_storage_map_; std::optional storage_map_fallback; if (storage_map->empty()) { - auto web_storage_obj = getWebStorage(is_local_storage); + bool is_uninitialized = false; + auto web_storage_obj = getWebStorage(is_local_storage, &is_uninitialized); // Each way of failing below says something different about the store, so // each reports a different reason. A frontend that cannot read a store // otherwise has no way to tell a missing one from a corrupt one. - if (!web_storage_obj) { + if (!web_storage_obj && !is_uninitialized) { return protocol::DispatchResponse::ServerError( "Could not read DOM storage items: storage is unavailable"); } - // A message from a remote frontend is dispatched without a HandleScope - // on the stack, and opening the backing file can throw, so give the - // exception a scope to be allocated in and somewhere to land. - v8::HandleScope handle_scope(env_->isolate()); - v8::TryCatch try_catch(env_->isolate()); - storage_map_fallback = web_storage_obj.value()->GetAll(); - if (try_catch.HasCaught()) { - // Pass the reason along; "the file was written by a newer Node.js" and - // "the file is locked" are not the same problem to the user. Read it - // off the Message, which was built when the exception was thrown. - // Converting the exception itself would call a user-patchable - // Error.prototype.toString, and there is no JavaScript frame here to - // run it from. - Local message = try_catch.Message(); - if (!message.IsEmpty()) { - Utf8Value reason(env_->isolate(), message->Get()); - return protocol::DispatchResponse::ServerError( - std::string("Could not read DOM storage items: ") + reason.out()); + std::string open_error; + if (web_storage_obj) { + storage_map_fallback = web_storage_obj.value()->GetAll(&open_error); + } else if (is_local_storage) { + std::string location = env_->options()->localstorage_file; + if (location != node::webstorage::kInMemoryPath) { + location = ToNamespacedPath(env_, location); } - // V8 builds that Message on a best-effort basis, so the throw is all we - // can report when it is missing. + storage_map_fallback = + node::webstorage::Storage::ReadAllFromFile(location, &open_error); + } else { + storage_map_fallback.emplace(); + } + if (!open_error.empty()) { + // Pass the reason along; "the file was written by a newer Node.js" and + // "the file is locked" are not the same problem to the user. Prefix with + // "Uncaught Error: " to match the v8::Message format without invoking + // JavaScript during a V8 interrupt. return protocol::DispatchResponse::ServerError( - "Could not read DOM storage items: the backing store could not be " - "opened"); + "Could not read DOM storage items: Uncaught Error: " + open_error); } if (!storage_map_fallback.has_value()) { return protocol::DispatchResponse::ServerError( @@ -318,26 +318,60 @@ void DOMStorageAgent::registerStorage(Local context, } std::optional DOMStorageAgent::getWebStorage( - bool is_local_storage) { + bool is_local_storage, bool* is_uninitialized) { + *is_uninitialized = false; + if (!env_->options()->webstorage) { + return std::nullopt; + } + if (is_local_storage && env_->options()->localstorage_file.empty()) { + return std::nullopt; + } + if (auto* binding = env_->principal_realm() + ->GetBindingData()) { + const auto& storage_ptr = + is_local_storage ? binding->local_storage : binding->session_storage; + if (storage_ptr) { + return storage_ptr.get(); + } + } + if (env_->no_browser_globals()) { + return std::nullopt; + } v8::Isolate* isolate = env_->isolate(); v8::HandleScope handle_scope(isolate); - v8::Local global = env_->context()->Global(); + v8::Local context = env_->context(); + v8::Local global = context->Global(); + v8::Local prop_name = + is_local_storage ? FIXED_ONE_BYTE_STRING(isolate, "localStorage") + : FIXED_ONE_BYTE_STRING(isolate, "sessionStorage"); + // Protocol messages from a remote frontend are dispatched from a V8 API + // interrupt, where executing JavaScript is disallowed. Both globals are + // defined as lazy JS getters, so check whether the property is still an + // accessor before reading it. + bool is_accessor = false; + if (!global->HasRealNamedCallbackProperty(context, prop_name) + .To(&is_accessor)) { + return std::nullopt; + } + if (is_accessor) { + if (env_->is_processing_v8_interrupt()) { + *is_uninitialized = true; + return std::nullopt; + } + if (!env_->can_call_into_js()) { + return std::nullopt; + } + } v8::Local web_storage_val; v8::TryCatch try_catch(isolate); - if (!global - ->Get(env_->context(), - is_local_storage - ? FIXED_ONE_BYTE_STRING(isolate, "localStorage") - : FIXED_ONE_BYTE_STRING(isolate, "sessionStorage")) - .ToLocal(&web_storage_val) || + if (!global->Get(context, prop_name).ToLocal(&web_storage_val) || !web_storage_val->IsObject() || try_catch.HasCaught()) { return std::nullopt; - } else { - node::webstorage::Storage* storage; - ASSIGN_OR_RETURN_UNWRAP( - &storage, web_storage_val.As(), std::nullopt); - return storage; } + node::webstorage::Storage* storage; + ASSIGN_OR_RETURN_UNWRAP( + &storage, web_storage_val.As(), std::nullopt); + return storage; } bool DOMStorageAgent::canEmit(const std::string& domain) { diff --git a/src/inspector/dom_storage_agent.h b/src/inspector/dom_storage_agent.h index a6eecbb8d37c..08c2be150147 100644 --- a/src/inspector/dom_storage_agent.h +++ b/src/inspector/dom_storage_agent.h @@ -54,7 +54,7 @@ class DOMStorageAgent : public protocol::DOMStorage::Backend, private: typedef std::unordered_map StorageMap; std::optional getWebStorage( - bool is_local_storage); + bool is_local_storage, bool* is_uninitialized); std::unique_ptr frontend_; StorageMap local_storage_map_ = {}; StorageMap session_storage_map_ = {}; diff --git a/src/node_webstorage.cc b/src/node_webstorage.cc index 204fd463126f..52c3db7d9c21 100644 --- a/src/node_webstorage.cc +++ b/src/node_webstorage.cc @@ -8,6 +8,7 @@ #include "node.h" #include "node_errors.h" #include "node_mem-inl.h" +#include "node_realm-inl.h" #include "path.h" #include "simdutf.h" #include "sqlite3.h" @@ -98,6 +99,14 @@ static void ThrowQuotaExceededException(Local context) { isolate->ThrowException(exception); } +BindingData::BindingData(Realm* realm, Local wrap) + : BaseObject(realm, wrap) {} + +void BindingData::MemoryInfo(MemoryTracker* tracker) const { + tracker->TrackField("local_storage", local_storage); + tracker->TrackField("session_storage", session_storage); +} + Storage::Storage(Environment* env, Local object, std::string_view location) @@ -117,7 +126,8 @@ void Storage::MemoryInfo(MemoryTracker* tracker) const { tracker->TrackField("location", location_); } -Maybe Storage::Open() { +conn_unique_ptr Storage::OpenConnection(std::string_view location, + std::string* error) { static const int kCurrentSchemaVersion = 1; static constexpr std::string_view get_schema_version_sql = "SELECT schema_version FROM nodejs_webstorage_state"; @@ -180,21 +190,24 @@ Maybe Storage::Open() { "" "INSERT OR IGNORE INTO nodejs_webstorage_state (total_size) VALUES (0);"; - sqlite3* db = db_.get(); - if (db != nullptr) { - return JustVoid(); - } - - int r = sqlite3_open(location_.c_str(), &db); + sqlite3* db = nullptr; + std::string loc(location); + int r = sqlite3_open(loc.c_str(), &db); // Adopt the connection before anything below can return early, so that a // failure does not leak it. sqlite3_open() allocates a connection to be // closed even when it fails. This is declared ahead of the statement below // so that the statement is finalized first; sqlite3_close() fails while a // statement is still open, and conn_deleter treats that as fatal. auto conn = conn_unique_ptr(db); - CHECK_ERROR_OR_THROW(env(), r, SQLITE_OK, Nothing()); + if (r != SQLITE_OK) { + if (error != nullptr) *error = sqlite3_errstr(r); + return nullptr; + } r = sqlite3_exec(db, init_sql_v0.data(), nullptr, nullptr, nullptr); - CHECK_ERROR_OR_THROW(env(), r, SQLITE_OK, Nothing()); + if (r != SQLITE_OK) { + if (error != nullptr) *error = sqlite3_errstr(r); + return nullptr; + } // Get the current schema version, used to determine schema migrations. sqlite3_stmt* s = nullptr; @@ -204,22 +217,30 @@ Maybe Storage::Open() { &s, nullptr); auto stmt = stmt_unique_ptr(s); - CHECK_ERROR_OR_THROW(env(), r, SQLITE_OK, Nothing()); - CHECK_ERROR_OR_THROW( - env(), sqlite3_step(stmt.get()), SQLITE_ROW, Nothing()); - CHECK_COLUMN_TYPE_OR_THROW(env(), - stmt.get(), - 0, - SQLITE_INTEGER, - "expected schema_version to be an integer", - Nothing()); + if (r != SQLITE_OK) { + if (error != nullptr) *error = sqlite3_errstr(r); + return nullptr; + } + r = sqlite3_step(stmt.get()); + if (r != SQLITE_ROW) { + if (error != nullptr) *error = sqlite3_errstr(r); + return nullptr; + } + if (sqlite3_column_type(stmt.get(), 0) != SQLITE_INTEGER) { + if (error != nullptr) { + *error = "localStorage database is malformed: " + "expected schema_version to be an integer"; + } + return nullptr; + } int schema_version = sqlite3_column_int(stmt.get(), 0); stmt = nullptr; // Force finalization. if (schema_version > kCurrentSchemaVersion) { - THROW_ERR_INVALID_STATE( - env(), "localStorage was created with a newer version of Node.js"); - return Nothing(); + if (error != nullptr) { + *error = "localStorage was created with a newer version of Node.js"; + } + return nullptr; } if (schema_version < kCurrentSchemaVersion) { @@ -229,10 +250,25 @@ Maybe Storage::Open() { std::to_string(kCurrentSchemaVersion) + ";"; r = sqlite3_exec( db, set_user_version_sql.c_str(), nullptr, nullptr, nullptr); - CHECK_ERROR_OR_THROW(env(), r, SQLITE_OK, Nothing()); + if (r != SQLITE_OK) { + if (error != nullptr) *error = sqlite3_errstr(r); + return nullptr; + } } - db_ = std::move(conn); + return conn; +} + +Maybe Storage::Open() { + if (db_ != nullptr) { + return JustVoid(); + } + std::string error; + db_ = OpenConnection(location_, &error); + if (db_ == nullptr) { + THROW_ERR_INVALID_STATE(env(), "%s", error.c_str()); + return Nothing(); + } return JustVoid(); } @@ -255,7 +291,15 @@ void Storage::New(const FunctionCallbackInfo& args) { ToNamespacedPath(env, &location); } - new Storage(env, args.This(), location.ToStringView()); + Storage* storage = new Storage(env, args.This(), location.ToStringView()); + // The inspector subclasses pass whether this is localStorage, which the + // location cannot tell: --localstorage-file=:memory: is valid too. + BindingData* binding_data = realm->GetBindingData(); + if (binding_data != nullptr && args[2]->IsBoolean()) { + auto& slot = args[2]->IsTrue() ? binding_data->local_storage + : binding_data->session_storage; + slot = BaseObjectWeakPtr(storage); + } } Maybe Storage::Clear() { @@ -310,23 +354,18 @@ MaybeLocal Storage::Enumerate() { return Array::New(env()->isolate(), values.data(), values.size()); } -std::optional> -Storage::GetAll() { - if (!Open().IsJust()) { - return std::nullopt; - } - +static std::optional ReadAllFromDb(sqlite3* db) { static constexpr std::string_view sql = "SELECT key, value FROM nodejs_webstorage"; sqlite3_stmt* s = nullptr; - int r = sqlite3_prepare_v2(db_.get(), sql.data(), sql.size(), &s, nullptr); + int r = sqlite3_prepare_v2(db, sql.data(), sql.size(), &s, nullptr); auto stmt = stmt_unique_ptr(s); // Unlike the other accessors, this one has no JavaScript caller to throw at, // so every failure below is reported to the inspector agent instead. if (r != SQLITE_OK) { return std::nullopt; } - std::unordered_map result; + Storage::StorageMap result; while ((r = sqlite3_step(stmt.get())) == SQLITE_ROW) { if (sqlite3_column_type(stmt.get(), 0) != SQLITE_BLOB || sqlite3_column_type(stmt.get(), 1) != SQLITE_BLOB) { @@ -350,6 +389,25 @@ Storage::GetAll() { return result; } +std::optional Storage::GetAll(std::string* error) { + if (db_ == nullptr) { + db_ = OpenConnection(location_, error); + if (db_ == nullptr) { + return std::nullopt; + } + } + return ReadAllFromDb(db_.get()); +} + +std::optional Storage::ReadAllFromFile( + std::string_view location, std::string* error) { + conn_unique_ptr conn = OpenConnection(location, error); + if (conn == nullptr) { + return std::nullopt; + } + return ReadAllFromDb(conn.get()); +} + MaybeLocal Storage::Length() { if (!Open().IsJust()) { return {}; @@ -762,8 +820,13 @@ static void Initialize(Local target, Local unused, Local context, void* priv) { - Environment* env = Environment::GetCurrent(context); + Realm* realm = Realm::GetCurrent(context); + Environment* env = realm->env(); Isolate* isolate = env->isolate(); + Realm* principal = env->principal_realm(); + if (principal->GetBindingData() == nullptr) { + principal->AddBindingData(target); + } auto ctor_tmpl = NewFunctionTemplate(isolate, Storage::New); auto inst_tmpl = ctor_tmpl->InstanceTemplate(); diff --git a/src/node_webstorage.h b/src/node_webstorage.h index 02de9c79b84c..a28e929d9a6c 100644 --- a/src/node_webstorage.h +++ b/src/node_webstorage.h @@ -27,6 +27,22 @@ using stmt_unique_ptr = std::unique_ptr; static constexpr std::string_view kInMemoryPath = ":memory:"; +class Storage; + +class BindingData : public BaseObject { + public: + SET_BINDING_ID(webstorage_binding_data) + + BindingData(Realm* realm, v8::Local wrap); + + void MemoryInfo(MemoryTracker* tracker) const override; + SET_MEMORY_INFO_NAME(BindingData) + SET_SELF_SIZE(BindingData) + + BaseObjectWeakPtr local_storage; + BaseObjectWeakPtr session_storage; +}; + class Storage : public BaseObject { public: Storage(Environment* env, @@ -42,18 +58,22 @@ class Storage : public BaseObject { v8::MaybeLocal LoadKey(const int index); v8::Maybe Remove(v8::Local key); v8::Maybe Store(v8::Local key, v8::Local value); + using StorageMap = std::unordered_map; // Returns nothing if the backing store could not be read, e.g. because it - // holds values of an unexpected type. Opening the store can also throw, so - // the caller must hold a v8::TryCatch: an empty return does not say which of - // the two happened, and a pending exception is left for the caller to - // handle. - std::optional> GetAll(); + // holds values of an unexpected type. If opening the store fails, *error is + // populated with the failure reason instead of throwing a JS exception, so + // that this can be called from V8 interrupts where JS execution is forbidden. + std::optional GetAll(std::string* error = nullptr); + static std::optional ReadAllFromFile(std::string_view location, + std::string* error); SET_MEMORY_INFO_NAME(Storage) SET_SELF_SIZE(Storage) private: v8::Maybe Open(); + static conn_unique_ptr OpenConnection(std::string_view location, + std::string* error); ~Storage() override; std::string location_; diff --git a/src/path.cc b/src/path.cc index f2e1ae116c3d..687cd174fa6a 100644 --- a/src/path.cc +++ b/src/path.cc @@ -264,9 +264,20 @@ std::string PathResolve(Environment* env, void ToNamespacedPath(Environment* env, BufferValue* path) { #ifdef _WIN32 if (path->length() == 0) return; - std::string resolved_path = node::PathResolve(env, {path->ToStringView()}); + std::string namespaced = ToNamespacedPath(env, path->ToStringView()); + size_t new_length = namespaced.size(); + path->AllocateSufficientStorage(new_length + 1); + path->SetLength(new_length); + memcpy(path->out(), namespaced.c_str(), new_length + 1); +#endif +} + +std::string ToNamespacedPath(Environment* env, std::string_view path) { +#ifdef _WIN32 + if (path.empty()) return std::string(); + std::string resolved_path = node::PathResolve(env, {path}); if (resolved_path.size() <= 2) { - return; + return std::string(path); } // SAFETY: We know that resolved_path.size() > 2, therefore accessing [0], @@ -277,34 +288,19 @@ void ToNamespacedPath(Environment* env, BufferValue* path) { if (resolved_path[2] != '?' && resolved_path[2] != '.') { // Matched non-long UNC root, convert the path to a long UNC path std::string_view unc_prefix = R"(\\?\UNC\)"; - size_t new_length = unc_prefix.size() + resolved_path.size() - 2; - path->AllocateSufficientStorage(new_length + 1); - path->SetLength(new_length); - memcpy(path->out(), unc_prefix.data(), unc_prefix.size()); - memcpy(path->out() + unc_prefix.size(), - resolved_path.c_str() + 2, - resolved_path.size() - 2 + 1); - return; + return std::string(unc_prefix) + resolved_path.substr(2); } } } else if (IsWindowsDeviceRoot(resolved_path[0]) && resolved_path[1] == ':' && resolved_path[2] == '\\') { // Matched device root, convert the path to a long UNC path std::string_view new_prefix = R"(\\?\)"; - size_t new_length = new_prefix.size() + resolved_path.size(); - path->AllocateSufficientStorage(new_length + 1); - path->SetLength(new_length); - memcpy(path->out(), new_prefix.data(), new_prefix.size()); - memcpy(path->out() + new_prefix.size(), - resolved_path.c_str(), - resolved_path.size() + 1); - return; + return std::string(new_prefix) + resolved_path; } - size_t new_length = resolved_path.size(); - path->AllocateSufficientStorage(new_length + 1); - path->SetLength(new_length); - memcpy(path->out(), resolved_path.c_str(), resolved_path.size() + 1); + return resolved_path; +#else + return std::string(path); #endif } diff --git a/src/path.h b/src/path.h index 65f0eb858ca7..4066fce057f6 100644 --- a/src/path.h +++ b/src/path.h @@ -42,6 +42,7 @@ constexpr bool IsWindowsDriveLetter(const std::string_view path) noexcept { #endif // _WIN32 void ToNamespacedPath(Environment* env, BufferValue* path); +std::string ToNamespacedPath(Environment* env, std::string_view path); void FromNamespacedPath(std::string* path); } // namespace node From 486614974d8e950b4a4a7717dbb53596d7a14b6f Mon Sep 17 00:00:00 2001 From: Christian Aurich Zanettini Martins Date: Tue, 20 Oct 2026 10:05:21 -0300 Subject: [PATCH 3/3] test: cover DOMStorage reads from V8 interrupts Read both stores over a remote session while the main thread runs JavaScript, so that the messages are dispatched from a V8 API interrupt: stores the application never touched, both stores in memory with sessionStorage created first, and a malformed localStorage file. The script counts calls to the storage getters and to an Error.prototype.code setter, so the test fails if a read calls into JavaScript even when V8 does not enforce it. Signed-off-by: Christian Aurich Zanettini Martins --- .../test-inspector-dom-storage-interrupt.js | 138 ++++++++++++++++++ .../test-inspector-dom-storage-malformed.js | 8 +- 2 files changed, 141 insertions(+), 5 deletions(-) create mode 100644 test/parallel/test-inspector-dom-storage-interrupt.js diff --git a/test/parallel/test-inspector-dom-storage-interrupt.js b/test/parallel/test-inspector-dom-storage-interrupt.js new file mode 100644 index 000000000000..fc426beddee6 --- /dev/null +++ b/test/parallel/test-inspector-dom-storage-interrupt.js @@ -0,0 +1,138 @@ +'use strict'; +const common = require('../common'); +common.skipIfSQLiteMissing(); +common.skipIfInspectorDisabled(); + +const assert = require('assert'); +const { spawnSync } = require('child_process'); +const { join } = require('path'); +const { DatabaseSync } = require('node:sqlite'); +const { NodeInstance } = require('../common/inspector-helper.js'); +const tmpdir = require('../common/tmpdir'); +tmpdir.refresh(); + +// While JavaScript runs, a message from a remote frontend is dispatched from a +// V8 API interrupt, where JavaScript must not run. Reading a store has to work +// from there, whether or not the application has touched it yet. The script +// counts calls to the storage getters and to a setter that a JavaScript error +// created by Node.js would hit, and reports the count once the reads are done. +async function getItemsWhileBusy(localStorageFile, prelude, storageIds) { + const script = ` + globalThis.jsCalls = 0; + for (const name of ['localStorage', 'sessionStorage']) { + const desc = Object.getOwnPropertyDescriptor(globalThis, name); + Object.defineProperty(globalThis, name, { + ...desc, + get() { globalThis.jsCalls++; return desc.get.call(this); }, + }); + } + Object.defineProperty(Error.prototype, 'code', { + set() { globalThis.jsCalls++; }, configurable: true, + }); + ${prelude} + globalThis.jsCalls = 0; + console.log('Ready'); + const deadline = Date.now() + ${common.platformTimeout(10000)}; + while (!globalThis.done && Date.now() < deadline); + `; + const child = new NodeInstance([ + '--inspect-brk=0', + '--experimental-storage-inspection', + `--localstorage-file=${localStorageFile}`, + ], script); + const session = await child.connectInspectorSession(); + await session.send({ method: 'NodeRuntime.enable' }); + await session.waitForNotification('NodeRuntime.waitingForDebugger'); + await session.send([ + { 'method': 'Runtime.enable' }, + { 'method': 'Debugger.enable' }, + ]); + await session.send({ method: 'Runtime.runIfWaitingForDebugger' }); + await session.send({ method: 'NodeRuntime.disable' }); + await session.waitForNotification('Debugger.paused', 'Break on start'); + await session.send({ 'method': 'Debugger.resume' }); + await session.waitForConsoleOutput('log', ['Ready']); + // Enabled late, so items set by the prelude have to be read from the stores. + await session.send({ method: 'DOMStorage.enable' }); + + const { storageKey } = await session.send({ method: 'Storage.getStorageKey' }); + const results = []; + for (const isLocalStorage of storageIds) { + results.push(await session.send({ + method: 'DOMStorage.getDOMStorageItems', + params: { storageId: { isLocalStorage, securityOrigin: '', storageKey } }, + }).catch((err) => err)); + } + + const { result } = await session.send({ + method: 'Runtime.evaluate', + params: { expression: 'globalThis.done = true; globalThis.jsCalls' }, + }); + assert.strictEqual(result.value, 0); + await session.waitForDisconnect(); + assert.strictEqual((await child.expectShutdown()).exitCode, 0); + return results; +} + +// Node's own tables are created with IF NOT EXISTS, so a file that already +// holds tables of those names is adopted as-is, wrong types included. +function malformedLocalStorage(name) { + const file = join(tmpdir.path, name); + const db = new DatabaseSync(file); + db.exec(` + CREATE TABLE nodejs_webstorage( + key BLOB NOT NULL, value BLOB NOT NULL, PRIMARY KEY(key) + ); + CREATE TABLE nodejs_webstorage_state( + max_size INTEGER NOT NULL DEFAULT 10485760, + total_size INTEGER NOT NULL, + schema_version INTEGER NOT NULL DEFAULT 1, + single_row_ INTEGER NOT NULL DEFAULT 1 CHECK(single_row_ = 1), + PRIMARY KEY(single_row_) + ); + INSERT INTO nodejs_webstorage_state (total_size, schema_version) + VALUES (0, 'one'); + `); + db.close(); + return file; +} + +async function runTest() { + // Stores the application never touched, with localStorage written by an + // earlier run. + const file = join(tmpdir.path, 'storage.db'); + const { status } = spawnSync(process.execPath, [ + `--localstorage-file=${file}`, + '-e', 'localStorage.setItem("l", "local")', + ]); + assert.strictEqual(status, 0); + const [local, session] = await getItemsWhileBusy(file, '', [true, false]); + assert.deepStrictEqual(local.entries, [['l', 'local']]); + assert.deepStrictEqual(session.entries, []); + + // Both stores in memory and never touched. + const untouched = await getItemsWhileBusy(':memory:', '', [true, false]); + assert.deepStrictEqual(untouched.map((r) => r.entries), [[], []]); + + // Both stores in memory, sessionStorage created first. + const [memLocal, memSession] = await getItemsWhileBusy(':memory:', ` + sessionStorage.setItem('s', 'session'); + localStorage.setItem('l', 'local'); + `, [true, false]); + assert.deepStrictEqual(memLocal.entries, [['l', 'local']]); + assert.deepStrictEqual(memSession.entries, [['s', 'session']]); + + // A file that cannot be opened is reported without creating a JavaScript + // error. + for (const [name, touch] of [['untouched.db', ''], + ['touched.db', 'globalThis.localStorage;']]) { + const [err] = await getItemsWhileBusy( + malformedLocalStorage(name), touch, [true]); + assert.strictEqual( + err.message, + 'Could not read DOM storage items: Uncaught Error: localStorage ' + + 'database is malformed: expected schema_version to be an integer'); + } +} + +runTest().then(common.mustCall()); diff --git a/test/parallel/test-inspector-dom-storage-malformed.js b/test/parallel/test-inspector-dom-storage-malformed.js index d9ac2bbfd0be..6b1facd344bd 100644 --- a/test/parallel/test-inspector-dom-storage-malformed.js +++ b/test/parallel/test-inspector-dom-storage-malformed.js @@ -83,16 +83,14 @@ async function getDOMStorageItems(localStorageFile) { { message: 'Could not read DOM storage items: the backing file is malformed' }, ); - // A wrong-typed schema_version makes Storage::Open() throw, which has to be - // caught rather than left pending on an isolate with no JavaScript running. - // Its message reaches the frontend. + // A wrong-typed schema_version keeps the store from opening, and the reason + // reaches the frontend. await assert.rejects( getDOMStorageItems( malformedLocalStorage( 'bad-schema-version.db', 'one', Buffer.from('hello', 'utf16le'))), { - // The reason comes off the v8::Message, hence the "Uncaught" prefix; - // converting the exception itself would run user JavaScript. + // Formatted like a v8::Message, hence the "Uncaught Error" prefix. message: 'Could not read DOM storage items: Uncaught Error: ' + 'localStorage database is malformed: expected schema_version to be ' + 'an integer',