net: allow port 0 in connect() #9268

cjihrig · 2015-02-23T16:31:06Z

This allows port 0 to be passed to Socket.prototype.connect() and net.createConnection(). However, by allowing port 0, we are eliminating all type checking since var port = options.port | 0; converts everything to a number. More thorough checking could be added by verifying that options.port is undefined, a number, or a numeric string (which must be supported for compatibility with url.parse() results), but at what point does it become overkill.

Closes #9194. cc: @trevnorris

misterdjules · 2015-02-25T01:53:28Z

I think that adding additional checks to throw except for valid numerical strings and numbers is worthwhile, especially if the original code was doing that.

misterdjules · 2015-02-25T01:54:36Z

Adding to the 0.12.1 milestone as it seems like a significant regression to me.

cjihrig · 2015-02-25T22:40:25Z

@misterdjules added type checking back. PTAL

trevnorris · 2015-02-26T18:44:29Z

lib/net.js

@@ -906,8 +906,11 @@ Socket.prototype.connect = function(options, cb) {
    if (localPort && !util.isNumber(localPort))
      throw new TypeError('localPort should be a number: ' + localPort);

-    if (port <= 0 || port > 65535)
-      throw new RangeError('port should be > 0 and < 65536: ' + port);
+    if (!util.isUndefined(options.port) && options.port != port)


Do options.port >>> 0 === port && options.port >= 0. That's a uint check that excludes NaN and prevents wrap around.

This also means you can remove the additional below check. But you should do port >>>= options.port to make sure number strings (i.e. '0xfe') are properly coerced.

@trevnorris I'm not sure I understand what you mean in the previous two comments. Could you please elaborate?

@trevnorris I don't think that will work. I added more tests at @misterdjules request, and that validation failed on the empty string IIRC. What does seem to handle all cases is if (options.port !== undefined && parseInt(options.port, 10) !== port). I'm not big on using parseInt(), but it handles numeric strings well, which is the most annoying part of this check.

@cjihrig ah, my checks are to see if they are. so instead it would be:

if (!(options.port >>> 0 === options.port && options.port >= 0))

@trevnorris I'm still having some trouble with your checks, and variations on them:

if ((options.port >>> 0 === options.port && options.port >= 0)) throws on valid port 12346
if (!(options.port >>> 0 === options.port && options.port >= 0)) throws on valid port '12346'
if (!(options.port >>> 0 === port && options.port >= 0)) does not throw on invalid port ''
if ((options.port >>> 0 === port && options.port >= 0)) throws on valid port 12346

trevnorris · 2015-02-26T18:46:17Z

Couple comments, otherwise LGTM.

misterdjules · 2015-02-26T22:02:21Z

test/simple/test-net-localerror.js

-
-  connect({
-    host: 'localhost',
-    port: 0


Instead of removing this test, can we put it back and assert it doesn't throw?

misterdjules · 2015-02-26T22:14:32Z

@cjihrig Thanks Colin! I added my comments inline.

cjihrig · 2015-02-27T01:27:06Z

@misterdjules I made the test changes your requested.

misterdjules · 2015-02-27T02:04:32Z

lib/net.js

@@ -906,8 +906,11 @@ Socket.prototype.connect = function(options, cb) {
    if (localPort && !util.isNumber(localPort))
      throw new TypeError('localPort should be a number: ' + localPort);

-    if (port <= 0 || port > 65535)
-      throw new RangeError('port should be > 0 and < 65536: ' + port);
+    if (options.port !== undefined && parseInt(options.port, 10) !== port)


Using parseInt we would accept "42foo" as a valid port, which we wouldn't accept before this change. I don't think we should accept these values, what do you think?

If options.port was "42foo", then parseInt() would return 42, but port would be 0, and an exception would be thrown.

Right, I thought "42foo" | 0 would evaluate to 42, which it does not. Thanks for the clarification!

Using parseInt only with a radix of 10 makes strings representing hexadecimal numbers such as 0x12 throw. The current version of the code does, and it seems that it matches with the concept of "undefined, a number, or a string representing a number".

I'm not sure if we want to support it though. I added support for strings representing hexadecimal numbers in a gist, and the problem is that it would accept invalid values such as 0x123R, since 0x123R | 0 === 0 and parseInt(0x123R, 10) === 0 too.

Other than that, I think the above-mentioned gist improves the current changes by making the checks a bit clearer though, so feel free to reuse it without the hexadecimal string support.

I created another gist that I think makes the check even clearer: https://gist.github.com/misterdjules/741cb578638b18f139ef.

Unfortunately, I thought that parseInt('0xff', 10) would return 255, but that is not the case. I think this could be the answer:

if (options.port !== undefined && parseFloat(options.port) !== port)

EDIT: Still doesn't handle hex strings

@misterdjules OK, I found this, which appears to handle everything I've tried so far. It's less efficient than I'd like, but could also be added to util. Thoughts?

function isNumeric(n) { return !isNaN(parseFloat(n)) && isFinite(n); }

As mentioned in #libuv, isNumeric would not handle negative hexadecimal number strings, and thus isNumeric would be inconsistent. But we could have:

/* * This function returns `true` if `value` is a string that represents a valid * decimal or hexadecimal non-negative number, false otherwise. */ function isNonNegativeNumeric(n) { return !isNaN(parseFloat(n)) && isFinite(n) && n >= 0; }

and use it like in the following gist: https://gist.github.com/misterdjules/7e4f25997b3805792aac.

cjihrig · 2015-02-27T16:08:17Z

@misterdjules updated to use IsUint32()

misterdjules · 2015-02-27T20:58:59Z

lib/net.js

-    if (port <= 0 || port > 65535)
-      throw new RangeError('port should be > 0 and < 65536: ' + port);
+    if (options.port !== undefined && parseInt(options.port, 10) !== port)
+      throw new TypeError('port should be a number: ' + options.port);


We might also want to change the error message to 'port should be undefined or should represent a valid number: ' + options.port', but I'm not sure how much of a breaking change that would be for users who consume error messages.

cjihrig · 2015-03-01T23:17:07Z

@misterdjules updated.

misterdjules · 2015-03-03T00:33:03Z

@cjihrig Pasting what I said in #libuv to make sure it's not lost:

Another concern I had is that actually parseFloat doesn’t handle hexadecimal strings at all, for instance parseFloat(‘0xff’) === 0. In the current state of the PR, it works out correctly, but because we’re lucky.

It seems to me that your original method (comparing coercion to int’s result with parseInt’s output) would be more robust and consistent. We could add some additional checks and comments like something closer to what I had in this gist: https://gist.github.com/misterdjules/741cb578638b18f139ef with the error message mentioning that numbers and numeric strings are accepted instead of 'port shoud be a number'.

What do you think?

trevnorris · 2015-03-03T18:32:59Z

The best way for number coercion is +number. Use that instead of parseFloat().

cjihrig · 2015-03-03T20:59:33Z

Updated to a slightly new approach. Added more tests, and incorporated @trevnorris +number suggestion.

jasnell · 2015-03-03T23:01:05Z

LGTM

misterdjules · 2015-03-03T23:24:54Z

@cjihrig I like it, thank you very much for persisting!

LGTM and tests running here: http://jenkins.nodejs.org/job/node-test-commit-unix/ and here: http://jenkins.nodejs.org/job/node-test-commit-windows/. I don't expect any of them to fail since all tests pass locally, but better safe than sorry.

misterdjules · 2015-03-03T23:30:28Z

More precisely, tests for UNICEs here: http://jenkins.nodejs.org/job/node-test-commit-unix/23/ and Windows here: http://jenkins.nodejs.org/job/node-test-commit-windows/22/.

misterdjules · 2015-03-03T23:32:54Z

One test failing on SmartOS: http://jenkins.nodejs.org/job/node-test-commit-unix/23/DESTCPU=ia32,label=smartos/tapTestReport/simple.tap-437/, TCPWrap::Connect6 also needs to be updated.

The added validation allows non-negative numbers and numeric strings. All other values result in a thrown exception.

cjihrig · 2015-03-04T01:35:33Z

@misterdjules updated Connect6

misterdjules · 2015-03-04T02:08:49Z

@cjihrig Tests running on UNIX and Windows.

misterdjules · 2015-03-04T18:23:43Z

@cjihrig All tests passed.

The added validation allows non-negative numbers and numeric strings. All other values result in a thrown exception. Fixes: #9194 PR-URL: #9268 Reviewed-By: Julien Gilli <[email protected]> Reviewed-By: Trevor Norris <[email protected]> Reviewed-By: James M Snell <[email protected]>

cjihrig · 2015-03-05T14:40:56Z

Thanks! Landed in 1a2a4da

Nodejs-Jenkins added pr v0.12 labels Feb 23, 2015

tjfontaine added the in progress label Feb 23, 2015

misterdjules added this to the 0.12.1 milestone Feb 25, 2015

trevnorris reviewed Feb 26, 2015
View reviewed changes

misterdjules reviewed Feb 26, 2015
View reviewed changes

misterdjules reviewed Feb 27, 2015
View reviewed changes

net: allow port 0 in connect()

d027135

The added validation allows non-negative numbers and numeric strings. All other values result in a thrown exception.

cjihrig closed this Mar 5, 2015

tjfontaine removed the in progress label Mar 5, 2015

cjihrig deleted the 9194 branch March 5, 2015 14:40

rvagg mentioned this pull request Mar 5, 2015

Release proposal: v1.5.0 nodejs/node#1060

Closed

rvagg mentioned this pull request Mar 19, 2015

Release proposal: v1.6.0 nodejs/node#1157

Closed

julianduque mentioned this pull request Mar 20, 2015

Evangelism: Weekly Update March 20th nodejs/evangelism#46

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

net: allow port 0 in connect() #9268

net: allow port 0 in connect() #9268

cjihrig commented Feb 23, 2015

misterdjules commented Feb 25, 2015

misterdjules commented Feb 25, 2015

cjihrig commented Feb 25, 2015

trevnorris Feb 26, 2015

trevnorris Feb 26, 2015

misterdjules Feb 26, 2015

cjihrig Feb 27, 2015

trevnorris Feb 27, 2015

cjihrig Feb 27, 2015

trevnorris commented Feb 26, 2015

misterdjules Feb 26, 2015

misterdjules commented Feb 26, 2015

cjihrig commented Feb 27, 2015

misterdjules Feb 27, 2015

cjihrig Feb 27, 2015

misterdjules Feb 27, 2015

misterdjules Feb 27, 2015

cjihrig Feb 27, 2015

cjihrig Feb 27, 2015

misterdjules Feb 27, 2015

cjihrig commented Feb 27, 2015

misterdjules Feb 27, 2015

cjihrig commented Mar 1, 2015

misterdjules commented Mar 3, 2015

trevnorris commented Mar 3, 2015

cjihrig commented Mar 3, 2015

jasnell commented Mar 3, 2015

misterdjules commented Mar 3, 2015

misterdjules commented Mar 3, 2015

misterdjules commented Mar 3, 2015

cjihrig commented Mar 4, 2015

misterdjules commented Mar 4, 2015

misterdjules commented Mar 4, 2015

cjihrig commented Mar 5, 2015

net: allow port 0 in connect() #9268

net: allow port 0 in connect() #9268

Conversation

cjihrig commented Feb 23, 2015

misterdjules commented Feb 25, 2015

misterdjules commented Feb 25, 2015

cjihrig commented Feb 25, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trevnorris commented Feb 26, 2015

Choose a reason for hiding this comment

misterdjules commented Feb 26, 2015

cjihrig commented Feb 27, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjihrig commented Feb 27, 2015

Choose a reason for hiding this comment

cjihrig commented Mar 1, 2015

misterdjules commented Mar 3, 2015

trevnorris commented Mar 3, 2015

cjihrig commented Mar 3, 2015

jasnell commented Mar 3, 2015

misterdjules commented Mar 3, 2015

misterdjules commented Mar 3, 2015

misterdjules commented Mar 3, 2015

cjihrig commented Mar 4, 2015

misterdjules commented Mar 4, 2015

misterdjules commented Mar 4, 2015

cjihrig commented Mar 5, 2015