Conversation
…p Map/Set entries The native console formatter's "never throw because of the value being printed" law had five unguarded accessor reads and an uncapped Map/Set entry loop. print_event read type/message/data/error through the user-visible getter with ?-propagation, so a subclass or own-property accessor that throws aborted the whole console.log/Bun.inspect call. Each read now catches the throw, clears it off the VM, and treats the value as absent, mirroring the existing print_to_json catch-then-clear pattern. print_errorlike_object's AggregateError branch reads .errors via getDirect, which for an own accessor yields the raw GetterSetter cell (not an object). Passing that to forEachInIterable either hit an assertion (debug) or threw a TypeError that was left pending on the VM for the next print_as_prelude to re-throw. It is now guarded by is_object() and the for_each result clears any pending exception. print_map_like and print_set rendered every entry; a million-entry Map produced 17 MB of output. Both iterator callbacks now stop formatting at 100 entries and emit a "... N more item(s)" elision marker, matching the array printer's existing cap and Node's default maxArrayLength behaviour for Map/Set.
|
Warning Review limit reached
Next review available in: 8 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Comment |
|
Status: diff is green. Build #81800: 195 jobs passed, 1 failed. The only hard failure is Gate: test fails without the fix and passes with it on both ASAN and release. Ready for a maintainer. Reproduce on bun -e 'const m = new Map(); for (let i=0;i<1e6;i++) m.set(i,i); console.log(Bun.inspect(m).length)'
# 17777796 |
|
Updated 6:23 PM PT - Jul 25th, 2026
❌ @robobun, your commit ed0d368 has 1 failures in
Add 🧪 To try this PR locally: bunx bun-pr 35826That installs a local version of the PR into your bun-35826 --bun |
||||||||||||||||||||||||||||||||||||||||||||||||||||||
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
robobun
changed the title
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🟡
src/jsc/ConsoleObject.rs:2950-2957— The cap guardthis.length > MAP_SET_ENTRY_CAP && this.count >= MAP_SET_ENTRY_CAPkeys offlength, which is populated from the user-visiblesizegetter (ConsoleObject.rs:4803/4950) — a subclass withget size() { return 1; }holding a million real entries makes the first conjunct false and every entry still prints. Sincecountis incremented for every callback regardless, the cap could be driven bycountalone (with the... N more itemsmarker printed after the loop from the finalcount), gating the whole thing on!IS_ITERATORinstead oflength: 0to keep iterators uncapped. Nit — adversarial input only; the honest-large-Map case this PR targets is fixed.Extended reasoning...
What the bug is
The new Map/Set entry cap in
MapIteratorCtx::for_each/SetIteratorCtx::for_eachis guarded by:if this.length > MAP_SET_ENTRY_CAP && this.count >= MAP_SET_ENTRY_CAP {
where
lengthis threaded through fromprint_map_like/print_set:let length_value = value.get(self.global_this, "size")?...; let length = length_value.coerce_to_i32(self.global_this)?;
value.get(global, "size")walks the prototype chain and invokes user getters. A Map subclass (or an own-propertydefineProperty) that reportssize≤ 100 while the map actually holds far more entries makesthis.length > MAP_SET_ENTRY_CAPevaluate to false, so the cap branch is never taken andfor_eachcontinues to render every real entry.Step-by-step proof
class M extends Map { get size() { return 1; } } const m = new M(); for (let i = 0; i < 1_000_000; i++) m.set(i, i); Bun.inspect(m);
print_map_likecallsvalue.get(global, "size")→ invokesM.prototype.size→ returns1.length = 1.max(0) as usize = 1is stored onMapIteratorCtx.value.for_each(...)iterates the real backing store (JSC'sforEachInIterablewalks the internal HashMapImpl, not the reported size), so the callback fires 1,000,000 times.- On every callback,
this.length > MAP_SET_ENTRY_CAPis1 > 100→false, so the guard short-circuits and the entry is printed. - Result: all 1,000,000 entries render — the same ~17 MB output the PR set out to bound.
The existing regression test
Set/Map with overridden size propertyin this same file already establishes thatsizeis user-overridable on this exact code path, so no new mechanism is being hypothesized here.Why the guard is written this way
The
length > CAPconjunct isn't gratuitous:print_map_iterator_likereusesMapIteratorCtxwithlength: 0precisely so that Map/Set iterators (whose size is unknown up front) stay uncapped. Dropping the conjunct outright would inadvertently cap iterators too. That's why the fix isn't just "delete the first half of the&&."Suggested fix
Since the callback is invoked for every real entry regardless of what
sizereports,countends up equal to the true entry count. The cap can therefore key oncountalone:- In the callback:
if !IS_ITERATOR && this.count >= MAP_SET_ENTRY_CAP { this.count += 1; return; }(Map already has theIS_ITERATORconst; Set doesn't need it sinceprint_setis the only caller). - After
for_eachreturns, ifiter.count > MAP_SET_ENTRY_CAP, callprint_more_items_markerwithiter.countastotal. This also means the... N more itemscount reflects the true remaining entries even whensizelies in the other direction (over-reports).
This removes the need for the
lengthfield on both context structs entirely.Why this is a nit
This only manifests with a deliberately hostile Map/Set subclass. Console output size is not a security boundary — an attacker who can construct such a subclass can already do
console.log('x'.repeat(1e8))or install a custom[Symbol.for('nodejs.util.inspect.custom')]that returns arbitrary-length strings. REVIEW.md's "assume userland is hostile" section explicitly scopes itself to security-relevant paths (rejectUnauthorized, credential merging), not display formatting. The PR's motivating case — an honest million-entry Map producing 17 MB — is correctly fixed, and the author has already noted a formatter-wide byte-budget backstop as deferred follow-up that would defeat this and every other adversarial-width vector uniformly. Not worth blocking merge; worth folding in if there's another revision. -
🟡
src/jsc/ConsoleObject.rs:4833— Same-class sites left unguarded:print_map_like/print_setstill?-propagate on the.get("size")?/coerce_to_i32(...)?reads (lines 4802-4805, 4949-4952) and on the fourvalue.for_each(...)?calls (lines 4841-4845, 4860-4864, 4988-4992, 5007-5011) — a Map/Set with a throwingsizegetter or a throwingSymbol.iterator/next()still abortsBun.inspect. Since this PR already edits both functions to thread the newlengthfield and definesget_swallowing_throw+ thefor_each().is_err() → clear_exception()pattern for exactly this, consider routing these sibling reads through the same helpers. Pre-existing behavior, adversarial-only — not blocking.Extended reasoning...
What
The PR's stated goal is that user-installed accessors/hooks that throw must not abort
console.log/Bun.inspect. It adds two mechanisms for this:get_swallowing_throw/fast_get_swallowing_throw(used for the fourprint_eventreads) and thefor_each(...).is_err() → clear_exception()wrapper (used for AggregateError'serrorsiterator inVirtualMachine.rs). But the sibling Map/Set printers — which this PR is already editing to add the newlengthfield at lines 4833 and 4980 — still leave both patterns unguarded.Sites
sizegetter (print_map_like4802-4805,print_set4949-4952):let length_value = value .get(self.global_this, "size")? .unwrap_or_else(|| JSValue::js_number_from_int32(0)); let length = length_value.coerce_to_i32(self.global_this)?;
Both
.get(...)?and.coerce_to_i32(...)?propagate a user exception straight out ofBun.inspect.Iterator drive (
print_map_like4841-4845 / 4860-4864,print_set4988-4992 / 5007-5011):value.for_each( global_this, (&raw mut iter).cast::<c_void>(), MapIteratorCtx::<C, false, true>::for_each, )?;
JSValue::for_each→JSC__JSValue__forEach(bindings.cpp:4025) →JSC::forEachInIterable, which looks up the user-visibleSymbol.iterator, calls it, and drivesnext(). A throwing iterator surfaces asErrhere and the?aborts the whole inspect.Step-by-step repro
// size getter const m = new Map([[1, 1]]); Object.defineProperty(m, "size", { get() { throw new Error("boom"); } }); Bun.inspect(m); // still throws "boom" after this PR Bun.inspect([m, "after"]); // "after" is never printed // Symbol.iterator const m2 = new Map([[1, 1]]); m2[Symbol.iterator] = () => { throw new Error("boom"); }; Bun.inspect(m2); // still throws after this PR // next() throws const s = new Set([1]); s[Symbol.iterator] = () => ({ next() { throw new Error("boom"); } }); Bun.inspect(s); // still throws after this PR
Trace for the first case:
mis a realJSMap, soTag::getroutes toprint_map_like. Line 4803 callsvalue.get(self.global_this, "size"); the own accessor shadowsMap.prototype.size, its getter throws,getreturnsErr,?propagates out throughformat→Bun.inspectthrows. The existing regression test in inspect.test.js (setWithOverriddenSize) only covers a data-propertysizeoverride (value: Set), whichcoerce_to_i32handles without throwing — it does not guard the throwing-getter case.For the iterator case:
print_map_likeclassifies viavalue.js_type()(line 4811), so a real Map with an ownSymbol.iteratorstill routes here and reachesvalue.for_each(...)?at line 4845. The PR's own "AggregateError with errors whose iterator throws" test provesfor_eachhonours a user-installedSymbol.iteratorand returnsErrwhen it throws — that's exactly whyVirtualMachine.rs:4850-4856now wraps it. The four Map/Set call sites are the same shape but were left with?.Why this belongs in this PR
REVIEW.md, Correctness: "Fix the whole class in the same PR (same-class sites are ONE concern, not scope creep). Grep for every sibling site sharing the pattern." Both
print_map_likeandprint_setare edited by this diff (thelet length = length.max(0) as usize;insertions and the ctx-struct threading), the newget_swallowing_throwhelper is defined 240 lines up in the sameimplblock, and thefor_each().is_err() → clear_exception()fix pattern is demonstrated for AggregateError. These are the direct siblings of the Event and AggregateError sites the PR fixes.Note the two sub-issues are distinct: routing
sizethroughget_swallowing_throwalone would not fix a throwingSymbol.iterator(execution would proceed tofor_each(...)?and still abort), and vice-versa.Fix
- Replace the
sizereads withself.get_swallowing_throw(value, "size")and swallow the coerce failure (defaultlengthto 0), mirroring the Event reads. - Replace each
value.for_each(...)?;withif value.for_each(...).is_err() { global_this.clear_exception(); }, mirroringVirtualMachine.rs:4850-4856.
Severity
Nit / pre-existing. This is not a regression — the
?-propagation on these lines predates the PR — and requires a deliberately hostile own-property override on a real Map/Set to trigger. The PR title explicitly scopes the accessor-swallowing toprint_event/AggregateError, and merging as-is is a strict improvement. Flagging only because the author already has both fix patterns in hand and REVIEW.md's same-class rule applies to functions the diff touches; fine to defer alongside the formatter-wide byte-budget follow-up mentioned in the PR. - Replace the
A Map subclass that reports size <= 100 while holding more entries defeated the cap because length came from the user-visible size getter. The callback now keys on count alone (incremented for every real entry) and the '... N more items' marker is emitted after for_each from the final count, so the remaining-items number is correct regardless of what size reports. Iterators stay uncapped via the existing IS_ITERATOR const; the length field and its threading are removed.
|
Addressed the first review finding in 848d562: the cap now keys on The second finding (throwing |
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🟡
src/jsc/ConsoleObject.rs:2940-2950— The cap gates onthis.length > MAP_SET_ENTRY_CAP, butlengthcomes fromvalue.get(global, "size")— a normal property lookup that respects own-property overrides and subclass getters. A Map with 1M real entries andObject.defineProperty(m, 'size', {value: 1})(orclass M extends Map { get size(){return 1} }) reportslength = 1, so the guard never fires andfor_eachstill prints every entry. SinceMapIteratorCtxalready has anIS_ITERATORconst generic andSetIteratorCtxis only instantiated fromprint_set, the guard could be!IS_ITERATOR && this.count >= MAP_SET_ENTRY_CAP(Map) /this.count >= MAP_SET_ENTRY_CAP(Set) instead of overloadinglength— no need to trust user-visible.sizeat all.Extended reasoning...
What the bug is
The new truncation guard in both
MapIteratorCtx::for_eachandSetIteratorCtx::for_eachis:if this.length > MAP_SET_ENTRY_CAP && this.count >= MAP_SET_ENTRY_CAP {
this.lengthis populated fromvalue.get(self.global_this, "size")?.coerce_to_i32(...)at ConsoleObject.rs:4768-4771 (Map) and 4915-4918 (Set)..get()is an ordinary JS property lookup — it walks the prototype chain and respects own-property shadows and subclass accessor overrides. It does not read JSC's internalJSMap/JSSetbucket count. Meanwhile,value.for_each()iterates the internal storage regardless of what.sizereports.Step-by-step proof
const m = new Map(); for (let i = 0; i < 1_000_000; i++) m.set(i, i);Object.defineProperty(m, 'size', { value: 1 });— shadowsMap.prototype.sizewith an own data property.Bun.inspect(m)→print_mapreadslength = m.size = 1.length.max(0) as usize→1, stored on the ctx.for_eachvisits internal bucket 0…999999. On each call,this.length (1) > MAP_SET_ENTRY_CAP (100)is false, so the truncation branch is skipped.- All 1,000,000 entries print — ~17 MB of output, exactly the behavior this PR set out to fix.
The same holds for
class M extends Map { get size() { return 1; } }, and symmetrically forSet. The existing regression test in inspect.test.js only exercises overriddensizeon an empty Map/Set (wherelength == 0short-circuits toMap {}), so it doesn't cover this.Why the
length > CAPconjunct existsIt's doing double duty as the sentinel for the Map-iterator path:
print_map_iterator_likepasseslength: 0to keep MapIterator/SetIterator output uncapped (their size isn't known upfront). So dropping the conjunct outright would start truncating iterators.Why this is a nit, not a blocker
- The reliance on user-visible
.sizeis pre-existing — it's already used for theMap(N)header and thelength == 0early-return. This PR extends that reliance, it doesn't introduce it. - Triggering the bypass requires deliberately tampering with
.size, which is self-inflicted;console.logis not a security boundary. - Even in the tampered case, output is no worse than the pre-PR status quo (unbounded). This PR is a strict improvement for every non-adversarial input.
Suggested fix
Don't overload
lengthas the iterator sentinel — use the const generic that already exists:MapIteratorCtxalready carriesconst IS_ITERATOR: bool(print_mapusesfalse,print_map_iterator_likeusestrue). Change the guard toif !IS_ITERATOR && this.count >= MAP_SET_ENTRY_CAP, and computeremainingin the marker asthis.length.saturating_sub(MAP_SET_ENTRY_CAP)(which just affects the printed count, not whether truncation happens).SetIteratorCtxis only ever constructed fromprint_set(grep confirms — 4948/4967 are the only instantiation sites), so its guard can simply beif this.count >= MAP_SET_ENTRY_CAPwith nolengthcheck at all.
This makes the cap depend solely on the actual iteration count, which
for_eachcontrols and userland cannot spoof.
There was a problem hiding this comment.
No issues found — the cap logic, is_first → count refactor, and marker placement all check out, and the test coverage is thorough. Deferring to a human because this is a user-visible default-behavior change (anyone currently relying on Bun.inspect(bigMap) dumping every entry now gets truncated output), and the hardcoded 100 is a product decision worth a maintainer nod even if it matches Node and the array printer.
What was reviewed:
count > MAP_SET_ENTRY_CAPboundary vscount >= MAP_SET_ENTRY_CAPin the callback — off-by-one is correct (exactly 100 → no marker).IS_ITERATORgating keepsprint_map_iterator_likeuncapped as intended.- Single-line separator ordering in
print_more_items_markervs the trailingif count > 0 { " " }— no double-space or missing comma. - The array printer's marker (line ~4514) doesn't singularize; this one does — minor inconsistency, not a bug.
Extended reasoning...
Overview
Two files: src/jsc/ConsoleObject.rs adds a MAP_SET_ENTRY_CAP = 100 constant, an early-return in MapIteratorCtx::for_each / SetIteratorCtx::for_each once count reaches the cap (still incrementing count so the caller can compute the remainder), a new print_more_items_marker helper, and post-loop marker emission at the four Map/Set call sites. SetIteratorCtx.is_first: bool becomes count: usize with the one reader updated. test/js/bun/util/inspect.test.js gains a 7-test describe block covering truncation, the exact-100 boundary, singularization, compact-mode separators, an output-size bound, and a lying size getter.
Security risks
None. Pure output-formatting logic; no new JS re-entrancy, no allocation changes, no external input parsing. If anything this reduces a mild resource-exhaustion vector (17 MB of output from one console.log).
Level of scrutiny
Medium. The mechanics are simple and follow the existing array-truncation pattern in the same file, but this changes the default output of console.log / Bun.inspect for any Map/Set over 100 entries — a user-visible behavior change. It matches Node's maxArrayLength default and Bun's own array printer, so it's well-motivated, but a maintainer should confirm the hardcoded 100 (vs waiting for #31809's configurable maxArrayLength) is the shape they want.
Other factors
- Prior review round already caught and fixed the
size-getter bypass; the cap now keys purely on the iteratedcount, verified by a dedicated test. - The
pretty_format.rssibling exclusion is documented in the PR description with a rationale (diff formatter shouldn't hide the mismatched entry). - I noted the array printer's
... N more itemsat line ~4514 always pluralizes while this one singularizes at 1 — a cosmetic inconsistency, not worth blocking on. - CI build #81709 was still marked "building" in the timeline; worth confirming green before merge.
There was a problem hiding this comment.
LGTM — the size-getter bypass from the first pass is fixed (cap keys on the iterated count, marker emitted post-loop) and the pretty_format.rs sibling is now called out as an intentional exclusion in the description.
What was reviewed:
- Boundary math: callback gates on
count >= 100, caller oncount > 100, so exactly-100 emits no marker and 101 emits1 more item— matches the tests. SetIteratorCtxhas noIS_ITERATORconst, but that's fine:Tag::SetIteratorroutes throughprint_map_iterator_like(which usesMapIteratorCtx<_, true, _>), so real Set iterators stay uncapped andSetIteratorCtxis only reached fromprint_set.- Marker styling (
<r><d>...<r>) and wording match the existing array-printer marker at ~L4514.
Extended reasoning...
Overview
Two files: src/jsc/ConsoleObject.rs adds MAP_SET_ENTRY_CAP = 100 with an early-return in the MapIteratorCtx/SetIteratorCtx for_each callbacks and a new print_more_items_marker helper called from the four Map/Set call sites (single-line × multi-line); SetIteratorCtx.is_first: bool becomes count: usize. test/js/bun/util/inspect.test.js adds a 7-test describe block covering truncation, singularization, exact-cap, single-line separators, output-size bound, and a lying-size-getter subclass.
Security risks
None. This is output-formatting only — no parsing of untrusted input, no allocation sized from external data (saturating_sub guards the marker arithmetic), no new FFI surface.
Level of scrutiny
Low-to-medium. It's a self-contained formatter change that strictly reduces output volume, mirrors what the array printer already does at ~L4506–4520, and matches Node's default behavior. The only subtle bits are the off-by-one at the cap boundary and the separator sequencing in single-line vs multi-line mode, both of which I traced against the callback bodies and both of which are pinned by exact-string test assertions (toEndWith("98, 99, ... 5 more items }"), toContain("... 1 more item,"), exactly-100 negative case).
Other factors
- My prior review's two concerns are resolved: the cap now keys on the real iterated
count(not the user-visiblesizegetter), with a dedicated test for a subclass that lies aboutsize; andpretty_format.rsis explicitly excluded in the PR description with a stated rationale. - I confirmed
SetIteratorCtxdoesn't need anIS_ITERATORgate becauseTag::SetIteratordispatches toprint_map_iterator_like(which usesMapIteratorCtxwithIS_ITERATOR=true), soSetIteratorCtxis only ever reached fromprint_set. - The comment-cop bot flags are all resolved and were on earlier-commit doc comments that no longer exist in the diff.
- Tests were verified locally per the description (80 pass; 6/7 new tests fail under
USE_SYSTEM_BUN=1).
|
#42264 includes this 100-entry limit for Map and Set, and also stops the walk at the limit. Here the callback skips the print past the limit but |
Carries over the two cases from #35826 that this branch did not cover yet.
|
Closing in favor of #42264. It has the same 100-entry limit for Map and Set, with the same Five of the seven test cases here already had an equivalent in #42264. The other two (a 10,000-entry Map, and a Map subclass whose Please reopen this PR if #42264 does not land. |
Carries over the two cases from #35826 that this branch did not cover yet.
|
#42264 no longer carries the 100-entry limit for Map and Set, so this PR is not covered by it any more. A review of #42264 found that the limit is not needed to stop the hang there, and that it is a change of default output with no opt-out: The limit fits better here as its own change, on top of #31809 so that Reopening so that the question of the default stays visible. |
|
Closing as part of a cleanup of stale pull requests. This PR has had no new commits since 2026-07-26, it conflicts with main, and its last CI run failed. This is not a judgment on the fix itself. If the problem still reproduces on a current build, reopen this PR after a rebase or open a new one against main. |
2 participants
What
The native console formatter rendered every Map/Set entry with no truncation, so a million-entry Map produced ~17 MB of output from a single
console.log/Bun.inspectcall. Node caps Map/Set atmaxArrayLength(default 100) and emits... N more items, matching what Bun's array printer already does.Repro
Fix
src/jsc/ConsoleObject.rs: add aMAP_SET_ENTRY_CAP = 100check at the top of theMapIteratorCtx/SetIteratorCtxfor_eachcallbacks. Past the cap, entries are skipped butcountkeeps incrementing; afterfor_eachreturns the caller emits a single... N more item(s)marker (singularized at 1) from the finalcount. The cap keys on the real iterated count, so a subclass that lies aboutsizestill truncates.print_map_iterator_likestays uncapped via the existingIS_ITERATORconst (iterator size is unknown up front).SetIteratorCtx.is_first: boolbecomescount: usizeso the same check works there; the one caller that readis_firstnow readscount > 0.Verification
New
describe("Map/Set entry count is capped")block intest/js/bun/util/inspect.test.js(7 tests): 150-entry Map/Set truncate at 100 with the right marker, 101-entry singularizes to1 more item, exactly-100 is unchanged, single-line mode has the right separators, a 10k-entry Map's output stays under 4 KB, and a 500-entry Map subclass withget size() { return 1 }still truncates at 100.bun bd test test/js/bun/util/inspect.test.js: 80 pass.USE_SYSTEM_BUN=1: 6 of the 7 new tests fail (the one that passes is the exactly-100 control). Existingtest/js/web/console/andtest/js/bun/console/suites pass unchanged.Related
The Event/AggregateError throwing-accessor guards originally in this branch are in #35816 (opened first, also covers
pretty_format.rs). #35288 adds the orthogonal depth cap to Map/Set/Array. #31809 wiresmaxArrayLengththrough to the array printer; once that landsMAP_SET_ENTRY_CAPcan become the same configurable field.src/runtime/test_runner/pretty_format.rs(theexpect().toEqual()diff formatter) is intentionally left uncapped: its array printer does not truncate either, and truncating a diff could hide the mismatched entry. If that formatter gains an entry/length cap it should cover arrays + Map/Set together.[review] gate passed · iteration 1 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 1
evidence per changed file