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

node_api: add napi_fatal_exception #19337

Closed
wants to merge 6 commits into from

Conversation

mafintosh
Copy link
Member

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

Adds void napi_fatal_exception(napi_env env) which triggers an node::FatalException.
Currently there is no way of doing this when using async callbacks, which makes error handling quite tricky

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Mar 14, 2018
@mafintosh mafintosh force-pushed the napi_fatal_exception branch from a5ede72 to 7829f9c Compare March 14, 2018 02:48
@mafintosh mafintosh changed the title add napi_fatal_exception node_api: add napi_fatal_exception Mar 14, 2018
Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Does this supersede #19336?

@mafintosh
Copy link
Member Author

@cjihrig ya builds on that one

Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’m not sure – the “tests and/or benchmarks are included” checkbox shouldn’t be checked, should it?

doc/api/n-api.md Outdated
@@ -541,6 +541,19 @@ This API returns true if an exception is pending.

This API can be called even if there is a pending JavaScript exception.

#### napi_fatal_exception
<!-- YAML
added: TBA
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

REPLACEME is the magic word that gets picked up by the release tooling, please change to that :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice! i was offline and trying hard to remember what it was

src/node_api.h Outdated
@@ -112,6 +112,8 @@ NAPI_EXTERN napi_status
napi_get_last_error_info(napi_env env,
const napi_extended_error_info** result);

NAPI_EXTERN void napi_fatal_exception(napi_env env);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I would prefer it if you could pass in the exception here, like the existing Node APIs do – you should be able to get a currently pending exception with napi_get_and_clear_last_exception.

Actually passing the exception along to Node’s fatal exception handling seems orthogonal to that?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

orthogonal? i'm ok with added a napi_value that is used as the exception yes

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mh, sorry – orthogonal ~= independent … basically, I’d suggest that we provide users with the smallest pieces that make sense to be exposed in the API, rather than exposing a function that actually does two operations masquerading as one (getting the last exception + passing it to the fatal exception handler)

Copy link
Member

@richardlau richardlau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For consistency, the subsystem should be n-api.

@mafintosh mafintosh force-pushed the napi_fatal_exception branch from 7829f9c to a575495 Compare March 14, 2018 21:57
@mafintosh
Copy link
Member Author

@richardlau fixed

@mafintosh
Copy link
Member Author

@addaleax fixed all your comments! :D

src/node_api.cc Outdated
v8::Local<v8::Message> local_msg = v8::Exception::CreateMessage( \
env->isolate, (local_err)); \
node::FatalException((env)->isolate, (local_err), local_msg); \
} while (0)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One final nit: We generally prefer inline functions to preprocessor macros :)

@mafintosh
Copy link
Member Author

@addaleax using an inline fn instead of macro now

Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still LGTM

@mafintosh
Copy link
Member Author

Can we land this? @mhdawson @addaleax

Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mhdawson
Copy link
Member

@mhdawson
Copy link
Member

This will partially help with this one as well nodejs/node-addon-api#233

@mhdawson
Copy link
Member

There were failures in CI, I think they were unrelated so new CI here: https://ci.nodejs.org/job/node-test-pull-request/13736/

@mhdawson
Copy link
Member

mhdawson commented Mar 19, 2018

CI was good, this is ready to land. One thing I've realized is that we need to update the N-API version number, but should likely be a separate PR as it will need to be backported if any of the PRs (not that this will happen, but for example another PR with a new API could be backported and this one not be backported and the N-API version would still need to be updated) that N-API are backported to a particular version. Will update the NAPI version as a follow up PR.

At a conference so can't land right now but will try to do it later unless somebody beats me to it.

@mhdawson mhdawson added the node-api Issues and PRs related to the Node-API. label Mar 19, 2018
MylesBorins pushed a commit that referenced this pull request Mar 22, 2018
Add function to trigger and uncaught exception.
Useful if an async callback throws an exception with
no way to recover.

PR-URL: #19337
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@tniessen tniessen removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 24, 2018
targos added a commit that referenced this pull request Mar 27, 2018
Notable changes:

* cluster:
  - Add support for `NODE_OPTIONS="--inspect"` (Sameer Srivastava)
    #19165
* crypto:
  - Expose the public key of a certificate (Hannes Magnusson)
    #17690
