Skip to content
This repository has been archived by the owner on Nov 6, 2022. It is now read-only.

src: introduce http_parser_url_init #225

Closed
wants to merge 1 commit into from

Conversation

indutny
Copy link
Member

@indutny indutny commented Feb 24, 2015

The struct must be zero-initialized, but this wasn't explicitly stated
anywhere in headers. Introduce http_parser_url_init API method that
will do it.

Fix #209

cc @bnoordhuis

The struct must be zero-initialized, but this wasn't explicitly stated
anywhere in headers. Introduce `http_parser_url_init` API method that
will do it.

Fix nodejs#209
@jay
Copy link
Contributor

jay commented Feb 24, 2015

My pr at #222 is more comprehensive, I'm not sure why you closed it as unnecessary. I added the parameter checking and zeroed the structure. In http_parse_host there's no point continuing without the host field and in http_parser_parse_url there's no point calling the former without the host field. Also something should be done to make sure that the buflen isn't greater than max uint16_t, which I covered in both.

Also it occurs to me now an overflow check is also due for size_t buflen assignment in http_parse_host since there's no guarantee size_t is greater than uint16_t (although it's extremely likely), which would probably look something like this, placed immediately after the assignment:

if(buflen < u->field_data[UF_HOST].off) { // overflow
    return 1;
}

@indutny
Copy link
Member Author

indutny commented Feb 24, 2015

It won't call it without host field, because UF_HOST won't be set. In case of test string it is doing a right thing and treating test as a schema, and then awaits for the hostname.

@jay
Copy link
Contributor

jay commented Feb 24, 2015

In the case a schema field was found but a host field was not then http_parser_parse_url will still call http_parse_host. I changed that by failing if a schema field is found without a host field and calling http_parse_host only if a host field was found as seen here.

@indutny
Copy link
Member Author

indutny commented Feb 24, 2015

Right and http_parse_host is called because it is what is expected after the scheme. It is intentionally invoked for this thing to fail. Practically, is there any difference in behavior your PR and current implementation (in master branch)?

@jay
Copy link
Contributor

jay commented Feb 24, 2015

Yes it's different. The current implementation is calling http_parse_host when UF_HOST is not set which contradicts what you said earlier. Whether that's correct or not you would know having more familiarity with the code base, however I did not think that was correct.

@indutny
Copy link
Member Author

indutny commented Feb 24, 2015

Right, I was wrong about it. Sorry. However, except this I don't see any behavior difference.

@indutny
Copy link
Member Author

indutny commented Apr 23, 2015

@mscdex may I ask you to review it? ;)

@jasnell
Copy link
Member

jasnell commented Oct 26, 2015

@indutny @mscdex ... any further thoughts on this one?

@mscdex
Copy link
Contributor

mscdex commented Oct 26, 2015

LGTM

indutny added a commit that referenced this pull request Oct 27, 2015
The struct must be zero-initialized, but this wasn't explicitly stated
anywhere in headers. Introduce `http_parser_url_init` API method that
will do it.

Fixes: #209
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Brian White <[email protected]>
PR-URL: #225
@jasnell
Copy link
Member

jasnell commented Oct 27, 2015

Landed in 777ba4e

@jasnell jasnell closed this Oct 27, 2015
jasnell added a commit to jasnell/node that referenced this pull request Oct 28, 2015
significant updates:

* [[`777ba4eded`](nodejs@777ba4eded)] - **src**: introduce `http_parser_url_init` (Fedor Indutny) [nodejs/http-parser#225](nodejs/http-parser#225)
* [[`e557b62744`](nodejs@e557b62744)] - **src**: support LINK/UNLINK (RFC 2068, draft-snell-link-method) (Olivier Mengué) [nodejs/http-parser#267](nodejs/http-parser#267)
* [[`eb5e9928b4`](nodejs@eb5e9928b4)] - **src**: support ACL (WebDAV, RFC3744, Section 8.1). (Ivan Enderlin) [nodejs/http-parser#260](nodejs/http-parser#260)
* [[`8b1d652322`](nodejs@8b1d652322)] - **src**: support BIND/REBIND/UNBIND (WebDAV, RFC5842) (Ivan Enderlin) [nodejs/http-parser#242](nodejs/http-parser#242)
* [[`7d75dd7325`](nodejs@7d75dd7325)] - **src**: support IPv6 Zone ID as per RFC 6874 (Tatsuhiro Tsujikawa) [nodejs/http-parser#253](nodejs/http-parser#253)
jasnell added a commit to nodejs/node that referenced this pull request Nov 3, 2015
significant updates:

* [[`777ba4eded`](777ba4eded)] - **src**: introduce `http_parser_url_init` (Fedor Indutny) [nodejs/http-parser#225](nodejs/http-parser#225)
* [[`e557b62744`](e557b62744)] - **src**: support LINK/UNLINK (RFC 2068, draft-snell-link-method) (Olivier Mengué) [nodejs/http-parser#267](nodejs/http-parser#267)
* [[`eb5e9928b4`](eb5e9928b4)] - **src**: support ACL (WebDAV, RFC3744, Section 8.1). (Ivan Enderlin) [nodejs/http-parser#260](nodejs/http-parser#260)
* [[`8b1d652322`](8b1d652322)] - **src**: support BIND/REBIND/UNBIND (WebDAV, RFC5842) (Ivan Enderlin) [nodejs/http-parser#242](nodejs/http-parser#242)
* [[`7d75dd7325`](7d75dd7325)] - **src**: support IPv6 Zone ID as per RFC 6874 (Tatsuhiro Tsujikawa) [nodejs/http-parser#253](nodejs/http-parser#253)

PR-URL: #3569
Reviewed-By: Brian White <[email protected]>
Reviewed-By: Johan Bergström <[email protected]>
jasnell added a commit to nodejs/node that referenced this pull request Nov 7, 2015
significant updates:

* [[`777ba4eded`](777ba4eded)] - **src**: introduce `http_parser_url_init` (Fedor Indutny) [nodejs/http-parser#225](nodejs/http-parser#225)
* [[`e557b62744`](e557b62744)] - **src**: support LINK/UNLINK (RFC 2068, draft-snell-link-method) (Olivier Mengué) [nodejs/http-parser#267](nodejs/http-parser#267)
* [[`eb5e9928b4`](eb5e9928b4)] - **src**: support ACL (WebDAV, RFC3744, Section 8.1). (Ivan Enderlin) [nodejs/http-parser#260](nodejs/http-parser#260)
* [[`8b1d652322`](8b1d652322)] - **src**: support BIND/REBIND/UNBIND (WebDAV, RFC5842) (Ivan Enderlin) [nodejs/http-parser#242](nodejs/http-parser#242)
* [[`7d75dd7325`](7d75dd7325)] - **src**: support IPv6 Zone ID as per RFC 6874 (Tatsuhiro Tsujikawa) [nodejs/http-parser#253](nodejs/http-parser#253)

PR-URL: #3569
Reviewed-By: Brian White <[email protected]>
Reviewed-By: Johan Bergström <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

http_parser_parse_url crash when random string passed
4 participants