querystring: allow querystring parse to handle __proto__

[jasnell]

Pull Request check-list

• Does make -j8 test (UNIX) or vcbuild test nosign (Windows) pass with
  this change (including linting)?
• Is the commit message formatted according to [CONTRIBUTING.md][0]?
• If this change fixes a bug (or a performance problem), is a regression
  test (or a benchmark) included?

Affected core subsystem(s)

querystring

Description of change

Per #5642, using querystring.parse to parse 'a=b&__proto__=1' causes the __proto__ to be swallowed and ignored. This works around the limitation by temporarily setting the prototype of the parsed obj to null during the parse, then setting it back before returning.

The rest of the existing implementation remains the same.

Fixes: #5642

/cc @mscdex @WebReflection

[mscdex]

If this change is going to be made, wouldn't it be simpler to just use Object.create(null) instead of {} for obj? What is the performance difference if there is any?

[jasnell]

Interesting... using Object.create(null) gives a slight performance edge in a simple benchmark:

'use strict';
var common = require('../common.js');
var querystring = require('querystring');
var v8 = require('v8');

var bench = common.createBenchmark(main, {
  n: [1e6],
});

function main(conf) {
  var n = conf.n | 0;

  const input = 'a=b&__proto__=1';

  v8.setFlagsFromString('--allow_natives_syntax');
  querystring.parse(input);
  eval('%OptimizeFunctionOnNextCall(querystring.parse)');
  querystring.parse(input);

  var i;
  bench.start();
  for (i = 0; i < n; i += 1)
    querystring.parse(input);
  bench.end(n);
}

[jasnell]

@mscdex ... updated to use Object.create(null).

[mscdex]

Are we comfortable with this inconsistency?

[mscdex]

FWIW using Object.create(null) was already discussed in the linked issue and has its own set of performance regressions. Is this solution (out of the other suggested solutions in the linked issue) and its performance something that everyone can (mostly) agree on?

[Fishrock123]

👎 Unless we can see that Object.create(null) isn't orders of magnitude slower. :/

[mscdex]

@Fishrock123 It still is.

@jasnell I think I may have found a solution that doesn't cause a performance regression and may even provide somewhat of a performance boost.

[jasnell]

Sigh, every time I benchmark this I'm getting different results. I'll switch it back to {} for now. What's the alternative you found @mscdex ?

[WebReflection]

since you never know how much optimization could be done, I'd go for:

// begin
var obj = Object.setPrototypeOf({}, null);
// ... rest of the code ...
// end
return Object.setPrototypeOf(obj, Object.prototype);

at least it couldn't go more compact than that, and the returned value from setPrototypeOf doens't get wasted.

[benjamingr]

Why not Object.create(null)?

[benjamingr]

Sorry, had this open for a while and GH refreshed so I just now noticed this was already discussed.

[Sinewyk]

Related: #6055

[jasnell]

Closing in favor of @mscdex's #6055