From b44f5dd4efb55d13cdf37365d22299dffc9975c7 Mon Sep 17 00:00:00 2001 From: Attila Szegedi Date: Tue, 11 Aug 2026 10:41:04 +0200 Subject: [PATCH] fix(otel-thread-ctx): don't assert a record is valid when growing it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Append's reallocate path asserted that the record it had just copied was valid: memcpy(new_rec.get(), self->record_, ...); ... assert(new_rec->valid == 1); invalidate() sets that byte to 0, and appending afterwards is supported — there is a test for it — so the assert fires on any append too large to fit in place: Assertion `new_rec->valid == 1' failed. Aborted (exit 134) This is not debug-only. NDEBUG is never defined for this addon, so assert() is live in Release too; both configurations abort. Reproduced through the public API on Linux with invalidate() followed by a 200-byte attribute. Only the reallocate path is affected: an append that fits the current capacity is written in place and never copies the header. A fresh record has 36 bytes of attrs_data capacity, so an attribute over ~34 bytes on a fresh record is enough. The existing 'appendAttributes after invalidate' test appends 6 bytes, takes the in-place path, and so never reached the copy — right behaviour, wrong size. Assert what the check was actually for — that the memcpy carried the header across intact — by capturing the source's valid byte first and comparing against that. Still catches a genuine copy bug, such as shortening the memcpy so it no longer covers the header, and is correct whether the record is valid or not. The regression test forks, since the failure is an abort that would otherwise take the whole mocha run down. Verified it bites: against the pre-fix binding it reports signal=SIGABRT with the assertion above, and passes after. Reported by @nsavoire on #391. --- bindings/otel-thread-ctx.cc | 11 ++++-- ts/test/otel-invalidate-append.ts | 57 +++++++++++++++++++++++++++++++ ts/test/test-otel-thread-ctx.ts | 39 +++++++++++++++++++++ 3 files changed, 105 insertions(+), 2 deletions(-) create mode 100644 ts/test/otel-invalidate-append.ts diff --git a/bindings/otel-thread-ctx.cc b/bindings/otel-thread-ctx.cc index 69d7aa43..15477a11 100644 --- a/bindings/otel-thread-ctx.cc +++ b/bindings/otel-thread-ctx.cc @@ -614,14 +614,21 @@ void CtxWrap::Append(const FunctionCallbackInfo& args) { isolate->ThrowError("allocation failed"); return; } + // Capture before the copy: the point of the assert below is that the memcpy + // carried the header across intact, not that the record is valid. It used to + // assert `valid == 1`, which invalidate() legitimately makes false — and + // since NDEBUG is not defined for this addon, that aborted release builds + // too, not just debug ones. + const uint8_t src_valid = self->record_->valid; // Copy the existing record (header + already-written attrs_data). memcpy( new_rec.get(), self->record_, sizeof(OtelThreadCtxRecord) + current_used); // Append the new entries and update attrs_data_size. memcpy(&new_rec->attrs_data[current_used], appended.data(), appended.size()); new_rec->attrs_data_size = static_cast(new_used); - // The copy should've preserved valid=1 from the source record. - assert(new_rec->valid == 1); + // The copy should've carried the source record's header across verbatim, + // whatever its validity was. + assert(new_rec->valid == src_valid); // Publish: the pointer swap is the atomic boundary the reader sees. The // first fence keeps the new_rec content writes ordered before the pointer diff --git a/ts/test/otel-invalidate-append.ts b/ts/test/otel-invalidate-append.ts new file mode 100644 index 00000000..9d12dc16 --- /dev/null +++ b/ts/test/otel-invalidate-append.ts @@ -0,0 +1,57 @@ +/* + * 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 because the failure mode is an abort, which would +// take the whole mocha run down with it. +// +// Append's reallocate path used to assert that the copied record had +// `valid == 1`. invalidate() sets that byte to 0 and appending afterwards is +// supported, so an append too large to fit in place aborted the process: +// +// Assertion `new_rec->valid == 1' failed. +// +// Only the reallocate path is affected — an append that fits the current +// capacity is written in place and never copies the header. A fresh record has +// 36 bytes of attrs_data capacity (64 - sizeof(header)), so the value below is +// comfortably past it. + +import assert from 'assert'; + +import {otelThreadCtx} from '../src/index'; + +const VALUE = 'x'.repeat(200); + +const ctx = new otelThreadCtx.ThreadContext( + Buffer.alloc(16, 1), + Buffer.alloc(8, 2), +); + +ctx.run(() => { + ctx.invalidate(); + ctx.appendAttributes([VALUE]); + + const bytes = ctx.debugBytes(); + const attrsDataSize = bytes[26] | (bytes[27] << 8); + + // invalidate() must stick: growing the record does not resurrect it. + assert.strictEqual(bytes[24], 0, 'valid byte should still be 0'); + // key index (1) + length (1) + the value itself. + assert.strictEqual(attrsDataSize, VALUE.length + 2, 'attrs_data_size'); +}); + +console.log('ok'); diff --git a/ts/test/test-otel-thread-ctx.ts b/ts/test/test-otel-thread-ctx.ts index f26af9db..f4d6683b 100644 --- a/ts/test/test-otel-thread-ctx.ts +++ b/ts/test/test-otel-thread-ctx.ts @@ -770,6 +770,45 @@ function captureBytes(opts: { }); }); + describe('invalidate then grow', () => { + // Regression test: Append's reallocate path asserted that the copied + // record had valid == 1, which invalidate() legitimately makes false, so + // an append too large to fit in place aborted the process. NDEBUG is not + // defined for this addon, so that hit release builds too. The sibling + // test above appends 6 bytes, which fits the initial 36-byte capacity and + // is written in place, so it never reached the copy. Forked, because the + // failure is an abort rather than a test failure. + it('should survive an append that reallocates after invalidate', async function () { + this.timeout(30000); + + const proc = fork(join(__dirname, 'otel-invalidate-append.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-invalidate-append exited with code=${code} signal=${signal}\n${output}`, + ), + ); + } + }); + }); + }); + }); + describe('getProcessContextAttributes', () => { it('rejects non-array keys', () => { strictAssert.throws(