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: fix http-parser regression (v4.x) #5238

Closed
wants to merge 1 commit into from

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Feb 15, 2016

The new IS_HEADER_CHAR check in http-parser is improperly
checking char when it should be checking unsigned char.

/cc @ChALkeR

@jasnell jasnell added the http Issues or PRs related to the http subsystem. label Feb 15, 2016
@jasnell
Copy link
Member Author

jasnell commented Feb 15, 2016

/cc @thealphanerd @rvagg ... this will need to go into v4.3.1

Fixes http-parser regression with IS_HEADER_CHAR check
Add test case for obstext characters (> 0x80) is header
@jasnell jasnell force-pushed the fix-http-parser-regression-v4.x branch from 5f14f3f to fa6571c Compare February 15, 2016 17:46
@MylesBorins
Copy link
Contributor

@ChALkeR
Copy link
Member

ChALkeR commented Feb 15, 2016

LGTM

@MylesBorins
Copy link
Contributor

Only failure is infra related

@ChALkeR ChALkeR added the v4.x label Feb 15, 2016
@MylesBorins
Copy link
Contributor

LGTM

MylesBorins pushed a commit that referenced this pull request Feb 15, 2016
Fixes http-parser regression with IS_HEADER_CHAR check
Add test case for obstext characters (> 0x80) is header

PR-URL: #5238
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: Myles Borins <mborins@us.ibm.com>
@MylesBorins
Copy link
Contributor

Landed as 188cff3

@MylesBorins MylesBorins mentioned this pull request Feb 15, 2016
MylesBorins pushed a commit to MylesBorins/node that referenced this pull request Feb 16, 2016
Notable changes:

* buffer: make byteLength work with Buffer correctly (Jackson Tian)
  - nodejs#4738
* debugger: guard against call from non-node context (Ben Noordhuis)
  - nodejs#4328
* node_contextify: do not incept debug context (Myles Borins)
  - nodejs#4819
* deps: update to http-parser 2.5.2 (James Snell)
  - nodejs#5238
MylesBorins pushed a commit that referenced this pull request Feb 17, 2016
Notable changes:

* buffer: make byteLength work with Buffer correctly (Jackson Tian)
  - #4738
* debugger: guard against call from non-node context (Ben Noordhuis)
  - #4328
* node_contextify: do not incept debug context (Myles Borins)
  - #4819
* deps: update to http-parser 2.5.2 (James Snell)
  - #5238

PR-URL: #5200 (comment)
@tmont
Copy link

tmont commented Mar 3, 2016

This might help someone else, but this was the cause of a bunch of mystery nginx "upstream prematurely closed connection while reading response header from upstream" when proxying to node on v4.3.0. The proxied request bails so early that it appears nothing happens from the node side.

You can bind to HttpServer's clientError event to catch HTTP parser errors (or the underlying socket's error event).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
http Issues or PRs related to the http subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants