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

qs.parse stackoverflow on circular objects #31

Closed
dougwilson opened this issue Aug 28, 2014 · 8 comments
Closed

qs.parse stackoverflow on circular objects #31

dougwilson opened this issue Aug 28, 2014 · 8 comments
Assignees
Labels
Milestone

Comments

@dougwilson
Copy link
Contributor

So when using qs.parse and passing in an object to expand out the top-level keys, if somewhere in the value is a circular reference, it will fail with RangeError: Maximum call stack size exceeded. Here is a simple example:

var a = {}; a.b = a;
qs.parse({
  'foo[bar]' = 'baz',
  'foo[baz]' = a
})

I'm really only reporting this because it used to work with the old qs library. It was reported to me here: expressjs/connect-multiparty#11 (comment)

@nlf
Copy link
Collaborator

nlf commented Aug 28, 2014

Out of curiosity, what's the expected result here?

@nlf
Copy link
Collaborator

nlf commented Aug 28, 2014

Also it appears that the old version of qs crashed with this also:

$ npm i qs@0.x.x
qs@0.6.6 node_modules/qs
$ node
> var qs = require('qs');
> var a = {}; a.b = a; qs.parse({ 'foo[bar]': 'baz', 'foo[baz]': a })
RangeError: Maximum call stack size exceeded

@dougwilson
Copy link
Contributor Author

Hm, weird. All I really know is that something with domains used to work with the old qs but not with the new qs according to expressjs/connect-multiparty#11 . I don't use domains, so from the error I was just assuming it was a circular reference.

Anyway, I would assume that it would just copy the reference to the new object with different key names instead of trying to clone the value.

@dougwilson
Copy link
Contributor Author

So, removing the (useless?) clone call at https://github.com/hapijs/qs/blob/master/lib/parse.js#L141 seem to fix this.

@dougwilson
Copy link
Contributor Author

Also, I verified that qs@0.6.6 does not have the issue. You may have accidentally been using the wrong version of qs when you were trying to test qs@0.6.6. Here is my output from testing the old 0.6.6 version:

$ npm install qs@0.6.6
qs@0.6.6 node_modules\qs
$ node -pe 'require("qs/package").version'
0.6.6
$ node -pe 'a={};a.b=a;require("qs").parse({"a[0]": "b", "a[1]": a})'
{ a: [ 'b', { b: [Circular] } ] }

so, as you can see, 0.6.6 did work with circular structures.

@dougwilson
Copy link
Contributor Author

Actually, it seems like qs@0.6.6 has the issue only with certain input, and not with other input, while the current qs just always has a range error. Weird...

@nlf
Copy link
Collaborator

nlf commented Aug 28, 2014

I think I have a fix regardless :) Give me just a few minutes

@dougwilson
Copy link
Contributor Author

haha, no problem. I'm just trying to figure out this weirdness, haha :)

@nlf nlf closed this as completed in 130bdc2 Aug 28, 2014
geek added a commit that referenced this issue Aug 28, 2014
account for circular references properly, closes #31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants