[v2.x] Backport ObjectWrap reference-related changes #723

gabrielschulhof · 2020-05-11T16:17:26Z

No description provided.

gabrielschulhof · 2020-05-11T16:20:39Z

addaleax

👍

gabrielschulhof · 2020-05-11T18:11:27Z

@addaleax I actually had to add another commit, also cleanly applied – the one that updates the BigInt test for changes to the expected error message in core – since we're testing 2.x again.

gabrielschulhof · 2020-05-11T18:44:21Z

@addaleax this crashes on Node.js v8.x because we didn't backport the reference tracking changes before it went EOL.

addaleax · 2020-05-11T22:09:08Z

@gabrielschulhof Sigh … yeah, not sure what to do about that. I don’t suppose we want to detect the Node.js version in here and skip the relevant fixes for Node < 10? I would be okay with that but I’d also understand that it’s not a great solution.

(I mean, it’s also not clear that N-API had breaking changes in core. But that’s not really something we can do anything about…)

gabrielschulhof · 2020-05-12T04:49:50Z

@addaleax the detection would have to happen at runtime because anything built against Node.js 8.x must also run without recompilation on later versions.

addaleax · 2020-05-12T05:44:44Z

@gabrielschulhof Yes, I was talking about VersionManagement::GetNodeVersion().

gabrielschulhof · 2020-05-15T20:38:41Z

@addaleax I started implementing it but then I realized that we cannot just call Napi::VersionManagement::GetNodeVersion() from the destructor because that would slow down the destructor which can, after all, be called quite often if its instances are created/destroyed in a tight loop. We'd have to store the result once and reduce it to a boolean which evaluates to true only on old versions of Node.js (< 10.15.3, to be precise). That, in turn requires that we store the value somewhere. A global static is not a good option because that would break the whole context-awareness. My best guess would be a global static thread_local.

addaleax · 2020-05-15T23:06:51Z

A global static is not a good option because that would break the whole context-awareness.

I don’t quite see how it does that? It’s not like the Node.js version is going to vary between contexts, or is that a concern here?

gabrielschulhof · 2020-05-15T23:24:02Z

@addaleax no, but it should at least be assigned in a thread-safe fashion.

addaleax · 2020-05-15T23:33:17Z

@gabrielschulhof Do we assume C++11 here? It would come with std::atomic which solves that for us. Without it, thread_local won’t be available either…

gabrielschulhof · 2020-05-16T00:02:31Z

@addaleax I think it's safe to assume C++11.

gabrielschulhof · 2020-05-16T00:21:41Z

napi-inl.h

@@ -10,6 +10,7 @@
 // Note: Do not include this file directly! Include "napi.h" instead.

 #include <algorithm>
+#include <atomic>


This is a new change, not part of any cherry-pick.

gabrielschulhof · 2020-05-16T00:21:56Z

napi-inl.h

+extern std::atomic_bool needs_objectwrap_destructor_fix;
+


This is a new change, not part of any cherry-pick.

gabrielschulhof · 2020-05-16T00:22:08Z

napi-inl.h

+  namespace Napi {                                             \
+    namespace details {                                        \
+      std::atomic_bool needs_objectwrap_destructor_fix(false); \
+    }                                                          \
+  }                                                            \


This is a new change, not part of any cherry-pick.

gabrielschulhof · 2020-05-16T00:24:58Z

napi-inl.h

+  const napi_node_version* nver = Napi::VersionManagement::GetNodeVersion(env);
+  Napi::details::needs_objectwrap_destructor_fix =
+    (nver->major < 10 ||
+      (nver->major == 10 && nver->minor < 15) ||
+      (nver->major == 10 && nver->minor == 15 && nver->patch < 3));
+


This is a new change, not part of any cherry-pick

gabrielschulhof · 2020-05-16T00:25:26Z

napi-inl.h

+      if (Napi::details::needs_objectwrap_destructor_fix) {
+        // If construction failed we delete the reference via
+        // `napi_remove_wrap()`, not via `napi_delete_reference()` in the
+        // `Reference<Object>` destructor. This will prevent the
+        // `Reference<Object>` destructor from doing a double delete of this
+        // reference.
+        _ref = nullptr;
+        _env = nullptr;
+      }


