net: refactor overloaded argument handling

* Make normalizeArgs return either [options, null] or [options, cb]
  (the second element won't be undefined anymore) and avoid OOB read
* Use Socket.prototype.connect.call instead of .apply when the number
  of arguments is certain(returned by normalizeArgs).
* Rename some args[i] for readability
* Refactor Server.prototype.listen, separate backlogFromArgs and
  options.backlog, comment the overloading process, refactor control
  flow

PR-URL: https://github.com/nodejs/node/pull/11667
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

1 files changed, 101 insertions, 69 deletions

diff --git a/lib/net.js b/lib/net.js
index f3f8bb1a984..018b58b941f 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() {}
 
@@ -60,37 +59,50 @@ 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 s = 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) {
-    s.setTimeout(args[0].timeout);
+  if (options.timeout) {
+    socket.setTimeout(options.timeout);
   }
 
-  return Socket.prototype.connect.apply(s, args);
+  return Socket.prototype.connect.call(socket, options, cb);
 };
 
-// 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 +110,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;
 
@@ -892,13 +905,16 @@ Socket.prototype.connect = function(options, cb) {
 
   if (options === null || typeof options !== 'object') {
     // Old API:
-    // connect(port, [host], [cb])
-    // connect(path, [cb]);
-    var args = new Array(arguments.length);
+    // connect(port[, host][, cb])
+    // connect(path[, cb]);
+    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.apply(this, args);
+    const normalized = normalizeArgs(args);
+    const normalizedOptions = normalized[0];
+    const normalizedCb = normalized[1];
+    return Socket.prototype.connect.call(this,
+                                         normalizedOptions, normalizedCb);
   }
 
   if (this.destroyed) {
@@ -923,7 +939,7 @@ Socket.prototype.connect = function(options, cb) {
     initSocketHandle(this);
   }
 
-  if (typeof cb === 'function') {
+  if (cb !== null) {
     this.once('connect', cb);
   }
 
@@ -1333,57 +1349,73 @@ 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);
   }
-
-  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);
-  } else if (typeof options.fd === 'number' && options.fd >= 0) {
-    listen(this, null, null, null, backlog, options.fd);
-  } else {
-    backlog = options.backlog || backlog;
-
-    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().
-      assertPort(options.port);
-      if (options.host) {
-        lookupAndListen(this, options.port | 0, options.host, backlog,
-                        options.exclusive);
-      } else {
-        listen(this, null, options.port | 0, 4, backlog, undefined,
-               options.exclusive);
-      }
-    } else if (options.path && isPipeName(options.path)) {
-      // UNIX 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);
+    listen(this, null, -1, -1, backlogFromArgs);
+    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);
+    return this;
+  }
+
+  // ([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) {