From 65a7fd3e23b6f7be6a066f3f5ffe87a36ba66360 Mon Sep 17 00:00:00 2001 From: Michael Dawson Date: Tue, 1 Jun 2021 21:20:55 -0400 Subject: [PATCH] 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 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 95f8212d870a72..d972ee43c8861e 100644 --- a/src/js_native_api_v8.cc +++ b/src/js_native_api_v8.cc @@ -321,7 +321,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(); } @@ -348,7 +349,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; } } @@ -445,8 +446,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); } @@ -468,12 +468,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; }; enum UnwrapAction {