cluster,net: fix random port keying

This commit fixes an issue where sockets that are bound to a random
port are keyed on port 0 instead of the actual port that was assigned
to the socket.

Author: mscdex

lib/cluster.js

@@ -473,14 +473,21 @@ function masterInit() {
     // Stop processing if worker already disconnecting
     if (worker.exitedAfterDisconnect)
       return;
-    var args = [message.address,
-                message.port,
-                message.addressType,
-                message.fd,
-                message.index];
-    var key = args.join(':');
+
+    const port = +message.port;
+    var key = [message.address,
+               message.port,
+               message.addressType,
+               message.fd,
+               message.index].join(':');
     var handle = handles[key];
-    if (handle === undefined) {
+
+    // Keys containing port zero are special cases in that they represent
+    // temporary placeholders in the list of bound handles until the OS actually
+    // assigns the real port number. In these cases we must always create a new
+    // handle since there is no way the handle could be shared until the real
+    // port number is received.
+    if (handle === undefined || port === 0) {
       var constructor = RoundRobinHandle;
       // UDP is exempt from round-robin connection balancing for what should
       // be obvious reasons: it's connectionless. There is nothing to send to
@@ -501,15 +508,45 @@ function masterInit() {
     if (!handle.data) handle.data = message.data;
 
     // Set custom server data
-    handle.add(worker, function(errno, reply, handle) {
+    handle.add(worker, function(errno, reply, handle_) {
+      var data;
+      if (port === 0) {
+        delete handles[key];
+        var port_;
+        if (reply && reply.sockname && reply.sockname.port) {
+          port_ = reply.sockname.port;
+        } else if (handle_ && handle_.getsockname) {
+          const out = {};
+          handle_.getsockname(out);
+          port_ = out.port;
+        } else {
+          port_ = message.port;
+        }
+        key = [message.address,
+               port_,
+               message.addressType,
+               message.fd,
+               message.index].join(':');
+        if (!errno)
+          handles[key] = handle;
+        data = handle.data;
+        handle.key = key;
+      } else {
+        data = handles[key].data;
+      }
+
       reply = util._extend({
         errno: errno,
         key: key,
         ack: message.seq,
-        data: handles[key].data
+        data: data
       }, reply);
-      if (errno) delete handles[key];  // Gives other workers a chance to retry.
-      send(worker, reply, handle);
+
+      // Gives other workers a chance to retry.
+      if (errno && port !== 0)
+        delete handles[key];
+
+      send(worker, reply, handle_);
     });
   }
 
@@ -571,18 +608,23 @@ function workerInit() {
 
   // obj is a net#Server or a dgram#Socket object.
   cluster._getServer = function(obj, options, cb) {
-    const indexesKey = [ options.address,
+    var index;
+    if (+options.port !== 0) {
+      var indexesKey = [ options.address,
                          options.port,
                          options.addressType,
                          options.fd ].join(':');
-    if (indexes[indexesKey] === undefined)
-      indexes[indexesKey] = 0;
-    else
-      indexes[indexesKey]++;
+      if (indexes[indexesKey] === undefined)
+        index = indexes[indexesKey] = 0;
+      else
+        index = ++indexes[indexesKey];
+    } else {
+      index = 0;
+    }
 
     const message = util._extend({
       act: 'queryServer',
-      index: indexes[indexesKey],
+      index: index,
       data: null
     }, options);
 
@@ -591,6 +633,16 @@ function workerInit() {
     send(message, function(reply, handle) {
       if (obj._setServerData) obj._setServerData(reply.data);
 
+      if (+options.port === 0 && reply.sockname) {
+        indexesKey = [ options.address,
+                       reply.sockname.port,
+                       options.addressType,
+                       options.fd ].join(':');
+        if (indexes[indexesKey] === undefined)
+          index = indexes[indexesKey] = 0;
+        else
+          index = ++indexes[indexesKey];
+      }
       if (handle)
         shared(reply, handle, indexesKey, cb);  // Shared listen socket.
       else

lib/net.js

@@ -1259,6 +1259,7 @@ Server.prototype._listen2 = function(address, port, addressType, backlog, fd) {
   }
 
   // generate connection key, this should be unique to the connection
+  port = this.address().port;
   this._connectionKey = addressType + ':' + address + ':' + port;
 
   // unref the handle if the server was unref'ed prior to listening

test/parallel/test-dgram-exclusive-implicit-bind.js

@@ -75,8 +75,9 @@ if (cluster.isMaster) {
     cluster.fork();
     cluster.fork();
     if (!common.isWindows) {
-      cluster.fork({BOUND: 'y'});
-      cluster.fork({BOUND: 'y'});
+      cluster.fork({BOUND: '0'}).once('message', function(msg) {
+        cluster.fork({BOUND: msg});
+      });
     }
   });
 
@@ -87,8 +88,11 @@ if (cluster.isMaster) {
 
 var source = dgram.createSocket('udp4');
 
-if (process.env.BOUND === 'y') {
-  source.bind(0);
+if (process.env.BOUND) {
+  source.bind(+process.env.BOUND, function() {
+    if (process.env.BOUND === '0')
+      process.send(this.address().port);
+  });
 } else {
   // cluster doesn't know about exclusive sockets, so it won't close them. This
   // is expected, its the same situation for timers, outgoing tcp connections,

test/parallel/test-net-listen-multiple-random-ports.js

@@ -0,0 +1,54 @@
+'use strict';
+require('../common');
+var assert = require('assert');
+var cluster = require('cluster');
+var net = require('net');
+
+function noop() {}
+
+if (cluster.isMaster) {
+  var worker1 = cluster.fork();
+  var worker2 = cluster.fork();
+
+  worker1.on('message', onMessage);
+  worker2.on('message', onMessage);
+  function onMessage(obj) {
+    assert.strictEqual(typeof obj, 'object');
+    assert.strictEqual(obj.msg, 'success');
+    assert.strictEqual(typeof obj.port, 'number');
+    assert.ok(obj.port !== 0, 'Expected non-zero port number from worker');
+    this.listens = (this.listens || 0) + 1;
+    if (worker1.listens === 2 && worker2.listens === 2) {
+      worker1.kill();
+      worker2.kill();
+    }
+  }
+} else {
+  net.createServer(noop).on('error', function(err) {
+    // no errors expected
+    process.send('server1:' + err.code);
+  }).listen({
+    host: 'localhost',
+    port: 0,
+    exclusive: false
+  }, function() {
+    process.send({
+      msg: 'success',
+      port: this.address().port,
+    });
+  })
+
+  net.createServer(noop).on('error', function(err) {
+    // no errors expected
+    process.send('server2:' + err.code);
+  }).listen({
+    host: 'localhost',
+    port: 0,
+    exclusive: false
+  }, function() {
+    process.send({
+      msg: 'success',
+      port: this.address().port,
+    });
+  });
+}

test/parallel/test-net-listen-shared-ports.js

@@ -1,5 +1,5 @@
 'use strict';
-var common = require('../common');
+require('../common');
 var assert = require('assert');
 var cluster = require('cluster');
 var net = require('net');
@@ -9,19 +9,25 @@ function noop() {}
 if (cluster.isMaster) {
   var worker1 = cluster.fork();
 
-  worker1.on('message', function(msg) {
-    assert.equal(msg, 'success');
-    var worker2 = cluster.fork();
+  worker1.on('message', function(obj) {
+    assert.strictEqual(typeof obj, 'object');
+    assert.strictEqual(obj.msg, 'success');
+    var worker2 = cluster.fork({
+      NODE_TEST_PORT1: obj.port1,
+      NODE_TEST_PORT2: obj.port2
+    });
 
     worker2.on('message', function(msg) {
-      assert.equal(msg, 'server2:EADDRINUSE');
+      assert.strictEqual(msg, 'server2:EADDRINUSE');
       worker1.kill();
       worker2.kill();
     });
   });
 } else {
   var server1 = net.createServer(noop);
   var server2 = net.createServer(noop);
+  const port1 = (+process.env.NODE_TEST_PORT1) || 0;
+  const port2 = (+process.env.NODE_TEST_PORT2) || 0;
 
   server1.on('error', function(err) {
     // no errors expected
@@ -35,12 +41,16 @@ if (cluster.isMaster) {
 
   server1.listen({
     host: 'localhost',
-    port: common.PORT,
+    port: port1,
     exclusive: false
   }, function() {
-    server2.listen({port: common.PORT + 1, exclusive: true}, function() {
+    server2.listen({port: port2, exclusive: true}, function() {
       // the first worker should succeed
-      process.send('success');
+      process.send({
+        msg: 'success',
+        port1: server1.address().port,
+        port2: server2.address().port
+      });
     });
   });
 }

test/parallel/test-net-server-bind.js

@@ -1,9 +1,15 @@
 'use strict';
-var common = require('../common');
+require('../common');
 var assert = require('assert');
 var net = require('net');
 
 
+function assertValidPort(port) {
+  assert.strictEqual(typeof port, 'number');
+  assert.ok(isFinite(port));
+  assert.ok(port > 0);
+}
+
 // With only a callback, server should get a port assigned by the OS
 
 var address0;
@@ -22,7 +28,7 @@ var address1;
 var connectionKey1;
 var server1 = net.createServer(function(socket) { });
 
-server1.listen(common.PORT);
+server1.listen(0);
 
 setTimeout(function() {
   address1 = server1.address();
@@ -37,7 +43,7 @@ setTimeout(function() {
 var address2;
 var server2 = net.createServer(function(socket) { });
 
-server2.listen(common.PORT + 1, function() {
+server2.listen(0, function() {
   address2 = server2.address();
   console.log('address2 %j', address2);
   server2.close();
@@ -49,7 +55,7 @@ server2.listen(common.PORT + 1, function() {
 var address3;
 var server3 = net.createServer(function(socket) { });
 
-server3.listen(common.PORT + 2, '0.0.0.0', 127, function() {
+server3.listen(0, '0.0.0.0', 127, function() {
   address3 = server3.address();
   console.log('address3 %j', address3);
   server3.close();
@@ -61,7 +67,7 @@ server3.listen(common.PORT + 2, '0.0.0.0', 127, function() {
 var address4;
 var server4 = net.createServer(function(socket) { });
 
-server4.listen(common.PORT + 3, 127, function() {
+server4.listen(0, 127, function() {
   address4 = server4.address();
   console.log('address4 %j', address4);
   server4.close();
@@ -70,17 +76,16 @@ server4.listen(common.PORT + 3, 127, function() {
 
 process.on('exit', function() {
   assert.ok(address0.port > 100);
-  assert.equal(common.PORT, address1.port);
 
+  assertValidPort(address1.port);
   var expectedConnectionKey1;
-
   if (address1.family === 'IPv6')
     expectedConnectionKey1 = '6::::' + address1.port;
   else
     expectedConnectionKey1 = '4:0.0.0.0:' + address1.port;
-
   assert.equal(connectionKey1, expectedConnectionKey1);
-  assert.equal(common.PORT + 1, address2.port);
-  assert.equal(common.PORT + 2, address3.port);
-  assert.equal(common.PORT + 3, address4.port);
+
+  assertValidPort(address2.port);
+  assertValidPort(address3.port);
+  assertValidPort(address4.port);
 });

test/parallel/test-regress-GH-5727.js

@@ -6,9 +6,9 @@ const net = require('net');
 const invalidPort = -1 >>> 0;
 const errorMessage = /"port" argument must be \>= 0 and \< 65536/;
 
-net.Server().listen(common.PORT, function() {
+net.Server().listen(0, function() {
   const address = this.address();
-  const key = `${address.family.slice(-1)}:${address.address}:${common.PORT}`;
+  const key = `${address.family.slice(-1)}:${address.address}:${address.port}`;
 
   assert.equal(this._connectionKey, key);
   this.close();