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.

Sign up for GitHub

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

Jump to bottom

http_parser: fix error return in Finish() #24738

Closed
wants to merge 3 commits into from
Closed

http_parser: fix error return in Finish() #24738

wants to merge 3 commits into from

Conversation

indutny
Copy link
Member

@indutny indutny commented Nov 30, 2018 •
edited
Loading

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
Copy link
Collaborator

@indutny build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/1735/pipeline

@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 •
edited
Loading

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.

@indutny indutny mentioned this pull request Nov 30, 2018
Closed
@indutny
http: fix error return in Finish()
95bd7b2 
`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
@indutny indutny mentioned this pull request Nov 30, 2018
Closed
4 tasks
This was referenced Dec 1, 2018
Closed
Closed
@lpinca
Copy link
Member

lpinca commented Dec 2, 2018

CI: https://ci.nodejs.org/job/node-test-pull-request/19131/

@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
@indutny
http: fix error return in Finish()
175164e 
`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>
@joyeecheung joyeecheung mentioned this pull request Dec 5, 2018
Closed
BridgeAR pushed a commit that referenced this pull request Dec 5, 2018
@indutny @BridgeAR
http: fix error return in Finish()
3fe3bc9 
`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
Merged
4 tasks
This was referenced Dec 7, 2018
Closed
Closed
Closed
Closed
Closed
refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
@indutny @refack
http: fix error return in Finish()
1633365 
`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
@mscdex
http: fix error check in Execute()
7586e6b 
Refs: nodejs#24738
Fixes: nodejs#25858
This was referenced Feb 1, 2019
Closed
Closed
Closed
BethGriggs pushed a commit that referenced this pull request Feb 5, 2019
@mscdex @BethGriggs
http: fix error check in Execute()
741c5ef 
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
@mscdex @rvagg
http: fix error check in Execute()
b9204db 
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
http: fix error check in Execute()
4ceb467 
Refs: nodejs#24738
Fixes: nodejs#25858
mscdex added a commit to mscdex/io.js that referenced this pull request Mar 9, 2019
@mscdex
http: fix error check in Execute()
858004a 
Refs: nodejs#24738
Fixes: nodejs#25858
BethGriggs pushed a commit that referenced this pull request Mar 11, 2019
@mscdex @BethGriggs
http: fix error check in Execute()
8080a9b 
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
@mscdex @BethGriggs
http: fix error check in Execute()
ac9b8f7 
`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
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lpinca lpinca lpinca approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

HTTP/1 request parsing framing error event
5 participants
@indutny @nodejs-github-bot @lpinca @jasnell @BethGriggs