n-api: avoid crash in napi_escape_scope() #13651

mhdawson · 2017-06-13T07:10:57Z

V8 will crash if escape is called twice on the same
scope.

Add checks to avoid crashing if napi_escape_scope() is
called to try and do this.

Add test that tries to call napi_create_scope() twice.

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)

n-api

V8 will crash if escape is called twice on the same scope. Add checks to avoid crashing if napi_escape_scope() is called to try and do this. Add test that tries to call napi_create_scope() twice.

TimothyGu · 2017-06-13T08:53:38Z

src/node_api.cc

-  explicit EscapableHandleScopeWrapper(v8::Isolate* isolate) : scope(isolate) {}
+  explicit EscapableHandleScopeWrapper(v8::Isolate* isolate) :
+    scope(isolate), escapeCalled(false) {}
+  bool escapeAllreadyCalled(void) {


jasongin · 2017-06-13T16:22:33Z

src/node_api.cc

    return scope.Escape(handle);
  }

 private:
  v8::EscapableHandleScope scope;
+  bool escapeCalled;


Private members should have an underscore prefix. At least that is the style used in other classes in this file.

cjihrig

LGTM with nits addressed.

mhdawson · 2017-06-15T05:53:33Z

Pushed commit to address comments:

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

jasnell · 2017-06-15T17:49:24Z

src/node_api.cc

    return scope.Escape(handle);
  }

 private:
  v8::EscapableHandleScope scope;
-  bool escapeCalled;
+  bool _escapeCalled;


just an extremely minor nit... in various places in core we use _ as a suffix on private fields, in others we seem to use it as a prefix. It would be great to have consistency there.

bnoordhuis · 2017-06-15T19:35:58Z

src/node_api.cc

+    scope(isolate), _escapeCalled(false) {}
+  bool escapeAlreadyCalled(void) {
+    return _escapeCalled;
+  }


