Skip to content

Commit

Permalink
net: make connect() input validation synchronous
Browse files Browse the repository at this point in the history
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 <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Timothy J Fontaine <[email protected]>
  • Loading branch information
cjihrig committed Jan 4, 2015
1 parent 8cfbeed commit b636ba8
Show file tree
Hide file tree
Showing 3 changed files with 65 additions and 81 deletions.
77 changes: 29 additions & 48 deletions lib/net.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand All @@ -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;
Expand Down Expand Up @@ -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;

Expand All @@ -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);
}
});
}
Expand Down
53 changes: 22 additions & 31 deletions test/simple/test-net-localerror.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
16 changes: 14 additions & 2 deletions test/simple/test-process-active-wraps.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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);
});
}
}
})();

0 comments on commit b636ba8

Please sign in to comment.