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

QueryStringParser is not stack safe #147

Closed
rossabaker opened this issue Dec 22, 2014 · 11 comments
Closed

QueryStringParser is not stack safe #147

rossabaker opened this issue Dec 22, 2014 · 11 comments
Assignees
Labels
bug
Milestone

Comments

@rossabaker
Copy link
Member

@rossabaker rossabaker commented Dec 22, 2014

This fails:

    "be stack safe" in {
      val value = Stream.continually('X').take(1000000).mkString
      val query = s"little=x&big=${value}"
      parseQueryString(query) must beRightDisjunction(Seq("little" -> "x", "big" -> Some(value)))
    }
@rossabaker rossabaker added the bug label Dec 22, 2014
@rossabaker rossabaker added this to the 0.5.1 milestone Dec 22, 2014
@bryce-anderson
Copy link
Member

@bryce-anderson bryce-anderson commented Dec 22, 2014

I'm curious how you stumbled across this. On my system, I can do a query value up to 25K characters long. That is well past the default request line limits of pretty much all servers (for example, blaze is 10KB, Apache is 8190 bytes.

I suspect this will be a recurring problem using parboiled2. I'm not convinced its a realistic problem in any situation besides perhaps a client side session cookie, but even that might be a stretch.

@bryce-anderson
Copy link
Member

@bryce-anderson bryce-anderson commented Dec 22, 2014

This may be of some help.
sirthias/parboiled2#61

@rossabaker
Copy link
Member Author

@rossabaker rossabaker commented Dec 22, 2014

Well, @julien-truffaut found it's a realistic problem. :)

@rossabaker
Copy link
Member Author

@rossabaker rossabaker commented Dec 22, 2014

It's because this parser is also used for form encoding. This, unlike query strings, can grow quite large.

@julien-truffaut
Copy link
Member

@julien-truffaut julien-truffaut commented Dec 22, 2014

yeah I stumbled on that issue in my job today. We might are misusing url form, I wasn't aware of any payload limit.

@rossabaker
Copy link
Member Author

@rossabaker rossabaker commented Dec 22, 2014

There's typically a limit to query strings. Forms can be enormous.
On Dec 22, 2014 1:55 PM, "Julien Truffaut" notifications@github.com wrote:

yeah I stumbled on that issue in my job today. We might are misusing url
form, I wasn't aware of any payload limit.

Reply to this email directly or view it on GitHub
#147 (comment).

@bryce-anderson
Copy link
Member

@bryce-anderson bryce-anderson commented Dec 22, 2014

You're right, it is unsuitable for form duty. Hopefully we can figure out why we're not stack safe with parboiled2 or find some other solution. If its going to be used for parsing large bodies, it may be worth it to make a hand tuned parser.

@rossabaker
Copy link
Member Author

@rossabaker rossabaker commented Dec 22, 2014

@bryce-anderson, if you don't want it, throw it back, and I'll take it.

@bryce-anderson
Copy link
Member

@bryce-anderson bryce-anderson commented Dec 22, 2014

I've think I've got this under control.

On Mon, Dec 22, 2014 at 4:51 PM, Ross A. Baker notifications@github.com
wrote:

@bryce-anderson https://github.com/bryce-anderson, if you don't want
it, throw it back, and I'll take it.


Reply to this email directly or view it on GitHub
#147 (comment).

bryce-anderson added a commit that referenced this issue Dec 23, 2014
fixes #147
rossabaker added a commit that referenced this issue Dec 23, 2014
rossabaker added a commit that referenced this issue Dec 23, 2014
rossabaker added a commit that referenced this issue Dec 23, 2014
@rossabaker
Copy link
Member Author

@rossabaker rossabaker commented Dec 23, 2014

@julien-truffaut : Is it easy for you to test with a snapshot? If so, give 0.5.1-SNAPSHOT a try, and if it fixes your issue, I'll release it as v0.5.1 tomorrow.

rossabaker added a commit that referenced this issue Dec 23, 2014
@julien-truffaut
Copy link
Member

@julien-truffaut julien-truffaut commented Dec 23, 2014

I checked, it fixed the stack over flow. Thanks Ross

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants
You can’t perform that action at this time.