From b636ba8186d191c52ee36f2f2b1aebbbb4d95575 Mon Sep 17 00:00:00 2001 From: cjihrig Date: Sat, 3 Jan 2015 18:26:26 -0500 Subject: [PATCH] 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 Reviewed-By: Fedor Indutny Reviewed-By: Timothy J Fontaine --- lib/net.js | 77 +++++++++--------------- test/simple/test-net-localerror.js | 53 +++++++--------- test/simple/test-process-active-wraps.js | 16 ++++- 3 files changed, 65 insertions(+), 81 deletions(-) diff --git a/lib/net.js b/lib/net.js index f0075b5a33e..0ece1b0f97c 100644 --- 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 index c4d04aa921a..d04d9c70720 100644 --- 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 index 63fc218debc..bd4941b786e 100644 --- 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); + }); + } } })();