http_parser: assert on incoming argument's type #12288

gireeshpunathil · 2017-04-09T10:35:02Z

Unchecked argument conversion in Parser::Consume crashes node
in an slightly undesirable manner - 'unreachable code' in parser.

Make sure we validate the incoming type at the earliest point.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

http_parser

gireeshpunathil · 2017-04-09T10:35:50Z

Reference: #12178

bnoordhuis · 2017-04-09T10:38:43Z

src/node_http_parser.cc

+        String::NewFromUtf8(parser->env()->isolate(),
+        "Object conversion of argument failed.")));
+      return;
+    }


Why not a simple CHECK(args[0]->IsExternal())? If you want to throw an exception, don't reinvent the wheel and use parser->env()->ThrowError("error message").

out/Release/node[8983]: ../src/node_http_parser.cc:486:static void node::Parser::Consume(const FunctionCallbackInfo<v8::Value> &): Assertion `args[0]->IsExternal()' failed. ...

With the CHECK macro, the failure is a simple assertion as above (with callstack) and the message is not very intuitive and hence I went with a throw.

Regarding throw: made the changes are suggested.

Hope this is fine by you?

@gireeshpunathil FWIW, you can use a message with a CHECK assertion (though one may qualify it as a little hack). A common pattern used in some parts of the codebase is CHECK(expr && "message").

cjihrig

Can you add a test please. The current changes LGTM though.

aqrln · 2017-04-09T18:11:41Z

@gireeshpunathil do you mind adding the Fixes: https://github.com/nodejs/node/issues/12178 field to the commit message? Thanks!

bnoordhuis · 2017-04-09T19:15:22Z

src/node_http_parser.cc

+    // Argument coming from JS land with unchecked type.
+    // Make sure it is a valid external object.
+    if (!args[0]->IsExternal()) {
+      parser->env()->ThrowError("Object conversion of argument failed.");


The comment is superfluous and the error message is misleading: no conversion is attempted, it's just a type check.

I'd simply turn it into a CHECK and even then it's not all that useful; a few lines below we cast the wrapped value to a StreamBase without ever checking if it's actually a StreamBase instance.

@bnoordhuis - done, followed your suggestion.

gireeshpunathil · 2017-04-10T05:23:17Z

@aqrln - thanks, added the Fixes: tag into the commit message.
Regarding the CHECK (expr && msg) semantics: I found it less useful - this is how the output appears:

out/Release/node[2219]: ../src/node_http_parser.cc:476:static void node::Parser::Consume(const FunctionCallbackInfo<v8::Value> &): Assertion `args[0]->IsExternal() && "parser.consume requires a stream argument."' failed.
 1: node::Abort() [./node]
...

gireeshpunathil · 2017-04-10T05:30:46Z

@cjihrig - thanks. I have this test program (sort of a regression test) devised - based on the Exception throwing logic, but @bnoordhuis 's latest suggestion (use of CHECK) would mean that we would assert in C++ land on failure, and managing / validating that in a JS test case seems hacky. Any suggestions?

#cat test/parallel/test-http-parser-consume.js
'use strict';
const common = require('../common');
const assert = require('assert');
const http = require('http');
const spawn = require('child_process').spawn;

if (process.argv[2] === 'child') {
  var server = http.createServer(function(q, s) {
    s.end('hello');
  });

  server.listen(common.PORT, function(s) {
    var rr = http.get('http://localhost:' + common.PORT, function(d) {
      try {
        rr.parser.consume(0);
      }
      catch(e) {
        e.toString().includes('parser.consume requires a stream argument.');
        process.exit(0);
      }
    });
  });
}
else {
  const child = spawn(process.argv[0], [process.argv[1], 'child']);
  child.on('stdout', function(d) {
    console.log(d.toString());
  });
  child.on('stderr', function(d) {
    console.log(d.toString());
  });
  child.on('exit', common.mustCall(function(code, signal) {
    assert(signal === null && code === 0, 'Parser::Consume should not crash the process.');
  }));
}

aqrln · 2017-04-10T10:27:36Z

src/node_http_parser.cc

