Browse Source

fix: disable SIGUSR1 when --inspect is disabled (#34182)

* fix: disable SIGUSR1 when --inspect is disabled (#33188)

* Update .patches

* chore: rebase patches
Samuel Attard 2 years ago
parent
commit
a6cee3acd5

+ 1 - 0
patches/node/.patches

@@ -46,3 +46,4 @@ macos_don_t_use_thread-unsafe_strtok_3524.patch
 process_fix_hang_after_note_exit_3521.patch
 macos_avoid_posix_spawnp_cwd_bug_3597.patch
 build_disable_v8_pointer_compression_on_32bit_archs.patch
+feat_add_knostartdebugsignalhandler_to_environment_to_prevent.patch

+ 69 - 0
patches/node/feat_add_knostartdebugsignalhandler_to_environment_to_prevent.patch

@@ -0,0 +1,69 @@
+From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
+From: Samuel Attard <[email protected]>
+Date: Mon, 7 Mar 2022 16:36:28 -0800
+Subject: feat: add kNoStartDebugSignalHandler to Environment to prevent
+ SIGUSR1 handling
+
+This patch should be upstreamed, it allows embedders to prevent the call to StartDebugSignalHandler which handles SIGUSR1 and starts the inspector agent.  Apps that have --inspect disabled also don't want SIGUSR1 to have this affect.
+
+diff --git a/src/env-inl.h b/src/env-inl.h
+index 7c7ee3207089bf3e51db646a367bdd6deba18628..0144b7ca31a8c41a893d9549a9214c0c86477af7 100644
+--- a/src/env-inl.h
++++ b/src/env-inl.h
+@@ -885,6 +885,10 @@ inline bool Environment::should_initialize_inspector() const {
+   return (flags_ & EnvironmentFlags::kNoInitializeInspector) == 0;
+ }
+ 
++inline bool Environment::should_start_debug_signal_handler() const {
++  return (flags_ & EnvironmentFlags::kNoStartDebugSignalHandler) == 0;
++}
++
+ bool Environment::filehandle_close_warning() const {
+   return emit_filehandle_warning_;
+ }
+diff --git a/src/env.h b/src/env.h
+index 6b22cc4aaaf59d309d1bcebfbb3710ceeafd13e6..030a3b65c8c09aa71dcc7cdebee920574700807d 100644
+--- a/src/env.h
++++ b/src/env.h
+@@ -1205,6 +1205,7 @@ class Environment : public MemoryRetainer {
+   inline bool tracks_unmanaged_fds() const;
+   inline bool hide_console_windows() const;
+   inline bool should_initialize_inspector() const;
++  inline bool should_start_debug_signal_handler() const;
+   inline uint64_t thread_id() const;
+   inline worker::Worker* worker_context() const;
+   Environment* worker_parent_env() const;
+diff --git a/src/inspector_agent.cc b/src/inspector_agent.cc
+index c4a3322c6d972fc2052af75b79389c522924d9c5..39ff8df415be1908ba404fba34d9cedda04a88af 100644
+--- a/src/inspector_agent.cc
++++ b/src/inspector_agent.cc
+@@ -680,8 +680,10 @@ bool Agent::Start(const std::string& path,
+                               StartIoThreadAsyncCallback));
+     uv_unref(reinterpret_cast<uv_handle_t*>(&start_io_thread_async));
+     start_io_thread_async.data = this;
+-    // Ignore failure, SIGUSR1 won't work, but that should not block node start.
+-    StartDebugSignalHandler();
++    if (parent_env_->should_start_debug_signal_handler()) {
++      // Ignore failure, SIGUSR1 won't work, but that should not block node start.
++      StartDebugSignalHandler();
++    }
+ 
+     parent_env_->AddCleanupHook([](void* data) {
+       Environment* env = static_cast<Environment*>(data);
+diff --git a/src/node.h b/src/node.h
+index 22e095804bca9d73d22707169c5aff2b13994344..481898a1294bbf593b714173e0857cebf5368cd0 100644
+--- a/src/node.h
++++ b/src/node.h
+@@ -414,7 +414,11 @@ enum Flags : uint64_t {
+   // Controls whether or not the Environment should call InitializeInspector.
+   // This control is needed by embedders who may not want to initialize the V8
+   // inspector in situations where it already exists.
+-  kNoInitializeInspector = 1 << 6
++  kNoInitializeInspector = 1 << 6,
++  // Controls where or not the InspectorAgent for this Environment should
++  // call StartDebugSignalHandler.  This control is needed by embedders who may
++  // not want to allow other processes to start the V8 inspector.
++  kNoStartDebugSignalHandler = 1 << 7
+ };
+ }  // namespace EnvironmentFlags
+ 

+ 6 - 0
shell/common/node_bindings.cc

@@ -469,6 +469,12 @@ node::Environment* NodeBindings::CreateEnvironment(
   uint64_t flags = node::EnvironmentFlags::kDefaultFlags |
                    node::EnvironmentFlags::kHideConsoleWindows;
 
+  if (!electron::fuses::IsNodeCliInspectEnabled()) {
+    // If --inspect and friends are disabled we also shouldn't listen for
+    // SIGUSR1
+    flags |= node::EnvironmentFlags::kNoStartDebugSignalHandler;
+  }
+
   if (browser_env_ != BrowserEnvironment::kBrowser) {
     // Only one ESM loader can be registered per isolate -
     // in renderer processes this should be blink. We need to tell Node.js