Conversation
…g length The table printer read every array-like through the iterator protocol. The Array iterator trusts the length the object reports and yields undefined for every index that has no element. A Proxy that reports length 2^32 - 1 for a 2 element array, or a sparse array with a large length, produced one row per reported index and never returned. Arrays (through a Proxy too) and objects that borrow the intrinsic Array iterator, such as arguments, now take the own enumerable property walk that plain objects already use, like Node's console.table. Sets, Maps, generators and other iterables keep the iterator path.
|
Status: ready for review. Reproduced on stock bun 1.4.1 (linux x64):
With this branch both return in under 5 ms of table work. 8 of the 9 new tests in CI (build 109075): 181 of 182 jobs pass. The one red job is |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — focused fix that routes arrays/array-likes through the own-property walk so a lied-about length or sparse holes can't inflate console.table output.
What was reviewed:
- Exception-scope discipline in
Bun__JSValue__isNonArrayIterable:RETURN_IF_EXCEPTIONfollows bothisArrayandgetMethod; the tail (dynamicDowncast, pointer compare) can't throw, so noRELEASE_AND_RETURNneeded. - Cross-realm correctness: comparing against
function->globalObject()'s ownarrayProtoValuesFunctionintrinsic, not the caller's realm — anode:vmarray's iterator still matches. - Iterable classification: typed arrays, Map/Set, String, Headers keep the iterator path (their
@@ iteratoris not%Array.prototype.values%);argumentsand Proxied arrays fall to the property walk as intended. - Tests follow harness conventions (concurrent pipe drain,
await using, combined-object assertion) and cover the variant matrix including frozen-sparse and Array-subclass-with-custom-iterator.
Extended reasoning...
Overview
This PR fixes a hang/OOM in console.table and Bun.inspect.table when the input is a Proxy that reports an inflated length, or a very sparse array. The root cause was that TablePrinter used the iterator protocol for anything with Symbol.iterator, and the intrinsic Array iterator trusts the reported length and yields undefined for every hole. The fix adds a new predicate is_non_array_iterable (Rust wrapper + C++ binding) that returns false for arrays (via spec IsArray, so Proxy-aware) and for anything whose Symbol.iterator resolves to the intrinsic %Array.prototype.values% (covers arguments). Those objects now take the own-enumerable-property walk that plain objects already used — the same model as Node's Object.keys-based console.table. Four files touched: one-line switch in ConsoleObject.rs, a 6-line Rust wrapper in JSValue.rs, a ~30-line C++ binding in bindings.cpp, and 9 new tests appended to the existing console-table.test.ts.
Security risks
None. console.table is a formatting/diagnostic path with no auth, crypto, filesystem, or network implications. If anything, this reduces a resource-exhaustion surface: previously a Proxy reporting length 2^32-1 would hang the process; now the row set is bounded by the object's own keys. The new C++ binding calls getMethod, which can invoke a Proxy get trap for Symbol.iterator, but that was already the case in the pre-existing hasIteratorMethod path — no new user-code entry point is introduced.
Level of scrutiny
Moderate. The change adds a new FFI binding with JSC exception-scope handling, which REVIEW.md flags as a high-attention area. I traced the throw scope: DECLARE_THROW_SCOPE at the top, RETURN_IF_EXCEPTION after JSC::isArray (which can throw on a revoked Proxy) and after getMethod (which can throw if Symbol.iterator is present but not callable, or if a Proxy trap throws). The remaining code — dynamicDowncast<JSFunction> and a pointer comparison against arrayProtoValuesFunctionConcurrently() — cannot throw, so the plain return is correct without RELEASE_AND_RETURN. The [[ZIG_EXPORT(check_slow)]] attribute is applied per the src/CLAUDE.md convention for bindings that may throw. The cross-realm handling is correct: the intrinsic is looked up on the function's own global, not the caller's, so an array from node:vm still matches.
Other factors
Test coverage is thorough for the variant matrix REVIEW.md asks for: Proxy inflating length, Proxy get trap rewriting a cell (confirms [[Get]] still honored), sparse arrays, hole-only arrays, frozen arrays (elements moved to sparse map), the properties filter over sparse input, arrays with named own properties (bare and via Proxy), an Array subclass with a custom iterator (confirms IsArray wins over the iterator check), and one end-to-end console.table spawn. Tests are appended to the existing module test file, use bunExe/bunEnv, drain subprocess pipes concurrently with Promise.all, and assert a combined {stdout, stderr, exitCode} object. The user-visible behavior changes (holes skipped, named array properties become rows, custom iterator on Array subclass ignored) all move toward Node's behavior and are explicitly enumerated in the PR description. No CODEOWNERS entries cover the changed paths. The bug hunt ran to a dry streak with no findings.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (4)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. WalkthroughChangesThe console table implementation now excludes arrays from iterator-based row traversal. It uses own enumerable properties for arrays and other non-iterable inputs. The change adds a native iterable classifier and tests for array and proxy edge cases. Console table traversal
Suggested reviewers: Merge Risk: ⚪ Minimal · up to The PR makes a localized console.table row-enumeration change, with the supplied test suites passing; no actionable merge-blocking risk remains beyond normal review. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Full details: Description checkExplanation The description explains the problem, implementation, behavior changes, alternatives, and verification results. It does not use the exact template headings, but it provides the required change summary and verification details. Comment |
This branch has not been deployed
2 participants
Problem
console.table(px)andBun.inspect.table(px)never return whenpxis a Proxy whosegettrap reportslength4294967295 for a 2 element array. Withlength16M,Bun.inspect.tablereturns a 486,539,380 char string.Bun.inspect.table(new Array(50_000_000))returns 1.25 GB in 16 s, anda = []; a[1e9] = 1; console.table(a)hangs. Node prints the real rows, or an empty table, at once.TablePrinter::print_table(src/jsc/ConsoleObject.rs:904) reads every object that hasSymbol.iteratorthroughJSC::forEachInIterable. The Array iterator re-readslengthon each step and yieldsundefinedfor every index that has no element. Nothing bounds the row count by the elements that exist.Fix
IsArray) and any object whoseSymbol.iteratoris the intrinsicArray.prototype.values(arguments, a plain array-like) now take the own-property walk that plain objects already use. That is Node's model:console.tableusesObject.keysfor anything that is not a Map or a Set. The newBun__JSValue__isNonArrayIterablebinding makes the decision. Sets, Maps, generators, typed arrays and other iterables keep the iterator path.lengthis never read. Cell values still go through[[Get]], so a Proxygettrap is honored, as in Node.arr.foo), both as in Node. Dense arrays, the existing snapshots and the generator test render unchanged.test/js/bun/console/console-table.test.ts(9 new tests, 8 fail on stock bun 1.4.1). Alsobun-inspect-table.test.tsandtest/js/bun/util/inspect.test.js.Background
TablePrintercollects all rows in one width pass, then renders them. Rows come from one of two walks: the iterator protocol, or own enumerable properties throughJSPropertyIterator(getOwnPropertyNameswithDontEnumexcluded, the same key set asObject.keys). This PR only changes which objects take which walk.arrayProtoValuesFunction. ItsnextdoesToLength(Get(O, "length"))on every step. For a non-arrayOthat value is whatever the object says. The check compares the method against its own realm's intrinsic, so cross-realm objects (node:vm) are covered.JSC::isArrayis the specIsArray: true forArray, anArraysubclass, and a Proxy whose target is one.Notes
Behavior changes, all match Node:
undefinedrow.console.table([1, , 3])prints rows 0 and 2.Object.assign([1, 2], { foo: 3 })prints rows 0, 1, foo.Symbol.iteratoris walked by its elements, not by that iterator.Unchanged: Set, Map, Map/Set iterators, generators, typed arrays,
Stringobjects,Headers,arguments(same rows, now through the property walk), a Proxy around a Set (TypeError, as before), a non-callableSymbol.iterator(TypeError, as before). A revoked Proxy still throws a TypeError, now fromIsArray.Alternatives considered:
undefinedrows for the Proxy case.console.log. That bypassesgettraps, which Node honors, and does nothing for sparse arrays.Bun__JSArray__nextPresentIndex(the formatter's sparse array helper). Its output is identical to the property walk, but it rescans the whole sparse map per call, so a frozen array (Object.freezemoves every element into the sparse map) of 10k rows took 17 s in a debug build against 0.8 s for the property walk.Cost of the property walk for real arrays: one atomized index string per row. In a debug build a dense and a frozen 10k row table both take about 0.8 s. The 50M hole case spends its time allocating the array, not in the table.
Measurements on stock bun 1.4.1 (release, linux x64): the 2^32-1 Proxy was killed at 10 s;
new Array(50_000_000)took 16.4 s and returned 1,250,000,100 chars. With the fix both return in under 5 ms of table work.Self-reviewed: 8 concerns raised. Addressed: the first version kept a separate present-index walk for arrays (quadratic on sparse maps, and a bare array hid
arr.foowhile a Proxy around it did not) and usedisJSArray(missesArraysubclasses and Proxies). Both are gone.[human-review] gate passed · iteration 0 · 4 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file