querystring: check that maxKeys is finite #5066

MylesBorins · 2016-02-03T23:56:12Z

There was a very subtle change in behavior introduced with 27def4f

In the past if querystring.parse was given Infinity for maxKeys,
everything worked as expected.

Check to see is maxKeys is Infinity before forwarding the value to
String.prototype.split which causes this regression

MylesBorins · 2016-02-03T23:57:59Z

/cc @jasnell @targos @thefourtheye

evanlucas · 2016-02-03T23:58:13Z

LGTM, but should the test be in a separate commit?

MylesBorins · 2016-02-03T23:59:57Z

LGTM, but should the test be in a separate commit?

I'm really not sure, I can check previous commits. To me it makes sense to include the test with the commit removing the regression.

thefourtheye · 2016-02-04T00:02:41Z

lib/querystring.js

  // maxKeys <= 0 means that we should not limit keys count
-  if (maxKeys > 0) {


Wouldn't it be better, if we just throw an isFinite check here?

that is actually a great idea.

thefourtheye · 2016-02-04T00:12:52Z

The spec documents that the second argument must be an integer.
Infinity is being interpreted as 0... instead of MAX_INT
(perhaps this should be a fix upstream to v8).

Sadly, it is as per the spec only.

String.prototype.split says,

If limit is undefined, let lim = 232–1; else let lim = ToUint32(limit).

and 9.6 ToUint32: (Unsigned 32 Bit Integer) says,

If number is NaN, +0, −0, +∞, or −∞, return +0.

MylesBorins · 2016-02-04T00:13:44Z

@thefourtheye great idea, I've restructured this commit

@evanlucas I'll need another LGTM. Rather than reverting I've added an extra check for infinity. It now makes much more sense to include the test

MylesBorins · 2016-02-04T00:14:36Z

@nodejs/testing is there a preference to writing more smaller tests, or putting this in with another test already testing querystring?

evanlucas · 2016-02-04T00:16:30Z

Yea, I like this. LGTM. Definitely need CI on this one though to make sure arm is happy with it too

thefourtheye · 2016-02-04T00:17:08Z

Adding a new test is fine in this case, and it would be better to have this issue also mentioned somewhere in the comments.

thefourtheye · 2016-02-04T00:21:35Z

test/parallel/test-querystring-maxKeys-Infinity.js

@@ -0,0 +1,36 @@
+'use strict';
+const common = require('../common');


Didn't our linter complain about unused variable? I thought this,

## disallow declaration of variables that are not used in the code no-unused-vars: [2, {"args": "none"}]

forbids us

yeah... I just pushed an update that fixes the linting issues

thefourtheye · 2016-02-04T00:28:25Z

test/parallel/test-querystring-maxKeys-Infinity.js

+// 27def4f introduced a change to parse that would cause Inifity
+// to be passed to String.prototype.split as an argument for limit
+// In this instance split will always return an empty array
+// this test confirms that the output of parse is the expe4cted length


expe4cted -> expected

thefourtheye · 2016-02-04T00:29:29Z

test/parallel/test-querystring-maxKeys-Infinity.js

+// In this instance split will always return an empty array
+// this test confirms that the output of parse is the expe4cted length
+// when passed Infinity as the argument for maxKeys
+const result = parse(params, undefined, undefined, {maxKeys: Infinity});


Now that we are doing it, let's introduce tests with NaN, 'Infinity' and 'NaN'

@thefourtheye I've updated the test to include tests for all four "non finite" values

MylesBorins · 2016-02-04T00:41:29Z

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

thefourtheye · 2016-02-04T00:58:09Z

I couldn't think of any other corner cases. LGTM.

cjihrig · 2016-02-04T01:24:22Z

It might be a little more efficient to add the isFinite() check to the line if (options && typeof options.maxKeys === 'number') {. Then you would only get the overhead if a number is provided as input.

cjihrig · 2016-02-04T01:24:35Z

LGTM with a comment.

thefourtheye · 2016-02-04T01:41:11Z

In that case we can simply do if (options && Number.isFinite(options.maxKeys)

Trott · 2016-02-04T04:10:26Z

@nodejs/testing is there a preference to writing more smaller tests, or putting this in with another test already testing querystring?

Speaking for myself only, I definitely prefer smaller, isolated tests.

rvagg · 2016-02-04T10:09:34Z

test/parallel/test-querystring-maxKeys-non-finite.js

+const assert = require('assert');
+const parse = require('querystring').parse;
+
+// taken from express-js/body-parser


put in a full github url including hash for the exact version you're pulling from

rvagg · 2016-02-04T10:15:54Z

/cc @manvalls

@thealphanerd I think this is ok, lgtm

@Trott:

@nodejs/testing is there a preference to writing more smaller tests, or putting this in with another test already testing querystring?

Speaking for myself only, I definitely prefer smaller, isolated tests.

One thing to keep in mind is that separate tests take longer to run in CI than compacting them into fewer tests. Primarily due to very slow startup time on slower ARM machines but also it seems like this AIX and musl libc based systems have slow exit times (ref #5056).

More files, separate tests is certainly the nicer way to approach it but I just wanted to register the above concern so that @nodejs/testing is aware of this and can (maybe) take it into account when forming opinions on how to best structure tests.

MylesBorins · 2016-02-04T21:23:46Z

@cjihrig @thefourtheye if we made the suggested change then maxKeys will always = 1000, this would also result in a behaviour change

MylesBorins · 2016-02-04T21:27:17Z

@rvagg I've updated the test with a comment that includes the github link, with sha and line numbers.

As you can expect this link is far over the max characters per line. I've split it on to two lines, hopefully that isn't confusing

cjihrig · 2016-02-04T21:32:57Z

@thealphanerd no, this is what I was proposing. It should be functionally equivalent to what you have, but slightly more efficient. And, as @thefourtheye pointed out, the last two conditions can be condensed to Number.isFinite().

  var maxKeys = 1000;
  if (options && typeof options.maxKeys === 'number' && isFinite(options.maxKeys)) {
    maxKeys = options.maxKeys;
  }

  // maxKeys <= 0 means that we should not limit keys count
  if (maxKeys > 0) {

MylesBorins · 2016-02-04T22:20:42Z

maxKeys is set just above though. Meaning that if(maxKeys > 0) will be true if options.MaxKeys === Infinity

thus the split will be called with maxKeys set to 1000 which is still a behavior change.

To double check that I am correct I have implemented your change and run it against the tests and we get a failure

AssertionError: 1000 == 10000

cjihrig · 2016-02-05T03:35:12Z

Sorry, you're right. I was thinking we wanted maxKeys to be the default value on bad input. The existing behavior seems silly... Use 1000 as a default, but if a number is provided use that, but if it's not positive and finite, ignore it completely.

thefourtheye · 2016-02-05T04:39:43Z

Use 1000 as a default, but if a number is provided use that, but if it's not positive and finite, ignore it completely.

Ya, that doesn't look right. Can we fix this behavior here?

MylesBorins · 2016-02-05T08:08:46Z

@thefourtheye I think we can fix that behavior, but that would be a Major change. In the mean time we do need to patch back the behavior that is expected (imho).

Or are you arguing that the behavior itself was a bug?

thefourtheye · 2016-02-05T08:13:02Z

@thealphanerd Though it seems odd, as you pointed out, our priority here is to get this fixed asap, with the current functionality. Perhaps we can improve this in a separate patch. LGTM.

MylesBorins · 2016-02-05T08:14:55Z

I think it is also worth mentioning that #5012 rewrite all the code this is touching.

ncopa · 2016-02-05T11:22:49Z

it seems like this AIX and musl libc based systems have slow exit times (ref #5056).

Technically, exit itself is quick. What happens is that the parent exits without reaping the child processes, so init (pid 1) has to reap those zombies. The slow timeout on Alpine Linux was due to busybox init has a "design flaw" that will intentionally delay the reaping of the orphaned child processes. This has actually nothing to do with musl libc and I am pretty sure that busybox init with glibc has the exact same issue. Most likely this is what AIX init does too.

Exit times are fast. But someone/something needs to reap the dead processes. Ideally should the parent process take care of that instead of letting init (pid 1) do it.

Fishrock123 · 2016-02-05T21:52:44Z

Rubber-stamp LGTM

There was a very subtle change in behavior introduced with 27def4f In the past if querystring.parse was given Infinity for maxKeys, everything worked as expected. Check to see is maxKeys is Infinity before forwarding the value to String.prototype.split which causes this regression PR-URL: nodejs#5066 Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Rod Vagg <rod@vagg.org> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>

MylesBorins · 2016-02-05T22:00:08Z

Landed as 878bcd4

thefourtheye · 2016-02-06T04:49:40Z

Express must be happy now, right?

rvagg · 2016-02-08T04:03:50Z

Thanks @ncopa, that's very interesting background

There was a very subtle change in behavior introduced with 27def4f In the past if querystring.parse was given Infinity for maxKeys, everything worked as expected. Check to see is maxKeys is Infinity before forwarding the value to String.prototype.split which causes this regression PR-URL: #5066 Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Rod Vagg <rod@vagg.org> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>

There was a very subtle change in behavior introduced with 27def4f In the past if querystring.parse was given Infinity for maxKeys, everything worked as expected. Check to see is maxKeys is Infinity before forwarding the value to String.prototype.split which causes this regression PR-URL: nodejs#5066 Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Rod Vagg <rod@vagg.org> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>

thefourtheye reviewed Feb 4, 2016
View reviewed changes

MylesBorins force-pushed the query-string-infinity branch from 568da30 to 83d9a13 Compare February 4, 2016 00:11

MylesBorins changed the title ~~querystring: revert "use String.prototype..."~~ querystring: check that maxKeys is finite Feb 4, 2016

mscdex added the querystring Issues and PRs related to the built-in querystring module. label Feb 4, 2016

MylesBorins added the dont-land-on-v4.x label Feb 4, 2016

MylesBorins mentioned this pull request Feb 4, 2016

querystring: use String.prototype.split's limit #2288

Closed

thefourtheye reviewed Feb 4, 2016
View reviewed changes

MylesBorins force-pushed the query-string-infinity branch 2 times, most recently from 928bb12 to 34c252c Compare February 4, 2016 00:26

thefourtheye reviewed Feb 4, 2016
View reviewed changes

MylesBorins force-pushed the query-string-infinity branch from 34c252c to e754fe7 Compare February 4, 2016 00:29

thefourtheye reviewed Feb 4, 2016
View reviewed changes

MylesBorins force-pushed the query-string-infinity branch from e754fe7 to f65ba73 Compare February 4, 2016 00:39

rvagg reviewed Feb 4, 2016
View reviewed changes

MylesBorins force-pushed the query-string-infinity branch 2 times, most recently from 1856970 to 518f4f5 Compare February 4, 2016 21:26

MylesBorins force-pushed the query-string-infinity branch from 518f4f5 to e529f08 Compare February 5, 2016 21:58

MylesBorins force-pushed the query-string-infinity branch from e529f08 to 878bcd4 Compare February 5, 2016 21:58

MylesBorins merged commit 878bcd4 into nodejs:master Feb 5, 2016

rvagg mentioned this pull request Feb 8, 2016

Release proposal: v5.5.1 (Stable) #5141

Closed

MylesBorins deleted the query-string-infinity branch February 8, 2016 18:46

		// maxKeys <= 0 means that we should not limit keys count
		if (maxKeys > 0) {

		@@ -0,0 +1,36 @@
		'use strict';
		const common = require('../common');

querystring: check that maxKeys is finite #5066

querystring: check that maxKeys is finite #5066

Conversation

MylesBorins commented Feb 3, 2016

MylesBorins commented Feb 3, 2016

evanlucas commented Feb 3, 2016

MylesBorins commented Feb 3, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thefourtheye commented Feb 4, 2016

MylesBorins commented Feb 4, 2016

MylesBorins commented Feb 4, 2016

evanlucas commented Feb 4, 2016

thefourtheye commented Feb 4, 2016

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

MylesBorins commented Feb 4, 2016

thefourtheye commented Feb 4, 2016

cjihrig commented Feb 4, 2016

cjihrig commented Feb 4, 2016

thefourtheye commented Feb 4, 2016

Trott commented Feb 4, 2016

Choose a reason for hiding this comment

rvagg commented Feb 4, 2016

MylesBorins commented Feb 4, 2016

MylesBorins commented Feb 4, 2016

cjihrig commented Feb 4, 2016

MylesBorins commented Feb 4, 2016

cjihrig commented Feb 5, 2016

thefourtheye commented Feb 5, 2016

MylesBorins commented Feb 5, 2016

thefourtheye commented Feb 5, 2016

MylesBorins commented Feb 5, 2016

ncopa commented Feb 5, 2016

Fishrock123 commented Feb 5, 2016

MylesBorins commented Feb 5, 2016

thefourtheye commented Feb 6, 2016

rvagg commented Feb 8, 2016