cluster: only assign debug port if in valid range. #14816
Conversation
See: nodejs#8159 Per above discussion, cluster.fork() currently sends --debug-port to forked processes regardless of whether debug is enabled, and more importantly, without any bounds checking. This will rather unexpectedly crash any Node process that forks enough times. This patch simply bounds checks --debug-port and doesn't set arg if out of bounds (V8 requires 1024 < debug-port < 65535). This will prevent surprises to callers of cluster.fork() while waiting for the debug part of node to be rewritten as mentioned in issue discussion above.
mjseidel
force-pushed
the
fix-issue-8159
branch
from
167fbe4
to
c382ced
Compare
| @@ -307,17 +307,19 @@ function masterInit() { | |||
| workerEnv = util._extend(workerEnv, env); | |||
| workerEnv.NODE_UNIQUE_ID = '' + id; | |||
|
|
|||
| for (var i = 0; i < execArgv.length; i++) { | |||
| var match = execArgv[i].match(/^(--debug|--debug-brk)(=\d+)?$/); | |||
| if (1024 < debugPort < 65535) { | |||
There was a problem hiding this comment.
I don't know about this syntax...
$ node
> 1 < 2 < 3
true
> 1 < 5 < 3
true
> 1 < 0 < 3
true
you probably need
if (debugPort > 1024 && debugPort < 65535)
...
There was a problem hiding this comment.
Don't those numbers depend on your system configuration?
There was a problem hiding this comment.
@sam-github I don't believe so; these numbers are hardcoded in node.cc:
|
@bahamas10, changing to your syntax and re-committing, thanks. |
Additionally, fix previous commit's bug of disallowing 1024 and 65535, which are legal debug ports.
mjseidel
force-pushed
the
fix-issue-8159
branch
from
c382ced
to
3de2ae9
Compare
|
Instead of hard coding port numbers throughout the code, maybe they should be defined as constants. This approach also prevents workers from being debugged after they reach the upper limit. Perhaps the cluster master should track which ports have been allocated to workers. |
| @@ -307,17 +307,19 @@ function masterInit() { | |||
| workerEnv = util._extend(workerEnv, env); | |||
| workerEnv.NODE_UNIQUE_ID = '' + id; | |||
|
|
|||
| for (var i = 0; i < execArgv.length; i++) { | |||
| var match = execArgv[i].match(/^(--debug|--debug-brk)(=\d+)?$/); | |||
| if !(debug_port < 1024 || debug_port > 65535) { // If port in range according to node.cc:2903 | |||
There was a problem hiding this comment.
if (debug_port >= 1024 && debug_port <= 65536) {
|
I think workers should have a "secondary" ID. One only present in the environment, but maintained by cluster, and ID that has the property that new workers always get the lowest unallocated ID > 0. There are a number of use-cases for such an ID. One is to do things open ports at semi-stable locations, it would all the debug port to be: DEBUG_BASE + ID. I would have other uses for it as well, there are various things I've worked on that would benefit from such an ID. Of course, ID is a terrible name, workers already have a worker ID, perhaps "index": Thoughts, @cjhrig? |
|
Is there any reason that the existing |
|
@cjihrig, This definitely retains some undesirable behavior, like workers not being debuggable above the max port value. I just think it's strictly an improvement over what currently happens, which is a hard crash :) To your other point about lowest-available id value: forking in Node is already pretty costly timewise, and adding overhead to debug port assignment (even when debug will not be used) could make it costlier. That's just my two cents and I definitely have not deep-dived on this codebase. |
|
@mjseidel @cjihrig ... how would you like to proceed on this one? If it's a bug in v0.12 that needs to be fixed, then retargeting the PR there is fine. Otherwise, this may need to be reopened against master on https://github.com/nodejs/node |
|
This is mostly mitigated in 3.0.0. Now, a debug port is only assigned if the master is started in debug mode. Technically it's still possible to encounter, but you'd have to have a very long running process running in debug mode, which is a bit counter intuitive. |
|
Ok. then I'd be inclined to close. A new issue/PR can be opened in nodejs/node if it continues to be a problem |
|
Going ahead and closing here. We can backport the change from 3.x if necessary but pretty sure it's safe to leave this here. |
See: #8159
Per above discussion, cluster.fork() currently sends --debug-port to
forked processes regardless of whether debug is enabled, and more
importantly, without any bounds checking. This will rather unexpectedly
crash any Node process that forks enough times.
This patch simply bounds checks --debug-port and doesn't set arg if out
of bounds (V8 requires 1024 < debug-port < 65535). This will prevent
surprises to callers of cluster.fork() while waiting for the debug part
of node to be rewritten as mentioned in issue discussion above.