lib: move isLegalPort to validators, refactor

isLegalPort was used multiple places in the same way -- to validate the port and throw if necessary. Moved into internal/validators. PR-URL: #31851 Reviewed-By: Richard Lau <riclau@uk.ibm.com>
nodejs · Apr 28, 2020 · 434ca87 · 434ca87
1 parent 322a998
commit 434ca87
Show file tree

Hide file tree

Showing 10 changed files with 68 additions and 80 deletions.
diff --git a/lib/dgram.js b/lib/dgram.js
@@ -35,15 +35,11 @@ const {
   newHandle,
 } = require('internal/dgram');
 const { guessHandleType } = internalBinding('util');
-const {
-  isLegalPort,
-} = require('internal/net');
 const {
   ERR_INVALID_ARG_TYPE,
   ERR_MISSING_ARGS,
   ERR_SOCKET_ALREADY_BOUND,
   ERR_SOCKET_BAD_BUFFER_SIZE,
-  ERR_SOCKET_BAD_PORT,
   ERR_SOCKET_BUFFER_SIZE,
   ERR_SOCKET_CANNOT_SEND,
   ERR_SOCKET_DGRAM_IS_CONNECTED,
@@ -54,7 +50,8 @@ const {
 const {
   isInt32,
   validateString,
-  validateNumber
+  validateNumber,
+  validatePort,
 } = require('internal/validators');
 const { Buffer } = require('buffer');
 const { deprecate } = require('internal/util');
@@ -352,21 +349,8 @@ Socket.prototype.bind = function(port_, address_ /* , callback */) {
   return this;
 };
 
-
-function validatePort(port) {
-  const legal = isLegalPort(port);
-  if (legal)
-    port = port | 0;
-
-  if (!legal || port === 0)
-    throw new ERR_SOCKET_BAD_PORT(port);
-
-  return port;
-}
-
-
 Socket.prototype.connect = function(port, address, callback) {
-  port = validatePort(port);
+  port = validatePort(port, 'Port', { allowZero: false });
   if (typeof address === 'function') {
     callback = address;
     address = '';
@@ -612,7 +596,7 @@ Socket.prototype.send = function(buffer,
   }
 
   if (!connected)
-    port = validatePort(port);
+    port = validatePort(port, 'Port', { allowZero: false });
 
   // Normalize callback so it's either a function or undefined but not anything
   // else.

diff --git a/lib/dns.js b/lib/dns.js
@@ -29,7 +29,7 @@ const {
 
 const cares = internalBinding('cares_wrap');
 const { toASCII } = require('internal/idna');
-const { isIP, isLegalPort } = require('internal/net');
+const { isIP } = require('internal/net');
 const { customPromisifyArgs } = require('internal/util');
 const errors = require('internal/errors');
 const {
@@ -45,9 +45,11 @@ const {
   ERR_INVALID_CALLBACK,
   ERR_INVALID_OPT_VALUE,
   ERR_MISSING_ARGS,
-  ERR_SOCKET_BAD_PORT
 } = errors.codes;
-const { validateString } = require('internal/validators');
+const {
+  validatePort,
+  validateString,
+} = require('internal/validators');
 
 const {
   GetAddrInfoReqWrap,
@@ -171,8 +173,7 @@ function lookupService(address, port, callback) {
   if (isIP(address) === 0)
     throw new ERR_INVALID_OPT_VALUE('address', address);
 
-  if (!isLegalPort(port))
-    throw new ERR_SOCKET_BAD_PORT(port);
+  validatePort(port);
 
   if (typeof callback !== 'function')
     throw new ERR_INVALID_CALLBACK(callback);

diff --git a/lib/internal/cluster/master.js b/lib/internal/cluster/master.js
@@ -14,13 +14,12 @@ const RoundRobinHandle = require('internal/cluster/round_robin_handle');
 const SharedHandle = require('internal/cluster/shared_handle');
 const Worker = require('internal/cluster/worker');
 const { internal, sendHelper } = require('internal/cluster/utils');
-const { ERR_SOCKET_BAD_PORT } = require('internal/errors').codes;
 const cluster = new EventEmitter();
 const intercom = new EventEmitter();
 const SCHED_NONE = 1;
 const SCHED_RR = 2;
-const { isLegalPort } = require('internal/net');
 const [ minPort, maxPort ] = [ 1024, 65535 ];
+const { validatePort } = require('internal/validators');
 
 module.exports = cluster;
 
@@ -118,9 +117,7 @@ function createWorkerProcess(id, env) {
       else
         inspectPort = cluster.settings.inspectPort;
 
-      if (!isLegalPort(inspectPort)) {
-        throw new ERR_SOCKET_BAD_PORT(inspectPort);
-      }
+      validatePort(inspectPort);
     } else {
       inspectPort = process.debugPort + debugPortOffset;
       if (inspectPort > maxPort)

diff --git a/lib/internal/dns/promises.js b/lib/internal/dns/promises.js
@@ -14,7 +14,7 @@ const {
 } = require('internal/dns/utils');
 const { codes, dnsException } = require('internal/errors');
 const { toASCII } = require('internal/idna');
-const { isIP, isLegalPort } = require('internal/net');
+const { isIP } = require('internal/net');
 const {
   getaddrinfo,
   getnameinfo,
@@ -27,10 +27,11 @@ const {
   ERR_INVALID_ARG_TYPE,
   ERR_INVALID_OPT_VALUE,
   ERR_MISSING_ARGS,
-  ERR_SOCKET_BAD_PORT
 } = codes;
-const { validateString } = require('internal/validators');
-
+const {
+  validatePort,
+  validateString
+} = require('internal/validators');
 
 function onlookup(err, addresses) {
   if (err) {
@@ -162,8 +163,7 @@ function lookupService(address, port) {
   if (isIP(address) === 0)
     throw new ERR_INVALID_OPT_VALUE('address', address);
 
-  if (!isLegalPort(port))
-    throw new ERR_SOCKET_BAD_PORT(port);
+  validatePort(port);
 
   return createLookupServicePromise(address, +port);
 }

diff --git a/lib/internal/errors.js b/lib/internal/errors.js
@@ -1274,7 +1274,7 @@ E('ERR_SOCKET_ALREADY_BOUND', 'Socket is already bound', Error);
 E('ERR_SOCKET_BAD_BUFFER_SIZE',
   'Buffer size must be a positive integer', TypeError);
 E('ERR_SOCKET_BAD_PORT',
-  'Port should be >= 0 and < 65536. Received %s.', RangeError);
+  '%s should be >= 0 and < 65536. Received %s.', RangeError);
 E('ERR_SOCKET_BAD_TYPE',
   'Bad socket type specified. Valid types are: udp4, udp6', TypeError);
 E('ERR_SOCKET_BUFFER_SIZE',

diff --git a/lib/internal/net.js b/lib/internal/net.js
@@ -41,15 +41,6 @@ function isIP(s) {
   return 0;
 }
 
-// Check that the port number is not NaN when coerced to a number,
-// is an integer and that it falls within the legal range of port numbers.
-function isLegalPort(port) {
-  if ((typeof port !== 'number' && typeof port !== 'string') ||
-      (typeof port === 'string' && port.trim().length === 0))
-    return false;
-  return +port === (+port >>> 0) && port <= 0xFFFF;
-}
-
 function makeSyncWrite(fd) {
   return function(chunk, enc, cb) {
     if (enc !== 'buffer')
@@ -74,7 +65,6 @@ module.exports = {
   isIP,
   isIPv4,
   isIPv6,
-  isLegalPort,
   makeSyncWrite,
   normalizedArgsSymbol: Symbol('normalizedArgs')
 };
diff --git a/lib/internal/validators.js b/lib/internal/validators.js
@@ -9,6 +9,7 @@ const {
 const {
   hideStackFrames,
   codes: {
+    ERR_SOCKET_BAD_PORT,
     ERR_INVALID_ARG_TYPE,
     ERR_INVALID_ARG_VALUE,
     ERR_OUT_OF_RANGE,
@@ -144,15 +145,29 @@ const validateBuffer = hideStackFrames((buffer, name = 'buffer') => {
   }
 });
 
+// Check that the port number is not NaN when coerced to a number,
+// is an integer and that it falls within the legal range of port numbers.
+function validatePort(port, name = 'Port', { allowZero = true } = {}) {
+  if ((typeof port !== 'number' && typeof port !== 'string') ||
+      (typeof port === 'string' && port.trim().length === 0) ||
+      +port !== (+port >>> 0) ||
+      port > 0xFFFF ||
+      (port === 0 && !allowZero)) {
+    throw new ERR_SOCKET_BAD_PORT(name, port);
+  }
+  return port | 0;
+}
+
 module.exports = {
   isInt32,
   isUint32,
   parseMode,
   validateBuffer,
-  validateInteger,
   validateInt32,
-  validateUint32,
-  validateString,
+  validateInteger,
   validateNumber,
-  validateSignalName
+  validatePort,
+  validateSignalName,
+  validateString,
+  validateUint32,
 };
diff --git a/lib/net.js b/lib/net.js
@@ -41,7 +41,6 @@ const {
   isIP,
   isIPv4,
   isIPv6,
-  isLegalPort,
   normalizedArgsSymbol,
   makeSyncWrite
 } = require('internal/net');
@@ -92,15 +91,18 @@ const {
     ERR_INVALID_OPT_VALUE,
     ERR_SERVER_ALREADY_LISTEN,
     ERR_SERVER_NOT_RUNNING,
-    ERR_SOCKET_BAD_PORT,
     ERR_SOCKET_CLOSED
   },
   errnoException,
   exceptionWithHostPort,
   uvExceptionWithHostPort
 } = require('internal/errors');
 const { isUint8Array } = require('internal/util/types');
-const { validateInt32, validateString } = require('internal/validators');
+const {
+  validateInt32,
+  validatePort,
+  validateString
+} = require('internal/validators');
 const kLastWriteQueueSize = Symbol('lastWriteQueueSize');
 const {
   DTRACE_NET_SERVER_CONNECTION,
@@ -997,9 +999,7 @@ function lookupAndConnect(self, options) {
       throw new ERR_INVALID_ARG_TYPE('options.port',
                                      ['number', 'string'], port);
     }
-    if (!isLegalPort(port)) {
-      throw new ERR_SOCKET_BAD_PORT(port);
-    }
+    validatePort(port);
   }
   port |= 0;
 
@@ -1436,9 +1436,7 @@ Server.prototype.listen = function(...args) {
   // or if options.port is normalized as 0 before
   let backlog;
   if (typeof options.port === 'number' || typeof options.port === 'string') {
-    if (!isLegalPort(options.port)) {
-      throw new ERR_SOCKET_BAD_PORT(options.port);
-    }
+    validatePort(options.port, 'options.port');
     backlog = options.backlog || backlogFromArgs;
     // start TCP server listening on host:port
     if (options.host) {

diff --git a/test/parallel/test-internal-validators-validateport.js b/test/parallel/test-internal-validators-validateport.js
@@ -0,0 +1,23 @@
+// Flags: --expose-internals
+'use strict';
+
+require('../common');
+const assert = require('assert');
+const { validatePort } = require('internal/validators');
+
+for (let n = 0; n <= 0xFFFF; n++) {
+  validatePort(n);
+  validatePort(`${n}`);
+  validatePort(`0x${n.toString(16)}`);
+  validatePort(`0o${n.toString(8)}`);
+  validatePort(`0b${n.toString(2)}`);
+}
+
+[
+  -1, 'a', {}, [], false, true,
+  0xFFFF + 1, Infinity, -Infinity, NaN,
+  undefined, null, '', ' ', 1.1, '0x',
+  '-0x1', '-0o1', '-0b1', '0o', '0b'
+].forEach((i) => assert.throws(() => validatePort(i), {
+  code: 'ERR_SOCKET_BAD_PORT'
+}));
diff --git a/test/parallel/test-net-internal.js b/test/parallel/test-net-internal.js