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: handle normalized args in normalizeArgs() #13069

Merged
merged 1 commit into from May 19, 2017

Conversation

@cjihrig
Copy link
Contributor

commented May 16, 2017

This commit allows normalizeArgs() to be called with its own previous output. Prior to this change, multiple calls to normalizeArgs() would create a nested structure that produced invalid arguments.

cc: @vdeturckheim @joyeecheung @mcollina @jasnell (from othiym23/async-listener#110)

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

net

lib/net.js Outdated
@@ -125,6 +125,10 @@ function normalizeArgs(args) {
const arg0 = args[0];
var options = {};
if (typeof arg0 === 'object' && arg0 !== null) {
// Handle the case where the arguments were already normalized.
if (Array.isArray(arg0))

This comment has been minimized.

Copy link
@mscdex

mscdex May 16, 2017

Contributor

Using instanceof Array should be faster.

This comment has been minimized.

Copy link
@TimothyGu

TimothyGu May 17, 2017

Member

Actually, in my experiences Array.isArray is faster because of the difficult-to-optimize nature of the instanceof operator under ES2015+…

This comment has been minimized.

Copy link
@mscdex

mscdex May 17, 2017

Contributor

Do you have a benchmark that shows this on master?

This comment has been minimized.

Copy link
@watson

watson May 17, 2017

Member

I'm curious, wouldn't Array.isArray always be a safer choice? Or because we know how arg0 is created in this specific case we can safely use instanceof Array?

@refack

This comment has been minimized.

Copy link
Member

commented May 17, 2017

Just an idea, put a symbol on it, to distinguish from arbitrary Arrays

lib/net.js Outdated
@@ -125,6 +125,10 @@ function normalizeArgs(args) {
const arg0 = args[0];
var options = {};
if (typeof arg0 === 'object' && arg0 !== null) {

This comment has been minimized.

Copy link
@joyeecheung

joyeecheung May 17, 2017

Member

If this is doing the same thing as the if (arguments.length === 1 && Array.isArray(arguments[0])) block in Socket.prototype.connect, maybe remove the similar logic there?

This comment has been minimized.

Copy link
@cjihrig

cjihrig May 17, 2017

Author Contributor

@joyeecheung it is related, but not the same thing.

@vdeturckheim

This comment has been minimized.

Copy link
Member

commented May 17, 2017

LGTM!

@cjihrig

This comment has been minimized.

Copy link
Contributor Author

commented May 17, 2017

Just an idea, put a symbol on it, to distinguish from arbitrary Arrays

I'm open to it if others are.

@mcollina
Copy link
Member

left a comment

LGTM

@cjihrig

This comment has been minimized.

Copy link
Contributor Author

commented May 18, 2017

@watson

This comment has been minimized.

Copy link
Member

commented May 18, 2017

Out of curiosity, when is normalizeArgs() called recursively?

@cjihrig

This comment has been minimized.

Copy link
Contributor Author

commented May 18, 2017

@watson for example in othiym23/async-listener#110. normalizeArgs() returns an array of length 2. Node's check in Socket.prototype.connect() explicitly checks for a length of 1. This could be fixed in a few ways (add more logic in async-listener, remove the length of 1 check). I think allowing normalizeArgs() to consume its own output is the most flexible solution.

@cjihrig

This comment has been minimized.

Copy link
Contributor Author

commented May 18, 2017

Actually, the more I think about it, the more I like the idea of just dropping the arguments.length === 1 check in Socket.prototype.connect().

@watson

This comment has been minimized.

Copy link
Member

commented May 18, 2017

@cjihrig you mean the code I added? Weird, I thought we tested async-listener against that patch since that was the whole reason for the patch 😅

@watson

This comment has been minimized.

Copy link
Member

commented May 18, 2017

@cjihrig it was kind of an ugly hack. And allowing normalizeArgs to be called recursively would fix part of that. But we'll then still have to run these 3 lines twice: https://github.com/cjihrig/node-1/blob/27ae95a21958b3c7d91c688da6093e96fc167e5c/lib/net.js#L957-L959

@cjihrig

This comment has been minimized.

Copy link
Contributor Author

commented May 18, 2017

I do indeed mean that code :-D

We tested it, but @vdeturckheim came to me with this script, which was still failing.

And allowing normalizeArgs to be called recursively would fix part of that. But we'll then still have to run these 3 lines twice

Yea, removing the arguments.length === 1 check should fix the problem. @refack's suggestion of attaching a symbol might be helpful as well, since we don't want to support arbitrary arrays.

@watson

This comment has been minimized.

Copy link
Member

commented May 18, 2017

Ah, ok got it now thanks for the explanation 😄

Yeah, the symbol trick might be ok

@cjihrig cjihrig force-pushed the cjihrig:normalize branch May 18, 2017

@cjihrig

This comment has been minimized.

Copy link
Contributor Author

commented May 18, 2017

New commit (3564d322fd891dccc97e80c90f522412bad54da5) has the symbol approach.

lib/internal/net.js Outdated
@@ -10,5 +10,6 @@ function isLegalPort(port) {
}

module.exports = {
isLegalPort
isLegalPort,
kNormalizedConnectSymbol: Symbol('normalizedConnect')

This comment has been minimized.

Copy link
@mscdex

mscdex May 18, 2017

Contributor

It's also used by listen(). Perhaps we should just call it normalizedArgsSymbol with a 'normalizedArgs' symbol value? Keeping it generic may also help if we ever need/want it for other normalization functions, like the ones in child_process or something.

@cjihrig cjihrig force-pushed the cjihrig:normalize branch May 18, 2017

@cjihrig

This comment has been minimized.

Copy link
Contributor Author

commented May 18, 2017

@mscdex renamed the symbol as you suggested.

@watson

This comment has been minimized.

Copy link
Member

commented May 18, 2017

LGTM 👍

@refack
refack approved these changes May 18, 2017

@cjihrig cjihrig force-pushed the cjihrig:normalize branch May 19, 2017

@cjihrig

This comment has been minimized.

Copy link
Contributor Author

commented May 19, 2017

New CI: https://ci.nodejs.org/job/node-test-pull-request/8173/

EDIT: Some platforms fail with EADDRNOTAVAIL. Others fail with ECONNREFUSED. Updated the test to handle both. CI: https://ci.nodejs.org/job/node-test-pull-request/8174/

@cjihrig cjihrig force-pushed the cjihrig:normalize branch May 19, 2017

net: add symbol to normalized connect() args
This commit attaches a Symbol to the result of
net._normalizeArgs(). This prevents normal arrays from being
passed to the internal Socket.prototype.connect() bypass logic.

PR-URL: #13069
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

@cjihrig cjihrig force-pushed the cjihrig:normalize branch to 51664fc May 19, 2017

@cjihrig cjihrig merged commit 51664fc into nodejs:master May 19, 2017

@cjihrig cjihrig deleted the cjihrig:normalize branch May 19, 2017

@vdeturckheim

This comment has been minimized.

Copy link
Member

commented May 19, 2017

Awesome! Thank you @cjihrig !!

anchnk pushed a commit to anchnk/node that referenced this pull request May 19, 2017
net: add symbol to normalized connect() args
This commit attaches a Symbol to the result of
net._normalizeArgs(). This prevents normal arrays from being
passed to the internal Socket.prototype.connect() bypass logic.

PR-URL: nodejs#13069
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@jasnell jasnell referenced this pull request May 28, 2017
@gibfahn gibfahn referenced this pull request Jun 15, 2017
2 of 3 tasks complete
@MylesBorins

This comment has been minimized.

Copy link
Member

commented Jul 17, 2017

Should this land on v6.x?

If so it will need a manual backport

@mcollina

This comment has been minimized.

Copy link
Member

commented Jul 17, 2017

I'm 👎 to backport. This would need a lot of other changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
10 participants
You can’t perform that action at this time.