Skip to content

Commit

Permalink
lib: move isLegalPort to validators, refactor
Browse files Browse the repository at this point in the history
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>
  • Loading branch information
jasnell authored and MylesBorins committed Mar 10, 2020
1 parent ff58854 commit 897b1d2
Show file tree
Hide file tree
Showing 10 changed files with 69 additions and 81 deletions.
24 changes: 4 additions & 20 deletions lib/dgram.js
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -54,7 +50,8 @@ const {
const {
isInt32,
validateString,
validateNumber
validateNumber,
validatePort,
} = require('internal/validators');
const { Buffer } = require('buffer');
const { deprecate } = require('internal/util');
Expand Down Expand Up @@ -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 = '';
Expand Down Expand Up @@ -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.
Expand Down
11 changes: 6 additions & 5 deletions lib/dns.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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,
Expand Down Expand Up @@ -175,8 +177,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);
Expand Down
7 changes: 2 additions & 5 deletions lib/internal/cluster/master.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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)
Expand Down
12 changes: 6 additions & 6 deletions lib/internal/dns/promises.js
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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) {
Expand Down Expand Up @@ -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);
}
Expand Down
2 changes: 1 addition & 1 deletion lib/internal/errors.js
Original file line number Diff line number Diff line change
Expand Up @@ -1278,7 +1278,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',
Expand Down
10 changes: 0 additions & 10 deletions lib/internal/net.js
Original file line number Diff line number Diff line change
Expand Up @@ -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')
Expand All @@ -72,7 +63,6 @@ module.exports = {
isIP,
isIPv4,
isIPv6,
isLegalPort,
makeSyncWrite,
normalizedArgsSymbol: Symbol('normalizedArgs')
};
25 changes: 20 additions & 5 deletions lib/internal/validators.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ const {
const {
hideStackFrames,
codes: {
ERR_SOCKET_BAD_PORT,
ERR_INVALID_ARG_TYPE,
ERR_INVALID_ARG_VALUE,
ERR_OUT_OF_RANGE,
Expand Down Expand Up @@ -180,6 +181,19 @@ function validateEncoding(data, encoding) {
}
}

// 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,
Expand All @@ -188,11 +202,12 @@ module.exports = {
validateBoolean,
validateBuffer,
validateEncoding,
validateObject,
validateInteger,
validateInt32,
validateUint32,
validateString,
validateInteger,
validateNumber,
validateSignalName
validateObject,
validatePort,
validateSignalName,
validateString,
validateUint32,
};
16 changes: 7 additions & 9 deletions lib/net.js
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,6 @@ const {
isIP,
isIPv4,
isIPv6,
isLegalPort,
normalizedArgsSymbol,
makeSyncWrite
} = require('internal/net');
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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;

Expand Down Expand Up @@ -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) {
Expand Down
23 changes: 23 additions & 0 deletions test/parallel/test-internal-validators-validateport.js
Original file line number Diff line number Diff line change
@@ -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'
}));
20 changes: 0 additions & 20 deletions test/parallel/test-net-internal.js

This file was deleted.

0 comments on commit 897b1d2

Please sign in to comment.