From ba793bc3d9b888b4826fea4441a08e0f449f1148 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Sat, 4 Mar 2017 00:52:12 +0800 Subject: [PATCH 1/5] net: make normalizeArgs return tuples Make normalizeArgs return either [options, null] or [options, cb] (the second element won't be undefined anymore) and avoid OOB read --- lib/net.js | 43 +++++++++++++++++++++++++++---------------- 1 file changed, 27 insertions(+), 16 deletions(-) diff --git a/lib/net.js b/lib/net.js index d498efb184e713..109c677bff23a1 100644 --- a/lib/net.js +++ b/lib/net.js @@ -74,23 +74,33 @@ exports.connect = exports.createConnection = function() { return Socket.prototype.connect.apply(s, args); }; -// Returns an array [options, cb], where cb can be null. -// It is the same as the argument of Socket.prototype.connect(). -// This is used by Server.prototype.listen() and Socket.prototype.connect(). -function normalizeArgs(args) { - var options = {}; +// Returns an array [options, cb], where options is an object, +// cb is either a funciton or null. +// Used to normalize arguments of Socket.prototype.connect() and +// Server.prototype.listen(). Possible combinations of paramters: +// (options[...][, cb]) +// (path[...][, cb]) +// ([port][, host][...][, cb]) +// For Socket.prototype.connect(), the [...] part is ignored +// For Server.prototype.listen(), the [...] part is [, backlog] +// but will not be handled here (handled in listen()) +function normalizeArgs(args) { if (args.length === 0) { - return [options]; - } else if (args[0] !== null && typeof args[0] === 'object') { - // connect(options, [cb]) - options = args[0]; - } else if (isPipeName(args[0])) { - // connect(path, [cb]); - options.path = args[0]; + return [{}, null]; + } + + const arg0 = args[0]; + var options = {}; + if (typeof arg0 === 'object' && arg0 !== null) { + // (options[...][, cb]) + options = arg0; + } else if (isPipeName(arg0)) { + // (path[...][, cb]) + options.path = arg0; } else { - // connect(port, [host], [cb]) - options.port = args[0]; + // ([port][, host][...][, cb]) + options.port = arg0; if (args.length > 1 && typeof args[1] === 'string') { options.host = args[1]; } @@ -98,8 +108,9 @@ function normalizeArgs(args) { var cb = args[args.length - 1]; if (typeof cb !== 'function') - cb = null; - return [options, cb]; + return [options, null]; + else + return [options, cb]; } exports._normalizeArgs = normalizeArgs; From a8d0c2428abf8e529bdf4768bf579e8870032bc7 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Sat, 4 Mar 2017 01:24:08 +0800 Subject: [PATCH 2/5] net: replace apply with call Use Socket.prototype.connect.call instead of .apply when the number of arguments is certain(returned by normalizeArgs). --- lib/net.js | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/lib/net.js b/lib/net.js index 109c677bff23a1..dc6e5958f2589c 100644 --- a/lib/net.js +++ b/lib/net.js @@ -65,13 +65,13 @@ exports.connect = exports.createConnection = function() { args[i] = arguments[i]; args = normalizeArgs(args); debug('createConnection', args); - var s = new Socket(args[0]); + var socket = new Socket(args[0]); if (args[0].timeout) { - s.setTimeout(args[0].timeout); + socket.setTimeout(args[0].timeout); } - return Socket.prototype.connect.apply(s, args); + return Socket.prototype.connect.call(socket, args[0], args[1]); }; @@ -903,13 +903,13 @@ Socket.prototype.connect = function(options, cb) { if (options === null || typeof options !== 'object') { // Old API: - // connect(port, [host], [cb]) - // connect(path, [cb]); + // connect(port[, host][, cb]) + // connect(path[, cb]); var args = new Array(arguments.length); for (var i = 0; i < arguments.length; i++) args[i] = arguments[i]; args = normalizeArgs(args); - return Socket.prototype.connect.apply(this, args); + return Socket.prototype.connect.call(this, args[0], args[1]); } if (this.destroyed) { @@ -934,7 +934,7 @@ Socket.prototype.connect = function(options, cb) { initSocketHandle(this); } - if (typeof cb === 'function') { + if (cb !== null) { this.once('connect', cb); } From 1ea81d585179e2ac676220a20b2977868195f5bf Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Sat, 4 Mar 2017 01:32:43 +0800 Subject: [PATCH 3/5] net: rename some args[i] for readability --- lib/net.js | 26 ++++++++++++++++---------- 1 file changed, 16 insertions(+), 10 deletions(-) diff --git a/lib/net.js b/lib/net.js index dc6e5958f2589c..41d268b09b7711 100644 --- a/lib/net.js +++ b/lib/net.js @@ -60,18 +60,21 @@ exports.createServer = function(options, connectionListener) { // connect(path, [cb]); // exports.connect = exports.createConnection = function() { - var args = new Array(arguments.length); + const args = new Array(arguments.length); for (var i = 0; i < arguments.length; i++) args[i] = arguments[i]; - args = normalizeArgs(args); - debug('createConnection', args); - var socket = new Socket(args[0]); + // TODO(joyeecheung): use destructuring when V8 is fast enough + const normalized = normalizeArgs(args); + const options = normalized[0]; + const cb = normalized[1]; + debug('createConnection', normalized); + const socket = new Socket(options); - if (args[0].timeout) { - socket.setTimeout(args[0].timeout); + if (options.timeout) { + socket.setTimeout(options.timeout); } - return Socket.prototype.connect.call(socket, args[0], args[1]); + return Socket.prototype.connect.call(socket, options, cb); }; @@ -905,11 +908,14 @@ Socket.prototype.connect = function(options, cb) { // Old API: // connect(port[, host][, cb]) // connect(path[, cb]); - var args = new Array(arguments.length); + const args = new Array(arguments.length); for (var i = 0; i < arguments.length; i++) args[i] = arguments[i]; - args = normalizeArgs(args); - return Socket.prototype.connect.call(this, args[0], args[1]); + const normalized = normalizeArgs(args); + const normalizedOptions = normalized[0]; + const normalizedCb = normalized[1]; + return Socket.prototype.connect.call(this, + normalizedOptions, normalizedCb); } if (this.destroyed) { From ce3fc0583d2125d067dab13f0348391e82c3db2d Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Sat, 4 Mar 2017 02:09:57 +0800 Subject: [PATCH 4/5] net: refactor Server.prototype.listen * Separate backlogFromArgs and options.backlog * Comment the overloading process --- lib/net.js | 41 ++++++++++++++++++++++++++--------------- 1 file changed, 26 insertions(+), 15 deletions(-) diff --git a/lib/net.js b/lib/net.js index 41d268b09b7711..33fe2692fc6b91 100644 --- a/lib/net.js +++ b/lib/net.js @@ -1351,49 +1351,60 @@ function listen(self, address, port, addressType, backlog, fd, exclusive) { Server.prototype.listen = function() { - var args = new Array(arguments.length); + const args = new Array(arguments.length); for (var i = 0; i < arguments.length; i++) args[i] = arguments[i]; - var [options, cb] = normalizeArgs(args); + // TODO(joyeecheung): use destructuring when V8 is fast enough + const normalized = normalizeArgs(args); + var options = normalized[0]; + const cb = normalized[1]; - if (typeof cb === 'function') { + var hasCallback = (cb !== null); + if (hasCallback) { this.once('listening', cb); } + // ([port][, host][, backlog][, cb]) where port is omitted, + // that is, listen() or listen(cb), if (args.length === 0 || typeof args[0] === 'function') { // Bind to a random port. options.port = 0; } - // The third optional argument is the backlog size. - // When the ip is omitted it can be the second argument. - var backlog = toNumber(args.length > 1 && args[1]) || - toNumber(args.length > 2 && args[2]); + const backlogFromArgs = + // (handle, backlog) or (path, backlog) or (port, backlog) + toNumber(args.length > 1 && args[1]) || + toNumber(args.length > 2 && args[2]); // (port, host, backlog) options = options._handle || options.handle || options; - + // (handle[, backlog][, cb]) where handle is an object with a handle if (options instanceof TCP) { this._handle = options; - listen(this, null, -1, -1, backlog); + listen(this, null, -1, -1, backlogFromArgs); } else if (typeof options.fd === 'number' && options.fd >= 0) { - listen(this, null, null, null, backlog, options.fd); + // (handle[, backlog][, cb]) where handle is an object with a fd + listen(this, null, null, null, backlogFromArgs, options.fd); } else { - backlog = options.backlog || backlog; + const backlog = options.backlog || backlogFromArgs; + // ([port][, host][, backlog][, cb]) where port is specified + // or (options[, cb]) where options.port is specified if (typeof options.port === 'number' || typeof options.port === 'string' || (typeof options.port === 'undefined' && 'port' in options)) { - // Undefined is interpreted as zero (random port) for consistency - // with net.connect(). + // if (options[, cb]) where options.port is explicitly set as undefined, + // bind to an arbitrary unused port assertPort(options.port); + // start TCP server listening on host:port if (options.host) { lookupAndListen(this, options.port | 0, options.host, backlog, options.exclusive); - } else { + } else { // Undefined host, listens on unspecified IPv4 address listen(this, null, options.port | 0, 4, backlog, undefined, options.exclusive); } } else if (options.path && isPipeName(options.path)) { - // UNIX socket or Windows pipe. + // (path[, backlog][, cb]) or (options[, cb]) + // where path or options.path is a UNIX domain socket or Windows pipe const pipeName = this._pipeName = options.path; listen(this, pipeName, -1, -1, backlog, undefined, options.exclusive); } else { From bcd4ff7205064fe5af355643a112b9cca38214df Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Sat, 4 Mar 2017 02:37:53 +0800 Subject: [PATCH 5/5] net: refactor server.listen flow control --- lib/net.js | 76 ++++++++++++++++++++++++++++-------------------------- 1 file changed, 40 insertions(+), 36 deletions(-) diff --git a/lib/net.js b/lib/net.js index 33fe2692fc6b91..64e50e907719d5 100644 --- a/lib/net.js +++ b/lib/net.js @@ -24,7 +24,6 @@ var cluster; const errnoException = util._errnoException; const exceptionWithHostPort = util._exceptionWithHostPort; const isLegalPort = internalNet.isLegalPort; -const assertPort = internalNet.assertPort; function noop() {} @@ -1363,14 +1362,6 @@ Server.prototype.listen = function() { if (hasCallback) { this.once('listening', cb); } - - // ([port][, host][, backlog][, cb]) where port is omitted, - // that is, listen() or listen(cb), - if (args.length === 0 || typeof args[0] === 'function') { - // Bind to a random port. - options.port = 0; - } - const backlogFromArgs = // (handle, backlog) or (path, backlog) or (port, backlog) toNumber(args.length > 1 && args[1]) || @@ -1381,38 +1372,51 @@ Server.prototype.listen = function() { if (options instanceof TCP) { this._handle = options; listen(this, null, -1, -1, backlogFromArgs); - } else if (typeof options.fd === 'number' && options.fd >= 0) { - // (handle[, backlog][, cb]) where handle is an object with a fd + return this; + } + // (handle[, backlog][, cb]) where handle is an object with a fd + if (typeof options.fd === 'number' && options.fd >= 0) { listen(this, null, null, null, backlogFromArgs, options.fd); - } else { - const backlog = options.backlog || backlogFromArgs; + return this; + } - // ([port][, host][, backlog][, cb]) where port is specified - // or (options[, cb]) where options.port is specified - if (typeof options.port === 'number' || typeof options.port === 'string' || - (typeof options.port === 'undefined' && 'port' in options)) { - // if (options[, cb]) where options.port is explicitly set as undefined, - // bind to an arbitrary unused port - assertPort(options.port); - // start TCP server listening on host:port - if (options.host) { - lookupAndListen(this, options.port | 0, options.host, backlog, - options.exclusive); - } else { // Undefined host, listens on unspecified IPv4 address - listen(this, null, options.port | 0, 4, backlog, undefined, - options.exclusive); - } - } else if (options.path && isPipeName(options.path)) { - // (path[, backlog][, cb]) or (options[, cb]) - // where path or options.path is a UNIX domain socket or Windows pipe - const pipeName = this._pipeName = options.path; - listen(this, pipeName, -1, -1, backlog, undefined, options.exclusive); - } else { - throw new Error('Invalid listen argument: ' + options); + // ([port][, host][, backlog][, cb]) where port is omitted, + // that is, listen() or listen(cb), + // or (options[, cb]) where options.port is explicitly set as undefined, + // bind to an arbitrary unused port + if (args.length === 0 || typeof args[0] === 'function' || + (typeof options.port === 'undefined' && 'port' in options)) { + options.port = 0; + } + // ([port][, host][, backlog][, cb]) where port is specified + // or (options[, cb]) where options.port is specified + // or if options.port is normalized as 0 before + if (typeof options.port === 'number' || typeof options.port === 'string') { + if (!isLegalPort(options.port)) { + throw new RangeError('"port" argument must be >= 0 and < 65536'); + } + const backlog = options.backlog || backlogFromArgs; + // start TCP server listening on host:port + if (options.host) { + lookupAndListen(this, options.port | 0, options.host, backlog, + options.exclusive); + } else { // Undefined host, listens on unspecified address + listen(this, null, options.port | 0, 4, // addressType will be ignored + backlog, undefined, options.exclusive); } + return this; } - return this; + // (path[, backlog][, cb]) or (options[, cb]) + // where path or options.path is a UNIX domain socket or Windows pipe + if (options.path && isPipeName(options.path)) { + const pipeName = this._pipeName = options.path; + const backlog = options.backlog || backlogFromArgs; + listen(this, pipeName, -1, -1, backlog, undefined, options.exclusive); + return this; + } + + throw new Error('Invalid listen argument: ' + options); }; function lookupAndListen(self, port, address, backlog, exclusive) {