[v12.x] http: fix crash for sync write errors during header parsing #34251
Conversation
This file contains 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
Fix a crash that occurs when `parser.finish()` is called during `parser.execute()`. In this particular case, this happened because a 100 continue response is a place in which `.end()` can be called which can in turn lead to a write error, which is emitted synchronously, thus inside the outer `parser.execute()` call. Resolve that by delaying the `parser.finish()` call until after the `parser.execute()` call is done. This only affects v12.x, because on later versions, errors are not emitted synchronously. Fixes: nodejs#15102
Since the tests only crash on v12.x, this commit adds separate regression tests. Refs: nodejs#15102
addaleax
added
http
|
@nodejs/http Can anybody review this? Only the first commit needs reviews, the remainder is just a backport of #34250 |
codebytere
force-pushed
the
v12.x-staging
branch
2 times, most recently
from
b64963e to
3b1d9b3
Compare
|
ping @nodejs/http |
Flarna
approved these changes
Aug 18, 2020
|
Can I ask what is left in the process to get this merged into 12.x, or if there is anything we can do to help? |
addaleax
force-pushed
the
v12.x-staging
branch
from
be41c83 to
efb5192
Compare
@lsr0 I don’t think there’s anything missing here, I’ll try to make sure it goes into the next release. |
github-actions
bot
removed
the
request-ci
addaleax
force-pushed
the
v12.x-staging
branch
from
efb5192 to
26f2d97
Compare
addaleax
added
request-ci
addaleax
force-pushed
the
v12.x-staging
branch
from
26f2d97 to
55fe022
Compare
github-actions
bot
removed
the
request-ci
addaleax
force-pushed
the
v12.x-staging
branch
from
55fe022 to
65b7bf4
Compare
github-actions
bot
removed
the
request-ci
|
Landed in 65b7bf4...867c451 |
addaleax
added a commit
that referenced
this pull request
Sep 22, 2020
Fix a crash that occurs when `parser.finish()` is called during `parser.execute()`. In this particular case, this happened because a 100 continue response is a place in which `.end()` can be called which can in turn lead to a write error, which is emitted synchronously, thus inside the outer `parser.execute()` call. Resolve that by delaying the `parser.finish()` call until after the `parser.execute()` call is done. This only affects v12.x, because on later versions, errors are not emitted synchronously. PR-URL: #34251 Fixes: #15102 Fixes: #34016 Reviewed-By: Fedor Indutny <[email protected]> Reviewed-By: Gerhard Stöbich <[email protected]>
addaleax
added a commit
that referenced
this pull request
Sep 22, 2020
Since the tests only crash on v12.x, this commit adds separate regression tests. PR-URL: #34251 Refs: #15102 Refs: #34016 Reviewed-By: Fedor Indutny <[email protected]> Reviewed-By: Gerhard Stöbich <[email protected]>
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
http: fix crash for sync write errors during header parsing
Fix a crash that occurs when
parser.finish()is called duringparser.execute(). In this particular case, this happened becausea 100 continue response is a place in which
.end()can be calledwhich can in turn lead to a write error, which is emitted
synchronously, thus inside the outer
parser.execute()call.Resolve that by delaying the
parser.finish()call until afterthe
parser.execute()call is done.This only affects v12.x, because on later versions, errors are not
emitted synchronously.
Fixes: #15102
Fixes: #34016
test: add regression tests for HTTP parser crash
Since the tests only crash on v12.x, this commit adds separate
regression tests.
Refs: #15102
Refs: #34016
[This is #34250, which this PR is blocked on.]
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes