[v24.x backport] src: keep global list of addon-provided cleanup hooks

src/api/hooks.cc

@@ -127,20 +127,78 @@ struct ACHHandle final {
 // this.
 void DeleteACHHandle::operator ()(ACHHandle* handle) const { delete handle; }
 
+// TODO(addaleax): Having this extra set of data structures is far from
+// ideal, but unfortunately the public synchronous cleanup hook API was
+// slightly mis-designed; in particular, RemoveEnvironmentCleanupHook() needs
+// to keep working when the Isolate either has no active context (such as
+// during GC) or that context is associated with another Node.js Environment.
+// We should align this with the asynchronous API, which handles this properly
+// through an explicit reference to the cleanup hook instead of requiring
+// lookups in internal maps.
+struct CleanupHookThunk final {
+  Isolate* isolate;
+  Environment* env;
+  CleanupHook fun;
+  void* arg;
+
+  bool operator==(const CleanupHookThunk& other) const {
+    // `env` is intentionally not part of this comparison
+    return isolate == other.isolate && fun == other.fun && arg == other.arg;
+  }
+};
+struct CleanupHookThunkHash {
+  size_t operator()(const CleanupHookThunk& thunk) const {
+    return std::hash<void*>()(thunk.arg);
+  }
+};
+using CleanupHookRegistry =
+    std::unordered_set<CleanupHookThunk, CleanupHookThunkHash>;
+static ExclusiveAccess<CleanupHookRegistry> cleanup_hook_registry;
+
+static void CleanupHookThunkRun(void* arg) {
+  const CleanupHookThunk* thunk = static_cast<CleanupHookThunk*>(arg);
+  // `thunk->fun` may itself remove and free this CleanupHookThunk (e.g. via
+  // ~ObjectWrap(), which calls RemoveEnvironmentCleanupHook()), so cache the
+  // fields we still need before invoking it rather than reading them from
+  // `thunk` afterwards.
+  Isolate* isolate = thunk->isolate;
+  CleanupHook fun = thunk->fun;
+  void* fun_arg = thunk->arg;
+  fun(fun_arg);
+  RemoveEnvironmentCleanupHook(isolate, fun, fun_arg);
+}
+
 void AddEnvironmentCleanupHook(Isolate* isolate,
                                CleanupHook fun,
                                void* arg) {
   Environment* env = Environment::GetCurrent(isolate);
   CHECK_NOT_NULL(env);
-  env->AddCleanupHook(fun, arg);
+  void* wrapped_arg;
+  {
+    ExclusiveAccess<CleanupHookRegistry>::Scoped registry(
+        &cleanup_hook_registry);
+    auto result = registry->insert({isolate, env, fun, arg});
+    CHECK(result.second);
+    wrapped_arg = const_cast<CleanupHookThunk*>(&*result.first);
+  }
+  env->AddCleanupHook(CleanupHookThunkRun, wrapped_arg);
 }
 
 void RemoveEnvironmentCleanupHook(Isolate* isolate,
                                   CleanupHook fun,
                                   void* arg) {
-  Environment* env = Environment::GetCurrent(isolate);
-  CHECK_NOT_NULL(env);
-  env->RemoveCleanupHook(fun, arg);
+  CleanupHookThunk thunk;
+  void* wrapped_arg;
+  {
+    ExclusiveAccess<CleanupHookRegistry>::Scoped registry(
+        &cleanup_hook_registry);
+    auto result = registry->find({isolate, nullptr, fun, arg});
+    if (result == registry->end()) return;
+    wrapped_arg = const_cast<CleanupHookThunk*>(&*result);
+    thunk = *result;
+    registry->erase(result);
+  }
+  thunk.env->RemoveCleanupHook(CleanupHookThunkRun, wrapped_arg);
 }
 
 static void FinishAsyncCleanupHook(void* arg) {

src/string_decoder.cc

@@ -28,23 +28,21 @@ MaybeLocal<String> MakeString(Isolate* isolate,
                               const char* data,
                               size_t length,
                               enum encoding encoding) {
-  MaybeLocal<Value> ret;
-  if (encoding == UTF8) {
-    MaybeLocal<String> utf8_string;
-    if (length <= static_cast<size_t>(v8::String::kMaxLength)) {
-      utf8_string = String::NewFromUtf8(
-          isolate, data, v8::NewStringType::kNormal, length);
-    }
-    if (utf8_string.IsEmpty()) {
-      isolate->ThrowException(node::ERR_STRING_TOO_LONG(isolate));
-      return MaybeLocal<String>();
-    } else {
-      return utf8_string;
-    }
-  } else {
-    ret = StringBytes::Encode(isolate, data, length, encoding);
+  // StringBytes::Encode() would report an over-long UTF-8 input as
+  // ERR_BUFFER_TOO_LARGE (or clamp it); keep reporting it the way this
+  // decoder always has.
+  if (encoding == UTF8 && length > static_cast<size_t>(v8::String::kMaxLength))
+      [[unlikely]] {
+    isolate->ThrowException(node::ERR_STRING_TOO_LONG(isolate));
+    return MaybeLocal<String>();
   }
 
+  // For UTF-8 this takes the simdutf-backed ASCII / Latin-1 / UTF-16 paths and
+  // only falls back to v8::String::NewFromUtf8() (the previous unconditional
+  // path here) for input containing invalid sequences, so U+FFFD replacement
+  // is unchanged.
+  MaybeLocal<Value> ret = StringBytes::Encode(isolate, data, length, encoding);
+
   if (ret.IsEmpty()) {
     return {};
   }

test/addons/worker-addon/binding.cc

@@ -55,8 +55,23 @@ void Initialize(Local<Object> exports,
       context->GetIsolate(),
       Cleanup,
       const_cast<void*>(static_cast<const void*>("cleanup")));
-  node::AddEnvironmentCleanupHook(context->GetIsolate(), Dummy, nullptr);
-  node::RemoveEnvironmentCleanupHook(context->GetIsolate(), Dummy, nullptr);
+
+  // Test that adding and removing a cleanup hook works as expected
+  {
+    node::AddEnvironmentCleanupHook(context->GetIsolate(), Dummy, nullptr);
+    node::RemoveEnvironmentCleanupHook(context->GetIsolate(), Dummy, nullptr);
+  }
+
+  // Test that adding and removing a cleanup hook also works if there
+  // is no active context during removal
+  {
+    node::AddEnvironmentCleanupHook(context->GetIsolate(), Dummy, nullptr);
+    {
+      context->Exit();
+      node::RemoveEnvironmentCleanupHook(context->GetIsolate(), Dummy, nullptr);
+      context->Enter();
+    }
+  }
 
   if (getenv("addExtraItemToEventLoop") != nullptr) {
     // Add an item to the event loop that we do not clean up in order to make

test/cctest/test_environment.cc

@@ -27,6 +27,12 @@ static void at_exit_callback_ordered2(void* arg);
 static void at_exit_js(void* arg);
 static std::string cb_1_arg;  // NOLINT(runtime/string)
 
+struct SelfRemovingCleanupHookState {
+  v8::Isolate* isolate;
+  bool ran = false;
+};
+static void self_removing_cleanup_hook(void* arg);
+
 class EnvironmentTest : public EnvironmentTestFixture {
  private:
   void TearDown() override {
@@ -309,6 +315,27 @@ TEST_F(EnvironmentTest, AtExitRunsJS) {
   EXPECT_TRUE(called_at_exit_js);
 }
 
+// A cleanup hook that removes itself while the environment cleanup queue is
+// being drained must not cause a use-after-free. This registers such a hook
+// directly rather than through node::ObjectWrap, whose destructor removes
+// its own hook and is what makes this reachable for addons since #63642.
+// The use-after-free is silent in ordinary builds; it is caught by the
+// ASan/Valgrind CI, which is also how the original assertion (#63923)
+// surfaced. Regression test for https://github.com/nodejs/node/issues/65195.
+TEST_F(EnvironmentTest, RemoveEnvironmentCleanupHookDuringCleanup) {
+  const v8::HandleScope handle_scope(isolate_);
+  const Argv argv;
+  SelfRemovingCleanupHookState state{isolate_};
+  {
+    Env env{handle_scope, argv};
+    node::AddEnvironmentCleanupHook(
+        isolate_, self_removing_cleanup_hook, &state);
+    // Destroying `env` runs FreeEnvironment() -> RunCleanup(), which drains
+    // the cleanup queue and invokes CleanupHookThunkRun() for the hook above.
+  }
+  EXPECT_TRUE(state.ran);
+}
+
 TEST_F(EnvironmentTest, MultipleEnvironmentsPerIsolate) {
   const v8::HandleScope handle_scope(isolate_);
   const Argv argv;
@@ -392,6 +419,19 @@ static void at_exit_js(void* arg) {
   called_at_exit_js = true;
 }
 
+// Reproduces the sequence node::ObjectWrap performs since
+// https://github.com/nodejs/node/pull/63642, without using ObjectWrap
+// itself: the hook removes its own environment cleanup hook. When that runs
+// while the cleanup queue is being drained, CleanupHookThunkRun() must not
+// read the CleanupHookThunk after invoking the hook -- the hook has already
+// erased and freed it. See https://github.com/nodejs/node/issues/65195.
+static void self_removing_cleanup_hook(void* arg) {
+  auto* state = static_cast<SelfRemovingCleanupHookState*>(arg);
+  state->ran = true;
+  node::RemoveEnvironmentCleanupHook(
+      state->isolate, self_removing_cleanup_hook, state);
+}
+
 TEST_F(EnvironmentTest, SetImmediateCleanup) {
   int called = 0;
   int called_unref = 0;

test/parallel/test-string-decoder-utf8-large.js

@@ -0,0 +1,81 @@
+'use strict';
+// The UTF-8 StringDecoder shares its byte->string conversion with
+// Buffer#toString(): ASCII, Latin-1-representable and general inputs take
+// different (SIMD) paths depending on content and size, and invalid input
+// falls back to a replacing decoder. This test pins the decoder's output for
+// inputs that cross those size thresholds, for chunkings that split multibyte
+// sequences, and for invalid bytes embedded in otherwise large valid input.
+require('../common');
+const assert = require('assert');
+const { StringDecoder } = require('string_decoder');
+
+function decodeInChunks(buf, chunkSize) {
+  const decoder = new StringDecoder('utf8');
+  let out = '';
+  for (let i = 0; i < buf.length; i += chunkSize) {
+    out += decoder.write(buf.subarray(i, i + chunkSize));
+  }
+  return out + decoder.end();
+}
+
+function check(str, label) {
+  const buf = Buffer.from(str, 'utf8');
+  // Sanity: the expectation itself round-trips.
+  assert.strictEqual(buf.toString('utf8'), str, `${label}: toString`);
+  for (const chunkSize of [1, 2, 3, 4, 5, 7, 31, 32, 33, 255, 256, 257,
+                           4095, 4096, 65536, buf.length]) {
+    if (chunkSize > buf.length) continue;
+    // Keep the test fast: byte-sized chunks only for the smaller inputs.
+    if (buf.length > 100_000 && chunkSize < 4095) continue;
+    assert.strictEqual(decodeInChunks(buf, chunkSize), str,
+                       `${label}: chunkSize=${chunkSize}`);
+  }
+}
+
+const sizes = [31, 32, 33, 255, 256, 257, 4096, 70000, (1 << 20) + 5];
+for (const size of sizes) {
+  check('a'.repeat(size), `ascii ${size}`);
+  // Latin-1 range only (one-byte string in V8, two bytes each in UTF-8).
+  check('é'.repeat(size), `latin1 ${size}`);
+  // ASCII with a single Latin-1 character at the end / start.
+  check('a'.repeat(size - 1) + 'ÿ', `ascii+latin1 tail ${size}`);
+  check('Ä' + 'a'.repeat(size - 1), `latin1 head+ascii ${size}`);
+  // BMP beyond Latin-1 (three-byte sequences).
+  check('日'.repeat(size), `cjk ${size}`);
+  // Mixed, including astral plane characters (surrogate pairs, 4 bytes).
+  check(('abé日\u{1F600}').repeat(Math.ceil(size / 6)), `mixed ${size}`);
+}
+
+// Invalid bytes inside otherwise valid input of every size class must still be
+// replaced with U+FFFD exactly as before, regardless of chunking.
+for (const size of [8, 40, 300, 5000, (1 << 20) + 5]) {
+  const valid = Buffer.from('a'.repeat(size));
+  for (const bad of [[0xff], [0xc0, 0xaf], [0xe2, 0x28, 0xa1],
+                     [0xed, 0xa0, 0x80] /* encoded surrogate */,
+                     [0xf0, 0x9f, 0x98] /* truncated 4-byte */]) {
+    const buf = Buffer.concat([valid, Buffer.from(bad), valid]);
+    const expected = buf.toString('utf8');
+    assert.ok(expected.includes('�'), `size=${size} bad=${bad}`);
+    for (const chunkSize of [1, 3, 64, size, size + 1, buf.length]) {
+      if (buf.length > 100_000 && chunkSize < size) continue;
+      assert.strictEqual(decodeInChunks(buf, chunkSize), expected,
+                         `invalid ${bad} in ${size}, chunkSize=${chunkSize}`);
+    }
+  }
+}
+
+// A lone continuation / lead byte split across the size classes at the very
+// end is buffered by the decoder and flushed as U+FFFD by end().
+{
+  const decoder = new StringDecoder('utf8');
+  const big = Buffer.concat([Buffer.from('a'.repeat(300)), Buffer.from([0xe2, 0x82])]);
+  assert.strictEqual(decoder.write(big), 'a'.repeat(300));
+  assert.strictEqual(decoder.end(), '�');
+}
+{
+  const decoder = new StringDecoder('utf8');
+  const big = Buffer.concat([Buffer.from('é'.repeat(300)), Buffer.from([0xe2, 0x82])]);
+  assert.strictEqual(decoder.write(big), 'é'.repeat(300));
+  assert.strictEqual(decoder.write(Buffer.from([0xac])), '€');
+  assert.strictEqual(decoder.end(), '');
+}