From 7ff8795f850d6d73e3f9a68c8bfb2a5c7bda5a97 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Fri, 7 Aug 2020 16:24:05 +0200 Subject: [PATCH 1/3] src: fix `size` underflow in CallbackQueue Only decrease the size when actually removing items. --- src/callback_queue-inl.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/callback_queue-inl.h b/src/callback_queue-inl.h index 13561864027316..9e46ae48699320 100644 --- a/src/callback_queue-inl.h +++ b/src/callback_queue-inl.h @@ -22,8 +22,8 @@ CallbackQueue::Shift() { head_ = ret->get_next(); if (!head_) tail_ = nullptr; // The queue is now empty. + size_--; } - size_--; return ret; } From 60941d60650e2707cff5c995a2e81bdc1b7ac59a Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Fri, 7 Aug 2020 16:24:36 +0200 Subject: [PATCH 2/3] src: spin shutdown loop while immediates are pending This allows using `SetImmediate()` and friends at any point during cleanup. --- src/env.cc | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/env.cc b/src/env.cc index 62012262aa01fe..eb0fb2992502b9 100644 --- a/src/env.cc +++ b/src/env.cc @@ -634,7 +634,10 @@ void Environment::RunCleanup() { initial_base_object_count_ = 0; CleanupHandles(); - while (!cleanup_hooks_.empty()) { + while (!cleanup_hooks_.empty() || + native_immediates_.size() > 0 || + native_immediates_threadsafe_.size() > 0 || + native_immediates_interrupts_.size() > 0) { // Copy into a vector, since we can't sort an unordered_set in-place. std::vector callbacks( cleanup_hooks_.begin(), cleanup_hooks_.end()); From b94ceaf88d9c6662fc53f7991c967203c5007d85 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Fri, 7 Aug 2020 12:48:45 +0200 Subject: [PATCH 3/3] n-api: fix use-after-free with napi_remove_async_cleanup_hook Fixes: https://github.com/nodejs/node/issues/34657 Refs: https://github.com/nodejs/node/pull/34572 --- src/node_api.cc | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/node_api.cc b/src/node_api.cc index 8f5823d7820b38..4fbab771d58400 100644 --- a/src/node_api.cc +++ b/src/node_api.cc @@ -533,6 +533,7 @@ napi_status napi_add_async_cleanup_hook( auto handle = node::AddEnvironmentCleanupHook(env->isolate, fun, arg); if (remove_handle != nullptr) { *remove_handle = new napi_async_cleanup_hook_handle__ { std::move(handle) }; + env->Ref(); } return napi_clear_last_error(env); @@ -547,6 +548,11 @@ napi_status napi_remove_async_cleanup_hook( node::RemoveEnvironmentCleanupHook(std::move(remove_handle->handle)); delete remove_handle; + // Release the `env` handle asynchronously since it would be surprising if + // a call to a N-API function would destroy `env` synchronously. + static_cast(env)->node_env() + ->SetImmediate([env](node::Environment*) { env->Unref(); }); + return napi_clear_last_error(env); }