FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

src: introduce `http_parser_url_init` by indutny · Pull Request #225 · nodejs/http-parser · GitHub

This repository was archived by the owner on Nov 6, 2022. It is now read-only.
/ http-parser Public archive

src: introduce http_parser_url_init - #225

Closed
indutny wants to merge 1 commit into
nodejs:masterfrom
indutny:fix/gh-209
ConversationCommits (1)ChecksFiles changed
Closed

src: introduce http_parser_url_init#225
indutny wants to merge 1 commit into
nodejs:masterfrom
indutny:fix/gh-209

Conversation

indutny commented Feb 24, 2015

Copy link
Copy Markdown
Member

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

src: introduce http_parser_url_init
acd8bae 
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
indutny mentioned this pull request Feb 24, 2015
Closed

jay commented Feb 24, 2015

Copy link
Copy Markdown
Contributor

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 commented Feb 24, 2015

Copy link
Copy Markdown
Member Author

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 commented Feb 24, 2015

Copy link
Copy Markdown
Contributor

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 commented Feb 24, 2015

Copy link
Copy Markdown
Member Author

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 commented Feb 24, 2015

Copy link
Copy Markdown
Contributor

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 commented Feb 24, 2015

Copy link
Copy Markdown
Member Author

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

indutny commented Apr 23, 2015

Copy link
Copy Markdown
Member Author

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

OnixGH mentioned this pull request May 18, 2015
Closed

jasnell commented Oct 26, 2015

Copy link
Copy Markdown
Member

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

mscdex commented Oct 26, 2015

Copy link
Copy Markdown
Contributor

LGTM

indutny added a commit that referenced this pull request Oct 27, 2015
src: introduce http_parser_url_init
777ba4e 
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 <jasnell@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
PR-URL: #225

jasnell commented Oct 27, 2015

Copy link
Copy Markdown
Member

Landed in 777ba4e

jasnell closed this Oct 27, 2015
jasnell added a commit to jasnell/node that referenced this pull request Oct 28, 2015
deps: update http-parser to 2.6.0
9d7b698 
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 mentioned this pull request Oct 28, 2015
Closed
jasnell added a commit to nodejs/node that referenced this pull request Nov 3, 2015
deps: update http-parser to 2.6.0
d006d6a 
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 <mscdex@mscdex.net>
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
jasnell added a commit to nodejs/node that referenced this pull request Nov 7, 2015
deps: update http-parser to 2.6.0
7caeb14 
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 <mscdex@mscdex.net>
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

No one assigned

Labels

semver-minor

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

http_parser_parse_url crash when random string passed

4 participants

Back | FazBrowse Home | New Git URL