From 75971009d0ff45e9fd09dcd80e5b3763d6daf9aa Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Thu, 13 Feb 2020 16:31:05 +0100 Subject: [PATCH] src: wrap HostPort in ExclusiveAccess MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit I found it exceedingly hard to figure out if there is a race condition where one thread reads the inspector agent's HostPort's properties while another modifies them concurrently. I think the answer is "no, there isn't" but with this commit use sites are forced to unwrap the object (and acquire the mutex in the process), making it a great deal easier to reason about correctness. PR-URL: https://github.com/nodejs/node/pull/31717 Reviewed-By: James M Snell Reviewed-By: Colin Ihrig Reviewed-By: Anna Henningsen Reviewed-By: Tobias Nießen Reviewed-By: David Carlier --- src/env-inl.h | 3 ++- src/env.cc | 3 ++- src/env.h | 4 ++-- src/inspector_agent.cc | 2 +- src/inspector_agent.h | 6 +++--- src/inspector_io.cc | 21 +++++++++++++++------ src/inspector_io.h | 6 +++--- src/inspector_js_api.cc | 6 ++++-- src/node_process_object.cc | 6 ++++-- 9 files changed, 36 insertions(+), 21 deletions(-) diff --git a/src/env-inl.h b/src/env-inl.h index 99b5273b9b25ce..d833c025a3caad 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -732,7 +732,8 @@ inline uint64_t Environment::heap_prof_interval() const { #endif // HAVE_INSPECTOR -inline std::shared_ptr Environment::inspector_host_port() { +inline +std::shared_ptr> Environment::inspector_host_port() { return inspector_host_port_; } diff --git a/src/env.cc b/src/env.cc index 8426148ea74d79..dd3e08f0dcafa9 100644 --- a/src/env.cc +++ b/src/env.cc @@ -321,7 +321,8 @@ Environment::Environment(IsolateData* isolate_data, // part of the per-Isolate option set, for which in turn the defaults are // part of the per-process option set. options_.reset(new EnvironmentOptions(*isolate_data->options()->per_env)); - inspector_host_port_.reset(new HostPort(options_->debug_options().host_port)); + inspector_host_port_.reset( + new ExclusiveAccess(options_->debug_options().host_port)); #if HAVE_INSPECTOR // We can only create the inspector agent after having cloned the options. diff --git a/src/env.h b/src/env.h index f6f68a37681ff6..67e735cdf3be58 100644 --- a/src/env.h +++ b/src/env.h @@ -1232,7 +1232,7 @@ class Environment : public MemoryRetainer { void* data); inline std::shared_ptr options(); - inline std::shared_ptr inspector_host_port(); + inline std::shared_ptr> inspector_host_port(); inline AsyncRequest* thread_stopper() { return &thread_stopper_; } @@ -1331,7 +1331,7 @@ class Environment : public MemoryRetainer { // server starts listening), but when the inspector server starts // the inspector_host_port_->port() will be the actual port being // used. - std::shared_ptr inspector_host_port_; + std::shared_ptr> inspector_host_port_; std::vector exec_argv_; std::vector argv_; std::string exec_path_; diff --git a/src/inspector_agent.cc b/src/inspector_agent.cc index aff52a5a5eccd1..94433b75d0fdb2 100644 --- a/src/inspector_agent.cc +++ b/src/inspector_agent.cc @@ -757,7 +757,7 @@ Agent::~Agent() {} bool Agent::Start(const std::string& path, const DebugOptions& options, - std::shared_ptr host_port, + std::shared_ptr> host_port, bool is_main) { path_ = path; debug_options_ = options; diff --git a/src/inspector_agent.h b/src/inspector_agent.h index d5088a1b546904..089077370db049 100644 --- a/src/inspector_agent.h +++ b/src/inspector_agent.h @@ -48,7 +48,7 @@ class Agent { // Create client_, may create io_ if option enabled bool Start(const std::string& path, const DebugOptions& options, - std::shared_ptr host_port, + std::shared_ptr> host_port, bool is_main); // Stop and destroy io_ void Stop(); @@ -110,7 +110,7 @@ class Agent { void RequestIoThreadStart(); const DebugOptions& options() { return debug_options_; } - std::shared_ptr host_port() { return host_port_; } + std::shared_ptr> host_port() { return host_port_; } void ContextCreated(v8::Local context, const ContextInfo& info); // Interface for interacting with inspectors in worker threads @@ -133,7 +133,7 @@ class Agent { // pointer which is meant to store the actual host and port of the inspector // server. DebugOptions debug_options_; - std::shared_ptr host_port_; + std::shared_ptr> host_port_; bool pending_enable_async_hook_ = false; bool pending_disable_async_hook_ = false; diff --git a/src/inspector_io.cc b/src/inspector_io.cc index ed9035136c51db..75290317d2fcae 100644 --- a/src/inspector_io.cc +++ b/src/inspector_io.cc @@ -239,7 +239,7 @@ class InspectorIoDelegate: public node::inspector::SocketServerDelegate { std::unique_ptr InspectorIo::Start( std::shared_ptr main_thread, const std::string& path, - std::shared_ptr host_port, + std::shared_ptr> host_port, const InspectPublishUid& inspect_publish_uid) { auto io = std::unique_ptr( new InspectorIo(main_thread, @@ -254,7 +254,7 @@ std::unique_ptr InspectorIo::Start( InspectorIo::InspectorIo(std::shared_ptr main_thread, const std::string& path, - std::shared_ptr host_port, + std::shared_ptr> host_port, const InspectPublishUid& inspect_publish_uid) : main_thread_(main_thread), host_port_(host_port), @@ -293,10 +293,17 @@ void InspectorIo::ThreadMain() { std::unique_ptr delegate( new InspectorIoDelegate(queue, main_thread_, id_, script_path, script_name_)); + std::string host; + int port; + { + ExclusiveAccess::Scoped host_port(host_port_); + host = host_port->host(); + port = host_port->port(); + } InspectorSocketServer server(std::move(delegate), &loop, - host_port_->host(), - host_port_->port(), + std::move(host), + port, inspect_publish_uid_); request_queue_ = queue->handle(); // Its lifetime is now that of the server delegate @@ -304,7 +311,8 @@ void InspectorIo::ThreadMain() { { Mutex::ScopedLock scoped_lock(thread_start_lock_); if (server.Start()) { - host_port_->set_port(server.Port()); + ExclusiveAccess::Scoped host_port(host_port_); + host_port->set_port(server.Port()); } thread_start_condition_.Broadcast(scoped_lock); } @@ -313,7 +321,8 @@ void InspectorIo::ThreadMain() { } std::string InspectorIo::GetWsUrl() const { - return FormatWsAddress(host_port_->host(), host_port_->port(), id_, true); + ExclusiveAccess::Scoped host_port(host_port_); + return FormatWsAddress(host_port->host(), host_port->port(), id_, true); } InspectorIoDelegate::InspectorIoDelegate( diff --git a/src/inspector_io.h b/src/inspector_io.h index e9f94056733994..10cca26ff1b9ee 100644 --- a/src/inspector_io.h +++ b/src/inspector_io.h @@ -30,7 +30,7 @@ class InspectorIo { static std::unique_ptr Start( std::shared_ptr main_thread, const std::string& path, - std::shared_ptr host_port, + std::shared_ptr> host_port, const InspectPublishUid& inspect_publish_uid); // Will block till the transport thread shuts down @@ -42,7 +42,7 @@ class InspectorIo { private: InspectorIo(std::shared_ptr handle, const std::string& path, - std::shared_ptr host_port, + std::shared_ptr> host_port, const InspectPublishUid& inspect_publish_uid); // Wrapper for agent->ThreadMain() @@ -57,7 +57,7 @@ class InspectorIo { // Used to post on a frontend interface thread, lives while the server is // running std::shared_ptr request_queue_; - std::shared_ptr host_port_; + std::shared_ptr> host_port_; InspectPublishUid inspect_publish_uid_; // The IO thread runs its own uv_loop to implement the TCP server off diff --git a/src/inspector_js_api.cc b/src/inspector_js_api.cc index 45b1c83bea7f34..703c9ff598fdf2 100644 --- a/src/inspector_js_api.cc +++ b/src/inspector_js_api.cc @@ -278,12 +278,14 @@ void Open(const FunctionCallbackInfo& args) { if (args.Length() > 0 && args[0]->IsUint32()) { uint32_t port = args[0].As()->Value(); - agent->host_port()->set_port(static_cast(port)); + ExclusiveAccess::Scoped host_port(agent->host_port()); + host_port->set_port(static_cast(port)); } if (args.Length() > 1 && args[1]->IsString()) { Utf8Value host(env->isolate(), args[1].As()); - agent->host_port()->set_host(*host); + ExclusiveAccess::Scoped host_port(agent->host_port()); + host_port->set_host(*host); } agent->StartIoThread(); diff --git a/src/node_process_object.cc b/src/node_process_object.cc index 7cf8c125009606..ddbb58abe535b0 100644 --- a/src/node_process_object.cc +++ b/src/node_process_object.cc @@ -51,7 +51,8 @@ static void ProcessTitleSetter(Local property, static void DebugPortGetter(Local property, const PropertyCallbackInfo& info) { Environment* env = Environment::GetCurrent(info); - int port = env->inspector_host_port()->port(); + ExclusiveAccess::Scoped host_port(env->inspector_host_port()); + int port = host_port->port(); info.GetReturnValue().Set(port); } @@ -60,7 +61,8 @@ static void DebugPortSetter(Local property, const PropertyCallbackInfo& info) { Environment* env = Environment::GetCurrent(info); int32_t port = value->Int32Value(env->context()).FromMaybe(0); - env->inspector_host_port()->set_port(static_cast(port)); + ExclusiveAccess::Scoped host_port(env->inspector_host_port()); + host_port->set_port(static_cast(port)); } static void GetParentProcessId(Local property,