* n-api:
  - Add `napi_fatal_exception` to trigger an `uncaughtException` in
    JavaScript (Mathias Buus)
    #19337
* path:
  - Fix regression in `posix.normalize` (Michaël Zasso)
    #19520
* stream:
  - Improve stream creation performance (Brian White)
    #19401
* Added new collaborators
  - [BethGriggs](https://github.com/BethGriggs) Beth Griggs
MylesBorins pushed a commit that referenced this pull request Mar 28, 2018
This is a security release. All Node.js users should consult the
security release summary at:

https://nodejs.org/en/blog/vulnerability/march-2018-security-releases/

for details on patched vulnerabilities.

Fixes for the following CVEs are included in this release:

* CVE-2018-7158
* CVE-2018-7159
* CVE-2018-7160

Notable changes:

* Upgrade to OpenSSL 1.0.2o: Does not contain any security fixes that
  are known to impact Node.js.
* **Fix for inspector DNS rebinding vulnerability (CVE-2018-7160)**:
  A malicious website could use a DNS rebinding attack to trick a web
  browser to bypass same-origin-policy checks and allow HTTP
  connections to localhost or to hosts on the local network,
  potentially to an open inspector port as a debugger, therefore
  gaining full code execution access. The inspector now only allows
  connections that have a browser `Host` value of `localhost` or
  `localhost6`.
* **Fix for `'path'` module regular expression denial of service
  (CVE-2018-7158)**: A regular expression used for parsing POSIX an
  Windows paths could be used to cause a denial of service if an
  attacker were able to have a specially crafted path string passed
  through one of the impacted `'path'` module functions.
* **Reject spaces in HTTP `Content-Length` header values
  (CVE-2018-7159)**: The Node.js HTTP parser allowed for spaces inside
  `Content-Length` header values. Such values now lead to rejected
  connections in the same way as non-numeric values.
* **Update root certificates**: 5 additional root certificates have
  been added to the Node.js binary and 30 have been removed.

* cluster:
  - Add support for `NODE_OPTIONS="--inspect"` (Sameer Srivastava)
    #19165
* crypto:
  - Expose the public key of a certificate (Hannes Magnusson)
    #17690
* n-api:
  - Add `napi_fatal_exception` to trigger an `uncaughtException` in
    JavaScript (Mathias Buus)
    #19337
* path:
  - Fix regression in `posix.normalize` (Michaël Zasso)
    #19520
* stream:
  - Improve stream creation performance (Brian White)
    #19401
* Added new collaborators
  - [BethGriggs](https://github.com/BethGriggs) Beth Griggs

PR-URL: https://github.com/nodejs-private/node-private/pull/111
gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this pull request Apr 12, 2018
Add function to trigger and uncaught exception.
Useful if an async callback throws an exception with
no way to recover.

PR-URL: nodejs#19337
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this pull request Apr 16, 2018
Add function to trigger and uncaught exception.
Useful if an async callback throws an exception with
no way to recover.

PR-URL: nodejs#19337
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
MylesBorins pushed a commit that referenced this pull request Apr 16, 2018
Add function to trigger and uncaught exception.
Useful if an async callback throws an exception with
no way to recover.

Backport-PR-URL: #19447
PR-URL: #19337
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@MylesBorins MylesBorins mentioned this pull request Apr 16, 2018
MylesBorins pushed a commit that referenced this pull request May 1, 2018
Add function to trigger and uncaught exception.
Useful if an async callback throws an exception with
no way to recover.

Backport-PR-URL: #19265
PR-URL: #19337
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@MylesBorins MylesBorins mentioned this pull request May 2, 2018
@parro-it
Copy link

If this is backported, make sure to include this PR as well that bumps the n-api version #19497

Any news on the backporting? We were using napi_fatal_exception but we have to remove it to support previous node versions 😔 in libui-node

@mhdawson
Copy link
Member

Which version are you looking for a backport? I believe it should be there for latest version of current LTS versions except for 8.x which is waiting on the next 8.x release.

@parro-it
Copy link

Which version are you looking for a backport?

It's missing in 8.11.1: https://travis-ci.org/parro-it/libui-napi/jobs/376848346

I believe it should be there for latest version of current LTS versions except for 8.x which is waiting on the next 8.x release.

Thank you, great news!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. node-api Issues and PRs related to the Node-API. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants