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

querystring: don't inherit from Object.prototype #6055

Merged
merged 1 commit into from Apr 19, 2016

Conversation

Projects
None yet
8 participants
@mscdex
Contributor

mscdex commented Apr 5, 2016

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?
  • 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

This commit safely allows querystring keys that are named the same as
properties that are ordinarily inherited from Object.prototype such
as proto. Additionally, this commit provides a bit of a speed
improvement (~25% in the querystring-parse 'manypairs' benchmark)
when there are many unique keys.

Fixes: #5642

@mscdex mscdex added the querystring label Apr 5, 2016

@mscdex

This comment has been minimized.

Contributor

mscdex commented Apr 5, 2016

@cjihrig

View changes

lib/querystring.js Outdated
@@ -5,6 +5,18 @@
const QueryString = exports;
const Buffer = require('buffer').Buffer;
// This constructor is used to store parsed query string values. Instantiating
// this is faster than explicitly calling `Object.create(null)` to get a
// "clean" empty object

This comment has been minimized.

@cjihrig

cjihrig Apr 5, 2016

Contributor

Could you add a note about the version of V8 that this applies to.

@cjihrig

This comment has been minimized.

Contributor

cjihrig commented Apr 5, 2016

LGTM pending CI

@WebReflection

View changes

lib/querystring.js Outdated
// "clean" empty object
function ParsedQueryString() {}
ParsedQueryString.prototype = Object.create(null, {
constructor: {

This comment has been minimized.

@WebReflection

WebReflection Apr 5, 2016

Contributor

what's with the constructor if I might ask? why bothering?

Also, I think you still need to return an instanceof Object to avoid breaking code using this module.

This comment has been minimized.

@WebReflection

WebReflection Apr 5, 2016

Contributor

to be more specific:

function ParsedQueryString() {}
ParsedQueryString.prototype = Object.create(null);

// before returning, if we don't want outer code to break
return Object.setPrototypeOf(obj, Object.prototype);

This comment has been minimized.

@mscdex

mscdex Apr 5, 2016

Contributor

constructor is removed now.

IMHO I think more people are going to be using typeof foo === 'object' rather than foo instanceof Object. Perhaps @ChALkeR can give us some insight about this?

This comment has been minimized.

@WebReflection

WebReflection Apr 5, 2016

Contributor

It's not about instanceof per-se, it's about .hasOwnProperty and other
methods from Object.prototype that most devs give, wrongly, for granted
with objects. This won't affect my code but it will break every code with
for/in loops. I personally don't care
On Apr 5, 2016 1:59 PM, "Brian White" notifications@github.com wrote:

In lib/querystring.js
#6055 (comment):

@@ -5,6 +5,18 @@
const QueryString = exports;
const Buffer = require('buffer').Buffer;

+// This constructor is used to store parsed query string values. Instantiating
+// this is faster than explicitly calling Object.create(null) to get a
+// "clean" empty object
+function ParsedQueryString() {}
+ParsedQueryString.prototype = Object.create(null, {

  • constructor: {

constructor is removed now.

IMHO I think more people are going to be using typeof foo === 'object'
rather than foo instanceof Object. Perhaps @ChALkeR
https://github.com/ChALkeR can give us some insight about this?


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
https://github.com/nodejs/node/pull/6055/files/ef5f84a85820794a572ba0deb7e15a8b1622b9f4#r58532723

This comment has been minimized.

@jasnell

jasnell Apr 5, 2016

Member

I'm a bit concerned about not setting the object prototype also. We need to get some insight into just how significant of a change that is. Either way, this is definitely semver-major because of that change.

@mscdex mscdex force-pushed the mscdex:querystring-parse-empty-prototype branch 2 times, most recently Apr 5, 2016

@jasnell jasnell added the semver-major label Apr 5, 2016

@jasnell

This comment has been minimized.

Member

jasnell commented Apr 5, 2016

LGTM but I'd like to get more insight into what impact not setting the prototype on return would have.

@mscdex

This comment has been minimized.

Contributor

mscdex commented Apr 5, 2016

It'd definitely be a semver-major thing. I just ran some benchmarks with and without setting the prototype on return and there is definitely a noticeable performance regression when setting the prototype on return in almost all of the benchmark cases.

@jasnell

This comment has been minimized.

Member

jasnell commented Apr 5, 2016

Yep, I was consistently seeing the same thing. I'm happy with the change, just want to make sure we know what'll break

@mscdex

This comment has been minimized.

Contributor

mscdex commented Apr 5, 2016

Also for completeness here are benchmark results I am getting as this PR currently stands (no setting prototype on return) with the number of iterations bumped from 1e6 to 10e6:

querystring/querystring-parse.js type=noencode n=10000000: ./node: 689650 ./node-pre-qs: 597360 ...... 15.45%
querystring/querystring-parse.js type=multicharsep n=10000000: ./node: 633240 ./node-pre-qs: 534580 .. 18.46%
querystring/querystring-parse.js type=encodemany n=10000000: ./node: 367970 ./node-pre-qs: 301350 .... 22.11%
querystring/querystring-parse.js type=encodelast n=10000000: ./node: 563560 ./node-pre-qs: 424940 .... 32.62%
querystring/querystring-parse.js type=multivalue n=10000000: ./node: 540460 ./node-pre-qs: 502680 ..... 7.52%
querystring/querystring-parse.js type=multivaluemany n=10000000: ./node: 243350 ./node-pre-qs: 232420 . 4.70%
querystring/querystring-parse.js type=manypairs n=10000000: ./node: 189290 ./node-pre-qs: 151430 ..... 25.00%
@Fishrock123

This comment has been minimized.

Member

Fishrock123 commented Apr 5, 2016

Interesting. If this works out well, we should look into doing a similar thing for events, per #728.

Does this break any sort of enumeration i.e. Object.getOwnPropertyNames()?

Another thought, what if we were able to expose ourselves something like Object.create(null) more directly from v8?

@mscdex

This comment has been minimized.

Contributor

mscdex commented Apr 5, 2016

@Fishrock123 RE: EventEmitter, I already had that in mind, was going to test it later today.

@WebReflection

This comment has been minimized.

Contributor

WebReflection commented Apr 5, 2016

Does this break any sort of enumeration i.e. Object.getOwnPropertyNames()?

No, it doesn't affect any Object or Reflect operation.

what if we were able to expose ourselves something like Object.create(null) more directly from v8?

I don't know why Dictionary or Null constructors aren't natively available, these both have been used for ages to create null objects, having them native would be awesome.

// forcing `new Dictionary` initialization
class Dictionary {}
delete Object.setPrototypeOf(
  Dictionary.prototype,
  null
).constructor;

@ChALkeR ChALkeR added the performance label Apr 5, 2016

@Sinewyk

This comment has been minimized.

Sinewyk commented Apr 5, 2016

Related: #6044

@Sinewyk Sinewyk referenced this pull request Apr 5, 2016

Closed

querystring: allow querystring parse to handle __proto__ #6044

3 of 3 tasks complete

@mscdex mscdex added this to the 6.0.0 milestone Apr 6, 2016

@mscdex mscdex removed the performance label Apr 7, 2016

@Fishrock123

This comment has been minimized.

Member

Fishrock123 commented Apr 7, 2016

(same as #6092 (comment)) Hmmm, can this be back-ported? Theoretically, this could be considered a security issue for unvalidated input.

@mscdex mscdex referenced this pull request Apr 7, 2016

Closed

http: don't inherit from Object.prototype #6102

3 of 3 tasks complete
@mscdex

This comment has been minimized.

Contributor

mscdex commented Apr 7, 2016

@jasnell

This comment has been minimized.

Member

jasnell commented Apr 7, 2016

This LGTM if CI and CITGM look good. I'm currently -1 on backporting until we can get a better sense about what breaks with this change.

@mscdex

This comment has been minimized.

Contributor

mscdex commented Apr 7, 2016

@mscdex

This comment has been minimized.

Contributor

mscdex commented Apr 18, 2016

@jasnell

This comment has been minimized.

Member

jasnell commented Apr 18, 2016

LGTM but we should also get a CITGM run on it. /cc @thealphanerd

@mscdex

This comment has been minimized.

Contributor

mscdex commented Apr 18, 2016

I ran citgm 11 days ago, but we can do it again I suppose.

@jasnell

This comment has been minimized.

Member

jasnell commented Apr 18, 2016

oh! ha! missed that comment :-)

@jasnell

This comment has been minimized.

Member

jasnell commented Apr 19, 2016

CI is green

querystring: don't inherit from Object.prototype
This commit safely allows querystring keys that are named the same as
properties that are ordinarily inherited from Object.prototype such
as __proto__. Additionally, this commit provides a bit of a speed
improvement (~25% in the querystring-parse 'manypairs' benchmark)
when there are many unique keys.

Fixes: #5642
PR-URL: #6055
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

@mscdex mscdex force-pushed the mscdex:querystring-parse-empty-prototype branch to dba245f Apr 19, 2016

@mscdex mscdex merged commit dba245f into nodejs:master Apr 19, 2016

@mscdex mscdex deleted the mscdex:querystring-parse-empty-prototype branch Apr 19, 2016

@Trott

This comment has been minimized.

Member

Trott commented Apr 19, 2016

This PR appears to introduce an unfortunate side effect in lib/url.js where sometimes the query property will have a prototype (if there is no query string in the URL) and sometimes it won't. I'll be offering a fix for it in #6213, but if that doesn't land, we should still fix it...

joelostrowski added a commit to joelostrowski/node that referenced this pull request Apr 25, 2016

querystring: don't inherit from Object.prototype
This commit safely allows querystring keys that are named the same as
properties that are ordinarily inherited from Object.prototype such
as __proto__. Additionally, this commit provides a bit of a speed
improvement (~25% in the querystring-parse 'manypairs' benchmark)
when there are many unique keys.

Fixes: nodejs#5642
PR-URL: nodejs#6055
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

jasnell added a commit that referenced this pull request Apr 26, 2016

querystring: don't inherit from Object.prototype
This commit safely allows querystring keys that are named the same as
properties that are ordinarily inherited from Object.prototype such
as __proto__. Additionally, this commit provides a bit of a speed
improvement (~25% in the querystring-parse 'manypairs' benchmark)
when there are many unique keys.

Fixes: #5642
PR-URL: #6055
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

vHanda added a commit to Open365/restutils that referenced this pull request May 25, 2016

teohhanhui added a commit to teohhanhui/koa-prerender that referenced this pull request Jun 17, 2016

Don't use Object.prototpye.hasOwnProperty on query
Don't use Object.prototpye.hasOwnProperty on object returned by querystring.parse

Since it does not inherit from Object.prototype since nodejs/node#6055 (node v6.0.0+)

strugee added a commit to strugee/vows that referenced this pull request Apr 7, 2017

Don't assume obj.__proto__ === Object in assertion helpers
Previously, we assumed that whatever object the user passed to the
object assertion helpers had its prototype set to Object and would
thus have things like a hasOwnProperty property.

However, it turns out that this is not a valid assumption. This isn't
limited to just corner cases; Node 6.x changed the behavior of its
`querystring` module (which is also used by its `url` module) to
explicitly set the prototype of querystring objects to `null` in order
to prevent key collisions if someone sent a URL query string parameter
with the same key as a property on the prototype. See nodejs/node#6055
for where this change was made.

Therefore, we use Object.prototype.hasOwnProperty, which is guaranteed
to always exist, and call it on the object the user passed in. We do
the same for propertyIsEnumerable as applicable.

@emilymcafee emilymcafee referenced this pull request Apr 8, 2018

Merged

upgrade to node6 #4

3 of 3 tasks complete

@abovedave abovedave referenced this pull request Jun 7, 2018

Closed

HTTP2 error on req.headers #56

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment