Skip to content

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: Сковорода Никита Андреевич <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
@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