From 05eec43789251f545b8360ee676037bd30d9b89c Mon Sep 17 00:00:00 2001 From: Attila Szegedi Date: Fri, 3 Jul 2026 14:07:12 +0200 Subject: [PATCH 1/2] Address review feedback from PR #347 Four independently-flagged concerns: - Accept optional third `attributes` arg in ThreadContext constructor. The TS ThreadContextCtor.new type declares `attributes` optional, but the C++ side hard-errored on `args.Length() != 3`. Loosen to accept 2 or 3 args; EncodeAttrs already handled undefined/null attrs_val correctly. - Fold `cleanup_registered` into `undefined_addr`. Uses the field's zero / non-zero state as the 'already-registered' flag: it starts at zero, ResetDiscoveryStruct clears it back to zero (so a re-init on the same thread would re-register), and any real V8 undefined singleton address is non-zero. Removes the separate thread_local static. - Coerce attribute values to strings in a pre-pass in EncodeAttrs before writing to the output buffer. Value->ToString may execute user JS (custom toString methods) which could re-enter into the ThreadContext via appendAttributes and interleave with our writes. Separating the coerce phase from the encode phase keeps the encode phase re-entrancy-free. - Make the 'enterWithContext attaches the record to the current async scope' test callback `async` and `await tcRun(...)`. Previously the callback returned a promise via `void tcRun(...)` and the promise's inner `.then` assertion could fire an unhandled rejection instead of a test failure. --- bindings/otel-thread-ctx.cc | 41 +++++++++++++++++++++++---------- ts/test/test-otel-thread-ctx.ts | 4 ++-- 2 files changed, 31 insertions(+), 14 deletions(-) diff --git a/bindings/otel-thread-ctx.cc b/bindings/otel-thread-ctx.cc index db76c16b..1c982dc6 100644 --- a/bindings/otel-thread-ctx.cc +++ b/bindings/otel-thread-ctx.cc @@ -289,17 +289,29 @@ bool CtxWrap::EncodeAttrs(Isolate* isolate, isolate->ThrowError("attributes array length must not exceed 256"); return false; } - // Reserve a conservative upper bound; reallocations are cheap but - // unnecessary for the typical small attribute set. - out->reserve(out->size() + n * 4); + + // Phase 1: coerce every present value to a string up front. `ToString` + // may invoke user JS (a custom `toString` on the value), which could + // re-enter into this ThreadContext (e.g. via `appendAttributes`) and + // interleave with our writes to `out`. Doing all such coercion BEFORE + // touching `out` keeps the encode phase re-entrancy-free. + std::vector>> coerced; + coerced.reserve(n); for (uint32_t i = 0; i < n; ++i) { Local val_val; if (!attrs->Get(context, i).ToLocal(&val_val)) return false; // null / undefined / array holes mean "no value at this key index". if (val_val->IsUndefined() || val_val->IsNull()) continue; - Local v; if (!val_val->ToString(context).ToLocal(&v)) return false; + coerced.emplace_back(i, v); + } + + // Phase 2: encode. From here on we don't call any V8 function that can + // enter user JS; the record is safe to mutate under the assumption that + // no re-entrant Append will interleave. + out->reserve(out->size() + coerced.size() * 4); + for (const auto& [i, v] : coerced) { #if NODE_MAJOR_VERSION >= 24 int v_utf8_len = static_cast(v->Utf8LengthV2(isolate)); #else @@ -356,9 +368,10 @@ void CtxWrap::New(const FunctionCallbackInfo& args) { isolate->ThrowError("ThreadContext must be called with `new`"); return; } - if (args.Length() != 3) { + if (args.Length() < 2 || args.Length() > 3) { isolate->ThrowError( - "ThreadContext expects 3 arguments: traceId, spanId, attributes"); + "ThreadContext expects 2 or 3 arguments: traceId, spanId, " + "attributes?"); return; } @@ -581,8 +594,6 @@ void ResetDiscoveryStruct(void* /*arg*/) { } void StoreAls(const FunctionCallbackInfo& args) { - static thread_local bool cleanup_registered = false; - Isolate* isolate = args.GetIsolate(); if (!args[0]->IsObject()) { isolate->ThrowError("First argument must be the AsyncLocalStorage object."); @@ -604,15 +615,21 @@ void StoreAls(const FunctionCallbackInfo& args) { // addon compiles on the older Node versions the package supports. otel_thread_ctx_nodejs_v1.cped_slot = nullptr; #endif + // `undefined_addr == 0` doubles as the "not yet initialized on this + // isolate" flag: it starts at zero (thread-local zero-init), any real + // V8 undefined singleton address is non-zero, and ResetDiscoveryStruct + // clears it back to zero — so a subsequent StoreAls (e.g. isolate + // tear-down then re-init on the same thread) re-registers the cleanup + // hook. Register BEFORE the write so the flag transition is the last + // observable step. + if (otel_thread_ctx_nodejs_v1.undefined_addr == 0) { + node::AddEnvironmentCleanupHook(isolate, ResetDiscoveryStruct, nullptr); + } // Cache the per-isolate undefined singleton's tagged address. Undefined // is a read-only-roots heap object, never moves, so a cached numeric // address is fine — no Global<> tracking needed. otel_thread_ctx_nodejs_v1.undefined_addr = reinterpret_cast(*v8::Undefined(isolate)); - if (!cleanup_registered) { - node::AddEnvironmentCleanupHook(isolate, ResetDiscoveryStruct, nullptr); - cleanup_registered = true; - } } // Without a function that explicitly reads the TLS variable, on x86 the diff --git a/ts/test/test-otel-thread-ctx.ts b/ts/test/test-otel-thread-ctx.ts index adad1841..070cf5c0 100644 --- a/ts/test/test-otel-thread-ctx.ts +++ b/ts/test/test-otel-thread-ctx.ts @@ -424,8 +424,8 @@ function captureBytes(opts: { }); describe('enterWithContext', () => { - it('attaches the record to the current async scope', () => { - void tcRun( + it('attaches the record to the current async scope', async () => { + await tcRun( () => { strictAssert.deepEqual( decodeHeader(_currentRecordBytes()!).spanId, From 82e70874ea39d6b68882f777b3e0a32ae7220f69 Mon Sep 17 00:00:00 2001 From: Attila Szegedi Date: Mon, 13 Jul 2026 16:13:06 +0200 Subject: [PATCH 2/2] Explicitly guard against reentrant Append calls. Also undo the two-phase encoding loop that was the previous (insufficient) attempt at fixing the reentrancy. --- bindings/otel-thread-ctx.cc | 51 +++++++++++++++++++++++++------------ 1 file changed, 35 insertions(+), 16 deletions(-) diff --git a/bindings/otel-thread-ctx.cc b/bindings/otel-thread-ctx.cc index 1c982dc6..574a87cf 100644 --- a/bindings/otel-thread-ctx.cc +++ b/bindings/otel-thread-ctx.cc @@ -31,6 +31,8 @@ #include "otel-thread-ctx.hh" +#include "defer.hh" + #include #include #include @@ -225,6 +227,16 @@ class CtxWrap : public ObjectWrap { // attribute had to be dropped because it would have pushed attrs_data // past MAX_ATTRS_DATA_SIZE. bool truncated_; + // Reentrancy guard for Append(). EncodeAttrs calls ToString on each + // attribute value, which can execute user JS (e.g. a custom + // `toString`) that in turn calls `appendAttributes` on the same + // ThreadContext. A reentrant Append would mutate attrs_data_size out + // from under the outer call's `current_used` snapshot, causing the + // outer memcpy to overwrite the reentrant call's bytes and the outer + // attrs_data_size write to shrink the record. We reject the reentrant + // call instead. New() doesn't need the guard because a freshly constructed + // CtxWrap isn't observable to JS until New() returns. + bool encoding_; }; // Pin the offset of `record_` — the field the reader walks to from the @@ -247,7 +259,10 @@ CtxWrap::~CtxWrap() { } CtxWrap::CtxWrap(OtelThreadCtxRecord* record, size_t capacity, bool truncated) - : record_(record), capacity_(capacity), truncated_(truncated) {} + : record_(record), + capacity_(capacity), + truncated_(truncated), + encoding_(false) {} // 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 @@ -290,13 +305,9 @@ bool CtxWrap::EncodeAttrs(Isolate* isolate, return false; } - // Phase 1: coerce every present value to a string up front. `ToString` - // may invoke user JS (a custom `toString` on the value), which could - // re-enter into this ThreadContext (e.g. via `appendAttributes`) and - // interleave with our writes to `out`. Doing all such coercion BEFORE - // touching `out` keeps the encode phase re-entrancy-free. - std::vector>> coerced; - coerced.reserve(n); + // Reserve a conservative upper bound; reallocations are cheap but + // unnecessary for the typical small attribute set. + out->reserve(out->size() + n * 4); for (uint32_t i = 0; i < n; ++i) { Local val_val; if (!attrs->Get(context, i).ToLocal(&val_val)) return false; @@ -304,14 +315,6 @@ bool CtxWrap::EncodeAttrs(Isolate* isolate, if (val_val->IsUndefined() || val_val->IsNull()) continue; Local v; if (!val_val->ToString(context).ToLocal(&v)) return false; - coerced.emplace_back(i, v); - } - - // Phase 2: encode. From here on we don't call any V8 function that can - // enter user JS; the record is safe to mutate under the assumption that - // no re-entrant Append will interleave. - out->reserve(out->size() + coerced.size() * 4); - for (const auto& [i, v] : coerced) { #if NODE_MAJOR_VERSION >= 24 int v_utf8_len = static_cast(v->Utf8LengthV2(isolate)); #else @@ -451,6 +454,22 @@ void CtxWrap::Append(const FunctionCallbackInfo& args) { return; } + // Reject reentrant Append on the same wrap. EncodeAttrs' `ToString` + // below can execute user JS, and if that JS calls `appendAttributes` + // on this same ThreadContext, the reentrant call would grow + // attrs_data_size out from under the outer call's `current_used` + // snapshot, causing the outer memcpy to overwrite the reentrant call's + // bytes and the outer attrs_data_size write to shrink the record. + if (self->encoding_) { + isolate->ThrowError( + "reentrant appendAttributes on the same ThreadContext is not allowed"); + return; + } + self->encoding_ = true; + defer { + self->encoding_ = false; + }; + const size_t current_used = self->record_->attrs_data_size; std::vector appended; bool truncated = false;