net: allow port 0 in connect()

The added validation allows non-negative numbers and numeric
strings. All other values result in a thrown exception.

Fixes: https://github.com/joyent/node/issues/9194
PR-URL: https://github.com/joyent/node/pull/9268
Reviewed-By: Julien Gilli <julien.gilli@joyent.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: James M Snell <jasnell@users.noreply.github.com>

4 files changed, 97 insertions, 33 deletions

diff --git a/lib/net.js b/lib/net.js
index 61cb42095..6d12a70ae 100644
--- a/lib/net.js
+++ b/lib/net.js
@@ -892,7 +892,7 @@ Socket.prototype.connect = function(options, cb) {
   } else {
     var dns = require('dns');
     var host = options.host || 'localhost';
-    var port = options.port | 0;
+    var port = 0;
     var localAddress = options.localAddress;
     var localPort = options.localPort;
     var dnsopts = {
@@ -906,8 +906,16 @@ Socket.prototype.connect = function(options, cb) {
     if (localPort && !util.isNumber(localPort))
       throw new TypeError('localPort should be a number: ' + localPort);
 
-    if (port <= 0 || port > 65535)
-      throw new RangeError('port should be > 0 and < 65536: ' + port);
+    if (typeof options.port === 'number')
+      port = options.port;
+    else if (typeof options.port === 'string')
+      port = options.port.trim() === '' ? -1 : +options.port;
+    else if (options.port !== undefined)
+      throw new TypeError('port should be a number or string: ' + options.port);
+
+    if (port < 0 || port > 65535 || isNaN(port))
+      throw new RangeError('port should be >= 0 and < 65536: ' +
+                           options.port);
 
     if (dnsopts.family !== 4 && dnsopts.family !== 6)
       dnsopts.hints = dns.ADDRCONFIG | dns.V4MAPPED;
diff --git a/src/tcp_wrap.cc b/src/tcp_wrap.cc
index 6b2408c98..4d66c2912 100644
--- a/src/tcp_wrap.cc
+++ b/src/tcp_wrap.cc
@@ -428,7 +428,7 @@ void TCPWrap::Connect(const FunctionCallbackInfo<Value>& args) {
 
   assert(args[0]->IsObject());
   assert(args[1]->IsString());
-  assert(args[2]->Uint32Value());
+  assert(args[2]->IsUint32());
 
   Local<Object> req_wrap_obj = args[0].As<Object>();
   node::Utf8Value ip_address(args[1]);
@@ -460,7 +460,7 @@ void TCPWrap::Connect6(const FunctionCallbackInfo<Value>& args) {
 
   assert(args[0]->IsObject());
   assert(args[1]->IsString());
-  assert(args[2]->Uint32Value());
+  assert(args[2]->IsUint32());
 
   Local<Object> req_wrap_obj = args[0].As<Object>();
   node::Utf8Value ip_address(args[1]);
diff --git a/test/simple/test-net-create-connection.js b/test/simple/test-net-create-connection.js
index 12f7f0be6..2dee02966 100644
--- a/test/simple/test-net-create-connection.js
+++ b/test/simple/test-net-create-connection.js
@@ -24,33 +24,99 @@ var assert = require('assert');
 var net = require('net');
 
 var tcpPort = common.PORT;
+var expectedConnections = 7;
 var clientConnected = 0;
 var serverConnected = 0;
 
 var server = net.createServer(function(socket) {
   socket.end();
-  if (++serverConnected === 4) {
+  if (++serverConnected === expectedConnections) {
     server.close();
   }
 });
+
 server.listen(tcpPort, 'localhost', function() {
   function cb() {
     ++clientConnected;
   }
 
+  function fail(opts, errtype, msg) {
+    assert.throws(function() {
+      var client = net.createConnection(opts, cb);
+    }, function (err) {
+      return err instanceof errtype && msg === err.message;
+    });
+  }
+
   net.createConnection(tcpPort).on('connect', cb);
   net.createConnection(tcpPort, 'localhost').on('connect', cb);
   net.createConnection(tcpPort, cb);
   net.createConnection(tcpPort, 'localhost', cb);
+  net.createConnection(tcpPort + '', 'localhost', cb);
+  net.createConnection({port: tcpPort + ''}).on('connect', cb);
+  net.createConnection({port: '0x' + tcpPort.toString(16)}, cb);
+
+  fail({
+    port: true
+  }, TypeError, 'port should be a number or string: true');
+
+  fail({
+    port: false
+  }, TypeError, 'port should be a number or string: false');
+
+  fail({
+    port: []
+  }, TypeError, 'port should be a number or string: ');
+
+  fail({
+    port: {}
+  }, TypeError, 'port should be a number or string: [object Object]');
+
+  fail({
+    port: null
+  }, TypeError, 'port should be a number or string: null');
+
+  fail({
+    port: ''
+  }, RangeError, 'port should be >= 0 and < 65536: ');
+
+  fail({
+    port: ' '
+  }, RangeError, 'port should be >= 0 and < 65536:  ');
 
-  assert.throws(function () {
-    net.createConnection({
-      port: 'invalid!'
-    }, cb);
-  });
+  fail({
+    port: '0x'
+  }, RangeError, 'port should be >= 0 and < 65536: 0x');
+
+  fail({
+    port: '-0x1'
+  }, RangeError, 'port should be >= 0 and < 65536: -0x1');
+
+  fail({
+    port: NaN
+  }, RangeError, 'port should be >= 0 and < 65536: NaN');
+
+  fail({
+    port: Infinity
+  }, RangeError, 'port should be >= 0 and < 65536: Infinity');
+
+  fail({
+    port: -1
+  }, RangeError, 'port should be >= 0 and < 65536: -1');
+
+  fail({
+    port: 65536
+  }, RangeError, 'port should be >= 0 and < 65536: 65536');
 });
 
-process.on('exit', function() {
-  assert.equal(clientConnected, 4);
+// Try connecting to random ports, but do so once the server is closed
+server.on('close', function() {
+  function nop() {}
+
+  net.createConnection({port: 0}).on('error', nop);
+  net.createConnection({port: undefined}).on('error', nop);
 });
 
+process.on('exit', function() {
+  assert.equal(clientConnected, expectedConnections);
+});
diff --git a/test/simple/test-net-localerror.js b/test/simple/test-net-localerror.js
index d04d9c707..cf113d499 100644
--- a/test/simple/test-net-localerror.js
+++ b/test/simple/test-net-localerror.js
@@ -23,27 +23,17 @@ var common = require('../common');
 var assert = require('assert');
 var net = require('net');
 
-  connect({
-    host: 'localhost',
-    port: common.PORT,
-    localPort: 'foobar',
-  }, 'localPort should be a number: foobar');
+connect({
+  host: 'localhost',
+  port: common.PORT,
+  localPort: 'foobar',
+}, 'localPort should be a number: foobar');
 
-  connect({
-    host: 'localhost',
-    port: common.PORT,
-    localAddress: 'foobar',
-  }, 'localAddress should be a valid IP: foobar');
-
-  connect({
-    host: 'localhost',
-    port: 65536
-  }, 'port should be > 0 and < 65536: 65536');
-
-  connect({
-    host: 'localhost',
-    port: 0
-  }, 'port should be > 0 and < 65536: 0');
+connect({
+  host: 'localhost',
+  port: common.PORT,
+  localAddress: 'foobar',
+}, 'localAddress should be a valid IP: foobar');
 
 function connect(opts, msg) {
   assert.throws(function() {