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

http_parser: fix error return in Finish() #24738

Closed
wants to merge 3 commits into from

Conversation

indutny
Copy link
Member

@indutny indutny commented Nov 30, 2018

http_parser_execute(..., nullptr, 0) returns either 0 or 1. The
expectation is that no error must be returned if it is 0, and if
it is 1 - a Error object must be returned back to user.

The introduction of llhttp and the refactor that happened during it
accidentally removed the error-returning code. This commit reverts it
back to its original state.

Fix: #24585

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. http_parser Issues and PRs related to the HTTP Parser dependency or the http_parser binding. labels Nov 30, 2018
@indutny
Copy link
Member Author

indutny commented Nov 30, 2018

cc @nodejs/http

@indutny
Copy link
Member Author

indutny commented Nov 30, 2018

NOTE: there are few llhttp-specific fixes that I'd prefer to do separately. It is imperative to fix http_parser promptly, before diving into fixes for llhttp.

Update: No changes to llhttp needed, as it turned out today.

`http_parser_execute(..., nullptr, 0)` returns either `0` or `1`. The
expectation is that no error must be returned if it is `0`, and if
it is `1` - a `Error` object must be returned back to user.

The introduction of `llhttp` and the refactor that happened during it
accidentally removed the error-returning code. This commit reverts it
back to its original state.

Fix: nodejs#24585
@lpinca
Copy link
Member

lpinca commented Dec 2, 2018

@indutny
Copy link
Member Author

indutny commented Dec 2, 2018

Landed in 84aba432bf, thank you!

@indutny indutny closed this Dec 2, 2018
@indutny indutny deleted the fix/http-finish branch December 2, 2018 17:51
indutny added a commit that referenced this pull request Dec 2, 2018
`http_parser_execute(..., nullptr, 0)` returns either `0` or `1`. The
expectation is that no error must be returned if it is `0`, and if
it is `1` - a `Error` object must be returned back to user.

The introduction of `llhttp` and the refactor that happened during it
accidentally removed the error-returning code. This commit reverts it
back to its original state.

Fix: #24585
PR-URL: #24738
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
BridgeAR pushed a commit that referenced this pull request Dec 5, 2018
`http_parser_execute(..., nullptr, 0)` returns either `0` or `1`. The
expectation is that no error must be returned if it is `0`, and if
it is `1` - a `Error` object must be returned back to user.

The introduction of `llhttp` and the refactor that happened during it
accidentally removed the error-returning code. This commit reverts it
back to its original state.

Fix: #24585
PR-URL: #24738
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@BridgeAR BridgeAR mentioned this pull request Dec 5, 2018
4 tasks
refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
`http_parser_execute(..., nullptr, 0)` returns either `0` or `1`. The
expectation is that no error must be returned if it is `0`, and if
it is `1` - a `Error` object must be returned back to user.

The introduction of `llhttp` and the refactor that happened during it
accidentally removed the error-returning code. This commit reverts it
back to its original state.

Fix: nodejs#24585
PR-URL: nodejs#24738
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
mscdex added a commit to mscdex/io.js that referenced this pull request Feb 1, 2019
BethGriggs pushed a commit that referenced this pull request Feb 5, 2019
Refs: #24738
Fixes: #25858

PR-URL: #25863
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
rvagg pushed a commit that referenced this pull request Feb 28, 2019
Refs: #24738
Fixes: #25858

PR-URL: #25863
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
mscdex added a commit to mscdex/io.js that referenced this pull request Feb 28, 2019
mscdex added a commit to mscdex/io.js that referenced this pull request Mar 9, 2019
BethGriggs pushed a commit that referenced this pull request Mar 11, 2019
Refs: #24738
Fixes: #25858

PR-URL: #25939
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Beth Griggs <Bethany.Griggs@uk.ibm.com>
BethGriggs pushed a commit that referenced this pull request Mar 20, 2019
`http_parser_execute(..., nullptr, 0)` returns either `0` or `1`. The
expectation is that no error must be returned if it is `0`, and if
it is `1` - a `Error` object must be returned back to user.

The introduction of `llhttp` and the refactor that happened during it
accidentally removed the error-returning code. This commit reverts it
back to its original state.

Backport-PR-URL: #25938
Fix: #24585
PR-URL: #24738
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Mar 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. http_parser Issues and PRs related to the HTTP Parser dependency or the http_parser binding.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

HTTP/1 request parsing framing error event
5 participants