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'],