Don't derive CtxWrap from node::ObjectWrap

Port of DataDog/pprof-nodejs#388, which fixes this in the vendored copy.

node::ObjectWrap registers a per-instance environment cleanup hook in its
constructor and calls RemoveEnvironmentCleanupHook from its destructor,
which CHECKs that an Environment is current. A CtxWrap is owned by a weak
V8 handle, so V8 picks the moment it dies, and weak callbacks run during
isolate teardown with no context entered:

    Assertion failed: (env) != nullptr
     2: node::RemoveEnvironmentCleanupHook(...)
     3: otel_thread_ctx_nodejs::CtxWrap::~CtxWrap()

Create a few thousand ThreadContexts and exit normally and it aborts every
time, on a plain release build. Nothing below ~1000 instances reproduces
it — V8 has to still have some left to collect at teardown.

The CHECK guards something real, so it must not be worked around by
skipping the removal. Environment::GetCurrent(isolate) returns null on
`!isolate->InContext()` alone, so the Environment may well still be alive;
leaving a hook behind whose arg is a freed pointer would turn the abort
into a use-after-free when CleanupQueue::Drain later calls it. The fix is
to not register the per-instance hook at all.

Dropping the base loses what that hook provided: deletion at teardown even
when V8 never collects the object. Without a replacement this would trade
an abort for a leak of every record still live at exit, since CtxWrap owns
its malloc'd record. Add the equivalent: a thread-local list of live
CtxWraps drained by a single per-isolate cleanup hook, registered from
Init() — module initialisation always runs with a context entered, so
AddEnvironmentCleanupHook is satisfied honestly, and Init() runs exactly
once per isolate, which is the lifetime the hook should match — and never
removed, since it fires once at teardown while the Environment is alive.
One hook per isolate instead of one per instance, with removal timing we
control rather than V8.

The drain also clears the holder's internal field before freeing the
CtxWrap it points at. That slot is exactly what the out-of-process
OTEP-4947 reader walks to reach record_, so leaving it pointing at freed
memory aims a dangling pointer at a consumer we do not control. Being on
the live list means V8 has not collected the holder, so reading the handle
there is safe; the WeakCallback path cannot do this and does not need to,
since there the holder is the object being collected.

With no base class, `record_` becomes CtxWrap's first member, so the
published threadlocal.native_wrap_fields_offset goes from 24 to 0 and is
now computed with offsetof rather than sizeof() of a foreign type. That is
a reader-contract change, made now because no readers exist yet.

Losing the base also makes CtxWrap standard-layout, so offsetof on it is
unconditionally valid and the two -Winvalid-offsetof suppressions the
inheriting version needed are gone. A static_assert on is_standard_layout
keeps it that way.

Taking over the internal-field access means handling the EmbedderDataTypeTag
that Node 26 requires on both the get and the set; the pair is kept together
so they cannot drift. This is new here — the ObjectWrap base was hiding the
version difference.

Verified on Node 22, 24 and 26: 48/48 tests pass on each, and the repro
goes from exit 134 to exit 0 at N=1000, 3000 and 10000. Confirmed the fix
is what does it by rebuilding the same tree with the original addon.cpp,
which still aborts with the CtxWrap::~CtxWrap stack above.

Author: szegedi

js/addon.cpp

@@ -7,7 +7,6 @@
 // finally the record it owns.
 
 #include <node.h>
-#include <node_object_wrap.h>
 #include <v8-internal.h>
 
 #include <stddef.h>
@@ -17,6 +16,7 @@
 
 #include <atomic>
 #include <memory>
