src: add --disable-sigusr1 to prevent signal i/o thread

[RafaelGSS]

This commit adds a new flag --disable-sigusr1 to prevent the SignalIOThread to be up listening the SIGUSR1 events and then starting the debugging session.

---

This flag is handy when dealing with multiple apps in the same environment. I wouldn't call it a full security guarantee since one would require access to the environment in order to expose the debugger, and with that assumption, one can do anything one wants. However, I do believe this would serve as a seatbelt in some scenarios.

Please note users already do it by require('inspector').close(), but a flag seems more suitable.

[juanarbol]

Idk if this is the wait to do it. There's also SIGUSR2 among other signals. Wouldn't that be better to have a "blacklist" approach instead? So the logic scales to any signal.

[RafaelGSS]

Idk if this is the wait to do it. There's also SIGUSR2 among other signals. Wouldn't that be better to have a "blacklist" approach instead? So the logic scales to any signal.

Initially, I thought about the --disable-signal=SIGUSR1 but then I realized that there's no "damage" in receiving a SIGUSR2. AFAIK only SIGUSR1 would initiate the debugging session.

I could also rename the flag to --disable-sigusr1-debug to make it more explicit but, I don't think there's a need for that.

Also, having a --disable-signal and not implementing other signals might sound confusing to users.

[juanarbol]

I don't disagree w/ this patch. But if this is intended to disable/flip a default behaviour, it may need some update in the documentation side as well.

Useful when activating the inspector by sending the SIGUSR1 signal.

Refs: https://nodejs.org/docs/latest/api/cli.html#--inspect-porthostport

[RafaelGSS]

Ok. I can update the --inspect documentation to explicitly mention when --disable-sigusr1 is passed, the debugging session initialized with SIGUSR1 won't work.

[codecov]

Codecov Report

Attention: Patch coverage is 75.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 89.13%. Comparing base (98d4ebc) to head (63d32a4).
Report is 71 commits behind head on main.

Files with missing lines	Patch %	Lines
src/env-inl.h	50.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #56441      +/-   ##
==========================================
+ Coverage   88.53%   89.13%   +0.59%     
==========================================
  Files         657      662       +5     
  Lines      190741   191558     +817     
  Branches    36607    36861     +254     
==========================================
+ Hits       168881   170743    +1862     
+ Misses      15036    13686    -1350     
- Partials     6824     7129     +305     

Files with missing lines	Coverage Δ
src/node_options.cc	87.94% <100.00%> (+0.01%)	⬆️
src/node_options.h	98.32% <100.00%> (+0.01%)	⬆️
src/env-inl.h	96.78% <50.00%> (-0.43%)	⬇️

... and 88 files with indirect coverage changes

[RafaelGSS]

PTAL @juanarbol

I have also changed the flag to be in stability 1.1 so we can re-evaluate if we need to rename or make it more generic in the future.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/64314/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/64393/ 💛

[juanarbol]

sgtm

[jasnell]

Suggested change

	setInterval(() => {}, 1000);
	setInterval(() => {}, 1000);

[nodejs-github-bot]