From 1a401e4aa3de4efa28bf23a4e93977b569689e96 Mon Sep 17 00:00:00 2001 From: Bryan English Date: Sat, 19 Jun 2021 13:47:43 -0400 Subject: [PATCH 1/3] async_hooks: check for empty contexts before removing This way we don't end up attempting to SetPromiseHooks on contexts that have already been collected. Fixes: https://github.com/nodejs/node/issues/39019 --- src/env-inl.h | 7 ++++++- test/parallel/test-async-hooks-vm-gc.js | 15 +++++++++++++++ 2 files changed, 21 insertions(+), 1 deletion(-) create mode 100644 test/parallel/test-async-hooks-vm-gc.js diff --git a/src/env-inl.h b/src/env-inl.h index 6fb8f137c37569..1c0afb20736ab8 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -256,8 +256,13 @@ inline void AsyncHooks::RemoveContext(v8::Local ctx) { v8::Isolate* isolate = env()->isolate(); v8::HandleScope handle_scope(isolate); for (auto it = contexts_.begin(); it != contexts_.end(); it++) { + if (it->IsEmpty()) { + contexts_.erase(it); + it--; + continue; + } v8::Local saved_context = - PersistentToLocal::Weak(env()->isolate(), *it); + PersistentToLocal::Weak(isolate, *it); if (saved_context == ctx) { it->Reset(); contexts_.erase(it); diff --git a/test/parallel/test-async-hooks-vm-gc.js b/test/parallel/test-async-hooks-vm-gc.js new file mode 100644 index 00000000000000..da95e3579dcca4 --- /dev/null +++ b/test/parallel/test-async-hooks-vm-gc.js @@ -0,0 +1,15 @@ +// Flags: --expose-gc +'use strict'; + +require('../common'); +const asyncHooks = require('async_hooks'); +const vm = require('vm'); + +// This is a regression test for https://github.com/nodejs/node/issues/39019 +// +// It should not segfault. + +const hook = asyncHooks.createHook({ init() {} }).enable(); +vm.createContext(); +globalThis.gc(); +hook.disable(); From 979bd0b4500dbb833d80fbc0dbf2b974653c3bef Mon Sep 17 00:00:00 2001 From: Bryan English Date: Sun, 20 Jun 2021 23:01:58 -0400 Subject: [PATCH 2/3] fixup! review feedback Co-authored-by: Anna Henningsen --- src/env-inl.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/env-inl.h b/src/env-inl.h index 1c0afb20736ab8..a0fcdfacdf4efc 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -257,7 +257,7 @@ inline void AsyncHooks::RemoveContext(v8::Local ctx) { v8::HandleScope handle_scope(isolate); for (auto it = contexts_.begin(); it != contexts_.end(); it++) { if (it->IsEmpty()) { - contexts_.erase(it); + it = contexts_.erase(it); it--; continue; } From 9acb32fbbdb3ff6613e810fda5effc065f8e3e43 Mon Sep 17 00:00:00 2001 From: Bryan English Date: Mon, 21 Jun 2021 14:52:35 -0400 Subject: [PATCH 3/3] fixup! check for empty contexts in SetJSPromiseHooks too --- src/env-inl.h | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/env-inl.h b/src/env-inl.h index a0fcdfacdf4efc..b3b1ea908253b9 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -104,6 +104,11 @@ inline void AsyncHooks::SetJSPromiseHooks(v8::Local init, js_promise_hooks_[2].Reset(env()->isolate(), after); js_promise_hooks_[3].Reset(env()->isolate(), resolve); for (auto it = contexts_.begin(); it != contexts_.end(); it++) { + if (it->IsEmpty()) { + it = contexts_.erase(it); + it--; + continue; + } PersistentToLocal::Weak(env()->isolate(), *it) ->SetPromiseHooks(init, before, after, resolve); }