net: make connect() input validation synchronous

Socket.prototype.connect() sometimes throws on bad inputs
after an asynchronous operation. This commit makes the input
validation synchronous. This commit also removes some hard
coded IP addresses.

PR-URL: https://github.com/joyent/node/pull/8180
Fixes: https://github.com/joyent/node/issues/8140
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: Timothy J Fontaine <tjfontaine@gmail.com>

lib/net.js

@@ -785,34 +785,18 @@ function connect(self, address, port, addressType, localAddress, localPort) {
   assert.ok(self._connecting);
 
   var err;
+
   if (localAddress || localPort) {
-    if (localAddress && !exports.isIP(localAddress))
-      err = new TypeError(
-          'localAddress should be a valid IP: ' + localAddress);
-
-    if (localPort && !util.isNumber(localPort))
-      err = new TypeError('localPort should be a number: ' + localPort);
-
     var bind;
 
-    switch (addressType) {
-      case 4:
-        if (!localAddress)
-          localAddress = '0.0.0.0';
-        bind = self._handle.bind;
-        break;
-      case 6:
-        if (!localAddress)
-          localAddress = '::';
-        bind = self._handle.bind6;
-        break;
-      default:
-        err = new TypeError('Invalid addressType: ' + addressType);
-        break;
-    }
-
-    if (err) {
-      self._destroy(err);
+    if (addressType === 4) {
+      localAddress = localAddress || '0.0.0.0';
+      bind = self._handle.bind;
+    } else if (addressType === 6) {
+      localAddress = localAddress || '::';
+      bind = self._handle.bind6;
+    } else {
+      self._destroy(new TypeError('Invalid addressType: ' + addressType));
       return;
     }
 
@@ -832,15 +816,12 @@ function connect(self, address, port, addressType, localAddress, localPort) {
   if (addressType === 6 || addressType === 4) {
     var req = new TCPConnectWrap();
     req.oncomplete = afterConnect;
-    port = port | 0;
-    if (port <= 0 || port > 65535)
-      throw new RangeError('Port should be > 0 and < 65536');
 
-    if (addressType === 6) {
-      err = self._handle.connect6(req, address, port);
-    } else if (addressType === 4) {
+    if (addressType === 4)
       err = self._handle.connect(req, address, port);
-    }
+    else
+      err = self._handle.connect6(req, address, port);
+
   } else {
     var req = new PipeConnectWrap();
     req.oncomplete = afterConnect;
@@ -898,19 +879,26 @@ Socket.prototype.connect = function(options, cb) {
   if (pipe) {
     connect(self, options.path);
 
-  } else if (!options.host) {
-    debug('connect: missing host');
-    self._host = '127.0.0.1';
-    connect(self, self._host, options.port, 4);
-
   } else {
     var dns = require('dns');
-    var host = options.host;
+    var host = options.host || 'localhost';
+    var port = options.port | 0;
+    var localAddress = options.localAddress;
+    var localPort = options.localPort;
     var dnsopts = {
       family: options.family,
       hints: 0
     };
 
+    if (localAddress && !exports.isIP(localAddress))
+      throw new TypeError('localAddress must be a valid IP: ' + localAddress);
+
+    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 (dnsopts.family !== 4 && dnsopts.family !== 6)
       dnsopts.hints = dns.ADDRCONFIG | dns.V4MAPPED;
 
@@ -936,19 +924,12 @@ Socket.prototype.connect = function(options, cb) {
         });
       } else {
         timers._unrefActive(self);
-
-        addressType = addressType || 4;
-
-        // node_net.cc handles null host names graciously but user land
-        // expects remoteAddress to have a meaningful value
-        ip = ip || (addressType === 4 ? '127.0.0.1' : '0:0:0:0:0:0:0:1');
-
         connect(self,
                 ip,
-                options.port,
+                port,
                 addressType,
-                options.localAddress,
-                options.localPort);
+                localAddress,
+                localPort);
       }
     });
   }

test/simple/test-net-localerror.js

@@ -23,39 +23,30 @@ var common = require('../common');
 var assert = require('assert');
 var net = require('net');
 
-var server = net.createServer(function(socket) {
-  assert.ok(false, 'no clients should connect');
-}).listen(common.PORT).on('listening', function() {
-  server.unref();
+  connect({
+    host: 'localhost',
+    port: common.PORT,
+    localPort: 'foobar',
+  }, 'localPort should be a number: foobar');
 
-  function test1(next) {
-    connect({
-      host: '127.0.0.1',
-      port: common.PORT,
-      localPort: 'foobar',
-    },
-    'localPort should be a number: foobar',
-    next);
-  }
+  connect({
+    host: 'localhost',
+    port: common.PORT,
+    localAddress: 'foobar',
+  }, 'localAddress should be a valid IP: foobar');
 
-  function test2(next) {
-    connect({
-      host: '127.0.0.1',
-      port: common.PORT,
-      localAddress: 'foobar',
-    },
-    'localAddress should be a valid IP: foobar',
-    next)
-  }
+  connect({
+    host: 'localhost',
+    port: 65536
+  }, 'port should be > 0 and < 65536: 65536');
 
-  test1(test2);
-})
+  connect({
+    host: 'localhost',
+    port: 0
+  }, 'port should be > 0 and < 65536: 0');
 
-function connect(opts, msg, cb) {
-  var client = net.connect(opts).on('connect', function() {
-    assert.ok(false, 'we should never connect');
-  }).on('error', function(err) {
-    assert.strictEqual(err.message, msg);
-    if (cb) cb();
-  });
+function connect(opts, msg) {
+  assert.throws(function() {
+    var client = net.connect(opts);
+  }, msg);
 }

test/simple/test-process-active-wraps.js

@@ -41,9 +41,16 @@ var handles = [];
 })();
 
 (function() {
+  function onlookup() {
+    setImmediate(function() {
+      assert.equal(process._getActiveRequests().length, 0);
+    });
+  };
+
   expect(1, 0);
   var conn = net.createConnection(common.PORT);
-  conn.on('error', function() { /* ignore */ });
+  conn.on('lookup', onlookup); 
+  conn.on('error', function() { assert(false); });
   expect(2, 1);
   conn.destroy();
   expect(2, 1); // client handle doesn't shut down until next tick
@@ -52,10 +59,15 @@ var handles = [];
 
 (function() {
   var n = 0;
+
   handles.forEach(function(handle) {
     handle.once('close', onclose);
   });
   function onclose() {
-    if (++n === handles.length) setImmediate(expect, 0, 0);
+    if (++n === handles.length) {
+      setImmediate(function() {
+        assert.equal(process._getActiveHandles().length, 0);
+      });
+    }
   }
 })();