This is a new change, not part of any cherry-pick.

gabrielschulhof · 2020-05-16T00:26:22Z

@addaleax PTAL. I pointed out the changes introduced by the commit that is not a cherry-pick.

gabrielschulhof · 2020-06-08T22:44:10Z

CI:

Version	Job	Status
v8.x	https://ci.nodejs.org/job/node-test-node-addon-api-new/2305/	✓
v10.x	https://ci.nodejs.org/job/node-test-node-addon-api-new/2302/	✓
v12.x	https://ci.nodejs.org/job/node-test-node-addon-api-new/2303/	✓
~~v14.x~~	~~https://ci.nodejs.org/job/node-test-node-addon-api-new/2304/~~	✗

gabrielschulhof · 2020-06-08T22:51:22Z

Oh, but wait! 2.x doesn't support Node.js 14.

gabrielschulhof · 2020-06-08T22:53:07Z

@mhdawson I think we're good to land this PR and make a final 2.x release. PTAL!

Ensure that no native instance pointer is associated with the JavaScript object under construction if the native constructor causes a JavaScript exception to be thrown. Two different cases must be taken into consideration: 1. The exception in the constructor was caused by `ObjectWrap<T>::ObjectWrap` when the call to `napi_wrap()` failed. 2. The exception in the constructor was caused by the constructor of the subclass of `ObjectWrap<T>` after `napi_wrap()` was already successful. Fixes: nodejs#599 Co-authored-by: blagoev <lubo@blagoev.com> PR-URL: nodejs#600 Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

Currently, when the `ObjectWrap` constructor runs, it calls `napi_wrap()`, adding a finalize callback to the freshly created JS object. However, if the `ObjectWrap` instance is prematurely deleted, for example because a subclass constructor throws – which seems like a reasonable scenario – that finalize callback was not removed, possibly leading to a use-after-free crash. This commit adds a call `napi_remove_wrap()` from the `ObjectWrap` destructor, and a test for that scenario. This also changes the code to use the correct pointer type in `FinalizeCallback`, which may not match the incorretct one in cases of multiple inheritance. Fixes: node-ffi-napi/weak-napi#16 PR-URL: nodejs#475 Reviewed-By: Hitesh Kanwathirtha <digitalinfinity@gmail.com> Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Co-authored-by: Gabriel Schulhof <gabriel.schulhof@intel.com>

`napi_remove_wrap()` was intended for objects that are alive for which the native addon wishes to withdraw its native pointer, and perhaps replace it with another. Therefore we need not `napi_remove_wrap()` during gc/env-cleanup. It is sufficient to `napi_delete_reference()`, as `Reference<Object>` already does. We need only `napi_remove_wrap()` if the construction failed and therefore no gc callback will ever happen. This change also removes references to `ObjectWrapConstructionContext` from the header because the class is not used anymore. Fixes: nodejs#660 Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Kevin Eady <kevin.c.eady@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

nodejs/node@689ab46 changed the expected error for one of the BigInt test cases. This is ok because BigInt is still in experimental. However, the result was a failed/hanging test. See nodejs/build#2131 This changes the test to accept either the new or old behaviour. We need that so that older versions of Node.js will also pass the test until the the node core commit above is backported. PR-URL: nodejs#649 Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com> Reviewed-By: Nicola Del Gobbo <nicoladelgobbo@gmail.com>

On Node.js versions earlier than 10.15.3 references were being cleaned up differently. This has resulted in a segfault because of a double free when the `ObjectWrap<T>` subclass constructor throws. We need a fix that only runs on Node.js < 10.15.3. We use `Napi::VersionManagement::GetNodeVersion()` at addon load time to determine whether we need the workaround. We store the result in an atomic boolean and consult its value whenever we destroy an `ObjectWrap<T>` instance.

gabrielschulhof · 2020-06-12T18:56:17Z

Rebased because of the security release.

mhdawson

LGTM

NickNaso

LGTM

gabrielschulhof mentioned this pull request May 11, 2020