@@ -472,6 +472,9 @@ class Parser : public AsyncWrap {
  static void Consume(const FunctionCallbackInfo<Value>& args) {
    Parser* parser;
    ASSIGN_OR_RETURN_UNWRAP(&parser, args.Holder());
+
+    CHECK(args[0]->IsExternal());
+


Nit: I'd remove this empty line.

@aqrln - done

I was only talking about removing the empty line I left the comment at, not both of them. The idea was to make everything related to extracting stream from args one visual block, if my comment was too short to be clear :)
You are not obliged to do that though, and I'm fine with both of your variants too, that's just my personal preference you may or may not agree with.

@aqrln - sure, understood.

gireeshpunathil · 2017-04-10T14:43:20Z

@cjihrig - Added a test case to capture this. As it stands out, the evolved change in the parser code means that the process now ABRTs as opposed to a SEGV in response to bad input, and the test is made to validate this fact only.

cjihrig · 2017-04-10T16:58:57Z

test/parallel/test-http-parser-consume.js

+const spawn = require('child_process').spawn;
+
+if (process.argv[2] === 'child') {
+  var server = http.createServer(function(q, s) {


Can you use const here.

Can you also use more descriptive names than q and s.

@cjihrig - done, followed your suggestions.

cjihrig · 2017-04-10T17:02:22Z

test/parallel/test-http-parser-consume.js

+    s.end('hello');
+  });
+
+  server.listen(common.PORT, function(s) {


Instead of using common.PORT, you can just request a port from the operating system. Also, please wrap the callback in common.mustCall().

cjihrig · 2017-04-10T17:03:05Z

test/parallel/test-http-parser-consume.js

+  });
+
+  server.listen(common.PORT, function(s) {
+    var rr = http.get('http://localhost:' + common.PORT, function(d) {


Could the first parameter just be { port: server.address().port }. Also, please use const.

cjihrig · 2017-04-10T17:03:45Z

test/parallel/test-http-parser-consume.js

+  server.listen(common.PORT, function(s) {
+    var rr = http.get('http://localhost:' + common.PORT, function(d) {
+      rr.parser.consume(0);
+      process.exit(0);


Can you close the server to let the process exit naturally.

cjihrig · 2017-04-10T17:03:59Z

test/parallel/test-http-parser-consume.js

+    });
+  });
+}
+else {


Style nit: else should go on the previous line.

cjihrig · 2017-04-10T17:04:48Z

test/parallel/test-http-parser-consume.js

+}
+else {
+  const child = spawn(process.argv[0], [process.argv[1], 'child']);
+  child.on('stdout', function(d) {


These can be removed right?

And even if you do need these for some reason, I think a better way to do that is

child.stdout.pipe(process.stdout); child.stderr.pipe(process.stderr);

EDIT: or, even better than that, pass { stdio: 'inherit' } to spawn().

I don't think child.on('stdout', ...) is even a thing.

Yeah, I don't recall those events too.

inheriting stdio is the right way

Sorry, don't know how I picked up 'stdout' as an event. Removed them and inherited from parent.

Debugging issues and isolating them between test issues and node issues have been effortful, so please allow me to retain the child output, if it is fine by you?

cjihrig · 2017-04-10T17:05:17Z

test/parallel/test-http-parser-consume.js

+  });
+  child.on('exit', common.mustCall(function(code, signal) {
+    assert(signal === 'SIGABRT', 
+          'parser.consume should not crash, but ended with ' + signal);


Please move this over one more space.

DemiMarie · 2017-04-10T20:56:11Z

Why is crashing with SIGABRT okay? I would expect that this should throw a proper JS exception.

addaleax · 2017-04-10T21:01:50Z

Why is crashing with SIGABRT okay? I would expect that this should throw a proper JS exception.

I think the reasoning of @bnoordhuis (and maybe others) here is that this kind of crash can only be produced by messing with or accessing what Node.js would consider its internals, here by using .parser.

I talked about this again with @bengl a few days ago, and while we agreed that we would like to see a more un-crashable Node, we also agreed that Ben has a good and valid point here.

cjihrig

One more nit in the test, but LGTM if the CI passes.

cjihrig · 2017-04-11T13:35:07Z

test/parallel/test-http-parser-consume.js

+    res.end('hello');
+  });
+
+  server.listen(0, function(s) {


This needs common.mustCall() on the callback.

made this change, thanks @cjihrig

aqrln · 2017-04-11T16:27:21Z

test/parallel/test-http-parser-consume.js

+  });
+
+  server.listen(0, common.mustCall(function(s) {
+    const rr = http.get({ port: server.address().port }, common.mustCall(function(d) {


This line is longer than 80 characters.

sliced it, thanks.

aqrln · 2017-04-11T16:27:30Z

test/parallel/test-http-parser-consume.js

+    }));
+  }));
+} else {
+  const child = spawn(process.argv[0], [process.argv[1], 'child'], { stdio: 'inherit' });


jasnell · 2017-04-12T10:39:01Z

what would the semver level be on this? Should we treat this as a major?

addaleax · 2017-04-12T11:33:49Z

what would the semver level be on this? Should we treat this as a major?

I’d say it’s semver-patch at most (semver-doesntreallymatteranyway if we’re being honest). It turns one kind of hard crash into another, and practically speaking it’s not a bug that gets triggered unless you are actively looking into how to crash node.

aqrln

LGTM. Now that the PR performs a CHECK() instead of throwing an exception like it did initially, I'm not sure that it fixes #12178 in a way one would expect it to, so, arguably, it's worth changing "Fixes:" to "Refs:". That said, I'm +1 for this change.

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

aqrln · 2017-04-12T12:16:02Z

test/parallel/test-http-parser-consume.js

+const spawn = require('child_process').spawn;
+
+if (process.argv[2] === 'child') {
+  const server = http.createServer(function(req, res) {


Nit: why not make these callbacks arrow functions?

done, all the callbacks are arrow'ed.

aqrln · 2017-04-12T12:16:24Z

test/parallel/test-http-parser-consume.js

+  server.listen(0, common.mustCall(function(s) {
+    const rr = http.get({ port: server.address().port },
+                        common.mustCall(function(d) {
+      rr.parser.consume(0);


Not sure about the indentation here.

aqrln

node-test-linter on CI doesn't seem to be happy. Can you please fix the linter errors?

jasnell

I'm good with semver-patch. LGTM with the linting issue fixed.

gireeshpunathil · 2017-06-04T05:46:03Z

all clear - a CI please!
/cc @nodejs/http
/cc @nodejs/platform-windows

santigimeno · 2017-06-04T07:28:41Z

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

gibfahn · 2017-06-04T10:30:09Z

cc/ @nodejs/http
cc/ @nodejs/platform-windows

Duplicating cc/ as I don't think it works if you're not a collaborator (which is a bit of a pain).

gireeshpunathil · 2017-06-04T11:09:08Z

o! I never knew - thanks @gibfahn for that. I would have (and had) spent a lot of cc/ stuff thinking that someone will get a mail.

refack · 2017-06-04T14:12:49Z

Duplicating cc/ as I don't think it works if you're not a collaborator (which is a bit of a pain).

fullish GitHub 🤦‍♂️

refack · 2017-06-04T14:19:22Z

o! I never knew - thanks @gibfahn for that. I would have (and had) spent a lot of cc/ stuff thinking that someone will get a mail.

I think the limitation it's just for @nodejs/ teams. This and a inability to assign issues to everyone, IMHO very foolish.

refack

2 nits

refack · 2017-06-04T19:01:27Z

test/abort/test-http-parser-consume.js

+  }));
+
+  server.listen(0, common.mustCall((s) => {
+    const rr = http.get({ port: server.address().port },


I know people have asked for a 1e6 changes... but, would you reindent with both arguments in a new line (nececitating less whitespace):

server.listen(0, common.mustCall((s) => { const rr = http.get( { port: server.address().port }, common.mustCall((d) => { rr.parser.consume(0); server.close(); })); }) );

I'm fine if you don't want to.

@refack - thanks, done

refack · 2017-06-04T19:33:54Z

test/abort/test-http-parser-consume.js

+  server.listen(0, common.mustCall((s) => {
+    const rr = http.get({ port: server.address().port },
+                        common.mustCall((d) => {
+                          rr.parser.consume(0);


Nit: Add comment that this line should abort the process

added a comment to explain the code

Unchecked argument conversion in Parser::Consume crashes node in an slightly undesirable manner - 'unreachable code' in parser. Make sure we validate the incoming type at the earliest point. Refs: nodejs#12178

refack · 2017-06-05T16:45:00Z

Pre-land CI: https://ci.nodejs.org/job/node-test-commit/10368/
@gireeshpunathil did you address all comments (I couldn't find @sam-github's)?

sam-github · 2017-06-05T16:51:37Z

I can't find mine either, ignore me please.

gibfahn · 2017-06-05T16:52:08Z

(I couldn't find @sam-github's)?

#12288 (comment)

Unchecked argument conversion in Parser::Consume crashes node in an slightly undesirable manner - 'unreachable code' in parser. Make sure we validate the incoming type at the earliest point. PR-URL: nodejs#12288 Fixes: nodejs#12178 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Alexey Orlenko <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Santiago Gimeno <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>

refack · 2017-06-05T21:15:25Z

landed in efab784

refack · 2017-06-05T21:18:06Z

extra CI to check master's sanity: https://ci.nodejs.org/job/node-test-commit-linuxone/6400/

Unchecked argument conversion in Parser::Consume crashes node in an slightly undesirable manner - 'unreachable code' in parser. Make sure we validate the incoming type at the earliest point. PR-URL: #12288 Fixes: #12178 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Alexey Orlenko <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Santiago Gimeno <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>

MylesBorins · 2017-07-17T17:46:56Z

Should this land on v6.x?

MylesBorins · 2017-08-14T18:49:07Z

ping

addaleax · 2017-08-14T18:51:41Z

@MylesBorins yes.

Unchecked argument conversion in Parser::Consume crashes node in an slightly undesirable manner - 'unreachable code' in parser. Make sure we validate the incoming type at the earliest point. PR-URL: #12288 Fixes: #12178 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Alexey Orlenko <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Santiago Gimeno <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>

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 Apr 9, 2017

bnoordhuis requested changes Apr 9, 2017

View reviewed changes

gireeshpunathil force-pushed the parsertype branch from 1e0305a to b403883 Compare April 9, 2017 10:54

addaleax approved these changes Apr 9, 2017

View reviewed changes

cjihrig requested changes Apr 9, 2017

View reviewed changes

bnoordhuis requested changes Apr 9, 2017

View reviewed changes

gireeshpunathil force-pushed the parsertype branch from b403883 to 54cdc1c Compare April 10, 2017 05:13

aqrln reviewed Apr 10, 2017

View reviewed changes

gireeshpunathil force-pushed the parsertype branch 2 times, most recently from 15793fc to 86e18ec Compare April 10, 2017 14:37

cjihrig requested changes Apr 10, 2017

View reviewed changes

gireeshpunathil force-pushed the parsertype branch from 86e18ec to 698cae1 Compare April 11, 2017 04:10

cjihrig approved these changes Apr 11, 2017

View reviewed changes

gireeshpunathil force-pushed the parsertype branch from 698cae1 to 321369e Compare April 11, 2017 15:43

aqrln reviewed Apr 11, 2017

View reviewed changes

gireeshpunathil force-pushed the parsertype branch from 321369e to 3ab6774 Compare April 12, 2017 03:22

aqrln approved these changes Apr 12, 2017

View reviewed changes

aqrln suggested changes Apr 12, 2017

View reviewed changes

jasnell approved these changes Apr 12, 2017

View reviewed changes

bnoordhuis approved these changes Jun 3, 2017

View reviewed changes

tniessen approved these changes Jun 4, 2017

View reviewed changes

refack self-assigned this Jun 4, 2017

refack approved these changes Jun 4, 2017

View reviewed changes

http_parser: assert on incoming argument's type

e340d28

Unchecked argument conversion in Parser::Consume crashes node in an slightly undesirable manner - 'unreachable code' in parser. Make sure we validate the incoming type at the earliest point. Refs: nodejs#12178

gireeshpunathil force-pushed the parsertype branch from 6300f18 to e340d28 Compare June 5, 2017 08:33

refack closed this Jun 5, 2017

gibfahn mentioned this pull request Jun 15, 2017

Auditing for 6.11.1 nodejs/Release#230

Closed

3 tasks

MylesBorins added the lts-watch-v6.x label Jul 17, 2017

MylesBorins added land-on-v6.x and removed lts-watch-v6.x labels Sep 19, 2017

MylesBorins mentioned this pull request Sep 20, 2017

v6.11.4 proposal #15506

Merged

refack removed their assignment Oct 20, 2018

http_parser: assert on incoming argument's type #12288

http_parser: assert on incoming argument's type #12288

Conversation

gireeshpunathil commented Apr 9, 2017

Checklist

Affected core subsystem(s)

gireeshpunathil commented Apr 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjihrig left a comment

Choose a reason for hiding this comment

aqrln commented Apr 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gireeshpunathil commented Apr 10, 2017

gireeshpunathil commented Apr 10, 2017 • edited by gibfahn Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gireeshpunathil commented Apr 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aqrln Apr 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DemiMarie commented Apr 10, 2017

addaleax commented Apr 10, 2017

cjihrig left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasnell commented Apr 12, 2017

addaleax commented Apr 12, 2017

aqrln left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aqrln left a comment

Choose a reason for hiding this comment

jasnell left a comment

Choose a reason for hiding this comment

gireeshpunathil commented Jun 4, 2017

santigimeno commented Jun 4, 2017

gibfahn commented Jun 4, 2017

gireeshpunathil commented Jun 4, 2017

refack commented Jun 4, 2017

refack commented Jun 4, 2017

refack left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

refack commented Jun 5, 2017

sam-github commented Jun 5, 2017

gibfahn commented Jun 5, 2017

refack commented Jun 5, 2017

gireeshpunathil commented Apr 10, 2017 •

edited by gibfahn

Loading

aqrln Apr 10, 2017 •

edited

Loading