src: wrap HostPort in ExclusiveAccess

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.
nodejs · bnoordhuis · Feb 13, 2020 · Feb 13, 2020 · Feb 13, 2020 · 18b5694246aefc197aed6f88773bded07cdba66d
commit 18b5694246aefc197aed6f88773bded07cdba66d
diff --git a/src/env-inl.h b/src/env-inl.h
@@ -743,7 +743,8 @@ inline uint64_t Environment::heap_prof_interval() const {
 
 #endif  // HAVE_INSPECTOR
 
-inline std::shared_ptr<HostPort> Environment::inspector_host_port() {
+inline
+std::shared_ptr<ExclusiveAccess<HostPort>> Environment::inspector_host_port() {
   return inspector_host_port_;
 }
 

diff --git 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<HostPort>(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
@@ -1227,7 +1227,7 @@ class Environment : public MemoryRetainer {
                                  void* data);
 
   inline std::shared_ptr<EnvironmentOptions> options();
-  inline std::shared_ptr<HostPort> inspector_host_port();
+  inline std::shared_ptr<ExclusiveAccess<HostPort>> inspector_host_port();
 
   // The BaseObject count is a debugging helper that makes sure that there are
   // no memory leaks caused by BaseObjects staying alive longer than expected
@@ -1326,7 +1326,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<HostPort> inspector_host_port_;
+  std::shared_ptr<ExclusiveAccess<HostPort>> inspector_host_port_;
   std::vector<std::string> exec_argv_;
   std::vector<std::string> argv_;
   std::string exec_path_;

diff --git 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<HostPort> host_port,
+                  std::shared_ptr<ExclusiveAccess<HostPort>> host_port,
                   bool is_main) {
   path_ = path;
   debug_options_ = options;

diff --git 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<HostPort> host_port,
+             std::shared_ptr<ExclusiveAccess<HostPort>> 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<HostPort> host_port() { return host_port_; }
+  std::shared_ptr<ExclusiveAccess<HostPort>> host_port() { return host_port_; }
   void ContextCreated(v8::Local<v8::Context> 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<HostPort> host_port_;
+  std::shared_ptr<ExclusiveAccess<HostPort>> 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
@@ -239,7 +239,7 @@ class InspectorIoDelegate: public node::inspector::SocketServerDelegate {
 std::unique_ptr<InspectorIo> InspectorIo::Start(
     std::shared_ptr<MainThreadHandle> main_thread,
     const std::string& path,
-    std::shared_ptr<HostPort> host_port,
+    std::shared_ptr<ExclusiveAccess<HostPort>> host_port,
     const InspectPublishUid& inspect_publish_uid) {
   auto io = std::unique_ptr<InspectorIo>(
       new InspectorIo(main_thread,
@@ -254,7 +254,7 @@ std::unique_ptr<InspectorIo> InspectorIo::Start(
 
 InspectorIo::InspectorIo(std::shared_ptr<MainThreadHandle> main_thread,
                          const std::string& path,
-                         std::shared_ptr<HostPort> host_port,
+                         std::shared_ptr<ExclusiveAccess<HostPort>> host_port,
                          const InspectPublishUid& inspect_publish_uid)
     : main_thread_(main_thread),
       host_port_(host_port),
@@ -293,18 +293,26 @@ void InspectorIo::ThreadMain() {
   std::unique_ptr<InspectorIoDelegate> delegate(
       new InspectorIoDelegate(queue, main_thread_, id_,
                               script_path, script_name_));
+  std::string host;
+  int port;
+  {
+    ExclusiveAccess<HostPort>::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
   queue.reset();
   {
     Mutex::ScopedLock scoped_lock(thread_start_lock_);
     if (server.Start()) {
-      host_port_->set_port(server.Port());
+      ExclusiveAccess<HostPort>::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<HostPort>::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
@@ -30,7 +30,7 @@ class InspectorIo {
   static std::unique_ptr<InspectorIo> Start(
       std::shared_ptr<MainThreadHandle> main_thread,
       const std::string& path,
-      std::shared_ptr<HostPort> host_port,
+      std::shared_ptr<ExclusiveAccess<HostPort>> 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<MainThreadHandle> handle,
               const std::string& path,
-              std::shared_ptr<HostPort> host_port,
+              std::shared_ptr<ExclusiveAccess<HostPort>> 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<RequestQueue> request_queue_;
-  std::shared_ptr<HostPort> host_port_;
+  std::shared_ptr<ExclusiveAccess<HostPort>> 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
@@ -278,12 +278,14 @@ void Open(const FunctionCallbackInfo<Value>& args) {
 
   if (args.Length() > 0 && args[0]->IsUint32()) {
     uint32_t port = args[0].As<Uint32>()->Value();
-    agent->host_port()->set_port(static_cast<int>(port));
+    ExclusiveAccess<HostPort>::Scoped host_port(agent->host_port());
+    host_port->set_port(static_cast<int>(port));
   }
 
   if (args.Length() > 1 && args[1]->IsString()) {
     Utf8Value host(env->isolate(), args[1].As<String>());
-    agent->host_port()->set_host(*host);
+    ExclusiveAccess<HostPort>::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
@@ -51,7 +51,8 @@ static void ProcessTitleSetter(Local<Name> property,
 static void DebugPortGetter(Local<Name> property,
                             const PropertyCallbackInfo<Value>& info) {
   Environment* env = Environment::GetCurrent(info);
-  int port = env->inspector_host_port()->port();
+  ExclusiveAccess<HostPort>::Scoped host_port(env->inspector_host_port());
+  int port = host_port->port();
   info.GetReturnValue().Set(port);
 }
 
@@ -60,7 +61,8 @@ static void DebugPortSetter(Local<Name> property,
                             const PropertyCallbackInfo<void>& info) {
   Environment* env = Environment::GetCurrent(info);
   int32_t port = value->Int32Value(env->context()).FromMaybe(0);
-  env->inspector_host_port()->set_port(static_cast<int>(port));
+  ExclusiveAccess<HostPort>::Scoped host_port(env->inspector_host_port());
+  host_port->set_port(static_cast<int>(port));
 }
 
 static void GetParentProcessId(Local<Name> property,