node_api: add napi_fatal_exception #19337

mafintosh · 2018-03-14T02:47:53Z

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

cjihrig

LGTM. Does this supersede #19336?

mafintosh · 2018-03-14T14:23:41Z

@cjihrig ya builds on that one

addaleax

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

addaleax · 2018-03-14T16:20:30Z

doc/api/n-api.md

@@ -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


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

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

addaleax · 2018-03-14T16:23:42Z

src/node_api.h

@@ -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);


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?

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

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)

richardlau

For consistency, the subsystem should be n-api.

mafintosh · 2018-03-14T22:00:20Z

@richardlau fixed

mafintosh · 2018-03-15T18:28:48Z

@addaleax fixed all your comments! :D

addaleax · 2018-03-15T18:54:42Z

src/node_api.cc

+    v8::Local<v8::Message> local_msg = v8::Exception::CreateMessage( \
+      env->isolate, (local_err));                                    \
+    node::FatalException((env)->isolate, (local_err), local_msg);    \
+  } while (0)


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

mafintosh · 2018-03-15T19:57:17Z

@addaleax using an inline fn instead of macro now

addaleax

Still LGTM

mafintosh · 2018-03-18T10:33:09Z

Can we land this? @mhdawson @addaleax

mhdawson

LGTM

mhdawson · 2018-03-18T18:23:53Z

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

mhdawson · 2018-03-18T18:28:29Z

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

mhdawson · 2018-03-18T21:30:25Z

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 · 2018-03-19T21:18:59Z

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.

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>

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

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

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>

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>

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>

parro-it · 2018-05-11T08:27:02Z

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 · 2018-05-11T14:48:35Z

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 · 2018-05-11T16:36:44Z

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!

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 force-pushed the napi_fatal_exception branch from a5ede72 to 7829f9c Compare March 14, 2018 02:48

mafintosh changed the title ~~add napi_fatal_exception~~ node_api: add napi_fatal_exception Mar 14, 2018

jasnell approved these changes Mar 14, 2018

View reviewed changes

cjihrig approved these changes Mar 14, 2018

View reviewed changes

addaleax reviewed Mar 14, 2018

View reviewed changes

richardlau reviewed Mar 14, 2018

View reviewed changes

mafintosh added 3 commits March 14, 2018 22:57

n-api: test uncaught exception in worker

7fefdda

n-api: fix fatalexception handling in workers

e602e9c

n-api: add napi_fatal_exception

a575495

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

TBA -> REPLACEME

de3c603

add test and support passing in an error

aa5498b

addaleax approved these changes Mar 15, 2018

View reviewed changes

use inline fun instead of macro

5ff7ba4

addaleax approved these changes Mar 15, 2018

View reviewed changes

mafintosh mentioned this pull request Mar 18, 2018

N-API request: Ability to report fatal exception resulting from callbacks that are thrown outside the regular JS call stack #15371

Closed

mhdawson approved these changes Mar 18, 2018

View reviewed changes

mhdawson mentioned this pull request Mar 18, 2018

Async handles and Errors nodejs/node-addon-api#233

Closed

mhdawson added the node-api Issues and PRs related to the Node-API. label Mar 19, 2018

mhdawson mentioned this pull request Mar 20, 2018

n-api: bump version of n-api supported #19497

Closed

3 tasks

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

MylesBorins mentioned this pull request Apr 16, 2018

v6.14.2 proposal #19996

Merged

MylesBorins mentioned this pull request May 2, 2018

v8.11.2 proposal #20478

Merged

mhdawson mentioned this pull request May 25, 2018

N-API: pass error from execute callback in napi_create_async_work to main loop #20964

Closed

ekraihan mentioned this pull request Dec 6, 2020

Pass exceptions to JS that are thrown by JS functions in an async context #36402

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

node_api: add napi_fatal_exception #19337

node_api: add napi_fatal_exception #19337

mafintosh commented Mar 14, 2018

cjihrig left a comment

mafintosh commented Mar 14, 2018

addaleax left a comment

addaleax Mar 14, 2018

mafintosh Mar 14, 2018

addaleax Mar 14, 2018

mafintosh Mar 14, 2018

addaleax Mar 14, 2018

richardlau left a comment

mafintosh commented Mar 14, 2018

mafintosh commented Mar 15, 2018

addaleax Mar 15, 2018

mafintosh commented Mar 15, 2018

addaleax left a comment

mafintosh commented Mar 18, 2018

mhdawson left a comment

mhdawson commented Mar 18, 2018

mhdawson commented Mar 18, 2018

mhdawson commented Mar 18, 2018

mhdawson commented Mar 19, 2018 •

edited

Loading

parro-it commented May 11, 2018

mhdawson commented May 11, 2018

parro-it commented May 11, 2018

node_api: add napi_fatal_exception #19337

node_api: add napi_fatal_exception #19337

Conversation

mafintosh commented Mar 14, 2018

Checklist

cjihrig left a comment

Choose a reason for hiding this comment

mafintosh commented Mar 14, 2018

addaleax left a comment

Choose a reason for hiding this comment

addaleax Mar 14, 2018

Choose a reason for hiding this comment

mafintosh Mar 14, 2018

Choose a reason for hiding this comment

addaleax Mar 14, 2018

Choose a reason for hiding this comment

mafintosh Mar 14, 2018

Choose a reason for hiding this comment

addaleax Mar 14, 2018

Choose a reason for hiding this comment

richardlau left a comment

Choose a reason for hiding this comment

mafintosh commented Mar 14, 2018

mafintosh commented Mar 15, 2018

addaleax Mar 15, 2018

Choose a reason for hiding this comment

mafintosh commented Mar 15, 2018

addaleax left a comment

Choose a reason for hiding this comment

mafintosh commented Mar 18, 2018

mhdawson left a comment

Choose a reason for hiding this comment

mhdawson commented Mar 18, 2018

mhdawson commented Mar 18, 2018

mhdawson commented Mar 18, 2018

mhdawson commented Mar 19, 2018 • edited Loading

parro-it commented May 11, 2018

mhdawson commented May 11, 2018

parro-it commented May 11, 2018

mhdawson commented Mar 19, 2018 •

edited

Loading