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: nodejs/node-v0.x-archive#8180 Fixes: nodejs/node-v0.x-archive#8140 Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: Fedor Indutny <fedor@indutny.com> Reviewed-By: Timothy J Fontaine <tjfontaine@gmail.com>
nodejs · Jan 4, 2015 · b636ba8 · b636ba8
1 parent 8cfbeed
commit b636ba8
Show file tree

Hide file tree

Showing 3 changed files with 65 additions and 81 deletions.
diff --git a/lib/net.js b/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);
 
+  if (localAddress || 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);
       }
     });
   }

diff --git a/test/simple/test-net-localerror.js b/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);
 }
diff --git a/test/simple/test-process-active-wraps.js b/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);
+      });
+    }
   }
 })();