From 54e10ab6d2004250d904edd1c4c14c598c447c9b Mon Sep 17 00:00:00 2001 From: Attila Szegedi Date: Fri, 7 Aug 2026 12:03:34 +0200 Subject: [PATCH] fix(otel-thread-ctx): don't derive CtxWrap from node::ObjectWrap MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CtxWrap has the same defect #385 fixed in the wall profiler's PersistentContextPtr. 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() This one is not subtle: create a few thousand ThreadContexts and exit normally and it aborts every time, on a plain release build. No ASAN needed, unlike the PCP case. Nothing below ~1000 instances reproduces it — V8 has to still have some left to collect at teardown. Note 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. PCP could rely on ~WallProfiler walking its live list; CtxWrap has no such owner and owns a malloc'd record, so without a replacement this would trade an abort for a leak. Add the equivalent: a thread-local list of live CtxWraps drained by a single per-isolate cleanup hook, registered from Wrap() — inside a JS constructor call, where a context is entered, so AddEnvironmentCleanupHook is satisfied honestly — 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. 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 — no base subobject, no virtuals, all data members in one access section — so offsetof on it is now 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, since the reader contract depends on offsetof(record_) being well-defined. The two internal-field accessors move to a new internal-field.hh: Node 26 requires an EmbedderDataTypeTag on both the get and the set, and having the pair in one place stops them drifting when only one is exercised on the version you build against. wall.cc keeps its own copies for now to avoid conflicting with in-flight work there; folding those in is a follow-up. Verified on Node 20, 24 and 26, with both clang and gcc. New regression test fails with signal=SIGABRT against the pre-fix binding and passes after; ASAN exit 0 with zero leaks on 20 and 24, which is the check that the drain hook really does replace what ObjectWrap was doing. --- bindings/internal-field.hh | 47 ++++++++++ bindings/otel-thread-ctx.cc | 155 +++++++++++++++++++++++++------- ts/src/otel-thread-ctx.ts | 2 +- ts/test/otel-ctx-teardown.ts | 67 ++++++++++++++ ts/test/test-otel-thread-ctx.ts | 41 ++++++++- 5 files changed, 279 insertions(+), 33 deletions(-) create mode 100644 bindings/internal-field.hh create mode 100644 ts/test/otel-ctx-teardown.ts diff --git a/bindings/internal-field.hh b/bindings/internal-field.hh new file mode 100644 index 00000000..c7f5f48c --- /dev/null +++ b/bindings/internal-field.hh @@ -0,0 +1,47 @@ +/* + * Copyright 2026 Datadog, Inc + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#pragma once + +#include +#include + +namespace dd { + +// Read and write the embedder pointer stored in an object's internal field. +// Node 26 requires an EmbedderDataTypeTag on both ends. + +inline void* GetAlignedPointerFromInternalField(v8::Object* object, int index) { +#if NODE_MAJOR_VERSION >= 26 + return object->GetAlignedPointerFromInternalField( + index, v8::kEmbedderDataTypeTagDefault); +#else + return object->GetAlignedPointerFromInternalField(index); +#endif +} + +inline void SetAlignedPointerInInternalField(v8::Local object, + int index, + void* value) { +#if NODE_MAJOR_VERSION >= 26 + object->SetAlignedPointerInInternalField( + index, value, v8::kEmbedderDataTypeTagDefault); +#else + object->SetAlignedPointerInInternalField(index, value); +#endif +} + +} // namespace dd diff --git a/bindings/otel-thread-ctx.cc b/bindings/otel-thread-ctx.cc index 2aaea7ba..69d7aa43 100644 --- a/bindings/otel-thread-ctx.cc +++ b/bindings/otel-thread-ctx.cc @@ -32,9 +32,9 @@ #include "otel-thread-ctx.hh" #include "defer.hh" +#include "internal-field.hh" #include -#include #include #include @@ -44,6 +44,7 @@ #include #include +#include #include // Single thread-local read from outside the process via TLSDESC. It @@ -105,7 +106,6 @@ static_assert(offsetof(otel_thread_ctx_nodejs_v1_t, undefined_addr) == namespace dd { namespace { -using node::ObjectWrap; using v8::Array; using v8::Context; using v8::Function; @@ -173,12 +173,33 @@ constexpr size_t MAX_ATTRS_DATA_SIZE = 640 - sizeof(OtelThreadCtxRecord); // // 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_` and +// field of the class, at offset zero. `capacity_` and // `truncated_` sit after `record_` purely for the writer's own // bookkeeping — the reader never touches them. -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 today by creating a few +// thousand ThreadContexts and exiting normally; see the regression test. +// +// Note the CHECK is guarding 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 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 exports); CtxWrap(const CtxWrap&) = delete; @@ -208,7 +229,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 holder); + static CtxWrap* Unwrap(Local holder); + static void WeakCallback(const v8::WeakCallbackInfo& 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 @@ -238,32 +265,100 @@ class CtxWrap : public ObjectWrap { // call instead. New() doesn't need the guard because a freshly constructed // CtxWrap isn't observable to JS until New() returns. 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. Same shape as WallProfiler's PCP list. + CtxWrap** pprev_; + CtxWrap* next_; + // Weak handle on the holder object; owns this CtxWrap. + v8::Global 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. +static_assert(std::is_standard_layout::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 — the same reasoning the +// wall profiler uses for its active-profiler pointer. +// `otel_thread_ctx_nodejs_v1` above is thread-local for the same reason. +thread_local CtxWrap* g_live_ctx_wraps = nullptr; +// Whether DrainLiveCtxWraps is registered for the current isolate. Cleared by +// the drain itself so an isolate torn down and re-created on the same thread +// re-registers, matching how `undefined_addr` gates ResetDiscoveryStruct. +thread_local bool g_drain_hook_registered = false; + +// 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 Wrap(), which runs inside a JS constructor call where a +// context is 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*/) { + CtxWrap* p = g_live_ctx_wraps; + while (p != nullptr) { + CtxWrap* next = p->next_; + p->pprev_ = nullptr; + p->next_ = nullptr; + delete p; + p = next; + } + g_live_ctx_wraps = nullptr; + g_drain_hook_registered = false; +} 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_; + } free(record_); } +void CtxWrap::WeakCallback(const v8::WeakCallbackInfo& data) { + delete data.GetParameter(); +} + +void CtxWrap::Wrap(Local holder) { + Isolate* isolate = Isolate::GetCurrent(); + if (!g_drain_hook_registered) { + node::AddEnvironmentCleanupHook(isolate, DrainLiveCtxWraps, nullptr); + g_drain_hook_registered = true; + } + 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 holder) { + if (holder->InternalFieldCount() < 1) return nullptr; + return static_cast(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 @@ -445,7 +540,7 @@ void CtxWrap::Append(const FunctionCallbackInfo& args) { Isolate* isolate = args.GetIsolate(); Local context = isolate->GetCurrentContext(); - CtxWrap* self = ObjectWrap::Unwrap(args.This()); + CtxWrap* self = CtxWrap::Unwrap(args.This()); if (!self) { isolate->ThrowError("not a ThreadContext"); return; @@ -554,7 +649,7 @@ void CtxWrap::Append(const FunctionCallbackInfo& args) { // still exposing the finished span. Idempotent; safe to call multiple // times. void CtxWrap::Invalidate(const FunctionCallbackInfo& args) { - CtxWrap* self = ObjectWrap::Unwrap(args.This()); + CtxWrap* self = CtxWrap::Unwrap(args.This()); if (!self) { args.GetIsolate()->ThrowError("not a ThreadContext"); return; @@ -568,7 +663,7 @@ void CtxWrap::Invalidate(const FunctionCallbackInfo& args) { // CtxWrap::New() if the initial set didn't fit, or by any subsequent // CtxWrap::Append() call. void CtxWrap::IsTruncated(const FunctionCallbackInfo& args) { - CtxWrap* self = ObjectWrap::Unwrap(args.This()); + CtxWrap* self = CtxWrap::Unwrap(args.This()); if (!self) { args.GetIsolate()->ThrowError("not a ThreadContext"); return; @@ -581,7 +676,7 @@ void CtxWrap::IsTruncated(const FunctionCallbackInfo& args) { // API; intended for tests and out-of-process-reader development. void CtxWrap::DebugBytes(const FunctionCallbackInfo& args) { Isolate* isolate = args.GetIsolate(); - CtxWrap* self = ObjectWrap::Unwrap(args.This()); + CtxWrap* self = CtxWrap::Unwrap(args.This()); if (!self) { isolate->ThrowError("not a ThreadContext"); return; @@ -702,13 +797,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(sizeof(node::ObjectWrap)); + static_cast(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 diff --git a/ts/src/otel-thread-ctx.ts b/ts/src/otel-thread-ctx.ts index b5d9e75b..f59a976b 100644 --- a/ts/src/otel-thread-ctx.ts +++ b/ts/src/otel-thread-ctx.ts @@ -134,7 +134,7 @@ const SCHEMA_VERSION = 'nodejs_v1_dev'; // consistent in shape. 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; diff --git a/ts/test/otel-ctx-teardown.ts b/ts/test/otel-ctx-teardown.ts new file mode 100644 index 00000000..1780b73f --- /dev/null +++ b/ts/test/otel-ctx-teardown.ts @@ -0,0 +1,67 @@ +/* + * Copyright 2026 Datadog, Inc + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +'use strict'; + +// Runs in a forked process: the failure mode under test is a SIGABRT, which +// would take the whole mocha run down. +// +// When CtxWrap derived from node::ObjectWrap, a CtxWrap collected during +// isolate teardown ran ~ObjectWrap -> RemoveEnvironmentCleanupHook, which +// CHECKs that an Environment is current. It is not, during teardown, so the +// process aborted: +// +// Assertion failed: (env) != nullptr +// 3: otel_thread_ctx_nodejs::CtxWrap::~CtxWrap() +// +// It needs enough instances that V8 still has some left to collect at +// teardown — nothing below ~1000 reproduced it — hence the count here. + +import {otelThreadCtx} from '../src/index'; + +const N = 3000; + +function id(n: number, len: number): Uint8Array { + const b = new Uint8Array(len); + b[0] = (n >> 24) & 0xff; + b[1] = (n >> 16) & 0xff; + b[2] = (n >> 8) & 0xff; + b[3] = n & 0xff; + return b; +} + +const retained: unknown[] = []; + +for (let i = 0; i < N; i++) { + const ctx = new otelThreadCtx.ThreadContext(id(i, 16), id(i, 8), [ + 'k', + String(i), + ]); + if (i % 4 === 0) { + // Still strongly reachable at exit. + retained.push(ctx); + } else { + // Reachable only through the async context frame; collectable whenever + // V8 decides, including during teardown. + ctx.enter(); + } +} + +(globalThis as unknown as {__retained: unknown}).__retained = retained; + +// Exit through the normal path, so the Environment is torn down and the +// isolate disposed. That is where the weak callbacks in question fire. +console.log(`created ${N}, retained ${retained.length}`); diff --git a/ts/test/test-otel-thread-ctx.ts b/ts/test/test-otel-thread-ctx.ts index d19167d1..f26af9db 100644 --- a/ts/test/test-otel-thread-ctx.ts +++ b/ts/test/test-otel-thread-ctx.ts @@ -26,7 +26,7 @@ import assert from 'assert'; import {strict as strictAssert} from 'assert'; -import {spawnSync} from 'node:child_process'; +import {fork, spawnSync} from 'node:child_process'; import {existsSync} from 'node:fs'; import {join} from 'node:path'; @@ -151,6 +151,43 @@ function captureBytes(opts: { (isLinux && isAsyncContextFrameAvailable ? describe : describe.skip)( 'OTEP-4947 thread context (Linux-only)', () => { + describe('isolate teardown', () => { + // Regression test: CtxWrap used to derive from node::ObjectWrap, whose + // destructor calls RemoveEnvironmentCleanupHook. A CtxWrap collected + // during isolate teardown hit that function's CHECK that an Environment + // is current and aborted the process. Forked, because the failure is a + // SIGABRT rather than a test failure. + it('should not abort when contexts are collected during teardown', async function () { + this.timeout(60000); + + const proc = fork(join(__dirname, 'otel-ctx-teardown.js'), { + silent: true, + }); + let output = ''; + proc.stdout?.on('data', chunk => { + output += chunk; + }); + proc.stderr?.on('data', chunk => { + output += chunk; + }); + + await new Promise((resolve, reject) => { + proc.on('error', reject); + proc.on('close', (code, signal) => { + if (code === 0) { + resolve(); + } else { + reject( + new Error( + `otel-ctx-teardown exited with code=${code} signal=${signal}\n${output}`, + ), + ); + } + }); + }); + }); + }); + describe('ThreadContext construction', () => { it('accepts Uint8Array trace and span IDs', () => { const bytes = captureBytes({ @@ -770,7 +807,7 @@ function captureBytes(opts: { strictAssert.deepEqual(pca['threadlocal.attribute_key_map'], keys); strictAssert.equal(pca['threadlocal.wrapped_object_offset'], 24); strictAssert.equal(pca['threadlocal.tagged_size'], 8); - strictAssert.equal(pca['threadlocal.native_wrap_fields_offset'], 24); + strictAssert.equal(pca['threadlocal.native_wrap_fields_offset'], 0); strictAssert.equal(pca['threadlocal.js_map_table_offset'], 0x18); strictAssert.equal( pca['threadlocal.ordered_hash_map_header_size'],