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

onHeadersComplete should return a number and crashes a debug node 4.8.3 #21

Closed
sdarnell opened this Issue Jun 2, 2017 · 15 comments

Comments

Projects
None yet
5 participants
@sdarnell

sdarnell commented Jun 2, 2017

My apologies for not having a an easy reproduction, but my guess is that it will be clear from the details below what the issue is.

I was testing an issue with Meteor (which uses faye-websocket) on node 4.8.3 running a debug version of node. The problem is that the node process dies on a debug check when parsing the headers: nodejs/node#13351

In the stack traces attached to that issue, the Parser::on_headers_complete_() calls header_response->IntegerValue(), and internally this asserts that the response is indeed a number (at least, it doesn't like undefined).

frame #5: 0x0000000100b0ff43 node`node::Parser::on_headers_complete_(this=0x0000000109790c00) at node_http_parser.cc:298
   295 	      return -1;
   296 	    }
   297 	
-> 298 	    return head_response->IntegerValue();
   299 	  }
   300 	
   301 	

If you look at the parserOnHeadersComplete() in node/_http_common.js it always returns a number:
https://github.com/nodejs/node/blob/v4.x/lib/_http_common.js#L100
But it appears that this routine replaced with the one in this package.

So, my conclusion is that the definition of header parsing in this websocket driver should do the same. https://github.com/faye/websocket-driver-node/blob/master/lib/websocket/http_parser.js#L32

There's possibly an argument that node (Parser::on_headers_complete_) should be more defensive given it is calling out, but that's probably independent.

@jcoglan

This comment has been minimized.

Collaborator

jcoglan commented Jun 4, 2017

@sdarnell Thanks for spotting this. Do you know what my implementation should return? I've no idea what the numeric return value is used for.

@sdarnell

This comment has been minimized.

sdarnell commented Jun 4, 2017

From the link above, the node implementation returns 1 if the body should be skipped, and 0 if not.
The comments suggest 0 should be returned for HEAD and CONNECT, but that UPGRADE may return 1 to delay some processing. At a guess, I suspect returning 0 is the right thing.
But the two implementations of the header processing are so different I'm not sure I can help much more than that.

Note that there's some detail/explanation in the readme: https://github.com/nodejs/http-parser
that talks about upgrade and the callbacks.

The code at https://github.com/nodejs/http-parser/blob/master/http_parser.c#L1760 also seems to accept 2 related to UPGRADE.

@bnoordhuis

This comment has been minimized.

bnoordhuis commented Jul 2, 2017

Node.js core received two bug reports about this in the last month. It would be nice if this got fixed because people are clearly running into it.

(I will also admit to an ulterior motive of not liking to spend a lot of time debugging issues that turn out to be bugs in third-party modules.)

@jcoglan

This comment has been minimized.

Collaborator

jcoglan commented Jul 2, 2017

@bnoordhuis Can you shed some light on what value my function should be returning in this case?

@bnoordhuis

This comment has been minimized.

bnoordhuis commented Jul 3, 2017

I'd rather not. Internals can change at any time. You shouldn't be touching them.

@jcoglan

This comment has been minimized.

Collaborator

jcoglan commented Jul 3, 2017

I appreciate that I'm taking a risk depending on internal APIs. I originally did this because I wasn't aware of a blessed public API in Node for parsing HTTP. Does one now exist?

If not, it would help me to close this issue and therefore stop annoying the node core team if you could provide even a little guidance on how this API is supposed to work. I fully understand that it's on me to maintain this code and I should not expect Node to guarantee its continued compatibility.

@bnoordhuis

This comment has been minimized.

bnoordhuis commented Jul 4, 2017

Not in core but you can use e.g. https://www.npmjs.com/package/http-parser-js. It has roughly the same API and is what I use in my own projects.

FWIW, I wrote most of the http parser in node and I don't even use it outside core. It's simply too dependent on version-specific implementation details.

@jcoglan

This comment has been minimized.

Collaborator

jcoglan commented Jul 30, 2017

I've now replaced this with http-parser-js, thanks @bnoordhuis. @sdarnell can you let me know if that fixes your problem?

@sdarnell

This comment has been minimized.

sdarnell commented Jul 30, 2017

Thanks @jcoglan, I have repeated the test. First checking that I got the same error (in a debug build), then updated the websocket driver and adding the http-parser-js package. With those changes everything seems fine and there's no crash in the debug build.
Many thanks!

@jcoglan

This comment has been minimized.

Collaborator

jcoglan commented Jul 30, 2017

@sdarnell Thanks for the quick response. I'll try to get this into a release as soon as possible.

@jcoglan jcoglan closed this Jul 30, 2017

@hwillson

This comment has been minimized.

hwillson commented Aug 1, 2017

@jcoglan Thanks for fixing this - any idea when you'll be publishing a new release? We're interested in getting these changes in Meteor.

@jcoglan

This comment has been minimized.

Collaborator

jcoglan commented Aug 1, 2017

@hwillson I'm afraid I'm not able to give guarantees about release dates. All the modules involved in faye-websocket for both Ruby and Node go through a long integration test run before all releases so I try to bundle many changes together so I'm not waiting on the build for too long in the limited time I have available.

benjamn added a commit to meteor/meteor that referenced this issue Aug 7, 2017

@abernix

This comment has been minimized.

abernix commented Aug 16, 2017

@jcoglan If at all possible, please ping this issue when you've cut a new release. Would be much appreciated! 😉

(also, apologies for the premature Cmd+Enter which sent this before I had finished typing it!)

@bnoordhuis bnoordhuis referenced this issue Aug 20, 2017

Closed

CITGM abuse #164

@jcoglan

This comment has been minimized.

Collaborator

jcoglan commented Sep 11, 2017

Hi everyone, these changes are now released in version 0.7.0. While trying to get this released, I ran into a security issue in permessage-deflate that took a while to sort out:

https://github.com/faye/permessage-deflate-node/wiki/Denial-of-service-caused-by-invalid-windowBits-parameter-passed-to-zlib.createDeflateRaw()

hwillson added a commit to hwillson/meteor that referenced this issue Sep 12, 2017

Remove ddp-client's direct websocket-driver dependency
A new version of the `websocket-driver` package has been released,
(0.7.0) that includes the fix discussed in
faye/websocket-driver-node#21, so the
direct `websocket-driver` dependency is no longer needed.

Relates to
meteor@43ba3c9.

benjamn added a commit to meteor/meteor that referenced this issue Sep 13, 2017

Remove ddp-client's direct websocket-driver dependency
A new version of the `websocket-driver` package has been released,
(0.7.0) that includes the fix discussed in
faye/websocket-driver-node#21, so the
direct `websocket-driver` dependency is no longer needed.

Relates to
43ba3c9.
@abernix

This comment has been minimized.

abernix commented Sep 20, 2017

Thanks, @jcoglan!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment