Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

net: Validate port in createServer().listen() #5732

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion lib/internal/net.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
'use strict';

module.exports = { isLegalPort };
module.exports = { isLegalPort, assertPort };

// 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.
Expand All @@ -10,3 +10,9 @@ function isLegalPort(port) {
return false;
return +port === (+port >>> 0) && port <= 0xFFFF;
}


function assertPort(port) {
if (typeof port !== 'undefined' && !isLegalPort(port))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can I remove typeof port !== 'undefined' && since #5733 already landed?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so. The changes in #5733 make isLegalPort() validate the value of port. undefined isn't a valid port, and is specific to this use case.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although you could change it to port === undefined :-)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-1... the current code allows undefined to pass through.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the following a test?

typeof net.createServer(()=>{}).listen().address().port === 'number';

If undefined is meant to be allowed to pass through then should probably document that at least in the form of a test.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at the code where assertPort is being used, we see:

        assertPort(h.port);
        if (h.host)
          listenAfterLookup(h.port | 0, h.host, backlog, h.exclusive);
        else
          listen(self, null, h.port | 0, 4, backlog, undefined, h.exclusive);

Notice that listen is called with h.port | 0, so that when h.port is undefined, assertPort falls through and it defaults to 0... specifically to account for when h.port is not explicitly specified.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@trevnorris: we do have a test that uses undefined (https://github.com/dirceu/node/blob/fix-5727/test/parallel/test-net-listen-port-option.js#L7). Should I change it to explicitly check if the port is a number?

@jasnell: got it. Speaking of which, wouldn't be clearer to use something like this?

    let port = h.port | 0;
    assertPort(port);
    if (h.host)
      listenAfterLookup(port, h.host, backlog, h.exclusive);
    else
      listen(self, null, port, 4, backlog, undefined, h.exclusive);

Otherwise, is there anything else that could be improved on the PR?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, just getting back to this one. Since #5733 landed, isLegalPort(undefined) now returns false. @dirceu, not certain if that changes any of the details that you're doing here. Can you take another look?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jasnell I just rebased with master and everything seems to be OK.

throw new RangeError('"port" argument must be >= 0 and < 65536');
}
7 changes: 4 additions & 3 deletions lib/net.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ var cluster;
const errnoException = util._errnoException;
const exceptionWithHostPort = util._exceptionWithHostPort;
const isLegalPort = internalNet.isLegalPort;
const assertPort = internalNet.assertPort;

function noop() {}

Expand Down Expand Up @@ -1352,9 +1353,7 @@ Server.prototype.listen = function() {
(typeof h.port === 'undefined' && 'port' in h)) {
// Undefined is interpreted as zero (random port) for consistency
// with net.connect().
if (typeof h.port !== 'undefined' && !isLegalPort(h.port))
throw new RangeError('"port" option should be >= 0 and < 65536: ' +
h.port);
assertPort(h.port);
if (h.host)
listenAfterLookup(h.port | 0, h.host, backlog, h.exclusive);
else
Expand All @@ -1375,10 +1374,12 @@ Server.prototype.listen = function() {
typeof arguments[1] === 'function' ||
typeof arguments[1] === 'number') {
// The first argument is the port, no IP given.
assertPort(port);
listen(self, null, port, 4, backlog);

} else {
// The first argument is the port, the second an IP.
assertPort(port);
listenAfterLookup(port, arguments[1], backlog);
}

Expand Down
2 changes: 1 addition & 1 deletion test/parallel/test-net-listen-port-option.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ net.Server().listen({ port: '' + common.PORT }, close);
].forEach(function(port) {
assert.throws(function() {
net.Server().listen({ port: port }, assert.fail);
}, /"port" option should be >= 0 and < 65536/i);
}, /"port" argument must be >= 0 and < 65536/i);
});

[null, true, false].forEach(function(port) {
Expand Down
27 changes: 27 additions & 0 deletions test/parallel/test-regress-GH-5727.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
'use strict';
const common = require('../common');
const assert = require('assert');
const net = require('net');

const invalidPort = -1 >>> 0;
const errorMessage = /"port" argument must be \>= 0 and \< 65536/;

net.Server().listen(common.PORT, function() {
assert.equal(this._connectionKey, '6::::' + common.PORT);
this.close();
});

// The first argument is a configuration object
assert.throws(() => {
net.Server().listen({ port: invalidPort }, common.fail);
}, errorMessage);

// The first argument is the port, no IP given.
assert.throws(() => {
net.Server().listen(invalidPort, common.fail);
}, errorMessage);

// The first argument is the port, the second an IP.
assert.throws(() => {
net.Server().listen(invalidPort, '0.0.0.0', common.fail);
}, errorMessage);
15 changes: 0 additions & 15 deletions test/sequential/test-net-server-address.js
Original file line number Diff line number Diff line change
Expand Up @@ -87,18 +87,3 @@ server3.listen(0, function() {
assert.strictEqual(address.family, family_ipv6);
server3.close();
});

// Test without hostname, but with port -1
var server4 = net.createServer();

server4.on('error', function(e) {
console.log('Error on ip socket: ' + e.toString());
});

// Specify -1 as port number
server4.listen(-1, function() {
var address = server4.address();
assert.strictEqual(address.address, anycast_ipv6);
assert.strictEqual(address.family, family_ipv6);
server4.close();
});