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

Trailing ? in URL silently fails #86

Closed
adam12 opened this issue Nov 13, 2019 · 4 comments
Closed

Trailing ? in URL silently fails #86

adam12 opened this issue Nov 13, 2019 · 4 comments
Labels

Comments

@adam12
Copy link
Contributor

adam12 commented Nov 13, 2019

Hi Bo,

Hope all is well.

I tried to debug and submit a patch for this but I'm not 100% sure where it's occurring in the source code.

Using an incredibly simple Rack application (listed below) as an example, requests to http://localhost:port/? (with the trailing ?) will fail silently, returning an empty reply. If the debugging is cranked up with -V 5 then the error is logged as DEBUG (http1.c:671): HTTP parser error..

I'm not sure if I'm even in the right area, but I dug into the internals and ended up inside the consume_request_line function. At a certain point, it consumes the path from the request and I wasn't able to see where it went. The on_query call is never reached as it returns on line 186.

if (args->on_path(args->parser, (char *)start, tmp - start))
return -1;
tmp = start = tmp + 1;
if (!seek2ch(&tmp, end, ' '))
return -1;
if (tmp - start > 0 &&
args->on_query(args->parser, (char *)start, tmp - start))
return -1;

Here's the example Rack application:

run ->(env) {
  [200, {}, ["Hello World"]]
}

Cheers.

Adam

@boazsegev
Copy link
Owner

boazsegev commented Nov 13, 2019

Hi @adam12 ,

Thank you for opening this issue. 🙏🏻

I can confirm that I can re-create the issue on my machine.

It seems an edge-case issue, never cropped up in any of my use cases - thanks for discovering it 👏🏻👏🏻🙏🏻

I'll mark this as a bug and look for a solution (though I'm not sure an empty query is legal 🤔, I'm certain it should be supported).

I'll keep you posted.

Kindly,
Bo.

boazsegev added a commit that referenced this issue Nov 13, 2019
@boazsegev
Copy link
Owner

boazsegev commented Nov 13, 2019

The issue was with the seek2ch implementation. The memchr based implementation would return the wrong value if the character was found on the starting position 😅

I'll wait for the CI and some tests before I release the fix.

@boazsegev
Copy link
Owner

Released version 0.7.37 with the fix. 🎉

Thanks again for opening this issue 👍🏻🙏🏻

@adam12
Copy link
Contributor Author

adam12 commented Nov 13, 2019

I was staring at the seek2ch implementation in gdb but I didn't see the issue.

Thanks for the quick fix! 💯 👍

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