+#include <type_traits>
 #include <vector>
 
 extern "C" {
@@ -75,7 +75,6 @@ static_assert(offsetof(otel_thread_ctx_nodejs_v1_t, undefined_addr) ==
               "undefined_addr must follow als_identity_hash + padding");
 
 namespace otel_thread_ctx_nodejs {
-using node::ObjectWrap;
 using v8::Array;
 using v8::Context;
 using v8::Function;
@@ -136,18 +135,63 @@ constexpr size_t MIN_INITIAL_CAPACITY = 64 - sizeof(OtelThreadCtxRecord);
 // as best-effort.
 constexpr size_t MAX_ATTRS_DATA_SIZE = 640 - sizeof(OtelThreadCtxRecord);
 
+// Read and write the embedder pointer stored in an object's internal field.
+// Node 26 requires an EmbedderDataTypeTag on both ends; keeping the pair
+// together means the getter and the setter cannot drift apart.
+inline void* GetAlignedPointerFromInternalField(Object* object, int index) {
+#if NODE_MAJOR_VERSION >= 26
+  return object->GetAlignedPointerFromInternalField(
+      index, v8::kEmbedderDataTypeTagDefault);
+#else
+  return object->GetAlignedPointerFromInternalField(index);
+#endif
+}
+
+inline void SetAlignedPointerInInternalField(Local<Object> object,
+                                             int index,
+                                             void* value) {
+#if NODE_MAJOR_VERSION >= 26
+  object->SetAlignedPointerInInternalField(
+      index, value, v8::kEmbedderDataTypeTagDefault);
+#else
+  object->SetAlignedPointerInInternalField(index, value);
+#endif
+}
+
 // Wraps a heap-allocated OtelThreadCtxRecord. Lifetime is managed by V8 GC:
 // when no JS code (or AsyncLocalStorage entry) holds a reference, the record
 // is freed.
 //
 // Layout note for the reader: `record_` is private to C++ but its byte
 // position within CtxWrap is part of the reader contract. It is the first
-// field after the node::ObjectWrap base subobject. `capacity_` sits after
+// field of the class, at offset zero. `capacity_` sits after
 // `record_` purely for the writer's own bookkeeping — the reader never
 // touches it.
-class CtxWrap : public ObjectWrap {
+//
+// Deliberately not a node::ObjectWrap. That base registers a per-instance
+// environment cleanup hook in its constructor and calls
+// RemoveEnvironmentCleanupHook from its destructor, which CHECKs that an
+// Environment is current:
+//
+//   node[107]: void node::RemoveEnvironmentCleanupHook(...) hooks.cc:142
+//   Assertion failed: (env) != nullptr
+//    3: otel_thread_ctx_nodejs::CtxWrap::~CtxWrap()
+//
+// A CtxWrap is owned by a weak V8 handle, so V8 chooses when it dies, and
+// weak callbacks run during isolate teardown with no context entered —
+// Environment::GetCurrent(isolate) returns null on `!isolate->InContext()`
+// alone — so the CHECK fires and aborts. Reproducible by creating a few
+// thousand ThreadContexts and exiting normally.
+//
+// The CHECK guards something real, so this must not be worked around by
+// skipping the removal: the Environment may well still be alive, and leaving
+// a hook behind whose arg is a freed pointer turns an abort into a
+// use-after-free at CleanupQueue::Drain. The fix is to never register the
+// per-instance hook, and to provide the teardown deletion it was giving us
+// (see g_live_ctx_wraps below).
+class CtxWrap {
  public:
-  ~CtxWrap() override;
+  ~CtxWrap();
   static void Init(Local<Object> exports);
 
   CtxWrap(const CtxWrap&) = delete;
@@ -177,7 +221,13 @@ class CtxWrap : public ObjectWrap {
 
   CtxWrap(OtelThreadCtxRecord* record, size_t capacity, bool truncated);
 
-  // The three fields are kept in one access section because C++ leaves
+  // Attach to the holder JSObject: store `this` in internal field 0 and take
+  // a weak handle on the holder, so V8 deletes us once it collects it.
+  void Wrap(Local<Object> holder);
+  static CtxWrap* Unwrap(Local<Object> holder);
+  static void WeakCallback(const v8::WeakCallbackInfo<CtxWrap>& data);
+
+  // The fields are kept in one access section because C++ leaves
   // the relative layout of fields in different access controls
   // implementation-defined. `record_` must come first — its offset
   // within CtxWrap is part of the reader contract (see the
@@ -206,32 +256,109 @@ class CtxWrap : public ObjectWrap {
   // attrs_data_size write to shrink the record. We reject the reentrant
   // call instead.
   bool encoding_;
+  // Intrusive doubly-linked list of the CtxWraps still alive on this thread,
+  // threaded through g_live_ctx_wraps. `pprev_` is the address of the pointer
+  // currently referencing us, so unlinking needs no head/non-head branch;
+  // `pprev_ == nullptr` is the "already detached" sentinel set by the drain
+  // hook before it deletes us.
+  CtxWrap** pprev_;
+  CtxWrap* next_;
+  // Weak handle on the holder object; owns this CtxWrap.
+  v8::Global<v8::Object> handle_;
 };
 
 // Pin the offset of `record_` — the field the reader walks to from the
-// JSObject's internal field 0. We document it as "the first field after
-// the node::ObjectWrap base subobject", so equality with
-// sizeof(node::ObjectWrap) is the invariant. `offsetof` on a non-
-// standard-layout type (CtxWrap has private fields and inherits from
-// ObjectWrap) is conditionally supported per the standard but accepted
-// by every compiler this addon targets; suppress -Winvalid-offsetof so
-// the static_assert compiles cleanly under strict warning flags.
-#pragma GCC diagnostic push
-#pragma GCC diagnostic ignored "-Winvalid-offsetof"
-static_assert(offsetof(CtxWrap, record_) == sizeof(node::ObjectWrap),
-              "record_ must be the first field after the ObjectWrap base "
-              "subobject");
-#pragma GCC diagnostic pop
+// JSObject's internal field 0. With no base class it is simply the first
+// member, so the offset is zero and the published
+// `threadlocal.native_wrap_fields_offset` is computed from this rather than
+// from sizeof() of a foreign type we do not control.
+//
+// Dropping the ObjectWrap base also made CtxWrap standard-layout — no base
+// subobject, no virtuals, every data member in one access section — so
+// offsetof is unconditionally valid here and needs no -Winvalid-offsetof
+// suppression, unlike when it inherited.
+static_assert(std::is_standard_layout<CtxWrap>::value,
+              "CtxWrap must stay standard-layout: the reader contract depends "
+              "on offsetof(record_) being well-defined");
+static_assert(offsetof(CtxWrap, record_) == 0,
+              "record_ must be the first field of CtxWrap");
+
+// Head of the live-CtxWrap list for this thread. Node pins each isolate to a
+// thread, and CtxWraps are only ever constructed and destroyed on their own
+// isolate's thread, so a thread-local needs no lock.
+// `otel_thread_ctx_nodejs_v1` above is thread-local for the same reason.
+thread_local CtxWrap* g_live_ctx_wraps = nullptr;
+
+// Delete every CtxWrap V8 has not collected yet. This is the teardown deletion
+// that node::ObjectWrap's per-instance cleanup hook used to provide; without it
+// the records would simply leak at exit. Registered once per isolate from
+// Init(), which runs at module initialisation with a context entered, so
+// AddEnvironmentCleanupHook's own CHECK is satisfied, and never removed — it
+// fires exactly once, at teardown, while the Environment is still alive.
+void DrainLiveCtxWraps(void* arg) {
+  auto* isolate = static_cast<Isolate*>(arg);
+  v8::HandleScope scope(isolate);
+  CtxWrap* p = g_live_ctx_wraps;
+  while (p != nullptr) {
+    CtxWrap* next = p->next_;
+    p->pprev_ = nullptr;
+    p->next_ = nullptr;
+    // Clear the holder's internal field before freeing what it points at, so
+    // nothing can reach a dangling CtxWrap through it — including the
+    // out-of-process reader, which walks exactly this slot. Being on the live
+    // list means V8 has not collected the holder, so the handle is safe to
+    // read here; the WeakCallback path cannot do this and does not need to,
+    // since there the holder is the object being collected.
+    if (!p->handle_.IsEmpty()) {
+      SetAlignedPointerInInternalField(p->handle_.Get(isolate), 0, nullptr);
+    }
+    delete p;
+    p = next;
+  }
+  g_live_ctx_wraps = nullptr;
+}
 
 CtxWrap::~CtxWrap() {
+  // pprev_ != nullptr means we are still on the live list, i.e. V8 collected
+  // the holder and we got here from WeakCallback. If it is null the drain hook
+  // is walking the list and has already detached us.
+  if (pprev_ != nullptr) {
+    *pprev_ = next_;
+    if (next_ != nullptr) next_->pprev_ = pprev_;
+  }
+  // ~Global releases handle_. Arriving from WeakCallback, V8 requires the
+  // callback to reset the handle; arriving from the drain hook, resetting
+  // cancels a callback that would otherwise fire later.
   free(record_);
 }
 
+void CtxWrap::WeakCallback(const v8::WeakCallbackInfo<CtxWrap>& data) {
+  delete data.GetParameter();
+}
+
+void CtxWrap::Wrap(Local<Object> holder) {
+  Isolate* isolate = Isolate::GetCurrent();
+  SetAlignedPointerInInternalField(holder, 0, this);
+  handle_.Reset(isolate, holder);
+  handle_.SetWeak(this, &WeakCallback, v8::WeakCallbackType::kParameter);
+  next_ = g_live_ctx_wraps;
+  pprev_ = &g_live_ctx_wraps;
+  if (next_ != nullptr) next_->pprev_ = &next_;
+  g_live_ctx_wraps = this;
+}
+
+CtxWrap* CtxWrap::Unwrap(Local<Object> holder) {
+  if (holder->InternalFieldCount() < 1) return nullptr;
+  return static_cast<CtxWrap*>(GetAlignedPointerFromInternalField(*holder, 0));
+}
+
 CtxWrap::CtxWrap(OtelThreadCtxRecord* record, size_t capacity, bool truncated)
     : record_(record),
       capacity_(capacity),
       truncated_(truncated),
-      encoding_(false) {}
+      encoding_(false),
+      pprev_(nullptr),
+      next_(nullptr) {}
 
 // Copy exactly `expected_bytes` bytes out of a JS Uint8Array (or subclass such
 // as Buffer) into `out`. Returns false if the value isn't a Uint8Array or its
@@ -416,7 +543,7 @@ void CtxWrap::AppendAttributes(const FunctionCallbackInfo<Value>& args) {
   Isolate* isolate = args.GetIsolate();
   Local<Context> context = isolate->GetCurrentContext();
 
-  CtxWrap* self = ObjectWrap::Unwrap<CtxWrap>(args.This());
+  CtxWrap* self = CtxWrap::Unwrap(args.This());
   if (!self) {
     isolate->ThrowError("not a ThreadContext");
     return;
@@ -527,7 +654,7 @@ void CtxWrap::AppendAttributes(const FunctionCallbackInfo<Value>& args) {
 // still exposing the finished span. Idempotent; safe to call multiple
 // times.
 void CtxWrap::Invalidate(const FunctionCallbackInfo<Value>& args) {
-  CtxWrap* self = ObjectWrap::Unwrap<CtxWrap>(args.This());
+  CtxWrap* self = CtxWrap::Unwrap(args.This());
   if (!self) {
     args.GetIsolate()->ThrowError("not a ThreadContext");
     return;
@@ -541,7 +668,7 @@ void CtxWrap::Invalidate(const FunctionCallbackInfo<Value>& args) {
 // CtxWrap::New() if the initial set didn't fit, or by any subsequent
 // CtxWrap::AppendAttributes() call.
 void CtxWrap::IsTruncated(const FunctionCallbackInfo<Value>& args) {
-  CtxWrap* self = ObjectWrap::Unwrap<CtxWrap>(args.This());
+  CtxWrap* self = CtxWrap::Unwrap(args.This());
   if (!self) {
     args.GetIsolate()->ThrowError("not a ThreadContext");
     return;
@@ -554,7 +681,7 @@ void CtxWrap::IsTruncated(const FunctionCallbackInfo<Value>& args) {
 // API; intended for tests and out-of-process-reader development.
 void CtxWrap::DebugBytes(const FunctionCallbackInfo<Value>& args) {
   Isolate* isolate = args.GetIsolate();
-  CtxWrap* self = ObjectWrap::Unwrap<CtxWrap>(args.This());
+  CtxWrap* self = CtxWrap::Unwrap(args.This());
   if (!self) {
     isolate->ThrowError("not a ThreadContext");
     return;
@@ -569,6 +696,11 @@ void CtxWrap::DebugBytes(const FunctionCallbackInfo<Value>& args) {
 void CtxWrap::Init(Local<Object> exports) {
   Isolate* isolate = Isolate::GetCurrent();
   Local<Context> context = isolate->GetCurrentContext();
+  // One hook per isolate, registered here rather than lazily on first Wrap():
+  // module initialisation always runs with a context entered, so
+  // AddEnvironmentCleanupHook's CHECK is satisfied, and Init() runs exactly
+  // once per isolate, which is what the hook's lifetime should match.
+  node::AddEnvironmentCleanupHook(isolate, DrainLiveCtxWraps, isolate);
 
   Local<FunctionTemplate> tpl = FunctionTemplate::New(isolate, New);
   tpl->SetClassName(String::NewFromUtf8Literal(isolate, "ThreadContext"));
@@ -691,13 +823,13 @@ constexpr int WRAPPED_OBJECT_OFFSET = 0;
 #endif
 constexpr int TAGGED_SIZE = v8::internal::kApiTaggedSize;
 
-// sizeof(node::ObjectWrap). Given a pointer to a CtxWrap — or any other
-// ObjectWrap-derived C++ object attached to a JSObject via the V8
-// wrapped-object slot — add this offset to reach the derived class's own
-// fields. For CtxWrap, that's `record_` (see the static_assert on its
-// offset above).
+// Given a pointer to a CtxWrap — reached from the JSObject's V8
+// wrapped-object slot — add this offset to arrive at `record_`. CtxWrap has
+// no base class, so `record_` is its first member and the offset is zero;
+// computing it with offsetof keeps the published value correct if the layout
+// ever changes again.
 constexpr int NATIVE_WRAP_FIELDS_OFFSET =
-    static_cast<int>(sizeof(node::ObjectWrap));
+    static_cast<int>(offsetof(CtxWrap, record_));
 
 // V8 JSMap layout: kTableOffset within the JSMap object holds a tagged
 // pointer to the backing OrderedHashMap table. Not exposed in V8's

js/index.js

@@ -9,7 +9,7 @@ const SCHEMA_VERSION = 'nodejs_v1_dev';
 // see consistent values.
 let WRAPPED_OBJECT_OFFSET = 24;
 let TAGGED_SIZE = 8;
-let NATIVE_WRAP_FIELDS_OFFSET = 24;
+let NATIVE_WRAP_FIELDS_OFFSET = 0;
 let JS_MAP_TABLE_OFFSET = 0x18;
 let ORDERED_HASH_MAP_HEADER_SIZE = 0x10;
 

js/test/test.js

@@ -359,7 +359,9 @@ test('getProcessContextAttributes returns the expected shape', () => {
     // compression, no sandbox) these are 24 and 8 respectively.
     assert.equal(pca['threadlocal.wrapped_object_offset'], 24);
     assert.equal(pca['threadlocal.tagged_size'], 8);
-    assert.equal(pca['threadlocal.native_wrap_fields_offset'], 24);
+    // Zero since CtxWrap dropped its node::ObjectWrap base: `record_` is
+    // now the first member, so no base subobject to skip past.
+    assert.equal(pca['threadlocal.native_wrap_fields_offset'], 0);
     assert.equal(pca['threadlocal.js_map_table_offset'], 0x18);
     assert.equal(pca['threadlocal.ordered_hash_map_header_size'], 0x10);
     assert.deepEqual(Object.keys(pca).sort(), [