Style issues: method should be bool escape_called() const {, the data member should be escape_called_.

bnoordhuis · 2017-06-15T19:36:53Z

src/node_api_types.h

@@ -67,6 +67,7 @@ typedef enum {
  napi_generic_failure,
  napi_pending_exception,
  napi_cancelled,
+  napi_escape_called_twice,


If this enum is public, then inserting a field changes the ABI.

It is public. At this point since we are experimental it may be ok, but more generally we have to be able to add new error codes so assuming adding to an enum cannot preserve the abi we will have to define our errors in a different way.

@jasongin can you think of any alternative to changing the enums to #defines ? We definitely need to be able to add new errors to the list as we expand or update the api.

How does adding an enum value change the ABI?

Adding a value is okay, inserting it in the middle is not; it changes subsequent values. You're kind of painted in a corner here because of napi_status_last.

Maybe napi_status_last should be removed, or changed to a #define ?

A #define has the same issue: its value is fixed at compile time. If there is no real use case for napi_status_last, I'd remove it.

mhdawson · 2017-06-16T03:23:19Z

Pushed change to address comments. Would like to separate issue fixing use of enum for error code to a follow on PR as we should also look if there are any other instances of the same issue and then fix them and any other code that needs to be modified when we change how errors codes are returned together. Given that we are still in experimental I think even if we stick with the enum adding the new error at this point is probably ok. @bnoordhuis

mhdawson · 2017-06-20T19:31:08Z

I think the best way forward is to remove napi_status_last. I'll do that unless I hear other suggestions.

addaleax · 2017-06-20T21:16:25Z

@mhdawson I agree. We can always replace it with some napi_get_status_last() method that gives the highest status index, but dynamically.

mhdawson · 2017-06-20T21:47:30Z

Pushed commit to remove napi_status_last. Given existing approvals will assume I can land if I don't hear any objections before tomorrow.

bnoordhuis

LGTM modulo style nits.

bnoordhuis · 2017-06-21T08:31:05Z

src/node_api.cc

@@ -156,14 +156,20 @@ class HandleScopeWrapper {
 // across different versions.
 class EscapableHandleScopeWrapper {
 public:
-  explicit EscapableHandleScopeWrapper(v8::Isolate* isolate) : scope(isolate) {}
+  explicit EscapableHandleScopeWrapper(v8::Isolate* isolate) :
+    scope(isolate), escape_called_(false) {}


Minuscule style nit: the colon should go on the next line and have 4 spaces of indent.

bnoordhuis · 2017-06-21T08:31:52Z

src/node_api.cc

  static_assert(
-      (sizeof (error_messages) / sizeof (*error_messages)) == napi_status_last,
+      (sizeof (error_messages) / sizeof (*error_messages)) ==


Aside: is there a reason this doesn't use arraysize()?

don't think so, I'll change.

bnoordhuis · 2017-06-21T08:32:51Z

test/addons-napi/test_handle_scope/test_handle_scope.c

+napi_value NewScopeEscapeTwice(napi_env env, napi_callback_info info) {
+  napi_escapable_handle_scope scope;
+  napi_value output = NULL;
+  napi_value escapee = NULL;


This is C, not C++.

mhdawson · 2017-06-21T15:09:40Z

pushed commit to address comments.

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

addaleax · 2017-06-21T16:39:26Z

src/node_api.cc

@@ -746,10 +754,14 @@ napi_status napi_get_last_error_info(napi_env env,
  CHECK_ENV(env);
  CHECK_ARG(env, result);

+  // you must udpate this assert to reference the last message


typo: update

Thanks will fix. I wish I could type :)

mhdawson · 2017-06-21T19:17:58Z

Ci good landing.

mhdawson · 2017-06-21T20:58:49Z

Landed as 3e18c49

V8 will crash if escape is called twice on the same scope. Add checks to avoid crashing if napi_escape_scope() is called to try and do this. Add test that tries to call napi_create_scope() twice. PR-URL: #13651 Reviewed-By: Jason Ginchereau <jasongin@microsoft.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

V8 will crash if escape is called twice on the same scope. Add checks to avoid crashing if napi_escape_scope() is called to try and do this. Add test that tries to call napi_create_scope() twice. PR-URL: nodejs#13651 Reviewed-By: Jason Ginchereau <jasongin@microsoft.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

V8 will crash if escape is called twice on the same scope. Add checks to avoid crashing if napi_escape_scope() is called to try and do this. Add test that tries to call napi_create_scope() twice. Backport-PR-URL: #19447 PR-URL: #13651 Reviewed-By: Jason Ginchereau <jasongin@microsoft.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

n-api: avoid crash in napi_escape_scope()

da0edd4

V8 will crash if escape is called twice on the same scope. Add checks to avoid crashing if napi_escape_scope() is called to try and do this. Add test that tries to call napi_create_scope() twice.

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. node-api Issues and PRs related to the Node-API. labels Jun 13, 2017

TimothyGu reviewed Jun 13, 2017

View reviewed changes

jasongin reviewed Jun 13, 2017

View reviewed changes

jasongin approved these changes Jun 13, 2017

View reviewed changes

cjihrig approved these changes Jun 13, 2017

View reviewed changes

jasongin mentioned this pull request Jun 14, 2017

Explicitly restrict escapable scope to escape just one value nodejs/abi-stable-node#255

Closed

squash: address comments

039811f

jasnell reviewed Jun 15, 2017

View reviewed changes

jasnell approved these changes Jun 15, 2017

View reviewed changes

bnoordhuis reviewed Jun 15, 2017

View reviewed changes

squash: address next set of comments

d534bd3

addaleax approved these changes Jun 16, 2017

View reviewed changes

squash: address comments

120eff1

bnoordhuis approved these changes Jun 21, 2017

View reviewed changes

squash: address comments

08c3151

addaleax approved these changes Jun 21, 2017

View reviewed changes

squash: address one more comment

f849ae6

mhdawson closed this Jun 21, 2017

This was referenced Jun 21, 2017

v8.2.0 proposal #13744

Merged

v8.1.3 proposal #13861

Merged

MylesBorins added the dont-land-on-v6.x label Jul 17, 2017

MylesBorins mentioned this pull request Apr 16, 2018

v6.14.2 proposal #19996

Merged

mhdawson deleted the napi-escape-crash branch September 30, 2019 13:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

n-api: avoid crash in napi_escape_scope() #13651

n-api: avoid crash in napi_escape_scope() #13651

mhdawson commented Jun 13, 2017

TimothyGu Jun 13, 2017

jasongin Jun 13, 2017

cjihrig left a comment

mhdawson commented Jun 15, 2017

jasnell Jun 15, 2017

bnoordhuis Jun 15, 2017

bnoordhuis Jun 15, 2017

mhdawson Jun 16, 2017

mhdawson Jun 16, 2017

jasongin Jun 16, 2017

bnoordhuis Jun 16, 2017

jasongin Jun 16, 2017

bnoordhuis Jun 16, 2017

mhdawson commented Jun 16, 2017

mhdawson commented Jun 20, 2017

addaleax commented Jun 20, 2017

mhdawson commented Jun 20, 2017

bnoordhuis left a comment

bnoordhuis Jun 21, 2017

mhdawson Jun 21, 2017

bnoordhuis Jun 21, 2017

mhdawson Jun 21, 2017

bnoordhuis Jun 21, 2017

TimothyGu Jun 21, 2017

mhdawson commented Jun 21, 2017

addaleax Jun 21, 2017

mhdawson Jun 21, 2017

mhdawson commented Jun 21, 2017

mhdawson commented Jun 21, 2017

n-api: avoid crash in napi_escape_scope() #13651

n-api: avoid crash in napi_escape_scope() #13651

Conversation

mhdawson commented Jun 13, 2017

Checklist

Affected core subsystem(s)

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

mhdawson commented Jun 15, 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

mhdawson commented Jun 16, 2017

mhdawson commented Jun 20, 2017

addaleax commented Jun 20, 2017

mhdawson commented Jun 20, 2017

bnoordhuis 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

mhdawson commented Jun 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhdawson commented Jun 21, 2017

mhdawson commented Jun 21, 2017