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/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(); }, 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 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',