fix(debugger): limit and safely stringify template log message values - #9766
fix(debugger): limit and safely stringify template log message values#9766watson wants to merge 1 commit into
Conversation
github-actions
Bot
added
semver-patch
debugger
Overall package sizeSelf size: 8.09 MB Dependency sizes| name | version | self size | total size | |------|---------|-----------|------------| | import-in-the-middle | 3.3.3 | 125.43 kB | 441.68 kB | | opentracing | 0.14.7 | 194.81 kB | 194.81 kB | | dc-polyfill | 0.1.11 | 25.74 kB | 25.74 kB |🤖 This report was automatically generated by heaviest-objects-in-the-universe |
|
Bits has a CI fix ready🟢 Investigated · 🟢 Fix prepared · ⚪ Validation skipped · 🟠 Ready
View in Datadog | Reviewed commit 964bab5 · Any feedback? Reach out in #deveng-pr-agent |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #9766 +/- ##
==========================================
+ Coverage 93.41% 97.39% +3.97%
==========================================
Files 950 970 +20
Lines 140420 141221 +801
Branches 11077 12007 +930
==========================================
+ Hits 131170 137538 +6368
+ Misses 9250 3683 -5567
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
00161cd to
dfabaf8
Compare
BenchmarksBenchmark execution time: 2026-08-13 10:04:06 Comparing candidate commit 4b3ad48 in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 2311 metrics, 47 unstable metrics.
|
| const symbols = Object.getOwnPropertySymbols(value) | ||
| for (let i = 0; i < symbols.length; i++) { | ||
| if (Object.getOwnPropertyDescriptor(value, symbols[i])?.enumerable === true) { | ||
| propertyCount++ | ||
| if (keys.length < maxProperties) keys.push(symbols[i]) | ||
| } | ||
| } |
There was a problem hiding this comment.
If keys.length (propertyCount, while pulling out the length is not changing performance) is above maxProperties, this whole code will never add any keys and I think we should skip it (getting the symbols and descriptors is actually expensive)
There was a problem hiding this comment.
I was originally thinking the same, but ended up with this implementation to ensure the n more properties was counted correctly... but it's a tough choice. Do we optimize for performance or correctness 🤷
There was a problem hiding this comment.
Hm, in new Node.js versions having objects with zero symbols is faster again. So it might not hurt that badly anymore.
| Array.isArray(value) || | ||
| types.isTypedArray(value) || | ||
| types.isAnyArrayBuffer(value) || | ||
| types.isDataView(value) || | ||
| types.isMap(value) || | ||
| types.isSet(value) || | ||
| types.isWeakMap(value) || | ||
| types.isWeakSet(value) || | ||
| types.isMapIterator(value) || | ||
| types.isSetIterator(value) |
There was a problem hiding this comment.
We could check for an iterable symbol. That is not perfect, since it could still be a regular object, while unlikely and it would allow to skip multiple checks.
There was a problem hiding this comment.
Yeah I don't like this wall of checks either... but was hoping the V8 optimizer would find a way to make it performant.
I am working on a PR to support this directly in Node core though. If that lands, we can live with a slower polyfill.
There was a problem hiding this comment.
These are implemented in Node.js in C++ as fast calls. So they are inlined, while we still need to go through each.
| const descriptor = /** @type {PropertyDescriptor} */ (Object.getOwnPropertyDescriptor(value, keys[i])) | ||
| if ( | ||
| (keys[i] === Symbol.toStringTag && descriptor.get !== undefined) || | ||
| (descriptor.value !== value && inspectionCanRunUserCode(descriptor.value)) | ||
| ) { | ||
| return '[Value omitted: inspection may execute user code]' | ||
| } | ||
| if (descriptor.value === value) descriptor.value = truncated | ||
| Object.defineProperty(truncated, keys[i], descriptor) |
There was a problem hiding this comment.
Since the object will likely be truncated and it is a different object anyway, is copying the descriptor crucial?
If not, we could just check for the presence of the toStringTag with e.g., in and get the descriptor directly.
There was a problem hiding this comment.
We don't need all the descriptor flags, but I don't think we can get around having to use descriptor in some capacity. It all depends how many side effects you're willing to potentially trigger. Or maybe I'm misunderstanding your proposal?
dfabaf8 to
964bab5
Compare
Debugger log templates previously used util.inspect directly, which could stringify objects with unbounded property counts and invoke user code via prototype-chain proxy traps and Symbol.toStringTag getters. Add inspectSegment to cap enumerable object properties at five, omit values whose inspection may execute user code, and render direct proxies as [Proxy] for predictable output. Wire the helper into the devtools client via a dd-trace global so template expressions use the safe formatter.
964bab5 to
4b3ad48
Compare
2 participants
What does this PR do?
Adds a dedicated
inspectSegmenthelper for Debugger log template expressions. It limits enumerable object properties to five, omits values that could run user code during inspection, and renders direct proxies as[Proxy]. The devtools client now calls this helper via a dd-trace global instead of inliningutil.inspectoptions.Motivation
Template log messages could stringify objects with unbounded property counts. Inspection can also invoke user code when walking a prototype-chain proxy (
getPrototypeOftraps) or formatting values withSymbol.toStringTaggetters (notably class instances). Note thatutil.inspecton a directProxyvalue generally does not exerciseownKeys/gettraps on current Node; the[Proxy]label is mainly for predictable, bounded output rather than trap avoidance alone. DEBUG-5974.Additional Notes
Symbol.toStringTaggetter are fully omitted rather than partially inspected; a TODO ininspect-segment.jstracks whether that trade-off should change.