Make next release semver-major/Request for 2.x release with backports #646

Closed

gabrielschulhof requested review from NickNaso, legendecas, addaleax and mhdawson May 11, 2020 16:21

gabrielschulhof added the v2.x label May 11, 2020

gabrielschulhof changed the title ~~Backport ObjectWrap reference-related changes~~ [v2.x] Backport ObjectWrap reference-related changes May 11, 2020

addaleax approved these changes May 11, 2020

View reviewed changes

gabrielschulhof commented May 16, 2020

View reviewed changes

addaleax approved these changes May 16, 2020

View reviewed changes

legendecas approved these changes Jun 8, 2020

View reviewed changes

Gabriel Schulhof and others added 4 commits June 12, 2020 11:55

gabrielschulhof force-pushed the backport-4e885069-pr-475 branch from 31be779 to 470f130 Compare June 12, 2020 18:55

mhdawson approved these changes Jun 15, 2020

View reviewed changes

NickNaso approved these changes Jun 30, 2020

View reviewed changes

NickNaso merged commit 5abf602 into nodejs:v2.x-staging Jul 1, 2020

gabrielschulhof deleted the backport-4e885069-pr-475 branch February 1, 2021 16:39

gabrielschulhof restored the backport-4e885069-pr-475 branch February 1, 2021 16:40

gabrielschulhof deleted the backport-4e885069-pr-475 branch March 7, 2021 18:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[v2.x] Backport ObjectWrap reference-related changes #723

[v2.x] Backport ObjectWrap reference-related changes #723

gabrielschulhof commented May 11, 2020

gabrielschulhof commented May 11, 2020

addaleax left a comment

gabrielschulhof commented May 11, 2020

gabrielschulhof commented May 11, 2020

addaleax commented May 11, 2020

gabrielschulhof commented May 12, 2020

addaleax commented May 12, 2020

gabrielschulhof commented May 15, 2020

addaleax commented May 15, 2020

gabrielschulhof commented May 15, 2020

addaleax commented May 15, 2020

gabrielschulhof commented May 16, 2020

gabrielschulhof May 16, 2020

gabrielschulhof May 16, 2020

gabrielschulhof May 16, 2020

gabrielschulhof May 16, 2020

gabrielschulhof May 16, 2020

gabrielschulhof commented May 16, 2020

gabrielschulhof commented Jun 8, 2020 •

edited

Loading

gabrielschulhof commented Jun 8, 2020

gabrielschulhof commented Jun 8, 2020

gabrielschulhof commented Jun 12, 2020

mhdawson left a comment

NickNaso left a comment

[v2.x] Backport ObjectWrap reference-related changes #723

[v2.x] Backport ObjectWrap reference-related changes #723

Conversation

gabrielschulhof commented May 11, 2020

gabrielschulhof commented May 11, 2020

addaleax left a comment

Choose a reason for hiding this comment

gabrielschulhof commented May 11, 2020

gabrielschulhof commented May 11, 2020

addaleax commented May 11, 2020

gabrielschulhof commented May 12, 2020

addaleax commented May 12, 2020

gabrielschulhof commented May 15, 2020

addaleax commented May 15, 2020

gabrielschulhof commented May 15, 2020

addaleax commented May 15, 2020

gabrielschulhof commented May 16, 2020

gabrielschulhof May 16, 2020

Choose a reason for hiding this comment

gabrielschulhof May 16, 2020

Choose a reason for hiding this comment

gabrielschulhof May 16, 2020

Choose a reason for hiding this comment

gabrielschulhof May 16, 2020

Choose a reason for hiding this comment

gabrielschulhof May 16, 2020

Choose a reason for hiding this comment

gabrielschulhof commented May 16, 2020

gabrielschulhof commented Jun 8, 2020 • edited Loading

gabrielschulhof commented Jun 8, 2020

gabrielschulhof commented Jun 8, 2020

gabrielschulhof commented Jun 12, 2020

mhdawson left a comment

Choose a reason for hiding this comment

NickNaso left a comment

Choose a reason for hiding this comment

gabrielschulhof commented Jun 8, 2020 •

edited

Loading