From a35f553889927ed379e9c3f9971d97ae61d267e9 Mon Sep 17 00:00:00 2001 From: Bryan English Date: Wed, 23 Feb 2022 19:04:44 -0500 Subject: [PATCH 01/15] doc: add release key for Bryan English MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds Bryan English and his public key to the README for releases. PR-URL: https://github.com/nodejs/node/pull/42102 Reviewed-By: Colin Ihrig Reviewed-By: Beth Griggs Reviewed-By: Rich Trott Reviewed-By: Michaël Zasso Reviewed-By: Richard Lau Reviewed-By: Luigi Pinca Reviewed-By: Mestery Reviewed-By: Michael Dawson Reviewed-By: Danielle Adams --- README.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/README.md b/README.md index a5066a675f0a8e..8b515562c21ccf 100644 --- a/README.md +++ b/README.md @@ -581,6 +581,8 @@ Primary GPG keys for Node.js Releasers (some Releasers sign with subkeys): * **Beth Griggs** <bgriggs@redhat.com> `4ED778F539E3634C779C87C6D7062848A1AB005C` +* **Bryan English** <bryan@bryanenglish.com> +`141F07595B7B3FFE74309A937405533BE57C7D57` * **Colin Ihrig** <cjihrig@gmail.com> `94AE36675C464D64BAFA68DD7434390BDBE9B9C5` * **James M Snell** <jasnell@keybase.io> @@ -604,6 +606,7 @@ To import the full set of trusted release keys: ```bash gpg --keyserver pool.sks-keyservers.net --recv-keys 4ED778F539E3634C779C87C6D7062848A1AB005C +gpg --keyserver pool.sks-keyservers.net --recv-keys 141F07595B7B3FFE74309A937405533BE57C7D57 gpg --keyserver pool.sks-keyservers.net --recv-keys 94AE36675C464D64BAFA68DD7434390BDBE9B9C5 gpg --keyserver pool.sks-keyservers.net --recv-keys 71DCFD284A79C3B38668286BC97EC7A07EDE3FC1 gpg --keyserver pool.sks-keyservers.net --recv-keys 8FCCA13FEF1D0C2E91008E09770F7A9A5AE15600 From 1193290f3f3ae6a0845b47ba082dc85eb48d6d44 Mon Sep 17 00:00:00 2001 From: devsnek Date: Wed, 30 Mar 2022 07:01:01 -0500 Subject: [PATCH 02/15] deps: V8: cherry-pick cc9a8a37445e Original commit message: fix overflow check in error formatting Bug: v8:12494 Change-Id: Iba2684173296aa236f1a1c73a5606c21472eff06 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3426634 Reviewed-by: Jakob Kummerow Commit-Queue: Gus Caplan Cr-Commit-Position: refs/heads/main@{#78909} Refs: https://github.com/v8/v8/commit/cc9a8a37445eeffff17474020bb6038c2f9af9fc PR-URL: https://github.com/nodejs/node/pull/42065 Reviewed-By: Rich Trott Reviewed-By: Jiawen Geng Reviewed-By: Antoine du Hamel Reviewed-By: Mestery Reviewed-By: Darshan Sen --- common.gypi | 2 +- deps/v8/src/execution/messages.cc | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/common.gypi b/common.gypi index 34bf547d5e9147..2bfae462d0e7fe 100644 --- a/common.gypi +++ b/common.gypi @@ -34,7 +34,7 @@ # Reset this number to 0 on major V8 upgrades. # Increment by one for each non-official patch applied to deps/v8. - 'v8_embedder_string': '-node.56', + 'v8_embedder_string': '-node.57', ##### V8 defaults for Node.js ##### diff --git a/deps/v8/src/execution/messages.cc b/deps/v8/src/execution/messages.cc index 63d1e2be1ff7af..b3e9d1a6b55fa6 100644 --- a/deps/v8/src/execution/messages.cc +++ b/deps/v8/src/execution/messages.cc @@ -823,7 +823,8 @@ MaybeHandle ErrorUtils::FormatStackTrace(Isolate* isolate, Handle elems = Handle::cast(raw_stack); const bool in_recursion = isolate->formatting_stack_trace(); - if (!in_recursion) { + const bool has_overflowed = i::StackLimitCheck{isolate}.HasOverflowed(); + if (!in_recursion && !has_overflowed) { Handle error_context = error->GetCreationContext(); DCHECK(error_context->IsNativeContext()); From 171bb66ccc34611e8b3a7d67af5280cd6b2c414c Mon Sep 17 00:00:00 2001 From: Gabriel Schulhof Date: Tue, 9 Feb 2021 22:52:54 -0800 Subject: [PATCH 03/15] node-api: force env shutdown deferring behavior The finalizer normally never gets called while a reference is strong. However, during environment shutdown all finalizers must get called. In order to unify the deferring behavior with that of a regular finalization, we must force the reference to be weak when we call its finalizer during environment shutdown. Fixes: https://github.com/nodejs/node/issues/37236 Co-authored-by: Chengzhong Wu PR-URL: https://github.com/nodejs/node/pull/37303 Backport-PR-URL: https://github.com/nodejs/node/pull/42512 Reviewed-By: Chengzhong Wu Reviewed-By: Michael Dawson --- src/js_native_api_v8.cc | 9 +++ .../test_reference_double_free/binding.gyp | 11 ++++ .../test_reference_double_free/test.js | 11 ++++ .../test_reference_double_free.c | 55 +++++++++++++++++++ 4 files changed, 86 insertions(+) create mode 100644 test/js-native-api/test_reference_double_free/binding.gyp create mode 100644 test/js-native-api/test_reference_double_free/test.js create mode 100644 test/js-native-api/test_reference_double_free/test_reference_double_free.c diff --git a/src/js_native_api_v8.cc b/src/js_native_api_v8.cc index 6a205fafcaa95f..5163503beea7c1 100644 --- a/src/js_native_api_v8.cc +++ b/src/js_native_api_v8.cc @@ -269,8 +269,17 @@ class RefBase : protected Finalizer, RefTracker { protected: inline void Finalize(bool is_env_teardown = false) override { + // During environment teardown we have to convert a strong reference to + // a weak reference to force the deferring behavior if the user's finalizer + // happens to delete this reference so that the code in this function that + // follows the call to the user's finalizer may safely access variables from + // this instance. + if (is_env_teardown && RefCount() > 0) _refcount = 0; + if (_finalize_callback != nullptr) { _env->CallFinalizer(_finalize_callback, _finalize_data, _finalize_hint); + // This ensures that we never call the finalizer twice. + _finalize_callback = nullptr; } // this is safe because if a request to delete the reference diff --git a/test/js-native-api/test_reference_double_free/binding.gyp b/test/js-native-api/test_reference_double_free/binding.gyp new file mode 100644 index 00000000000000..864846765d0132 --- /dev/null +++ b/test/js-native-api/test_reference_double_free/binding.gyp @@ -0,0 +1,11 @@ +{ + "targets": [ + { + "target_name": "test_reference_double_free", + "sources": [ + "../entry_point.c", + "test_reference_double_free.c" + ] + } + ] +} diff --git a/test/js-native-api/test_reference_double_free/test.js b/test/js-native-api/test_reference_double_free/test.js new file mode 100644 index 00000000000000..242d850ba9f677 --- /dev/null +++ b/test/js-native-api/test_reference_double_free/test.js @@ -0,0 +1,11 @@ +'use strict'; + +// This test makes no assertions. It tests a fix without which it will crash +// with a double free. + +const { buildType } = require('../../common'); + +const addon = require(`./build/${buildType}/test_reference_double_free`); + +{ new addon.MyObject(true); } +{ new addon.MyObject(false); } diff --git a/test/js-native-api/test_reference_double_free/test_reference_double_free.c b/test/js-native-api/test_reference_double_free/test_reference_double_free.c new file mode 100644 index 00000000000000..45329e075954c9 --- /dev/null +++ b/test/js-native-api/test_reference_double_free/test_reference_double_free.c @@ -0,0 +1,55 @@ +#include +#include +#include "../common.h" + +static size_t g_call_count = 0; + +static void Destructor(napi_env env, void* data, void* nothing) { + napi_ref* ref = data; + NODE_API_CALL_RETURN_VOID(env, napi_delete_reference(env, *ref)); + free(ref); +} + +static void NoDeleteDestructor(napi_env env, void* data, void* hint) { + napi_ref* ref = data; + size_t* call_count = hint; + + // This destructor must be called exactly once. + if ((*call_count) > 0) abort(); + *call_count = ((*call_count) + 1); + free(ref); +} + +static napi_value New(napi_env env, napi_callback_info info) { + size_t argc = 1; + napi_value js_this, js_delete; + bool delete; + napi_ref* ref = malloc(sizeof(*ref)); + + NAPI_CALL(env, + napi_get_cb_info(env, info, &argc, &js_delete, &js_this, NULL)); + NAPI_CALL(env, napi_get_value_bool(env, js_delete, &delete)); + + if (delete) { + NAPI_CALL(env, + napi_wrap(env, js_this, ref, Destructor, NULL, ref)); + } else { + NAPI_CALL(env, + napi_wrap(env, js_this, ref, NoDeleteDestructor, &g_call_count, ref)); + } + NAPI_CALL(env, napi_reference_ref(env, *ref, NULL)); + + return js_this; +} + +EXTERN_C_START +napi_value Init(napi_env env, napi_value exports) { + napi_value myobj_ctor; + NAPI_CALL(env, + napi_define_class( + env, "MyObject", NAPI_AUTO_LENGTH, New, NULL, 0, NULL, &myobj_ctor)); + NAPI_CALL(env, + napi_set_named_property(env, exports, "MyObject", myobj_ctor)); + return exports; +} +EXTERN_C_END From a2f42064159cabfa27e2d46208b58443069eec96 Mon Sep 17 00:00:00 2001 From: Gabriel Schulhof Date: Wed, 3 Mar 2021 20:37:27 -0800 Subject: [PATCH 04/15] node-api: stop ref gc during environment teardown A gc may happen during environment teardown. Thus, during finalization initiated by environment teardown we must remove the V8 finalizer before calling the Node-API finalizer. Fixes: https://github.com/nodejs/node/issues/37236 PR-URL: https://github.com/nodejs/node/pull/37616 Backport-PR-URL: https://github.com/nodejs/node/pull/42512 Reviewed-By: Chengzhong Wu Reviewed-By: Michael Dawson --- src/js_native_api_v8.cc | 28 +++++++++++++- .../test_reference_double_free.c | 2 +- test/node-api/test_env_teardown_gc/binding.c | 37 +++++++++++++++++++ .../node-api/test_env_teardown_gc/binding.gyp | 8 ++++ test/node-api/test_env_teardown_gc/test.js | 14 +++++++ 5 files changed, 87 insertions(+), 2 deletions(-) create mode 100644 test/node-api/test_env_teardown_gc/binding.c create mode 100644 test/node-api/test_env_teardown_gc/binding.gyp create mode 100644 test/node-api/test_env_teardown_gc/test.js diff --git a/src/js_native_api_v8.cc b/src/js_native_api_v8.cc index 5163503beea7c1..e969adc3918ec4 100644 --- a/src/js_native_api_v8.cc +++ b/src/js_native_api_v8.cc @@ -269,6 +269,20 @@ class RefBase : protected Finalizer, RefTracker { protected: inline void Finalize(bool is_env_teardown = false) override { + // In addition to being called during environment teardown, this method is + // also the entry point for the garbage collector. During environment + // teardown we have to remove the garbage collector's reference to this + // method so that, if, as part of the user's callback, JS gets executed, + // resulting in a garbage collection pass, this method is not re-entered as + // part of that pass, because that'll cause a double free (as seen in + // https://github.com/nodejs/node/issues/37236). + // + // Since this class does not have access to the V8 persistent reference, + // this method is overridden in the `Reference` class below. Therein the + // weak callback is removed, ensuring that the garbage collector does not + // re-enter this method, and the method chains up to continue the process of + // environment-teardown-induced finalization. + // During environment teardown we have to convert a strong reference to // a weak reference to force the deferring behavior if the user's finalizer // happens to delete this reference so that the code in this function that @@ -277,9 +291,10 @@ class RefBase : protected Finalizer, RefTracker { if (is_env_teardown && RefCount() > 0) _refcount = 0; if (_finalize_callback != nullptr) { - _env->CallFinalizer(_finalize_callback, _finalize_data, _finalize_hint); // This ensures that we never call the finalizer twice. + napi_finalize fini = _finalize_callback; _finalize_callback = nullptr; + _env->CallFinalizer(fini, _finalize_data, _finalize_hint); } // this is safe because if a request to delete the reference @@ -354,6 +369,17 @@ class Reference : public RefBase { } } + protected: + inline void Finalize(bool is_env_teardown = false) override { + // During env teardown, `~napi_env()` alone is responsible for finalizing. + // Thus, we don't want any stray gc passes to trigger a second call to + // `Finalize()`, so let's reset the persistent here. + if (is_env_teardown) _persistent.ClearWeak(); + + // Chain up to perform the rest of the finalization. + RefBase::Finalize(is_env_teardown); + } + private: // The N-API finalizer callback may make calls into the engine. V8's heap is // not in a consistent state during the weak callback, and therefore it does diff --git a/test/js-native-api/test_reference_double_free/test_reference_double_free.c b/test/js-native-api/test_reference_double_free/test_reference_double_free.c index 45329e075954c9..f38867d220ae68 100644 --- a/test/js-native-api/test_reference_double_free/test_reference_double_free.c +++ b/test/js-native-api/test_reference_double_free/test_reference_double_free.c @@ -6,7 +6,7 @@ static size_t g_call_count = 0; static void Destructor(napi_env env, void* data, void* nothing) { napi_ref* ref = data; - NODE_API_CALL_RETURN_VOID(env, napi_delete_reference(env, *ref)); + NAPI_CALL_RETURN_VOID(env, napi_delete_reference(env, *ref)); free(ref); } diff --git a/test/node-api/test_env_teardown_gc/binding.c b/test/node-api/test_env_teardown_gc/binding.c new file mode 100644 index 00000000000000..f894690b2470b2 --- /dev/null +++ b/test/node-api/test_env_teardown_gc/binding.c @@ -0,0 +1,37 @@ +#include +#include +#include "../../js-native-api/common.h" + +static void MyObject_fini(napi_env env, void* data, void* hint) { + napi_ref* ref = data; + napi_value global; + napi_value cleanup; + NAPI_CALL_RETURN_VOID(env, napi_get_global(env, &global)); + NAPI_CALL_RETURN_VOID( + env, napi_get_named_property(env, global, "cleanup", &cleanup)); + napi_status status = napi_call_function(env, global, cleanup, 0, NULL, NULL); + // We may not be allowed to call into JS, in which case a pending exception + // will be returned. + NAPI_ASSERT_RETURN_VOID(env, + status == napi_ok || status == napi_pending_exception, + "Unexpected status for napi_call_function"); + NAPI_CALL_RETURN_VOID(env, napi_delete_reference(env, *ref)); + free(ref); +} + +static napi_value MyObject(napi_env env, napi_callback_info info) { + napi_value js_this; + napi_ref* ref = malloc(sizeof(*ref)); + NAPI_CALL(env, napi_get_cb_info(env, info, NULL, NULL, &js_this, NULL)); + NAPI_CALL(env, napi_wrap(env, js_this, ref, MyObject_fini, NULL, ref)); + return NULL; +} + +NAPI_MODULE_INIT() { + napi_value ctor; + NAPI_CALL( + env, napi_define_class( + env, "MyObject", NAPI_AUTO_LENGTH, MyObject, NULL, 0, NULL, &ctor)); + NAPI_CALL(env, napi_set_named_property(env, exports, "MyObject", ctor)); + return exports; +} diff --git a/test/node-api/test_env_teardown_gc/binding.gyp b/test/node-api/test_env_teardown_gc/binding.gyp new file mode 100644 index 00000000000000..413621ade335a1 --- /dev/null +++ b/test/node-api/test_env_teardown_gc/binding.gyp @@ -0,0 +1,8 @@ +{ + 'targets': [ + { + 'target_name': 'binding', + 'sources': [ 'binding.c' ] + } + ] +} diff --git a/test/node-api/test_env_teardown_gc/test.js b/test/node-api/test_env_teardown_gc/test.js new file mode 100644 index 00000000000000..8f1c4f4038fbab --- /dev/null +++ b/test/node-api/test_env_teardown_gc/test.js @@ -0,0 +1,14 @@ +'use strict'; +// Flags: --expose-gc + +process.env.NODE_TEST_KNOWN_GLOBALS = 0; + +const common = require('../../common'); +const binding = require(`./build/${common.buildType}/binding`); + +global.it = new binding.MyObject(); + +global.cleanup = () => { + delete global.it; + global.gc(); +}; From 2aa9ca1ea93b9b9b92a303d3c8def777f3a1dfee Mon Sep 17 00:00:00 2001 From: Michael Dawson Date: Tue, 23 Mar 2021 11:26:18 -0400 Subject: [PATCH 05/15] node-api: fix crash in finalization Refs: https://github.com/nodejs/node-addon-api/issues/906 Refs: https://github.com/nodejs/node/pull/37616 Fix crash introduced by https://github.com/nodejs/node/pull/37616 Signed-off-by: Michael Dawson PR-URL: https://github.com/nodejs/node/pull/37876 Backport-PR-URL: https://github.com/nodejs/node/pull/42512 Reviewed-By: Anna Henningsen Reviewed-By: Colin Ihrig Reviewed-By: Rich Trott Reviewed-By: James M Snell --- src/js_native_api_v8.cc | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/js_native_api_v8.cc b/src/js_native_api_v8.cc index e969adc3918ec4..5106711bd2d1c1 100644 --- a/src/js_native_api_v8.cc +++ b/src/js_native_api_v8.cc @@ -373,8 +373,11 @@ class Reference : public RefBase { inline void Finalize(bool is_env_teardown = false) override { // During env teardown, `~napi_env()` alone is responsible for finalizing. // Thus, we don't want any stray gc passes to trigger a second call to - // `Finalize()`, so let's reset the persistent here. - if (is_env_teardown) _persistent.ClearWeak(); + // `Finalize()`, so let's reset the persistent here if nothing is + // keeping it alive. + if (is_env_teardown && _persistent.IsWeak()) { + _persistent.ClearWeak(); + } // Chain up to perform the rest of the finalization. RefBase::Finalize(is_env_teardown); From 81b4dc88f18f7196c36fd40be6494e602882bf50 Mon Sep 17 00:00:00 2001 From: Chengzhong Wu Date: Wed, 31 Mar 2021 14:42:11 +0800 Subject: [PATCH 06/15] node-api: make reference weak parameter an indirect link to references As the cancellation of second pass callbacks are not reachable from the current v8 API, and the second pass callbacks are scheduled with NodePlatform's task runner, we have to ensure that the weak parameter holds indirect access to the v8impl::Reference object so that the object can be destroyed on addon env teardown before the whole node env is able to shutdown. PR-URL: https://github.com/nodejs/node/pull/38000 Backport-PR-URL: https://github.com/nodejs/node/pull/42512 Reviewed-By: Michael Dawson Reviewed-By: Gabriel Schulhof --- src/js_native_api_v8.cc | 98 ++++++++++++++++++++++++++++++++++------- 1 file changed, 81 insertions(+), 17 deletions(-) diff --git a/src/js_native_api_v8.cc b/src/js_native_api_v8.cc index 5106711bd2d1c1..8254dc7d6c8aa2 100644 --- a/src/js_native_api_v8.cc +++ b/src/js_native_api_v8.cc @@ -313,16 +313,16 @@ class RefBase : protected Finalizer, RefTracker { }; class Reference : public RefBase { + using SecondPassCallParameterRef = Reference*; + protected: template - Reference(napi_env env, - v8::Local value, - Args&&... args) + Reference(napi_env env, v8::Local value, Args&&... args) : RefBase(env, std::forward(args)...), - _persistent(env->isolate, value) { + _persistent(env->isolate, value), + _secondPassParameter(new SecondPassCallParameterRef(this)) { if (RefCount() == 0) { - _persistent.SetWeak( - this, FinalizeCallback, v8::WeakCallbackType::kParameter); + SetWeak(); } } @@ -343,10 +343,19 @@ class Reference : public RefBase { finalize_hint); } + virtual ~Reference() { + // If the second pass callback is scheduled, it will delete the + // parameter passed to it, otherwise it will never be scheduled + // and we need to delete it here. + if (_secondPassParameter != nullptr) { + delete _secondPassParameter; + } + } + inline uint32_t Ref() { uint32_t refcount = RefBase::Ref(); if (refcount == 1) { - _persistent.ClearWeak(); + ClearWeak(); } return refcount; } @@ -355,8 +364,7 @@ class Reference : public RefBase { uint32_t old_refcount = RefCount(); uint32_t refcount = RefBase::Unref(); if (old_refcount == 1 && refcount == 0) { - _persistent.SetWeak( - this, FinalizeCallback, v8::WeakCallbackType::kParameter); + SetWeak(); } return refcount; } @@ -373,10 +381,11 @@ class Reference : public RefBase { inline void Finalize(bool is_env_teardown = false) override { // During env teardown, `~napi_env()` alone is responsible for finalizing. // Thus, we don't want any stray gc passes to trigger a second call to - // `Finalize()`, so let's reset the persistent here if nothing is - // keeping it alive. - if (is_env_teardown && _persistent.IsWeak()) { - _persistent.ClearWeak(); + // `RefBase::Finalize()`. ClearWeak will ensure that even if the + // gc is in progress no Finalization will be run for this Reference + // by the gc. + if (is_env_teardown) { + ClearWeak(); } // Chain up to perform the rest of the finalization. @@ -384,6 +393,35 @@ class Reference : public RefBase { } private: + // ClearWeak is marking the Reference so that the gc should not + // collect it, but it is possible that a second pass callback + // may have been scheduled already if we are in shutdown. We clear + // the secondPassParameter so that even if it has been + // secheduled no Finalization will be run. + inline void ClearWeak() { + if (!_persistent.IsEmpty()) { + _persistent.ClearWeak(); + } + if (_secondPassParameter != nullptr) { + *_secondPassParameter = nullptr; + } + } + + // Mark the reference as weak and eligible for collection + // by the gc. + inline void SetWeak() { + if (_secondPassParameter == nullptr) { + // This means that the Reference has already been processed + // by the second pass calback, so its already been Finalized, do + // nothing + return; + } + _persistent.SetWeak( + _secondPassParameter, FinalizeCallback, + v8::WeakCallbackType::kParameter); + *_secondPassParameter = this; + } + // The N-API finalizer callback may make calls into the engine. V8's heap is // not in a consistent state during the weak callback, and therefore it does // not support calls back into it. However, it provides a mechanism for adding @@ -391,20 +429,46 @@ class Reference : public RefBase { // attach such a second-pass finalizer from the first pass finalizer. Thus, // we do that here to ensure that the N-API finalizer callback is free to call // into the engine. - static void FinalizeCallback(const v8::WeakCallbackInfo& data) { - Reference* reference = data.GetParameter(); + static void FinalizeCallback( + const v8::WeakCallbackInfo& data) { + SecondPassCallParameterRef* parameter = data.GetParameter(); + Reference* reference = *parameter; + if (reference == nullptr) { + return; + } // The reference must be reset during the first pass. reference->_persistent.Reset(); + // Mark the parameter not delete-able until the second pass callback is + // invoked. + reference->_secondPassParameter = nullptr; data.SetSecondPassCallback(SecondPassCallback); } - static void SecondPassCallback(const v8::WeakCallbackInfo& data) { - data.GetParameter()->Finalize(); + // Second pass callbacks are scheduled with platform tasks. At env teardown, + // the tasks may have already be scheduled and we are unable to cancel the + // second pass callback task. We have to make sure that parameter is kept + // alive until the second pass callback is been invoked. In order to do + // this and still allow our code to Finalize/delete the Reference during + // shutdown we have to use a seperately allocated parameter instead + // of a parameter within the Reference object itself. This is what + // is stored in _secondPassParameter and it is alocated in the + // constructor for the Reference. + static void SecondPassCallback( + const v8::WeakCallbackInfo& data) { + SecondPassCallParameterRef* parameter = data.GetParameter(); + Reference* reference = *parameter; + delete parameter; + if (reference == nullptr) { + // the reference itself has already been deleted so nothing to do + return; + } + reference->Finalize(); } v8impl::Persistent _persistent; + SecondPassCallParameterRef* _secondPassParameter; }; class ArrayBufferReference final : public Reference { From e707514c80371971027cd42682931f171aedd125 Mon Sep 17 00:00:00 2001 From: James M Snell Date: Thu, 15 Apr 2021 09:05:48 -0700 Subject: [PATCH 07/15] src: fix finalization crash PR-URL: https://github.com/nodejs/node/pull/38250 Backport-PR-URL: https://github.com/nodejs/node/pull/42512 Fixes: https://github.com/nodejs/node/issues/38040 Reviewed-By: Beth Griggs Reviewed-By: Antoine du Hamel --- src/js_native_api_v8.cc | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/js_native_api_v8.cc b/src/js_native_api_v8.cc index 8254dc7d6c8aa2..81619b914153c6 100644 --- a/src/js_native_api_v8.cc +++ b/src/js_native_api_v8.cc @@ -379,6 +379,9 @@ class Reference : public RefBase { protected: inline void Finalize(bool is_env_teardown = false) override { + if (is_env_teardown) env_teardown_finalize_started_ = true; + if (!is_env_teardown && env_teardown_finalize_started_) return; + // During env teardown, `~napi_env()` alone is responsible for finalizing. // Thus, we don't want any stray gc passes to trigger a second call to // `RefBase::Finalize()`. ClearWeak will ensure that even if the @@ -467,6 +470,7 @@ class Reference : public RefBase { reference->Finalize(); } + bool env_teardown_finalize_started_ = false; v8impl::Persistent _persistent; SecondPassCallParameterRef* _secondPassParameter; }; From a7224c9559519c830d751766b4d874b21f896fa9 Mon Sep 17 00:00:00 2001 From: Michael Dawson Date: Fri, 30 Apr 2021 17:51:11 -0400 Subject: [PATCH 08/15] node-api: fix shutdown crashes Refs: https://github.com/nodejs/node-addon-api/issues/906 Ensure that finalization is not defered during shutdown. The env for the addon is deleted immediately after iterating the list of finalizers to be run. Defering causes crashes as the finalization uses the already deleted env. Signed-off-by: Michael Dawson PR-URL: https://github.com/nodejs/node/pull/38492 Backport-PR-URL: https://github.com/nodejs/node/pull/42512 Reviewed-By: Anna Henningsen Reviewed-By: Chengzhong Wu Reviewed-By: Gabriel Schulhof --- src/js_native_api_v8.h | 31 +++++++++++++++++++++++++++++++ src/node_api.cc | 9 +++++++-- 2 files changed, 38 insertions(+), 2 deletions(-) diff --git a/src/js_native_api_v8.h b/src/js_native_api_v8.h index 06b8049ec46db0..95f390b58287e8 100644 --- a/src/js_native_api_v8.h +++ b/src/js_native_api_v8.h @@ -122,6 +122,37 @@ struct napi_env__ { void* instance_data = nullptr; }; +// This class is used to keep a napi_env live in a way that +// is exception safe versus calling Ref/Unref directly +class EnvRefHolder { + public: + explicit EnvRefHolder(napi_env env) : _env(env) { + _env->Ref(); + } + + explicit EnvRefHolder(const EnvRefHolder& other): _env(other.env()) { + _env->Ref(); + } + + EnvRefHolder(EnvRefHolder&& other) { + _env = other._env; + other._env = nullptr; + } + + ~EnvRefHolder() { + if (_env != nullptr) { + _env->Unref(); + } + } + + napi_env env(void) const { + return _env; + } + + private: + napi_env _env; +}; + static inline napi_status napi_clear_last_error(napi_env env) { env->last_error.error_code = napi_ok; diff --git a/src/node_api.cc b/src/node_api.cc index 211386ba6d502d..a153e5dfcd204d 100644 --- a/src/node_api.cc +++ b/src/node_api.cc @@ -35,8 +35,13 @@ struct node_napi_env__ : public napi_env__ { } void CallFinalizer(napi_finalize cb, void* data, void* hint) override { - napi_env env = static_cast(this); - node_env()->SetImmediate([=](node::Environment* node_env) { + // we need to keep the env live until the finalizer has been run + // EnvRefHolder provides an exception safe wrapper to Ref and then + // Unref once the lamba is freed + EnvRefHolder liveEnv(static_cast(this)); + node_env()->SetImmediate([=, liveEnv = std::move(liveEnv)] + (node::Environment* node_env) { + napi_env env = liveEnv.env(); v8::HandleScope handle_scope(env->isolate); v8::Context::Scope context_scope(env->context()); env->CallIntoModule([&](napi_env env) { From e23c04f0dcb4a7fbe40bfdac842f15147a819111 Mon Sep 17 00:00:00 2001 From: Michael Dawson Date: Tue, 1 Jun 2021 21:20:55 -0400 Subject: [PATCH 09/15] node-api: avoid SecondPassCallback crash PR https://github.com/nodejs/node/pull/38000 added indirection so that we could stop finalization in cases where it had been scheduled in a second pass callback but we were doing it in advance in environment teardown. Unforunately we missed that the code which tries to clear the second pass parameter checked if the pointer to the parameter (_secondPassParameter) was nullptr and that when the second pass callback was scheduled we set _secondPassParameter to nullptr in order to avoid it being deleted outside of the second pass callback. The net result was that we would not clear the _secondPassParameter contents and failed to avoid the Finalization in the second pass callback. This PR adds an additional boolean for deciding if the secondPassParameter should be deleted outside of the second pass callback instead of setting secondPassParameter to nullptr thus avoiding the conflict between the 2 ways it was being used. See the discussion starting at: https://github.com/nodejs/node/pull/38273#issuecomment-852403751 for how this was discovered on OSX while trying to upgrade to a new V8 version. Signed-off-by: Michael Dawson PR-URL: https://github.com/nodejs/node/pull/38899 Backport-PR-URL: https://github.com/nodejs/node/pull/42512 Reviewed-By: Chengzhong Wu Reviewed-By: James M Snell --- src/js_native_api_v8.cc | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/js_native_api_v8.cc b/src/js_native_api_v8.cc index 81619b914153c6..8401ed30a64748 100644 --- a/src/js_native_api_v8.cc +++ b/src/js_native_api_v8.cc @@ -320,7 +320,8 @@ class Reference : public RefBase { Reference(napi_env env, v8::Local value, Args&&... args) : RefBase(env, std::forward(args)...), _persistent(env->isolate, value), - _secondPassParameter(new SecondPassCallParameterRef(this)) { + _secondPassParameter(new SecondPassCallParameterRef(this)), + _secondPassScheduled(false) { if (RefCount() == 0) { SetWeak(); } @@ -347,7 +348,7 @@ class Reference : public RefBase { // If the second pass callback is scheduled, it will delete the // parameter passed to it, otherwise it will never be scheduled // and we need to delete it here. - if (_secondPassParameter != nullptr) { + if (!_secondPassScheduled) { delete _secondPassParameter; } } @@ -444,8 +445,7 @@ class Reference : public RefBase { reference->_persistent.Reset(); // Mark the parameter not delete-able until the second pass callback is // invoked. - reference->_secondPassParameter = nullptr; - + reference->_secondPassScheduled = true; data.SetSecondPassCallback(SecondPassCallback); } @@ -467,12 +467,14 @@ class Reference : public RefBase { // the reference itself has already been deleted so nothing to do return; } + reference->_secondPassParameter = nullptr; reference->Finalize(); } bool env_teardown_finalize_started_ = false; v8impl::Persistent _persistent; SecondPassCallParameterRef* _secondPassParameter; + bool _secondPassScheduled; }; class ArrayBufferReference final : public Reference { From 5f104e3218f7b71f6fa9ba3b98a272843685568c Mon Sep 17 00:00:00 2001 From: legendecas Date: Tue, 8 Jun 2021 21:53:07 +0800 Subject: [PATCH 10/15] node-api: cctest on v8impl::Reference PR-URL: https://github.com/nodejs/node/pull/38970 Backport-PR-URL: https://github.com/nodejs/node/pull/42512 Reviewed-By: Michael Dawson Reviewed-By: Gabriel Schulhof --- LICENSE | 2 +- node.gyp | 2 + src/gtest/LICENSE | 28 ++ src/gtest/gtest_prod.h | 61 +++ src/js_native_api_v8.cc | 556 +++++++++++++-------------- src/js_native_api_v8.h | 75 ++++ src/js_native_api_v8_internals.h | 1 + src/node_api.cc | 68 ++-- src/node_api_internals.h | 30 ++ test/cctest/test_js_native_api_v8.cc | 102 +++++ test/cctest/test_node_api.cc | 41 ++ 11 files changed, 635 insertions(+), 331 deletions(-) create mode 100644 src/gtest/LICENSE create mode 100644 src/gtest/gtest_prod.h create mode 100644 src/node_api_internals.h create mode 100644 test/cctest/test_js_native_api_v8.cc create mode 100644 test/cctest/test_node_api.cc diff --git a/LICENSE b/LICENSE index 298631ee09e2dd..e18293b3628173 100644 --- a/LICENSE +++ b/LICENSE @@ -1294,7 +1294,7 @@ The externally maintained libraries used by Node.js are: WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. """ -- gtest, located at test/cctest/gtest, is licensed as follows: +- gtest, located at src/gtest and test/cctest/gtest, is licensed as follows: """ Copyright 2008, Google Inc. All rights reserved. diff --git a/node.gyp b/node.gyp index 48fd7a124a6961..3bb6ce32fab8e5 100644 --- a/node.gyp +++ b/node.gyp @@ -1232,7 +1232,9 @@ 'test/cctest/test_base_object_ptr.cc', 'test/cctest/test_node_postmortem_metadata.cc', 'test/cctest/test_environment.cc', + 'test/cctest/test_js_native_api_v8.cc', 'test/cctest/test_linked_binding.cc', + 'test/cctest/test_node_api.cc', 'test/cctest/test_per_process.cc', 'test/cctest/test_platform.cc', 'test/cctest/test_json_utils.cc', diff --git a/src/gtest/LICENSE b/src/gtest/LICENSE new file mode 100644 index 00000000000000..1941a11f8ce943 --- /dev/null +++ b/src/gtest/LICENSE @@ -0,0 +1,28 @@ +Copyright 2008, Google Inc. +All rights reserved. + +Redistribution and use in source and binary forms, with or without +modification, are permitted provided that the following conditions are +met: + + * Redistributions of source code must retain the above copyright +notice, this list of conditions and the following disclaimer. + * Redistributions in binary form must reproduce the above +copyright notice, this list of conditions and the following disclaimer +in the documentation and/or other materials provided with the +distribution. + * Neither the name of Google Inc. nor the names of its +contributors may be used to endorse or promote products derived from +this software without specific prior written permission. + +THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS +"AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT +LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR +A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT +OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, +SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT +LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, +DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY +THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT +(INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE +OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. diff --git a/src/gtest/gtest_prod.h b/src/gtest/gtest_prod.h new file mode 100644 index 00000000000000..473387f170c838 --- /dev/null +++ b/src/gtest/gtest_prod.h @@ -0,0 +1,61 @@ +// Copyright 2006, Google Inc. +// All rights reserved. +// +// Redistribution and use in source and binary forms, with or without +// modification, are permitted provided that the following conditions are +// met: +// +// * Redistributions of source code must retain the above copyright +// notice, this list of conditions and the following disclaimer. +// * Redistributions in binary form must reproduce the above +// copyright notice, this list of conditions and the following disclaimer +// in the documentation and/or other materials provided with the +// distribution. +// * Neither the name of Google Inc. nor the names of its +// contributors may be used to endorse or promote products derived from +// this software without specific prior written permission. +// +// THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS +// "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT +// LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR +// A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT +// OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, +// SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT +// LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, +// DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY +// THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT +// (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE +// OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + +// +// Google C++ Testing and Mocking Framework definitions useful in production +// code. GOOGLETEST_CM0003 DO NOT DELETE + +#ifndef GOOGLETEST_INCLUDE_GTEST_GTEST_PROD_H_ // NOLINT +#define GOOGLETEST_INCLUDE_GTEST_GTEST_PROD_H_ + +// When you need to test the private or protected members of a class, +// use the FRIEND_TEST macro to declare your tests as friends of the +// class. For example: +// +// class MyClass { +// private: +// void PrivateMethod(); +// FRIEND_TEST(MyClassTest, PrivateMethodWorks); +// }; +// +// class MyClassTest : public testing::Test { +// // ... +// }; +// +// TEST_F(MyClassTest, PrivateMethodWorks) { +// // Can call MyClass::PrivateMethod() here. +// } +// +// Note: The test class must be in the same namespace as the class being tested. +// For example, putting MyClassTest in an anonymous namespace will not work. + +#define FRIEND_TEST(test_case_name, test_name) \ + friend class test_case_name##_##test_name##_Test + +#endif // GOOGLETEST_INCLUDE_GTEST_GTEST_PROD_H_ // NOLINT diff --git a/src/js_native_api_v8.cc b/src/js_native_api_v8.cc index 8401ed30a64748..d8239fb1cb1c36 100644 --- a/src/js_native_api_v8.cc +++ b/src/js_native_api_v8.cc @@ -188,295 +188,6 @@ inline static napi_status ConcludeDeferred(napi_env env, return GET_RETURN_STATUS(env); } -// Wrapper around v8impl::Persistent that implements reference counting. -class RefBase : protected Finalizer, RefTracker { - protected: - RefBase(napi_env env, - uint32_t initial_refcount, - bool delete_self, - napi_finalize finalize_callback, - void* finalize_data, - void* finalize_hint) - : Finalizer(env, finalize_callback, finalize_data, finalize_hint), - _refcount(initial_refcount), - _delete_self(delete_self) { - Link(finalize_callback == nullptr - ? &env->reflist - : &env->finalizing_reflist); - } - - public: - static RefBase* New(napi_env env, - uint32_t initial_refcount, - bool delete_self, - napi_finalize finalize_callback, - void* finalize_data, - void* finalize_hint) { - return new RefBase(env, - initial_refcount, - delete_self, - finalize_callback, - finalize_data, - finalize_hint); - } - - inline void* Data() { - return _finalize_data; - } - - // Delete is called in 2 ways. Either from the finalizer or - // from one of Unwrap or napi_delete_reference. - // - // When it is called from Unwrap or napi_delete_reference we only - // want to do the delete if the finalizer has already run or - // cannot have been queued to run (ie the reference count is > 0), - // otherwise we may crash when the finalizer does run. - // If the finalizer may have been queued and has not already run - // delay the delete until the finalizer runs by not doing the delete - // and setting _delete_self to true so that the finalizer will - // delete it when it runs. - // - // The second way this is called is from - // the finalizer and _delete_self is set. In this case we - // know we need to do the deletion so just do it. - static inline void Delete(RefBase* reference) { - reference->Unlink(); - if ((reference->RefCount() != 0) || - (reference->_delete_self) || - (reference->_finalize_ran)) { - delete reference; - } else { - // defer until finalizer runs as - // it may alread be queued - reference->_delete_self = true; - } - } - - inline uint32_t Ref() { - return ++_refcount; - } - - inline uint32_t Unref() { - if (_refcount == 0) { - return 0; - } - return --_refcount; - } - - inline uint32_t RefCount() { - return _refcount; - } - - protected: - inline void Finalize(bool is_env_teardown = false) override { - // In addition to being called during environment teardown, this method is - // also the entry point for the garbage collector. During environment - // teardown we have to remove the garbage collector's reference to this - // method so that, if, as part of the user's callback, JS gets executed, - // resulting in a garbage collection pass, this method is not re-entered as - // part of that pass, because that'll cause a double free (as seen in - // https://github.com/nodejs/node/issues/37236). - // - // Since this class does not have access to the V8 persistent reference, - // this method is overridden in the `Reference` class below. Therein the - // weak callback is removed, ensuring that the garbage collector does not - // re-enter this method, and the method chains up to continue the process of - // environment-teardown-induced finalization. - - // During environment teardown we have to convert a strong reference to - // a weak reference to force the deferring behavior if the user's finalizer - // happens to delete this reference so that the code in this function that - // follows the call to the user's finalizer may safely access variables from - // this instance. - if (is_env_teardown && RefCount() > 0) _refcount = 0; - - if (_finalize_callback != nullptr) { - // This ensures that we never call the finalizer twice. - napi_finalize fini = _finalize_callback; - _finalize_callback = nullptr; - _env->CallFinalizer(fini, _finalize_data, _finalize_hint); - } - - // this is safe because if a request to delete the reference - // is made in the finalize_callback it will defer deletion - // to this block and set _delete_self to true - if (_delete_self || is_env_teardown) { - Delete(this); - } else { - _finalize_ran = true; - } - } - - private: - uint32_t _refcount; - bool _delete_self; -}; - -class Reference : public RefBase { - using SecondPassCallParameterRef = Reference*; - - protected: - template - Reference(napi_env env, v8::Local value, Args&&... args) - : RefBase(env, std::forward(args)...), - _persistent(env->isolate, value), - _secondPassParameter(new SecondPassCallParameterRef(this)), - _secondPassScheduled(false) { - if (RefCount() == 0) { - SetWeak(); - } - } - - public: - static inline Reference* New(napi_env env, - v8::Local value, - uint32_t initial_refcount, - bool delete_self, - napi_finalize finalize_callback = nullptr, - void* finalize_data = nullptr, - void* finalize_hint = nullptr) { - return new Reference(env, - value, - initial_refcount, - delete_self, - finalize_callback, - finalize_data, - finalize_hint); - } - - virtual ~Reference() { - // If the second pass callback is scheduled, it will delete the - // parameter passed to it, otherwise it will never be scheduled - // and we need to delete it here. - if (!_secondPassScheduled) { - delete _secondPassParameter; - } - } - - inline uint32_t Ref() { - uint32_t refcount = RefBase::Ref(); - if (refcount == 1) { - ClearWeak(); - } - return refcount; - } - - inline uint32_t Unref() { - uint32_t old_refcount = RefCount(); - uint32_t refcount = RefBase::Unref(); - if (old_refcount == 1 && refcount == 0) { - SetWeak(); - } - return refcount; - } - - inline v8::Local Get() { - if (_persistent.IsEmpty()) { - return v8::Local(); - } else { - return v8::Local::New(_env->isolate, _persistent); - } - } - - protected: - inline void Finalize(bool is_env_teardown = false) override { - if (is_env_teardown) env_teardown_finalize_started_ = true; - if (!is_env_teardown && env_teardown_finalize_started_) return; - - // During env teardown, `~napi_env()` alone is responsible for finalizing. - // Thus, we don't want any stray gc passes to trigger a second call to - // `RefBase::Finalize()`. ClearWeak will ensure that even if the - // gc is in progress no Finalization will be run for this Reference - // by the gc. - if (is_env_teardown) { - ClearWeak(); - } - - // Chain up to perform the rest of the finalization. - RefBase::Finalize(is_env_teardown); - } - - private: - // ClearWeak is marking the Reference so that the gc should not - // collect it, but it is possible that a second pass callback - // may have been scheduled already if we are in shutdown. We clear - // the secondPassParameter so that even if it has been - // secheduled no Finalization will be run. - inline void ClearWeak() { - if (!_persistent.IsEmpty()) { - _persistent.ClearWeak(); - } - if (_secondPassParameter != nullptr) { - *_secondPassParameter = nullptr; - } - } - - // Mark the reference as weak and eligible for collection - // by the gc. - inline void SetWeak() { - if (_secondPassParameter == nullptr) { - // This means that the Reference has already been processed - // by the second pass calback, so its already been Finalized, do - // nothing - return; - } - _persistent.SetWeak( - _secondPassParameter, FinalizeCallback, - v8::WeakCallbackType::kParameter); - *_secondPassParameter = this; - } - - // The N-API finalizer callback may make calls into the engine. V8's heap is - // not in a consistent state during the weak callback, and therefore it does - // not support calls back into it. However, it provides a mechanism for adding - // a finalizer which may make calls back into the engine by allowing us to - // attach such a second-pass finalizer from the first pass finalizer. Thus, - // we do that here to ensure that the N-API finalizer callback is free to call - // into the engine. - static void FinalizeCallback( - const v8::WeakCallbackInfo& data) { - SecondPassCallParameterRef* parameter = data.GetParameter(); - Reference* reference = *parameter; - if (reference == nullptr) { - return; - } - - // The reference must be reset during the first pass. - reference->_persistent.Reset(); - // Mark the parameter not delete-able until the second pass callback is - // invoked. - reference->_secondPassScheduled = true; - data.SetSecondPassCallback(SecondPassCallback); - } - - // Second pass callbacks are scheduled with platform tasks. At env teardown, - // the tasks may have already be scheduled and we are unable to cancel the - // second pass callback task. We have to make sure that parameter is kept - // alive until the second pass callback is been invoked. In order to do - // this and still allow our code to Finalize/delete the Reference during - // shutdown we have to use a seperately allocated parameter instead - // of a parameter within the Reference object itself. This is what - // is stored in _secondPassParameter and it is alocated in the - // constructor for the Reference. - static void SecondPassCallback( - const v8::WeakCallbackInfo& data) { - SecondPassCallParameterRef* parameter = data.GetParameter(); - Reference* reference = *parameter; - delete parameter; - if (reference == nullptr) { - // the reference itself has already been deleted so nothing to do - return; - } - reference->_secondPassParameter = nullptr; - reference->Finalize(); - } - - bool env_teardown_finalize_started_ = false; - v8impl::Persistent _persistent; - SecondPassCallParameterRef* _secondPassParameter; - bool _secondPassScheduled; -}; - class ArrayBufferReference final : public Reference { public: // Same signatures for ctor and New() as Reference, except this only works @@ -823,6 +534,273 @@ inline napi_status Wrap(napi_env env, } // end of anonymous namespace +// Wrapper around v8impl::Persistent that implements reference counting. +RefBase::RefBase(napi_env env, + uint32_t initial_refcount, + bool delete_self, + napi_finalize finalize_callback, + void* finalize_data, + void* finalize_hint) + : Finalizer(env, finalize_callback, finalize_data, finalize_hint), + _refcount(initial_refcount), + _delete_self(delete_self) { + Link(finalize_callback == nullptr ? &env->reflist : &env->finalizing_reflist); +} + +RefBase* RefBase::New(napi_env env, + uint32_t initial_refcount, + bool delete_self, + napi_finalize finalize_callback, + void* finalize_data, + void* finalize_hint) { + return new RefBase(env, + initial_refcount, + delete_self, + finalize_callback, + finalize_data, + finalize_hint); +} + +RefBase::~RefBase() { + Unlink(); +} + +void* RefBase::Data() { + return _finalize_data; +} + +// Delete is called in 2 ways. Either from the finalizer or +// from one of Unwrap or napi_delete_reference. +// +// When it is called from Unwrap or napi_delete_reference we only +// want to do the delete if the finalizer has already run or +// cannot have been queued to run (ie the reference count is > 0), +// otherwise we may crash when the finalizer does run. +// If the finalizer may have been queued and has not already run +// delay the delete until the finalizer runs by not doing the delete +// and setting _delete_self to true so that the finalizer will +// delete it when it runs. +// +// The second way this is called is from +// the finalizer and _delete_self is set. In this case we +// know we need to do the deletion so just do it. +void RefBase::Delete(RefBase* reference) { + if ((reference->RefCount() != 0) || (reference->_delete_self) || + (reference->_finalize_ran)) { + delete reference; + } else { + // defer until finalizer runs as + // it may already be queued + reference->_delete_self = true; + } +} + +uint32_t RefBase::Ref() { + return ++_refcount; +} + +uint32_t RefBase::Unref() { + if (_refcount == 0) { + return 0; + } + return --_refcount; +} + +uint32_t RefBase::RefCount() { + return _refcount; +} + +void RefBase::Finalize(bool is_env_teardown) { + // In addition to being called during environment teardown, this method is + // also the entry point for the garbage collector. During environment + // teardown we have to remove the garbage collector's reference to this + // method so that, if, as part of the user's callback, JS gets executed, + // resulting in a garbage collection pass, this method is not re-entered as + // part of that pass, because that'll cause a double free (as seen in + // https://github.com/nodejs/node/issues/37236). + // + // Since this class does not have access to the V8 persistent reference, + // this method is overridden in the `Reference` class below. Therein the + // weak callback is removed, ensuring that the garbage collector does not + // re-enter this method, and the method chains up to continue the process of + // environment-teardown-induced finalization. + + // During environment teardown we have to convert a strong reference to + // a weak reference to force the deferring behavior if the user's finalizer + // happens to delete this reference so that the code in this function that + // follows the call to the user's finalizer may safely access variables from + // this instance. + if (is_env_teardown && RefCount() > 0) _refcount = 0; + + if (_finalize_callback != nullptr) { + // This ensures that we never call the finalizer twice. + napi_finalize fini = _finalize_callback; + _finalize_callback = nullptr; + _env->CallFinalizer(fini, _finalize_data, _finalize_hint); + } + + // this is safe because if a request to delete the reference + // is made in the finalize_callback it will defer deletion + // to this block and set _delete_self to true + if (_delete_self || is_env_teardown) { + Delete(this); + } else { + _finalize_ran = true; + } +} + +template +Reference::Reference(napi_env env, v8::Local value, Args&&... args) + : RefBase(env, std::forward(args)...), + _persistent(env->isolate, value), + _secondPassParameter(new SecondPassCallParameterRef(this)), + _secondPassScheduled(false) { + if (RefCount() == 0) { + SetWeak(); + } +} + +Reference* Reference::New(napi_env env, + v8::Local value, + uint32_t initial_refcount, + bool delete_self, + napi_finalize finalize_callback, + void* finalize_data, + void* finalize_hint) { + return new Reference(env, + value, + initial_refcount, + delete_self, + finalize_callback, + finalize_data, + finalize_hint); +} + +Reference::~Reference() { + // If the second pass callback is scheduled, it will delete the + // parameter passed to it, otherwise it will never be scheduled + // and we need to delete it here. + if (!_secondPassScheduled) { + delete _secondPassParameter; + } +} + +uint32_t Reference::Ref() { + uint32_t refcount = RefBase::Ref(); + if (refcount == 1) { + ClearWeak(); + } + return refcount; +} + +uint32_t Reference::Unref() { + uint32_t old_refcount = RefCount(); + uint32_t refcount = RefBase::Unref(); + if (old_refcount == 1 && refcount == 0) { + SetWeak(); + } + return refcount; +} + +v8::Local Reference::Get() { + if (_persistent.IsEmpty()) { + return v8::Local(); + } else { + return v8::Local::New(_env->isolate, _persistent); + } +} + +void Reference::Finalize(bool is_env_teardown) { + if (is_env_teardown) env_teardown_finalize_started_ = true; + if (!is_env_teardown && env_teardown_finalize_started_) return; + + // During env teardown, `~napi_env()` alone is responsible for finalizing. + // Thus, we don't want any stray gc passes to trigger a second call to + // `RefBase::Finalize()`. ClearWeak will ensure that even if the + // gc is in progress no Finalization will be run for this Reference + // by the gc. + if (is_env_teardown) { + ClearWeak(); + } + + // Chain up to perform the rest of the finalization. + RefBase::Finalize(is_env_teardown); +} + +// ClearWeak is marking the Reference so that the gc should not +// collect it, but it is possible that a second pass callback +// may have been scheduled already if we are in shutdown. We clear +// the secondPassParameter so that even if it has been +// scheduled no Finalization will be run. +void Reference::ClearWeak() { + if (!_persistent.IsEmpty()) { + _persistent.ClearWeak(); + } + if (_secondPassParameter != nullptr) { + *_secondPassParameter = nullptr; + } +} + +// Mark the reference as weak and eligible for collection +// by the gc. +void Reference::SetWeak() { + if (_secondPassParameter == nullptr) { + // This means that the Reference has already been processed + // by the second pass callback, so its already been Finalized, do + // nothing + return; + } + _persistent.SetWeak( + _secondPassParameter, FinalizeCallback, v8::WeakCallbackType::kParameter); + *_secondPassParameter = this; +} + +// The N-API finalizer callback may make calls into the engine. V8's heap is +// not in a consistent state during the weak callback, and therefore it does +// not support calls back into it. However, it provides a mechanism for adding +// a finalizer which may make calls back into the engine by allowing us to +// attach such a second-pass finalizer from the first pass finalizer. Thus, +// we do that here to ensure that the N-API finalizer callback is free to call +// into the engine. +void Reference::FinalizeCallback( + const v8::WeakCallbackInfo& data) { + SecondPassCallParameterRef* parameter = data.GetParameter(); + Reference* reference = *parameter; + if (reference == nullptr) { + return; + } + + // The reference must be reset during the first pass. + reference->_persistent.Reset(); + // Mark the parameter not delete-able until the second pass callback is + // invoked. + reference->_secondPassScheduled = true; + + data.SetSecondPassCallback(SecondPassCallback); +} + +// Second pass callbacks are scheduled with platform tasks. At env teardown, +// the tasks may have already be scheduled and we are unable to cancel the +// second pass callback task. We have to make sure that parameter is kept +// alive until the second pass callback is been invoked. In order to do +// this and still allow our code to Finalize/delete the Reference during +// shutdown we have to use a separately allocated parameter instead +// of a parameter within the Reference object itself. This is what +// is stored in _secondPassParameter and it is allocated in the +// constructor for the Reference. +void Reference::SecondPassCallback( + const v8::WeakCallbackInfo& data) { + SecondPassCallParameterRef* parameter = data.GetParameter(); + Reference* reference = *parameter; + delete parameter; + if (reference == nullptr) { + // the reference itself has already been deleted so nothing to do + return; + } + reference->_secondPassParameter = nullptr; + reference->Finalize(); +} + } // end of namespace v8impl // Warning: Keep in-sync with napi_status enum diff --git a/src/js_native_api_v8.h b/src/js_native_api_v8.h index 95f390b58287e8..1ffbce8be2cefa 100644 --- a/src/js_native_api_v8.h +++ b/src/js_native_api_v8.h @@ -366,6 +366,81 @@ class TryCatch : public v8::TryCatch { napi_env _env; }; +// Wrapper around v8impl::Persistent that implements reference counting. +class RefBase : protected Finalizer, RefTracker { + protected: + RefBase(napi_env env, + uint32_t initial_refcount, + bool delete_self, + napi_finalize finalize_callback, + void* finalize_data, + void* finalize_hint); + + public: + static RefBase* New(napi_env env, + uint32_t initial_refcount, + bool delete_self, + napi_finalize finalize_callback, + void* finalize_data, + void* finalize_hint); + + static inline void Delete(RefBase* reference); + + virtual ~RefBase(); + void* Data(); + uint32_t Ref(); + uint32_t Unref(); + uint32_t RefCount(); + + protected: + void Finalize(bool is_env_teardown = false) override; + + private: + uint32_t _refcount; + bool _delete_self; +}; + +class Reference : public RefBase { + using SecondPassCallParameterRef = Reference*; + + protected: + template + Reference(napi_env env, v8::Local value, Args&&... args); + + public: + static Reference* New(napi_env env, + v8::Local value, + uint32_t initial_refcount, + bool delete_self, + napi_finalize finalize_callback = nullptr, + void* finalize_data = nullptr, + void* finalize_hint = nullptr); + + virtual ~Reference(); + uint32_t Ref(); + uint32_t Unref(); + v8::Local Get(); + + protected: + void Finalize(bool is_env_teardown = false) override; + + private: + void ClearWeak(); + void SetWeak(); + + static void FinalizeCallback( + const v8::WeakCallbackInfo& data); + static void SecondPassCallback( + const v8::WeakCallbackInfo& data); + + bool env_teardown_finalize_started_ = false; + v8impl::Persistent _persistent; + SecondPassCallParameterRef* _secondPassParameter; + bool _secondPassScheduled; + + FRIEND_TEST(JsNativeApiV8Test, Reference); +}; + } // end of namespace v8impl #endif // SRC_JS_NATIVE_API_V8_H_ diff --git a/src/js_native_api_v8_internals.h b/src/js_native_api_v8_internals.h index ddd219818cdfa9..8428390ef1eaf3 100644 --- a/src/js_native_api_v8_internals.h +++ b/src/js_native_api_v8_internals.h @@ -16,6 +16,7 @@ #include "node_version.h" #include "env.h" #include "node_internals.h" +#include "gtest/gtest_prod.h" #define NAPI_ARRAYSIZE(array) \ node::arraysize((array)) diff --git a/src/node_api.cc b/src/node_api.cc index a153e5dfcd204d..d5e05bcdd96ed0 100644 --- a/src/node_api.cc +++ b/src/node_api.cc @@ -2,6 +2,7 @@ #define NAPI_EXPERIMENTAL #include "js_native_api_v8.h" #include "node_api.h" +#include "node_api_internals.h" #include "node_binding.h" #include "node_buffer.h" #include "node_errors.h" @@ -11,51 +12,36 @@ #include -struct node_napi_env__ : public napi_env__ { - explicit node_napi_env__(v8::Local context, - const std::string& module_filename): - napi_env__(context), filename(module_filename) { - CHECK_NOT_NULL(node_env()); - } - - inline node::Environment* node_env() const { - return node::Environment::GetCurrent(context()); - } +node_napi_env__::node_napi_env__(v8::Local context, + const std::string& module_filename) + : napi_env__(context), filename(module_filename) { + CHECK_NOT_NULL(node_env()); +} - bool can_call_into_js() const override { - return node_env()->can_call_into_js(); - } +bool node_napi_env__::can_call_into_js() const { + return node_env()->can_call_into_js(); +} - v8::Maybe mark_arraybuffer_as_untransferable( - v8::Local ab) const override { - return ab->SetPrivate( - context(), - node_env()->untransferable_object_private_symbol(), - v8::True(isolate)); - } +v8::Maybe node_napi_env__::mark_arraybuffer_as_untransferable( + v8::Local ab) const { + return ab->SetPrivate(context(), + node_env()->untransferable_object_private_symbol(), + v8::True(isolate)); +} - void CallFinalizer(napi_finalize cb, void* data, void* hint) override { - // we need to keep the env live until the finalizer has been run - // EnvRefHolder provides an exception safe wrapper to Ref and then - // Unref once the lamba is freed - EnvRefHolder liveEnv(static_cast(this)); - node_env()->SetImmediate([=, liveEnv = std::move(liveEnv)] - (node::Environment* node_env) { - napi_env env = liveEnv.env(); - v8::HandleScope handle_scope(env->isolate); - v8::Context::Scope context_scope(env->context()); - env->CallIntoModule([&](napi_env env) { - cb(env, data, hint); +void node_napi_env__::CallFinalizer(napi_finalize cb, void* data, void* hint) { + // we need to keep the env live until the finalizer has been run + // EnvRefHolder provides an exception safe wrapper to Ref and then + // Unref once the lamba is freed + EnvRefHolder liveEnv(static_cast(this)); + node_env()->SetImmediate( + [=, liveEnv = std::move(liveEnv)](node::Environment* node_env) { + napi_env env = liveEnv.env(); + v8::HandleScope handle_scope(env->isolate); + v8::Context::Scope context_scope(env->context()); + env->CallIntoModule([&](napi_env env) { cb(env, data, hint); }); }); - }); - } - - const char* GetFilename() const { return filename.c_str(); } - - std::string filename; -}; - -typedef node_napi_env__* node_napi_env; +} namespace v8impl { diff --git a/src/node_api_internals.h b/src/node_api_internals.h new file mode 100644 index 00000000000000..318ada38083435 --- /dev/null +++ b/src/node_api_internals.h @@ -0,0 +1,30 @@ +#ifndef SRC_NODE_API_INTERNALS_H_ +#define SRC_NODE_API_INTERNALS_H_ + +#include "v8.h" +#define NAPI_EXPERIMENTAL +#include "env-inl.h" +#include "js_native_api_v8.h" +#include "node_api.h" +#include "util-inl.h" + +struct node_napi_env__ : public napi_env__ { + node_napi_env__(v8::Local context, + const std::string& module_filename); + + bool can_call_into_js() const override; + v8::Maybe mark_arraybuffer_as_untransferable( + v8::Local ab) const override; + void CallFinalizer(napi_finalize cb, void* data, void* hint) override; + + inline node::Environment* node_env() const { + return node::Environment::GetCurrent(context()); + } + inline const char* GetFilename() const { return filename.c_str(); } + + std::string filename; +}; + +using node_napi_env = node_napi_env__*; + +#endif // SRC_NODE_API_INTERNALS_H_ diff --git a/test/cctest/test_js_native_api_v8.cc b/test/cctest/test_js_native_api_v8.cc new file mode 100644 index 00000000000000..2d95c86d09d5d5 --- /dev/null +++ b/test/cctest/test_js_native_api_v8.cc @@ -0,0 +1,102 @@ +#include +#include +#include +#include "env-inl.h" +#include "gtest/gtest.h" +#include "js_native_api_v8.h" +#include "node_api_internals.h" +#include "node_binding.h" +#include "node_test_fixture.h" + +namespace v8impl { + +using v8::Local; +using v8::Object; + +static napi_env addon_env; +static uint32_t finalizer_call_count = 0; + +class JsNativeApiV8Test : public EnvironmentTestFixture { + private: + void SetUp() override { + EnvironmentTestFixture::SetUp(); + finalizer_call_count = 0; + } + + void TearDown() override { NodeTestFixture::TearDown(); } +}; + +TEST_F(JsNativeApiV8Test, Reference) { + const v8::HandleScope handle_scope(isolate_); + Argv argv; + + napi_ref ref; + void* embedder_fields[v8::kEmbedderFieldsInWeakCallback] = {nullptr, nullptr}; + v8::WeakCallbackInfo::Callback + callback; + Reference::SecondPassCallParameterRef* parameter = nullptr; + + { + Env test_env{handle_scope, argv}; + + node::Environment* env = *test_env; + node::LoadEnvironment(env, ""); + + napi_addon_register_func init = [](napi_env env, napi_value exports) { + addon_env = env; + return exports; + }; + Local module_obj = Object::New(isolate_); + Local exports_obj = Object::New(isolate_); + napi_module_register_by_symbol( + exports_obj, module_obj, env->context(), init); + ASSERT_NE(addon_env, nullptr); + node_napi_env internal_env = reinterpret_cast(addon_env); + EXPECT_EQ(internal_env->node_env(), env); + + // Create a new scope to manage the handles. + { + const v8::HandleScope handle_scope(isolate_); + napi_value value; + napi_create_object(addon_env, &value); + // Create a weak reference; + napi_add_finalizer( + addon_env, + value, + nullptr, + [](napi_env env, void* finalize_data, void* finalize_hint) { + finalizer_call_count++; + }, + nullptr, + &ref); + parameter = reinterpret_cast(ref)->_secondPassParameter; + } + + // We can hardly trigger a non-forced Garbage Collection in a stable way. + // Here we just invoke the weak callbacks directly. + // The persistant handles should be reset in the weak callback in respect + // to the API contract of v8 weak callbacks. + v8::WeakCallbackInfo data( + reinterpret_cast(isolate_), + parameter, + embedder_fields, + &callback); + Reference::FinalizeCallback(data); + EXPECT_EQ(callback, &Reference::SecondPassCallback); + } + // Env goes out of scope, the environment has been teardown. All node-api ref + // trackers should have been destroyed. + + // Now we call the second pass callback to verify the method do not abort with + // memory violations. + v8::WeakCallbackInfo data( + reinterpret_cast(isolate_), + parameter, + embedder_fields, + nullptr); + Reference::SecondPassCallback(data); + + // After Environment Teardown + EXPECT_EQ(finalizer_call_count, uint32_t(1)); +} +} // namespace v8impl diff --git a/test/cctest/test_node_api.cc b/test/cctest/test_node_api.cc new file mode 100644 index 00000000000000..ad5be52fc8ffcc --- /dev/null +++ b/test/cctest/test_node_api.cc @@ -0,0 +1,41 @@ +#include +#include +#include +#include "env-inl.h" +#include "gtest/gtest.h" +#include "node_api_internals.h" +#include "node_binding.h" +#include "node_test_fixture.h" + +using v8::Local; +using v8::Object; + +static napi_env addon_env; + +class NodeApiTest : public EnvironmentTestFixture { + private: + void SetUp() override { EnvironmentTestFixture::SetUp(); } + + void TearDown() override { NodeTestFixture::TearDown(); } +}; + +TEST_F(NodeApiTest, CreateNodeApiEnv) { + const v8::HandleScope handle_scope(isolate_); + Argv argv; + + Env test_env{handle_scope, argv}; + + node::Environment* env = *test_env; + node::LoadEnvironment(env, ""); + + napi_addon_register_func init = [](napi_env env, napi_value exports) { + addon_env = env; + return exports; + }; + Local module_obj = Object::New(isolate_); + Local exports_obj = Object::New(isolate_); + napi_module_register_by_symbol(exports_obj, module_obj, env->context(), init); + ASSERT_NE(addon_env, nullptr); + node_napi_env internal_env = reinterpret_cast(addon_env); + EXPECT_EQ(internal_env->node_env(), env); +} From a9c38f10030bb783e56c2feb1cb2ca9851ca5208 Mon Sep 17 00:00:00 2001 From: Danielle Adams Date: Wed, 7 Oct 2020 14:04:37 -0400 Subject: [PATCH 11/15] doc: add release key for Danielle Adams Add Danielle Adams's release key. PR-URL: https://github.com/nodejs/node/pull/35545 Reviewed-By: Richard Lau Reviewed-By: Beth Griggs Reviewed-By: Shelley Vohr Reviewed-By: Myles Borins Reviewed-By: Rich Trott Reviewed-By: Luigi Pinca --- README.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/README.md b/README.md index 8b515562c21ccf..17d084223d6fe6 100644 --- a/README.md +++ b/README.md @@ -585,6 +585,8 @@ Primary GPG keys for Node.js Releasers (some Releasers sign with subkeys): `141F07595B7B3FFE74309A937405533BE57C7D57` * **Colin Ihrig** <cjihrig@gmail.com> `94AE36675C464D64BAFA68DD7434390BDBE9B9C5` +* **Danielle Adams** <adamzdanielle@gmail.com> +`1C050899334244A8AF75E53792EF661D867B9DFA` * **James M Snell** <jasnell@keybase.io> `71DCFD284A79C3B38668286BC97EC7A07EDE3FC1` * **Michaël Zasso** <targos@protonmail.com> @@ -608,6 +610,7 @@ To import the full set of trusted release keys: gpg --keyserver pool.sks-keyservers.net --recv-keys 4ED778F539E3634C779C87C6D7062848A1AB005C gpg --keyserver pool.sks-keyservers.net --recv-keys 141F07595B7B3FFE74309A937405533BE57C7D57 gpg --keyserver pool.sks-keyservers.net --recv-keys 94AE36675C464D64BAFA68DD7434390BDBE9B9C5 +gpg --keyserver pool.sks-keyservers.net --recv-keys 1C050899334244A8AF75E53792EF661D867B9DFA gpg --keyserver pool.sks-keyservers.net --recv-keys 71DCFD284A79C3B38668286BC97EC7A07EDE3FC1 gpg --keyserver pool.sks-keyservers.net --recv-keys 8FCCA13FEF1D0C2E91008E09770F7A9A5AE15600 gpg --keyserver pool.sks-keyservers.net --recv-keys C4F0DFFF4E8C1A8236409D08E73BC641CC11F4C8 From 11aef2ad03ba7df91a4f25189e3691ce62c81299 Mon Sep 17 00:00:00 2001 From: Danielle Adams Date: Mon, 28 Dec 2020 18:20:19 -0500 Subject: [PATCH 12/15] doc: update release key for Danielle Adams MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR-URL: https://github.com/nodejs/node/pull/36793 Reviewed-By: Rich Trott Reviewed-By: Beth Griggs Reviewed-By: Michaël Zasso Reviewed-By: Richard Lau Reviewed-By: James M Snell --- README.md | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index 17d084223d6fe6..5421b6e946a0c6 100644 --- a/README.md +++ b/README.md @@ -586,7 +586,7 @@ Primary GPG keys for Node.js Releasers (some Releasers sign with subkeys): * **Colin Ihrig** <cjihrig@gmail.com> `94AE36675C464D64BAFA68DD7434390BDBE9B9C5` * **Danielle Adams** <adamzdanielle@gmail.com> -`1C050899334244A8AF75E53792EF661D867B9DFA` +`74F12602B6F1C4E913FAA37AD3A89613643B6201` * **James M Snell** <jasnell@keybase.io> `71DCFD284A79C3B38668286BC97EC7A07EDE3FC1` * **Michaël Zasso** <targos@protonmail.com> @@ -610,7 +610,7 @@ To import the full set of trusted release keys: gpg --keyserver pool.sks-keyservers.net --recv-keys 4ED778F539E3634C779C87C6D7062848A1AB005C gpg --keyserver pool.sks-keyservers.net --recv-keys 141F07595B7B3FFE74309A937405533BE57C7D57 gpg --keyserver pool.sks-keyservers.net --recv-keys 94AE36675C464D64BAFA68DD7434390BDBE9B9C5 -gpg --keyserver pool.sks-keyservers.net --recv-keys 1C050899334244A8AF75E53792EF661D867B9DFA +gpg --keyserver pool.sks-keyservers.net --recv-keys 74F12602B6F1C4E913FAA37AD3A89613643B6201 gpg --keyserver pool.sks-keyservers.net --recv-keys 71DCFD284A79C3B38668286BC97EC7A07EDE3FC1 gpg --keyserver pool.sks-keyservers.net --recv-keys 8FCCA13FEF1D0C2E91008E09770F7A9A5AE15600 gpg --keyserver pool.sks-keyservers.net --recv-keys C4F0DFFF4E8C1A8236409D08E73BC641CC11F4C8 @@ -628,6 +628,8 @@ Other keys used to sign some previous releases: * **Chris Dickinson** <christopher.s.dickinson@gmail.com> `9554F04D7259F04124DE6B476D5A82AC7E37093B` +* **Danielle Adams** <adamzdanielle@gmail.com> +`1C050899334244A8AF75E53792EF661D867B9DFA` * **Evan Lucas** <evanlucas@me.com> `B9AE9905FFD7803F25714661B63B535A4C206CA9` * **Gibson Fahnestock** <gibfahn@gmail.com> From 518a49c0c6e0ea5300d8257da843dd78ee59a09d Mon Sep 17 00:00:00 2001 From: Nick Schonning Date: Fri, 18 Feb 2022 08:54:15 -0500 Subject: [PATCH 13/15] doc: use openpgp.org for keyserver examples MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The sks-keyservers.net is no longer listed by DNS PR-URL: https://github.com/nodejs/node/pull/39227 Reviewed-By: Antoine du Hamel Reviewed-By: Michaël Zasso --- README.md | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/README.md b/README.md index 5421b6e946a0c6..f1a79dfcf08581 100644 --- a/README.md +++ b/README.md @@ -115,7 +115,7 @@ For Current and LTS, the GPG detached signature of `SHASUMS256.txt` is in import the keys: ```console -$ gpg --keyserver pool.sks-keyservers.net --recv-keys DD8F2338BAE7501E3DD5AC78C273792F7D83545D +$ gpg --keyserver hkps://keys.openpgp.org --recv-keys DD8F2338BAE7501E3DD5AC78C273792F7D83545D ``` See the bottom of this README for a full script to import active release keys. @@ -607,18 +607,18 @@ Primary GPG keys for Node.js Releasers (some Releasers sign with subkeys): To import the full set of trusted release keys: ```bash -gpg --keyserver pool.sks-keyservers.net --recv-keys 4ED778F539E3634C779C87C6D7062848A1AB005C -gpg --keyserver pool.sks-keyservers.net --recv-keys 141F07595B7B3FFE74309A937405533BE57C7D57 -gpg --keyserver pool.sks-keyservers.net --recv-keys 94AE36675C464D64BAFA68DD7434390BDBE9B9C5 -gpg --keyserver pool.sks-keyservers.net --recv-keys 74F12602B6F1C4E913FAA37AD3A89613643B6201 -gpg --keyserver pool.sks-keyservers.net --recv-keys 71DCFD284A79C3B38668286BC97EC7A07EDE3FC1 -gpg --keyserver pool.sks-keyservers.net --recv-keys 8FCCA13FEF1D0C2E91008E09770F7A9A5AE15600 -gpg --keyserver pool.sks-keyservers.net --recv-keys C4F0DFFF4E8C1A8236409D08E73BC641CC11F4C8 -gpg --keyserver pool.sks-keyservers.net --recv-keys C82FA3AE1CBEDC6BE46B9360C43CEC45C17AB93C -gpg --keyserver pool.sks-keyservers.net --recv-keys DD8F2338BAE7501E3DD5AC78C273792F7D83545D -gpg --keyserver pool.sks-keyservers.net --recv-keys A48C2BEE680E841632CD4E44F07496B3EB3C1762 -gpg --keyserver pool.sks-keyservers.net --recv-keys 108F52B48DB57BB0CC439B2997B01419BD92F80A -gpg --keyserver pool.sks-keyservers.net --recv-keys B9E2F5981AA6E0CD28160D9FF13993A75599653C +gpg --keyserver hkps://keys.openpgp.org --recv-keys 4ED778F539E3634C779C87C6D7062848A1AB005C +gpg --keyserver hkps://keys.openpgp.org --recv-keys 141F07595B7B3FFE74309A937405533BE57C7D57 +gpg --keyserver hkps://keys.openpgp.org --recv-keys 94AE36675C464D64BAFA68DD7434390BDBE9B9C5 +gpg --keyserver hkps://keys.openpgp.org --recv-keys 74F12602B6F1C4E913FAA37AD3A89613643B6201 +gpg --keyserver hkps://keys.openpgp.org --recv-keys 71DCFD284A79C3B38668286BC97EC7A07EDE3FC1 +gpg --keyserver hkps://keys.openpgp.org --recv-keys 8FCCA13FEF1D0C2E91008E09770F7A9A5AE15600 +gpg --keyserver hkps://keys.openpgp.org --recv-keys C4F0DFFF4E8C1A8236409D08E73BC641CC11F4C8 +gpg --keyserver hkps://keys.openpgp.org --recv-keys C82FA3AE1CBEDC6BE46B9360C43CEC45C17AB93C +gpg --keyserver hkps://keys.openpgp.org --recv-keys DD8F2338BAE7501E3DD5AC78C273792F7D83545D +gpg --keyserver hkps://keys.openpgp.org --recv-keys A48C2BEE680E841632CD4E44F07496B3EB3C1762 +gpg --keyserver hkps://keys.openpgp.org --recv-keys 108F52B48DB57BB0CC439B2997B01419BD92F80A +gpg --keyserver hkps://keys.openpgp.org --recv-keys B9E2F5981AA6E0CD28160D9FF13993A75599653C ``` See the section above on [Verifying Binaries](#verifying-binaries) for how to From 333eda8d03bef36cf951bf54b33c7bbf2c0ef75c Mon Sep 17 00:00:00 2001 From: Igor Mikhalev Date: Sat, 8 Aug 2020 10:01:36 +0300 Subject: [PATCH 14/15] doc: add a note about possible missing lines to readline.asyncIterator Fixes: https://github.com/nodejs/node/issues/33463 PR-URL: https://github.com/nodejs/node/pull/34675 Reviewed-By: Ruben Bridgewater Reviewed-By: Anto Aravinth Reviewed-By: James M Snell Reviewed-By: Gus Caplan Reviewed-By: Rich Trott --- doc/api/readline.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/doc/api/readline.md b/doc/api/readline.md index c5829bf746629f..40c9f8fd08ed63 100644 --- a/doc/api/readline.md +++ b/doc/api/readline.md @@ -352,6 +352,10 @@ async function processLineByLine() { } ``` +`readline.createInterface()` will start to consume the input stream once +invoked. Having asynchronous operations between interface creation and +asynchronous iteration may result in missed lines. + ### rl.line