From 864b26ba84ee5be703a1d0656e2bcddcfe400ff5 Mon Sep 17 00:00:00 2001 From: bm1549 Date: Fri, 8 May 2026 17:14:30 -0400 Subject: [PATCH 1/5] perf: add TracingChannel early-exit when no subscribers Match native Node.js >= 22 / >= 20.13 behavior introduced in nodejs/node#51915: when no channel has subscribers, traceSync / tracePromise / traceCallback skip publishing entirely and invoke fn directly. This was a known TODO in checks.js. Applied in two places: - patch-tracing-channel.js (full polyfill, Node < 18.19) - patch-tracing-channel-has-subscribers.js (native + getter wrap, Node 18.19-20.12) Per native semantics, the early-exit intentionally bypasses traceCallback's callback validation and tracePromise's thenable coercion. This matches Node 22+ behavior exactly. Microbenchmark on Node 24 (no subscribers): tracePromise sees a 2.5x speedup on the polyfill path. With subscribers, the slow path is within ~1% of baseline (the early-exit check has no measurable cost when it doesn't fire). Also renames the existing test/test-diagnostics-channel-tracing-channel-sync-early-exit.js to .spec.js so it actually runs (test.sh globs *.spec.js). Adds similar coverage for tracePromise and traceCallback early-exit. Co-Authored-By: Claude Opus 4.7 (1M context) --- checks.js | 4 +- patch-tracing-channel-has-subscribers.js | 30 +++++++++++++++ patch-tracing-channel.js | 38 ++++++++++++++++++- ...racing-channel-callback-early-exit.spec.js | 33 ++++++++++++++++ ...tracing-channel-promise-early-exit.spec.js | 35 +++++++++++++++++ ...l-tracing-channel-sync-early-exit.spec.js} | 6 +-- 6 files changed, 140 insertions(+), 6 deletions(-) create mode 100644 test/test-diagnostics-channel-tracing-channel-callback-early-exit.spec.js create mode 100644 test/test-diagnostics-channel-tracing-channel-promise-early-exit.spec.js rename test/{test-diagnostics-channel-tracing-channel-sync-early-exit.js => test-diagnostics-channel-tracing-channel-sync-early-exit.spec.js} (88%) diff --git a/checks.js b/checks.js index 54c14e6..0f24802 100644 --- a/checks.js +++ b/checks.js @@ -56,9 +56,9 @@ function hasSyncUnsubscribeBug() { } module.exports.hasSyncUnsubscribeBug = hasSyncUnsubscribeBug; -// if there is a TracingChannel#hasSubscribers() getter +// if there is a TracingChannel#hasSubscribers() getter and the trace*() +// early-exit when no subscribers — both shipped in the same Node PR. // @see https://github.com/nodejs/node/pull/51915 -// TODO: note that we still need to add the TC early exit from this same version function hasTracingChannelHasSubscribers() { return MAJOR >= 22 || (MAJOR == 20 && MINOR >= 13); diff --git a/patch-tracing-channel-has-subscribers.js b/patch-tracing-channel-has-subscribers.js index c852996..83f91d5 100644 --- a/patch-tracing-channel-has-subscribers.js +++ b/patch-tracing-channel-has-subscribers.js @@ -1,4 +1,5 @@ const { + ReflectApply, ObjectDefineProperty, ObjectGetPrototypeOf, } = require('./primordials.js'); @@ -21,6 +22,35 @@ module.exports = function (unpatched) { }, configurable: true }); + + // Match native Node.js >= 22 / >= 20.13 behavior: when no channel has + // subscribers, skip the entire tracing path and invoke fn directly. + // Per native semantics, this intentionally bypasses traceCallback's + // callback validation and tracePromise's thenable coercion. + // @see https://github.com/nodejs/node/pull/51915 + const origTraceSync = protoTrCh.traceSync; + if (typeof origTraceSync === 'function') { + protoTrCh.traceSync = function (fn, context, thisArg, ...args) { + if (!this.hasSubscribers) return ReflectApply(fn, thisArg, args); + return ReflectApply(origTraceSync, this, [fn, context, thisArg, ...args]); + }; + } + + const origTracePromise = protoTrCh.tracePromise; + if (typeof origTracePromise === 'function') { + protoTrCh.tracePromise = function (fn, context, thisArg, ...args) { + if (!this.hasSubscribers) return ReflectApply(fn, thisArg, args); + return ReflectApply(origTracePromise, this, [fn, context, thisArg, ...args]); + }; + } + + const origTraceCallback = protoTrCh.traceCallback; + if (typeof origTraceCallback === 'function') { + protoTrCh.traceCallback = function (fn, position, context, thisArg, ...args) { + if (!this.hasSubscribers) return ReflectApply(fn, thisArg, args); + return ReflectApply(origTraceCallback, this, [fn, position, context, thisArg, ...args]); + }; + } } return dc; diff --git a/patch-tracing-channel.js b/patch-tracing-channel.js index d3f957f..43ec262 100644 --- a/patch-tracing-channel.js +++ b/patch-tracing-channel.js @@ -73,8 +73,30 @@ module.exports = function (unpatched) { return done; } + get hasSubscribers() { + const { start, end, asyncStart, asyncEnd, error } = this; + return start.hasSubscribers + || end.hasSubscribers + || asyncStart.hasSubscribers + || asyncEnd.hasSubscribers + || error.hasSubscribers; + } + + // Each trace* method opens with an inline early-exit matching native + // Node >= 22 / >= 20.13: when no channel has subscribers, skip tracing + // and invoke fn directly. Per native semantics, this intentionally + // bypasses traceCallback's callback validation and tracePromise's + // thenable coercion. @see https://github.com/nodejs/node/pull/51915 + traceSync(fn, context = {}, thisArg, ...args) { - const { start, end, error } = this; + const { start, end, asyncStart, asyncEnd, error } = this; + if (!(start.hasSubscribers + || end.hasSubscribers + || asyncStart.hasSubscribers + || asyncEnd.hasSubscribers + || error.hasSubscribers)) { + return ReflectApply(fn, thisArg, args); + } return start.runStores(context, () => { try { @@ -93,6 +115,13 @@ module.exports = function (unpatched) { tracePromise(fn, context = {}, thisArg, ...args) { const { start, end, asyncStart, asyncEnd, error } = this; + if (!(start.hasSubscribers + || end.hasSubscribers + || asyncStart.hasSubscribers + || asyncEnd.hasSubscribers + || error.hasSubscribers)) { + return ReflectApply(fn, thisArg, args); + } function reject(err) { context.error = err; @@ -131,6 +160,13 @@ module.exports = function (unpatched) { traceCallback(fn, position = -1, context = {}, thisArg, ...args) { const { start, end, asyncStart, asyncEnd, error } = this; + if (!(start.hasSubscribers + || end.hasSubscribers + || asyncStart.hasSubscribers + || asyncEnd.hasSubscribers + || error.hasSubscribers)) { + return ReflectApply(fn, thisArg, args); + } function wrappedCallback(err, res) { if (err) { diff --git a/test/test-diagnostics-channel-tracing-channel-callback-early-exit.spec.js b/test/test-diagnostics-channel-tracing-channel-callback-early-exit.spec.js new file mode 100644 index 0000000..ae2d179 --- /dev/null +++ b/test/test-diagnostics-channel-tracing-channel-callback-early-exit.spec.js @@ -0,0 +1,33 @@ +'use strict'; + +const test = require('tape'); +const common = require('./common.js'); +const dc = require('../dc-polyfill.js'); + +test('test-diagnostics-channel-tracing-channel-callback-early-exit', (t) => { + const channel = dc.tracingChannel('test-early-exit-callback'); + + const handlers = { + start: common.mustNotCall(), + end: common.mustNotCall(), + asyncStart: common.mustNotCall(), + asyncEnd: common.mustNotCall(), + error: common.mustNotCall() + }; + + const expected = { ok: true }; + + function fn(arg, cb) { + // Subscribe inside the traced fn — early exit has committed already, + // so no events should be published for this call. + channel.subscribe(handlers); + process.nextTick(cb, null, arg); + } + + channel.traceCallback(fn, -1, {}, null, expected, (err, value) => { + t.error(err); + t.strictEqual(value, expected); + channel.unsubscribe(handlers); + t.end(); + }); +}); diff --git a/test/test-diagnostics-channel-tracing-channel-promise-early-exit.spec.js b/test/test-diagnostics-channel-tracing-channel-promise-early-exit.spec.js new file mode 100644 index 0000000..910eb16 --- /dev/null +++ b/test/test-diagnostics-channel-tracing-channel-promise-early-exit.spec.js @@ -0,0 +1,35 @@ +'use strict'; + +const test = require('tape'); +const common = require('./common.js'); +const dc = require('../dc-polyfill.js'); + +test('test-diagnostics-channel-tracing-channel-promise-early-exit', (t) => { + const channel = dc.tracingChannel('test-early-exit-promise'); + + const handlers = { + start: common.mustNotCall(), + end: common.mustNotCall(), + asyncStart: common.mustNotCall(), + asyncEnd: common.mustNotCall(), + error: common.mustNotCall() + }; + + const expected = { ok: true }; + + // Subscribe inside the traced fn — by then the early exit has already + // committed, so no events should be published for this call. + const result = channel.tracePromise(() => { + channel.subscribe(handlers); + return Promise.resolve(expected); + }, {}); + + result.then((value) => { + t.strictEqual(value, expected); + channel.unsubscribe(handlers); + t.end(); + }, (err) => { + t.fail(err); + t.end(); + }); +}); diff --git a/test/test-diagnostics-channel-tracing-channel-sync-early-exit.js b/test/test-diagnostics-channel-tracing-channel-sync-early-exit.spec.js similarity index 88% rename from test/test-diagnostics-channel-tracing-channel-sync-early-exit.js rename to test/test-diagnostics-channel-tracing-channel-sync-early-exit.spec.js index da15632..413c0a3 100644 --- a/test/test-diagnostics-channel-tracing-channel-sync-early-exit.js +++ b/test/test-diagnostics-channel-tracing-channel-sync-early-exit.spec.js @@ -5,9 +5,7 @@ const common = require('./common.js'); const dc = require('../dc-polyfill.js'); test('test-diagnostics-channel-tracing-channel-sync-early-exit', (t) => { - t.plan(0); - - const channel = dc.tracingChannel('test'); + const channel = dc.tracingChannel('test-early-exit-sync'); const handlers = { start: common.mustNotCall(), @@ -22,4 +20,6 @@ test('test-diagnostics-channel-tracing-channel-sync-early-exit', (t) => { channel.traceSync(() => { channel.subscribe(handlers); }, {}); + + t.end(); }); From 5842cc2f24ae576f05ac7af9b38e09203cfa2d53 Mon Sep 17 00:00:00 2001 From: bm1549 Date: Fri, 8 May 2026 17:29:54 -0400 Subject: [PATCH 2/5] perf: specialize trace*() wrappers by arity to cut slow-path overhead The previous wrapper used `(fn, context, thisArg, ...args)` rest + `[fn, context, thisArg, ...args]` re-spread, which allocated two arrays per call on the slow path (when subscribers exist). On Node 20.12.2 this added ~32 ns to traceSync and ~59 ns to traceCallback. Specialize for argc 3-6 (covers all common callers passing 0-3 user args), avoiding both allocations. Slow-path overhead drops to ~5 ns on traceSync and ~16 ns on traceCallback. Fast path (no subscribers) remains ~5 ns/op = 5-43x speedup vs unwrapped native. Co-Authored-By: Claude Opus 4.7 (1M context) --- patch-tracing-channel-has-subscribers.js | 69 ++++++++++++++++++++---- 1 file changed, 60 insertions(+), 9 deletions(-) diff --git a/patch-tracing-channel-has-subscribers.js b/patch-tracing-channel-has-subscribers.js index 83f91d5..be59866 100644 --- a/patch-tracing-channel-has-subscribers.js +++ b/patch-tracing-channel-has-subscribers.js @@ -28,30 +28,81 @@ module.exports = function (unpatched) { // Per native semantics, this intentionally bypasses traceCallback's // callback validation and tracePromise's thenable coercion. // @see https://github.com/nodejs/node/pull/51915 + // + // The wrappers avoid rest parameters and use Function.prototype.call + // for common arities, which lets V8 skip the rest-array allocation + // and the apply-array spread when forwarding to native. const origTraceSync = protoTrCh.traceSync; if (typeof origTraceSync === 'function') { - protoTrCh.traceSync = function (fn, context, thisArg, ...args) { - if (!this.hasSubscribers) return ReflectApply(fn, thisArg, args); - return ReflectApply(origTraceSync, this, [fn, context, thisArg, ...args]); + protoTrCh.traceSync = function (fn, context, thisArg, a, b, c) { + const argc = arguments.length; + if (!this.hasSubscribers) { + if (argc <= 3) return fn.call(thisArg); + if (argc === 4) return fn.call(thisArg, a); + if (argc === 5) return fn.call(thisArg, a, b); + if (argc === 6) return fn.call(thisArg, a, b, c); + return ReflectApply(fn, thisArg, sliceFrom(arguments, 3)); + } + if (argc <= 3) return origTraceSync.call(this, fn, context, thisArg); + if (argc === 4) return origTraceSync.call(this, fn, context, thisArg, a); + if (argc === 5) return origTraceSync.call(this, fn, context, thisArg, a, b); + if (argc === 6) return origTraceSync.call(this, fn, context, thisArg, a, b, c); + return ReflectApply(origTraceSync, this, copyAll(arguments)); }; } const origTracePromise = protoTrCh.tracePromise; if (typeof origTracePromise === 'function') { - protoTrCh.tracePromise = function (fn, context, thisArg, ...args) { - if (!this.hasSubscribers) return ReflectApply(fn, thisArg, args); - return ReflectApply(origTracePromise, this, [fn, context, thisArg, ...args]); + protoTrCh.tracePromise = function (fn, context, thisArg, a, b, c) { + const argc = arguments.length; + if (!this.hasSubscribers) { + if (argc <= 3) return fn.call(thisArg); + if (argc === 4) return fn.call(thisArg, a); + if (argc === 5) return fn.call(thisArg, a, b); + if (argc === 6) return fn.call(thisArg, a, b, c); + return ReflectApply(fn, thisArg, sliceFrom(arguments, 3)); + } + if (argc <= 3) return origTracePromise.call(this, fn, context, thisArg); + if (argc === 4) return origTracePromise.call(this, fn, context, thisArg, a); + if (argc === 5) return origTracePromise.call(this, fn, context, thisArg, a, b); + if (argc === 6) return origTracePromise.call(this, fn, context, thisArg, a, b, c); + return ReflectApply(origTracePromise, this, copyAll(arguments)); }; } const origTraceCallback = protoTrCh.traceCallback; if (typeof origTraceCallback === 'function') { - protoTrCh.traceCallback = function (fn, position, context, thisArg, ...args) { - if (!this.hasSubscribers) return ReflectApply(fn, thisArg, args); - return ReflectApply(origTraceCallback, this, [fn, position, context, thisArg, ...args]); + protoTrCh.traceCallback = function (fn, position, context, thisArg, a, b, c) { + const argc = arguments.length; + if (!this.hasSubscribers) { + if (argc <= 4) return fn.call(thisArg); + if (argc === 5) return fn.call(thisArg, a); + if (argc === 6) return fn.call(thisArg, a, b); + if (argc === 7) return fn.call(thisArg, a, b, c); + return ReflectApply(fn, thisArg, sliceFrom(arguments, 4)); + } + if (argc <= 4) return origTraceCallback.call(this, fn, position, context, thisArg); + if (argc === 5) return origTraceCallback.call(this, fn, position, context, thisArg, a); + if (argc === 6) return origTraceCallback.call(this, fn, position, context, thisArg, a, b); + if (argc === 7) return origTraceCallback.call(this, fn, position, context, thisArg, a, b, c); + return ReflectApply(origTraceCallback, this, copyAll(arguments)); }; } } return dc; }; + +function sliceFrom(argsLike, from) { + const len = argsLike.length; + const out = new Array(len - from); + for (let i = from; i < len; i++) out[i - from] = argsLike[i]; + return out; +} + +function copyAll(argsLike) { + const len = argsLike.length; + const out = new Array(len); + for (let i = 0; i < len; i++) out[i] = argsLike[i]; + return out; +} From 2e2d08816b1a6f9a21aeb863bdab78b1112ec7fd Mon Sep 17 00:00:00 2001 From: bm1549 Date: Fri, 8 May 2026 17:39:09 -0400 Subject: [PATCH 3/5] fix: dispatch fast path via primordial uncurriedCall, scope traceSync's check Two correctness fixes after Codex adversarial review: 1. Fast-path dispatch must not go through user-controlled fn.call. `fn.call(...)` is vulnerable when user code does `f.call = null`, which native trace*() never crashes on (it uses Reflect.apply). Switch to a captured `Function.prototype.call.bind(Function.prototype.call)` primordial that survives both `fn.call = null` and `Function.prototype.call = null` poisoning, with no perf regression vs the previous arity-specialized `.call` version. 2. Polyfill traceSync's early-exit must only check channels traceSync would publish to (start/end/error). The previous version touched asyncStart/asyncEnd, which crashed on partial object-form TracingChannels (e.g. tracingChannel({start, end, error})) that the pre-patch polyfill traceSync handled fine. Adds regression test for shadowed fn.call. Co-Authored-By: Claude Opus 4.7 (1M context) --- patch-tracing-channel-has-subscribers.js | 56 +++++++++---------- patch-tracing-channel.js | 10 ++-- ...nnel-tracing-channel-shadowed-call.spec.js | 37 ++++++++++++ 3 files changed, 69 insertions(+), 34 deletions(-) create mode 100644 test/test-diagnostics-channel-tracing-channel-shadowed-call.spec.js diff --git a/patch-tracing-channel-has-subscribers.js b/patch-tracing-channel-has-subscribers.js index be59866..931b571 100644 --- a/patch-tracing-channel-has-subscribers.js +++ b/patch-tracing-channel-has-subscribers.js @@ -4,6 +4,10 @@ const { ObjectGetPrototypeOf, } = require('./primordials.js'); +// Tamper-resistant equivalent of `fn.call(thisArg, ...rest)`. +// Survives `userFn.call = null` and `Function.prototype.call = null` poisoning. +const uncurriedCall = Function.prototype.call.bind(Function.prototype.call); + module.exports = function (unpatched) { const dc = { ...unpatched }; @@ -28,25 +32,21 @@ module.exports = function (unpatched) { // Per native semantics, this intentionally bypasses traceCallback's // callback validation and tracePromise's thenable coercion. // @see https://github.com/nodejs/node/pull/51915 - // - // The wrappers avoid rest parameters and use Function.prototype.call - // for common arities, which lets V8 skip the rest-array allocation - // and the apply-array spread when forwarding to native. const origTraceSync = protoTrCh.traceSync; if (typeof origTraceSync === 'function') { protoTrCh.traceSync = function (fn, context, thisArg, a, b, c) { const argc = arguments.length; if (!this.hasSubscribers) { - if (argc <= 3) return fn.call(thisArg); - if (argc === 4) return fn.call(thisArg, a); - if (argc === 5) return fn.call(thisArg, a, b); - if (argc === 6) return fn.call(thisArg, a, b, c); + if (argc <= 3) return uncurriedCall(fn, thisArg); + if (argc === 4) return uncurriedCall(fn, thisArg, a); + if (argc === 5) return uncurriedCall(fn, thisArg, a, b); + if (argc === 6) return uncurriedCall(fn, thisArg, a, b, c); return ReflectApply(fn, thisArg, sliceFrom(arguments, 3)); } - if (argc <= 3) return origTraceSync.call(this, fn, context, thisArg); - if (argc === 4) return origTraceSync.call(this, fn, context, thisArg, a); - if (argc === 5) return origTraceSync.call(this, fn, context, thisArg, a, b); - if (argc === 6) return origTraceSync.call(this, fn, context, thisArg, a, b, c); + if (argc <= 3) return uncurriedCall(origTraceSync, this, fn, context, thisArg); + if (argc === 4) return uncurriedCall(origTraceSync, this, fn, context, thisArg, a); + if (argc === 5) return uncurriedCall(origTraceSync, this, fn, context, thisArg, a, b); + if (argc === 6) return uncurriedCall(origTraceSync, this, fn, context, thisArg, a, b, c); return ReflectApply(origTraceSync, this, copyAll(arguments)); }; } @@ -56,16 +56,16 @@ module.exports = function (unpatched) { protoTrCh.tracePromise = function (fn, context, thisArg, a, b, c) { const argc = arguments.length; if (!this.hasSubscribers) { - if (argc <= 3) return fn.call(thisArg); - if (argc === 4) return fn.call(thisArg, a); - if (argc === 5) return fn.call(thisArg, a, b); - if (argc === 6) return fn.call(thisArg, a, b, c); + if (argc <= 3) return uncurriedCall(fn, thisArg); + if (argc === 4) return uncurriedCall(fn, thisArg, a); + if (argc === 5) return uncurriedCall(fn, thisArg, a, b); + if (argc === 6) return uncurriedCall(fn, thisArg, a, b, c); return ReflectApply(fn, thisArg, sliceFrom(arguments, 3)); } - if (argc <= 3) return origTracePromise.call(this, fn, context, thisArg); - if (argc === 4) return origTracePromise.call(this, fn, context, thisArg, a); - if (argc === 5) return origTracePromise.call(this, fn, context, thisArg, a, b); - if (argc === 6) return origTracePromise.call(this, fn, context, thisArg, a, b, c); + if (argc <= 3) return uncurriedCall(origTracePromise, this, fn, context, thisArg); + if (argc === 4) return uncurriedCall(origTracePromise, this, fn, context, thisArg, a); + if (argc === 5) return uncurriedCall(origTracePromise, this, fn, context, thisArg, a, b); + if (argc === 6) return uncurriedCall(origTracePromise, this, fn, context, thisArg, a, b, c); return ReflectApply(origTracePromise, this, copyAll(arguments)); }; } @@ -75,16 +75,16 @@ module.exports = function (unpatched) { protoTrCh.traceCallback = function (fn, position, context, thisArg, a, b, c) { const argc = arguments.length; if (!this.hasSubscribers) { - if (argc <= 4) return fn.call(thisArg); - if (argc === 5) return fn.call(thisArg, a); - if (argc === 6) return fn.call(thisArg, a, b); - if (argc === 7) return fn.call(thisArg, a, b, c); + if (argc <= 4) return uncurriedCall(fn, thisArg); + if (argc === 5) return uncurriedCall(fn, thisArg, a); + if (argc === 6) return uncurriedCall(fn, thisArg, a, b); + if (argc === 7) return uncurriedCall(fn, thisArg, a, b, c); return ReflectApply(fn, thisArg, sliceFrom(arguments, 4)); } - if (argc <= 4) return origTraceCallback.call(this, fn, position, context, thisArg); - if (argc === 5) return origTraceCallback.call(this, fn, position, context, thisArg, a); - if (argc === 6) return origTraceCallback.call(this, fn, position, context, thisArg, a, b); - if (argc === 7) return origTraceCallback.call(this, fn, position, context, thisArg, a, b, c); + if (argc <= 4) return uncurriedCall(origTraceCallback, this, fn, position, context, thisArg); + if (argc === 5) return uncurriedCall(origTraceCallback, this, fn, position, context, thisArg, a); + if (argc === 6) return uncurriedCall(origTraceCallback, this, fn, position, context, thisArg, a, b); + if (argc === 7) return uncurriedCall(origTraceCallback, this, fn, position, context, thisArg, a, b, c); return ReflectApply(origTraceCallback, this, copyAll(arguments)); }; } diff --git a/patch-tracing-channel.js b/patch-tracing-channel.js index 43ec262..5b454c0 100644 --- a/patch-tracing-channel.js +++ b/patch-tracing-channel.js @@ -87,14 +87,12 @@ module.exports = function (unpatched) { // and invoke fn directly. Per native semantics, this intentionally // bypasses traceCallback's callback validation and tracePromise's // thenable coercion. @see https://github.com/nodejs/node/pull/51915 + // Each method only checks the channels it would publish to, preserving + // backward compat with partial object-form TracingChannels. traceSync(fn, context = {}, thisArg, ...args) { - const { start, end, asyncStart, asyncEnd, error } = this; - if (!(start.hasSubscribers - || end.hasSubscribers - || asyncStart.hasSubscribers - || asyncEnd.hasSubscribers - || error.hasSubscribers)) { + const { start, end, error } = this; + if (!(start.hasSubscribers || end.hasSubscribers || error.hasSubscribers)) { return ReflectApply(fn, thisArg, args); } diff --git a/test/test-diagnostics-channel-tracing-channel-shadowed-call.spec.js b/test/test-diagnostics-channel-tracing-channel-shadowed-call.spec.js new file mode 100644 index 0000000..25656ea --- /dev/null +++ b/test/test-diagnostics-channel-tracing-channel-shadowed-call.spec.js @@ -0,0 +1,37 @@ +'use strict'; + +// Regression: trace*() must not dispatch through user-controlled fn.call. +// A function with `f.call = null` would crash a `fn.call(...)` based wrapper +// but native and the polyfill use Reflect.apply / primordials. + +const test = require('tape'); +const dc = require('../dc-polyfill.js'); + +test('traceSync survives shadowed fn.call when no subscribers', (t) => { + const channel = dc.tracingChannel('test-shadowed-call-sync'); + const fn = function () { return 42; }; + fn.call = null; + t.strictEqual(channel.traceSync(fn, {}, null), 42); + t.end(); +}); + +test('tracePromise survives shadowed fn.call when no subscribers', (t) => { + const channel = dc.tracingChannel('test-shadowed-call-promise'); + const expected = Promise.resolve(42); + const fn = function () { return expected; }; + fn.call = null; + const result = channel.tracePromise(fn, {}, null); + t.strictEqual(result, expected); + t.end(); +}); + +test('traceCallback survives shadowed fn.call when no subscribers', (t) => { + const channel = dc.tracingChannel('test-shadowed-call-callback'); + const fn = function (cb) { cb(null, 42); }; + fn.call = null; + channel.traceCallback(fn, -1, {}, null, (err, value) => { + t.error(err); + t.strictEqual(value, 42); + t.end(); + }); +}); From 817d7f903d648fadfc55a2780910382ea29bcf0f Mon Sep 17 00:00:00 2001 From: bm1549 Date: Fri, 8 May 2026 17:46:33 -0400 Subject: [PATCH 4/5] Address review feedback: null-safe getter, primordials, simpler helpers - Make polyfill TC's hasSubscribers getter null-safe so partial object-form construction (e.g. tracingChannel({start, end, error})) can still probe hasSubscribers without crashing on the missing asyncStart/asyncEnd channels. Add regression test. - Promote uncurriedCall to primordials.js as FunctionPrototypeCallApply so other patches can use the tamper-resistant call helper. - Drop the duplicate copyAll helper; use sliceFrom(args, 0) instead. - Trim unnecessary preamble comment from shadowed-call test. Co-Authored-By: Claude Opus 4.7 (1M context) --- patch-tracing-channel-has-subscribers.js | 66 ++++++++----------- patch-tracing-channel.js | 13 ++-- primordials.js | 4 ++ ...nel-tracing-channel-partial-object.spec.js | 23 +++++++ ...nnel-tracing-channel-shadowed-call.spec.js | 4 -- 5 files changed, 63 insertions(+), 47 deletions(-) create mode 100644 test/test-diagnostics-channel-tracing-channel-partial-object.spec.js diff --git a/patch-tracing-channel-has-subscribers.js b/patch-tracing-channel-has-subscribers.js index 931b571..e37fe7f 100644 --- a/patch-tracing-channel-has-subscribers.js +++ b/patch-tracing-channel-has-subscribers.js @@ -1,13 +1,10 @@ const { ReflectApply, + FunctionPrototypeCallApply, ObjectDefineProperty, ObjectGetPrototypeOf, } = require('./primordials.js'); -// Tamper-resistant equivalent of `fn.call(thisArg, ...rest)`. -// Survives `userFn.call = null` and `Function.prototype.call = null` poisoning. -const uncurriedCall = Function.prototype.call.bind(Function.prototype.call); - module.exports = function (unpatched) { const dc = { ...unpatched }; @@ -37,17 +34,17 @@ module.exports = function (unpatched) { protoTrCh.traceSync = function (fn, context, thisArg, a, b, c) { const argc = arguments.length; if (!this.hasSubscribers) { - if (argc <= 3) return uncurriedCall(fn, thisArg); - if (argc === 4) return uncurriedCall(fn, thisArg, a); - if (argc === 5) return uncurriedCall(fn, thisArg, a, b); - if (argc === 6) return uncurriedCall(fn, thisArg, a, b, c); + if (argc <= 3) return FunctionPrototypeCallApply(fn, thisArg); + if (argc === 4) return FunctionPrototypeCallApply(fn, thisArg, a); + if (argc === 5) return FunctionPrototypeCallApply(fn, thisArg, a, b); + if (argc === 6) return FunctionPrototypeCallApply(fn, thisArg, a, b, c); return ReflectApply(fn, thisArg, sliceFrom(arguments, 3)); } - if (argc <= 3) return uncurriedCall(origTraceSync, this, fn, context, thisArg); - if (argc === 4) return uncurriedCall(origTraceSync, this, fn, context, thisArg, a); - if (argc === 5) return uncurriedCall(origTraceSync, this, fn, context, thisArg, a, b); - if (argc === 6) return uncurriedCall(origTraceSync, this, fn, context, thisArg, a, b, c); - return ReflectApply(origTraceSync, this, copyAll(arguments)); + if (argc <= 3) return FunctionPrototypeCallApply(origTraceSync, this, fn, context, thisArg); + if (argc === 4) return FunctionPrototypeCallApply(origTraceSync, this, fn, context, thisArg, a); + if (argc === 5) return FunctionPrototypeCallApply(origTraceSync, this, fn, context, thisArg, a, b); + if (argc === 6) return FunctionPrototypeCallApply(origTraceSync, this, fn, context, thisArg, a, b, c); + return ReflectApply(origTraceSync, this, sliceFrom(arguments, 0)); }; } @@ -56,17 +53,17 @@ module.exports = function (unpatched) { protoTrCh.tracePromise = function (fn, context, thisArg, a, b, c) { const argc = arguments.length; if (!this.hasSubscribers) { - if (argc <= 3) return uncurriedCall(fn, thisArg); - if (argc === 4) return uncurriedCall(fn, thisArg, a); - if (argc === 5) return uncurriedCall(fn, thisArg, a, b); - if (argc === 6) return uncurriedCall(fn, thisArg, a, b, c); + if (argc <= 3) return FunctionPrototypeCallApply(fn, thisArg); + if (argc === 4) return FunctionPrototypeCallApply(fn, thisArg, a); + if (argc === 5) return FunctionPrototypeCallApply(fn, thisArg, a, b); + if (argc === 6) return FunctionPrototypeCallApply(fn, thisArg, a, b, c); return ReflectApply(fn, thisArg, sliceFrom(arguments, 3)); } - if (argc <= 3) return uncurriedCall(origTracePromise, this, fn, context, thisArg); - if (argc === 4) return uncurriedCall(origTracePromise, this, fn, context, thisArg, a); - if (argc === 5) return uncurriedCall(origTracePromise, this, fn, context, thisArg, a, b); - if (argc === 6) return uncurriedCall(origTracePromise, this, fn, context, thisArg, a, b, c); - return ReflectApply(origTracePromise, this, copyAll(arguments)); + if (argc <= 3) return FunctionPrototypeCallApply(origTracePromise, this, fn, context, thisArg); + if (argc === 4) return FunctionPrototypeCallApply(origTracePromise, this, fn, context, thisArg, a); + if (argc === 5) return FunctionPrototypeCallApply(origTracePromise, this, fn, context, thisArg, a, b); + if (argc === 6) return FunctionPrototypeCallApply(origTracePromise, this, fn, context, thisArg, a, b, c); + return ReflectApply(origTracePromise, this, sliceFrom(arguments, 0)); }; } @@ -75,17 +72,17 @@ module.exports = function (unpatched) { protoTrCh.traceCallback = function (fn, position, context, thisArg, a, b, c) { const argc = arguments.length; if (!this.hasSubscribers) { - if (argc <= 4) return uncurriedCall(fn, thisArg); - if (argc === 5) return uncurriedCall(fn, thisArg, a); - if (argc === 6) return uncurriedCall(fn, thisArg, a, b); - if (argc === 7) return uncurriedCall(fn, thisArg, a, b, c); + if (argc <= 4) return FunctionPrototypeCallApply(fn, thisArg); + if (argc === 5) return FunctionPrototypeCallApply(fn, thisArg, a); + if (argc === 6) return FunctionPrototypeCallApply(fn, thisArg, a, b); + if (argc === 7) return FunctionPrototypeCallApply(fn, thisArg, a, b, c); return ReflectApply(fn, thisArg, sliceFrom(arguments, 4)); } - if (argc <= 4) return uncurriedCall(origTraceCallback, this, fn, position, context, thisArg); - if (argc === 5) return uncurriedCall(origTraceCallback, this, fn, position, context, thisArg, a); - if (argc === 6) return uncurriedCall(origTraceCallback, this, fn, position, context, thisArg, a, b); - if (argc === 7) return uncurriedCall(origTraceCallback, this, fn, position, context, thisArg, a, b, c); - return ReflectApply(origTraceCallback, this, copyAll(arguments)); + if (argc <= 4) return FunctionPrototypeCallApply(origTraceCallback, this, fn, position, context, thisArg); + if (argc === 5) return FunctionPrototypeCallApply(origTraceCallback, this, fn, position, context, thisArg, a); + if (argc === 6) return FunctionPrototypeCallApply(origTraceCallback, this, fn, position, context, thisArg, a, b); + if (argc === 7) return FunctionPrototypeCallApply(origTraceCallback, this, fn, position, context, thisArg, a, b, c); + return ReflectApply(origTraceCallback, this, sliceFrom(arguments, 0)); }; } } @@ -99,10 +96,3 @@ function sliceFrom(argsLike, from) { for (let i = from; i < len; i++) out[i - from] = argsLike[i]; return out; } - -function copyAll(argsLike) { - const len = argsLike.length; - const out = new Array(len); - for (let i = 0; i < len; i++) out[i] = argsLike[i]; - return out; -} diff --git a/patch-tracing-channel.js b/patch-tracing-channel.js index 5b454c0..cf0a17e 100644 --- a/patch-tracing-channel.js +++ b/patch-tracing-channel.js @@ -74,12 +74,15 @@ module.exports = function (unpatched) { } get hasSubscribers() { + // Null-safe: partial object-form TracingChannels (e.g. {start, end, error}) + // are accepted by the constructor above, so missing async channels must + // not crash this probe. const { start, end, asyncStart, asyncEnd, error } = this; - return start.hasSubscribers - || end.hasSubscribers - || asyncStart.hasSubscribers - || asyncEnd.hasSubscribers - || error.hasSubscribers; + return (start && start.hasSubscribers) + || (end && end.hasSubscribers) + || (asyncStart && asyncStart.hasSubscribers) + || (asyncEnd && asyncEnd.hasSubscribers) + || (error && error.hasSubscribers); } // Each trace* method opens with an inline early-exit matching native diff --git a/primordials.js b/primordials.js index b00d084..a50d567 100644 --- a/primordials.js +++ b/primordials.js @@ -14,6 +14,9 @@ function arrayAtPolyfill(n) { } const ReflectApply = Reflect.apply; +// Tamper-resistant `fn.call(thisArg, ...rest)`. Survives userland setting +// `userFn.call = null` or `Function.prototype.call = null` after this load. +const FunctionPrototypeCallApply = Function.prototype.call.bind(Function.prototype.call); const PromiseReject = Promise.reject.bind(Promise); const PromiseResolve = Promise.resolve; const PromisePrototypeThen = makeCall(Promise.prototype.then); @@ -28,6 +31,7 @@ const SymbolFor = Symbol.for; module.exports = { ReflectApply, + FunctionPrototypeCallApply, PromiseReject, PromiseResolve, PromisePrototypeThen, diff --git a/test/test-diagnostics-channel-tracing-channel-partial-object.spec.js b/test/test-diagnostics-channel-tracing-channel-partial-object.spec.js new file mode 100644 index 0000000..241dac3 --- /dev/null +++ b/test/test-diagnostics-channel-tracing-channel-partial-object.spec.js @@ -0,0 +1,23 @@ +'use strict'; + +const test = require('tape'); +const checks = require('../checks.js'); +const dc = require('../dc-polyfill.js'); + +// Partial object-form TracingChannels are only accepted by the JS polyfill +// path (Node < 18.19). Native diagnostics_channel rejects them at construction. +if (checks.hasTracingChannel()) { + test.skip('partial object-form TC is only accepted by the polyfill', () => {}); +} else { + test('polyfill traceSync with partial {start, end, error} object works', (t) => { + const start = dc.channel('partial:start'); + const end = dc.channel('partial:end'); + const error = dc.channel('partial:error'); + const tc = dc.tracingChannel({ start, end, error }); + + t.doesNotThrow(() => tc.hasSubscribers, 'hasSubscribers must not throw'); + t.strictEqual(tc.hasSubscribers, false); + t.strictEqual(tc.traceSync(() => 7, {}, null), 7); + t.end(); + }); +} diff --git a/test/test-diagnostics-channel-tracing-channel-shadowed-call.spec.js b/test/test-diagnostics-channel-tracing-channel-shadowed-call.spec.js index 25656ea..69c9f30 100644 --- a/test/test-diagnostics-channel-tracing-channel-shadowed-call.spec.js +++ b/test/test-diagnostics-channel-tracing-channel-shadowed-call.spec.js @@ -1,9 +1,5 @@ 'use strict'; -// Regression: trace*() must not dispatch through user-controlled fn.call. -// A function with `f.call = null` would crash a `fn.call(...)` based wrapper -// but native and the polyfill use Reflect.apply / primordials. - const test = require('tape'); const dc = require('../dc-polyfill.js'); From 906ac4c191755736bcedebfcae07e02eeba9e063 Mon Sep 17 00:00:00 2001 From: bm1549 Date: Fri, 8 May 2026 17:49:19 -0400 Subject: [PATCH 5/5] fix: make has-subscribers getter null-safe too On Node <18.19, both patch-tracing-channel.js (full polyfill) and patch-tracing-channel-has-subscribers.js are applied. The latter's getter overrides the polyfill class's null-safe getter, so partial object-form TracingChannels still crash on hasSubscribers. Make this getter null-safe as well. Native TC channels are always defined, so the extra checks are free defensive cost. Co-Authored-By: Claude Opus 4.7 (1M context) --- patch-tracing-channel-has-subscribers.js | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/patch-tracing-channel-has-subscribers.js b/patch-tracing-channel-has-subscribers.js index e37fe7f..eadc5b8 100644 --- a/patch-tracing-channel-has-subscribers.js +++ b/patch-tracing-channel-has-subscribers.js @@ -15,11 +15,14 @@ module.exports = function (unpatched) { ObjectDefineProperty(protoTrCh, 'hasSubscribers', { get: function () { - return this.start.hasSubscribers - || this.end.hasSubscribers - || this.asyncStart.hasSubscribers - || this.asyncEnd.hasSubscribers - || this.error.hasSubscribers; + // Null-safe: this patch is also applied on top of the JS polyfill on + // older Node, where partial object-form TracingChannels are accepted. + const { start, end, asyncStart, asyncEnd, error } = this; + return (start && start.hasSubscribers) + || (end && end.hasSubscribers) + || (asyncStart && asyncStart.hasSubscribers) + || (asyncEnd && asyncEnd.hasSubscribers) + || (error && error.hasSubscribers); }, configurable: true });