debugger: make listen address configurable

`--debug=1.2.3.4:5678` and `--debug=example.com:5678` are now accepted,
likewise the `--debug-brk` and `--debug-port` switch.  The latter is
now something of a misnomer but it's undocumented and for internal use
only so it shouldn't matter too much.

`--inspect=1.2.3.4:5678` and `--inspect=example.com:5678` are also
accepted but don't use the host name yet; they still bind to the
default address.

Fixes: https://github.com/nodejs/node/issues/3306
PR-URL: https://github.com/nodejs/node/pull/3316
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>

lib/_debug_agent.js

@@ -18,9 +18,10 @@ exports.start = function start() {
     process._rawDebug(err.stack || err);
   });
 
-  agent.listen(process._debugAPI.port, function() {
-    var addr = this.address();
-    process._rawDebug('Debugger listening on port %d', addr.port);
+  agent.listen(process._debugAPI.port, process._debugAPI.host, function() {
+    const addr = this.address();
+    const host = net.isIPv6(addr.address) ? `[${addr.address}]` : addr.address;
+    process._rawDebug('Debugger listening on %s:%d', host, addr.port);
     process._debugAPI.notifyListen();
   });
 

src/debug-agent.cc

@@ -44,6 +44,7 @@ using v8::Integer;
 using v8::Isolate;
 using v8::Local;
 using v8::Locker;
+using v8::NewStringType;
 using v8::Object;
 using v8::String;
 using v8::Value;
@@ -69,7 +70,7 @@ Agent::~Agent() {
 }
 
 
-bool Agent::Start(int port, bool wait) {
+bool Agent::Start(const std::string& host, int port, bool wait) {
   int err;
 
   if (state_ == kRunning)
@@ -85,6 +86,7 @@ bool Agent::Start(int port, bool wait) {
     goto async_init_failed;
   uv_unref(reinterpret_cast<uv_handle_t*>(&child_signal_));
 
+  host_ = host;
   port_ = port;
   wait_ = wait;
 
@@ -207,6 +209,10 @@ void Agent::InitAdaptor(Environment* env) {
       t->GetFunction()->NewInstance(env->context()).ToLocalChecked();
   api->SetAlignedPointerInInternalField(0, this);
 
+  api->Set(String::NewFromUtf8(isolate, "host",
+                               NewStringType::kNormal).ToLocalChecked(),
+           String::NewFromUtf8(isolate, host_.data(), NewStringType::kNormal,
+                               host_.size()).ToLocalChecked());
   api->Set(String::NewFromUtf8(isolate, "port"), Integer::New(isolate, port_));
 
   env->process_object()->Set(String::NewFromUtf8(isolate, "_debugAPI"), api);

src/debug-agent.h

@@ -32,6 +32,7 @@
 #include "v8-debug.h"
 
 #include <string.h>
+#include <string>
 
 // Forward declaration to break recursive dependency chain with src/env.h.
 namespace node {
@@ -75,7 +76,7 @@ class Agent {
   typedef void (*DispatchHandler)(node::Environment* env);
 
   // Start the debugger agent thread
-  bool Start(int port, bool wait);
+  bool Start(const std::string& host, int port, bool wait);
   // Listen for debug events
   void Enable();
   // Stop the debugger agent
@@ -114,6 +115,7 @@ class Agent {
 
   State state_;
 
+  std::string host_;
   int port_;
   bool wait_;
 

src/node.cc

@@ -58,6 +58,8 @@
 #include <stdlib.h>
 #include <string.h>
 #include <sys/types.h>
+
+#include <string>
 #include <vector>
 
 #if defined(NODE_HAVE_I18N_SUPPORT)
@@ -140,10 +142,14 @@ static unsigned int preload_module_count = 0;
 static const char** preload_modules = nullptr;
 #if HAVE_INSPECTOR
 static bool use_inspector = false;
+#else
+static const bool use_inspector = false;
 #endif
 static bool use_debug_agent = false;
 static bool debug_wait_connect = false;
+static std::string debug_host;  // NOLINT(runtime/string)
 static int debug_port = 5858;
+static std::string inspector_host;  // NOLINT(runtime/string)
 static int inspector_port = 9229;
 static const int v8_default_thread_pool_size = 4;
 static int v8_thread_pool_size = v8_default_thread_pool_size;
@@ -202,23 +208,20 @@ static struct {
     platform_ = nullptr;
   }
 
-#if HAVE_INSPECTOR
   void StartInspector(Environment *env, int port, bool wait) {
+#if HAVE_INSPECTOR
     env->inspector_agent()->Start(platform_, port, wait);
-  }
 #endif  // HAVE_INSPECTOR
+  }
 
   v8::Platform* platform_;
 #else  // !NODE_USE_V8_PLATFORM
   void Initialize(int thread_pool_size) {}
   void PumpMessageLoop(Isolate* isolate) {}
   void Dispose() {}
-#if HAVE_INSPECTOR
   void StartInspector(Environment *env, int port, bool wait) {
     env->ThrowError("Node compiled with NODE_USE_V8_PLATFORM=0");
   }
-#endif  // HAVE_INSPECTOR
-
 #endif  // !NODE_USE_V8_PLATFORM
 } v8_platform;
 
@@ -3430,6 +3433,7 @@ static bool ParseDebugOpt(const char* arg) {
     debug_wait_connect = true;
     port = arg + sizeof("--debug-brk=") - 1;
   } else if (!strncmp(arg, "--debug-port=", sizeof("--debug-port=") - 1)) {
+    // XXX(bnoordhuis) Misnomer, configures port and listen address.
     port = arg + sizeof("--debug-port=") - 1;
 #if HAVE_INSPECTOR
   // Specifying both --inspect and --debug means debugging is on, using Chromium
@@ -3451,23 +3455,48 @@ static bool ParseDebugOpt(const char* arg) {
     return false;
   }
 
-  if (port != nullptr) {
-    int port_int = atoi(port);
-    if (port_int < 1024 || port_int > 65535) {
+  if (port == nullptr) {
+    return true;
+  }
+
+  std::string* const the_host = use_inspector ? &inspector_host : &debug_host;
+  int* const the_port = use_inspector ? &inspector_port : &debug_port;
+
+  // FIXME(bnoordhuis) Move IPv6 address parsing logic to lib/net.js.
+  // It seems reasonable to support [address]:port notation
+  // in net.Server#listen() and net.Socket#connect().
+  const size_t port_len = strlen(port);
+  if (port[0] == '[' && port[port_len - 1] == ']') {
+    the_host->assign(port + 1, port_len - 2);
+    return true;
+  }
+
+  const char* const colon = strrchr(port, ':');
+  if (colon == nullptr) {
+    // Either a port number or a host name.  Assume that
+    // if it's not all decimal digits, it's a host name.
+    for (size_t n = 0; port[n] != '\0'; n += 1) {
+      if (port[n] < '0' || port[n] > '9') {
+        *the_host = port;
+        return true;
+      }
+    }
+  } else {
+    const bool skip = (colon > port && port[0] == '[' && colon[-1] == ']');
+    the_host->assign(port + skip, colon - skip);
+  }
+
+  char* endptr;
+  errno = 0;
+  const char* const digits = colon != nullptr ? colon + 1 : port;
+  const long result = strtol(digits, &endptr, 10);  // NOLINT(runtime/int)
+  if (errno != 0 || *endptr != '\0' || result < 1024 || result > 65535) {
     fprintf(stderr, "Debug port must be in range 1024 to 65535.\n");
     PrintHelp();
     exit(12);
   }
-#if HAVE_INSPECTOR
-    if (use_inspector) {
-      inspector_port = port_int;
-    } else {
-#endif
-      debug_port = port_int;
-#if HAVE_INSPECTOR
-    }
-#endif
-  }
+
+  *the_port = static_cast<int>(result);
 
   return true;
 }
@@ -3726,34 +3755,31 @@ static void DispatchMessagesDebugAgentCallback(Environment* env) {
 
 static void StartDebug(Environment* env, bool wait) {
   CHECK(!debugger_running);
-#if HAVE_INSPECTOR
   if (use_inspector) {
     v8_platform.StartInspector(env, inspector_port, wait);
     debugger_running = true;
   } else {
-#endif
     env->debugger_agent()->set_dispatch_handler(
           DispatchMessagesDebugAgentCallback);
-    debugger_running = env->debugger_agent()->Start(debug_port, wait);
+    debugger_running =
+        env->debugger_agent()->Start(debug_host, debug_port, wait);
     if (debugger_running == false) {
-      fprintf(stderr, "Starting debugger on port %d failed\n", debug_port);
+      fprintf(stderr, "Starting debugger on %s:%d failed\n",
+              debug_host.c_str(), debug_port);
       fflush(stderr);
       return;
     }
-#if HAVE_INSPECTOR
   }
-#endif
 }
 
 
 // Called from the main thread.
 static void EnableDebug(Environment* env) {
   CHECK(debugger_running);
-#if HAVE_INSPECTOR
+
   if (use_inspector) {
     return;
   }
-#endif
 
   // Send message to enable debug in workers
   HandleScope handle_scope(env->isolate());

test/parallel/test-cluster-disconnect-handles.js

@@ -25,11 +25,8 @@ cluster.schedulingPolicy = cluster.SCHED_RR;
 if (cluster.isMaster) {
   let isKilling = false;
   const handles = require('internal/cluster').handles;
-  // FIXME(bnoordhuis) lib/cluster.js scans the execArgv arguments for
-  // debugger flags and renumbers any port numbers it sees starting
-  // from the default port 5858.  Add a '.' that circumvents the
-  // scanner but is ignored by atoi(3).  Heinous hack.
-  cluster.setupMaster({ execArgv: [`--debug=${common.PORT}.`] });
+  const address = common.hasIPv6 ? '[::1]' : common.localhostIPv4;
+  cluster.setupMaster({ execArgv: [`--debug=${address}:${common.PORT}`] });
   const worker = cluster.fork();
   worker.once('exit', common.mustCall((code, signal) => {
     assert.strictEqual(code, 0, 'worker did not exit normally');

test/parallel/test-debug-port-cluster.js

@@ -16,7 +16,8 @@ child.stderr.setEncoding('utf8');
 
 const checkMessages = common.mustCall(() => {
   for (let port = PORT_MIN; port <= PORT_MAX; port += 1) {
-    assert(stderr.includes(`Debugger listening on port ${port}`));
+    const re = RegExp(`Debugger listening on (\\[::\\]|0\\.0\\.0\\.0):${port}`);
+    assert(re.test(stderr));
   }
 });
 

test/parallel/test-debug-port-from-cmdline.js

@@ -39,11 +39,10 @@ function processStderrLine(line) {
 function assertOutputLines() {
   var expectedLines = [
     'Starting debugger agent.',
-    'Debugger listening on port ' + debugPort
+    'Debugger listening on (\\[::\\]|0\\.0\\.0\\.0):' + debugPort,
   ];
 
   assert.equal(outputLines.length, expectedLines.length);
   for (var i = 0; i < expectedLines.length; i++)
-    assert.equal(outputLines[i], expectedLines[i]);
-
+    assert(RegExp(expectedLines[i]).test(outputLines[i]));
 }

test/parallel/test-debug-port-numbers.js

@@ -52,8 +52,9 @@ function kill(child) {
 
 process.on('exit', function() {
   for (const child of children) {
-    const one = RegExp(`Debugger listening on port ${child.test.port}`);
-    const two = RegExp(`connecting to 127.0.0.1:${child.test.port}`);
+    const port = child.test.port;
+    const one = RegExp(`Debugger listening on (\\[::\\]|0\.0\.0\.0):${port}`);
+    const two = RegExp(`connecting to 127.0.0.1:${port}`);
     assert(one.test(child.test.stdout));
     assert(two.test(child.test.stdout));
   }

test/parallel/test-debug-signal-cluster.js

@@ -63,11 +63,11 @@ process.on('exit', function onExit() {
 
 var expectedLines = [
   'Starting debugger agent.',
-  'Debugger listening on port ' + (port + 0),
+  'Debugger listening on (\\[::\\]|0\\.0\\.0\\.0):' + (port + 0),
   'Starting debugger agent.',
-  'Debugger listening on port ' + (port + 1),
+  'Debugger listening on (\\[::\\]|0\\.0\\.0\\.0):' + (port + 1),
   'Starting debugger agent.',
-  'Debugger listening on port ' + (port + 2),
+  'Debugger listening on (\\[::\\]|0\\.0\\.0\\.0):' + (port + 2),
 ];
 
 function assertOutputLines() {
@@ -79,5 +79,5 @@ function assertOutputLines() {
 
   assert.equal(outputLines.length, expectedLines.length);
   for (var i = 0; i < expectedLines.length; i++)
-    assert.equal(outputLines[i], expectedLines[i]);
+    assert(RegExp(expectedLines[i]).test(outputLines[i]));
 }

test/sequential/test-debug-host-port.js

@@ -0,0 +1,47 @@
+'use strict';
+
+const common = require('../common');
+const assert = require('assert');
+const spawn = require('child_process').spawn;
+
+let run = () => {};
+function test(args, re) {
+  const next = run;
+  run = () => {
+    const options = {encoding: 'utf8'};
+    const proc = spawn(process.execPath, args.concat(['-e', '0']), options);
+    let stderr = '';
+    proc.stderr.setEncoding('utf8');
+    proc.stderr.on('data', (data) => {
+      stderr += data;
+      if (re.test(stderr)) proc.kill();
+    });
+    proc.on('exit', common.mustCall(() => {
+      assert(re.test(stderr));
+      next();
+    }));
+  };
+}
+
+test(['--debug-brk'], /Debugger listening on (\[::\]|0\.0\.0\.0):5858/);
+test(['--debug-brk=1234'], /Debugger listening on (\[::\]|0\.0\.0\.0):1234/);
+test(['--debug-brk=127.0.0.1'], /Debugger listening on 127\.0\.0\.1:5858/);
+test(['--debug-brk=127.0.0.1:1234'], /Debugger listening on 127\.0\.0\.1:1234/);
+test(['--debug-brk=localhost'],
+     /Debugger listening on (\[::\]|127\.0\.0\.1):5858/);
+test(['--debug-brk=localhost:1234'],
+     /Debugger listening on (\[::\]|127\.0\.0\.1):1234/);
+
+if (common.hasIPv6) {
+  test(['--debug-brk=::'], /Debug port must be in range 1024 to 65535/);
+  test(['--debug-brk=::0'], /Debug port must be in range 1024 to 65535/);
+  test(['--debug-brk=::1'], /Debug port must be in range 1024 to 65535/);
+  test(['--debug-brk=[::]'], /Debugger listening on \[::\]:5858/);
+  test(['--debug-brk=[::0]'], /Debugger listening on \[::\]:5858/);
+  test(['--debug-brk=[::]:1234'], /Debugger listening on \[::\]:1234/);
+  test(['--debug-brk=[::0]:1234'], /Debugger listening on \[::\]:1234/);
+  test(['--debug-brk=[::ffff:127.0.0.1]:1234'],
+       /Debugger listening on \[::ffff:127\.0\.0\.1\]:1234/);
+}
+
+run();  // Runs tests in reverse order.