repl: handle unexpected error objects by cjihrig · Pull Request #12400 · nodejs/node

cjihrig · 2017-04-13T15:58:22Z

This commit allows the repl's domain error handler to process
unexpected error formats, such as primitives.

Fixes: #12373

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

repl

aqrln · 2017-04-13T16:29:21Z

It will fail if e is a Symbol, won't it?

refack · 2017-04-13T16:01:48Z

We haven't moved over to let completely yet.

refack · 2017-04-13T16:18:30Z

if there a chance e.stack != true || !('replace' in e.stack)?
Maybe a custom error:
function MySillyError(a, b, c) { console.log(a) } MySillyError.prototype = SyntaxError

I don't think we need to jump through hoops for extreme edge cases like this. As long as they don't crash.

Cool, just tested, no crash just TypeError: Function.prototype.toString is not generic

refack · 2017-04-13T16:30:40Z

how about if delete e.toString?

refack · 2017-04-13T16:32:18Z

add this monstrosity:
send: 'function MySillyError() {}; MySillyError.prototype = SyntaxError; var e = new MySillyError(); throw e',expect: /^TypeError:Function.prototype.toStringisnotgeneric/

No crash, but INHO it's a good test
also @aqrln's
send: 'throw Symbol.for',expect: /^functionfor(){[nativecode]}/

refack · 2017-04-13T16:34:33Z

@cjihrig you're not a first time contributor 😁 kudos on the commitment...

I added some horrible code to try and crash your cases...

cjihrig · 2017-04-13T17:17:21Z

Symbols and other bad inputs are taken care of here if I move to out = util.inspect(e);. However, it causes test-repl-sigint.js and test-repl-sigint-nested-eval.js to fail. @addaleax any idea why that would be?

that’s odd… I’ll have a look

@addaleax I just pushed up the changes that include that and more tests, if you want to take a look.

@cjihrig I think this might be the missing puzzle piece. ;)

Anyway I would "de-morgan" it to:
(!(einstanceofError)||typeofe.stack!=='string')
then add the original ~~gourd🎃~~ guard
(typeofe==='undefined'||!(einstanceofError)||typeofe.stack!=='string')

Thanks @addaleax. The special snowflake that is the REPL. I added a length check for stack, so errors with empty stacks will print with util.inspect().

refack · 2017-04-13T17:28:49Z

mo' tests less trouble?!

refack · 2017-04-13T17:31:07Z

You could add the extreme edge cases as tests

send: 'function MySillyError() {}; MySillyError.prototype = SyntaxError; var e = new MySillyError(); throw e',expect: /^TypeError:Function.prototype.toStringisnotgeneric/

also @aqrln's

send: 'throw Symbol.for',expect: /^functionfor(){[nativecode]}/

benjamingr

Changes LGTM and improve current status quo.

This commit allows the repl's domain error handler to process unexpected error formats, such as primitives.

refack · 2017-04-14T00:41:17Z

+ // Throws Object with bad toString() method and prints out
+ {
+ client: client_unix,
+ send: 'var e = { toString() { throw new Error(\'test\'); } }; throw e;',


Even smarter! 👍

TimothyGu · 2017-04-14T06:35:35Z

- if (e instanceof SyntaxError && e.stack) {
+
+ if (!((e instanceof Error) && typeof e.stack === 'string' &&
+ e.stack.length > 0)) {


Not sure how robust you want this code to be, but the typeof e.stack will crash with
functionFakeError(){}FakeError.prototype=Object.create(Error.prototype,{stack: {get(){thrownewError();}}});vare=newFakeError();

I don't know how others feel, but I'm OK with it, at least in the context of this PR. It seems like any Node application can be crashed with a well crafted getter.

One thing that I would (personally) prefer is either using V8’s IsNativeError over instanceof Error or going full 🦆 here. Both would increase robustness in different ways :)

Just noting that whatever is done here would need to factor in compatibility with internal/errors.js

Refs: nodejs#12400 PR-URL: nodejs#12546 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Refs: #12400 PR-URL: #12546 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

addaleax · 2017-05-26T19:29:27Z

@cjihrig Did you mean to close this?

cjihrig · 2017-06-06T17:21:17Z

Sorry @addaleax. Yes, I did. The move to internal/errors.js is going to prevent IsNativeError() from working correctly, and I didn't want to go full on duck typing.

TimothyGu · 2017-06-17T16:03:16Z

@cjihrig

The move to internal/errors.js is going to prevent IsNativeError() from working correctly

Can you elaborate? The Node.js error classes all extend from the native error classes, and generally V8's methods recognize subclasses.

cjihrig · 2017-06-19T00:15:14Z

Hm, good question. I tested this before, but maybe I did something wrong. process.binding('util').isNativeError() does appear to work with internal/errors.

nodejs-github-bot added the repl Issues and PRs related to the REPL subsystem. label Apr 13, 2017

aqrln reviewed Apr 13, 2017
View reviewed changes

Comment threadlib/repl.js Outdated
aqrlnApr 13, 2017
Copy link
Copy Markdown
Contributor
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will fail if e is a Symbol, won't it?
jasnell reacted with thumbs up emoji

refack suggested changes Apr 13, 2017
View reviewed changes

cjihrig commented Apr 13, 2017
View reviewed changes

refack approved these changes Apr 13, 2017
View reviewed changes

cjihrig force-pushed the repl branch from 6052da6 to 9828645Compare April 13, 2017 17:23

princejwesley approved these changes Apr 13, 2017
View reviewed changes

benjamingr approved these changes Apr 13, 2017
View reviewed changes

cjihrig force-pushed the repl branch from 9828645 to 5262d6dCompare April 14, 2017 00:37

repl: handle unexpected error objects
9a8dec0
This commit allows the repl's domain error handler to process unexpected error formats, such as primitives.

cjihrig force-pushed the repl branch from 5262d6d to 9a8dec0Compare April 14, 2017 00:39

refack reviewed Apr 14, 2017
View reviewed changes

refack approved these changes Apr 14, 2017
View reviewed changes

cjihrig added the semver-major PRs that contain breaking changes and should be released in the next major version. label Apr 14, 2017

refack force-pushed the master branch from 16073c0 to fbe946bCompare April 14, 2017 04:12

aqrln approved these changes Apr 14, 2017
View reviewed changes

TimothyGu reviewed Apr 14, 2017
View reviewed changes

cjihrig mentioned this pull request Apr 20, 2017
src: expose V8's IsNativeError() in util bindings #12546
Merged
3 tasks

cjihrig closed this May 17, 2017

bnoordhuis mentioned this pull request Jun 16, 2017
setTimeout(function(){throw null},0) crashes toplevel #12373
Closed

TimothyGu mentioned this pull request Jul 18, 2017
repl: handle null thrown #14306
Closed
3 tasks

Uh oh!

Uh oh!

Conversation

cjihrig commented Apr 13, 2017

Checklist

Affected core subsystem(s)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

refackApr 13, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

refack commented Apr 13, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

refackApr 13, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

refack commented Apr 13, 2017

Uh oh!

refack commented Apr 13, 2017

Uh oh!

benjamingr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

addaleax commented May 26, 2017

Uh oh!

cjihrig commented Jun 6, 2017

Uh oh!

TimothyGu commented Jun 17, 2017

Uh oh!

cjihrig commented Jun 19, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

refackApr 13, 2017•
edited
Loading

refackApr 13, 2017•
edited
Loading