bun:jsc: filter internal cells out of getProtectedObjects() - #33183
bun:jsc: filter internal cells out of getProtectedObjects()#33183robobun wants to merge 2 commits into
Conversation
|
Warning Review limit reached
Next review available in: 3 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 (1)
Walkthrough
ChangesgetProtectedObjects Object Filtering
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 11:17 PM PT - Jul 10th, 2026
❌ @robobun, your commit d2774cd has 2 failures in
🧪 To try this PR locally: bunx bun-pr 33183That installs a local version of the PR into your bun-33183 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — one-line guard prevents internal JSC cells from leaking into JS as invalid values, with a subprocess test that reproduces the crash.
Extended reasoning...
Overview
This PR adds a single cell->isObject() filter inside functionGetProtectedObjects in src/jsc/modules/BunJSCModule.h, and a subprocess-based regression test in test/js/bun/jsc/bun-jsc.test.ts. Before the change, heap.forEachProtectedCell yielded every protected cell — including engine-internal ones like UnlinkedProgramCodeBlock, private symbols, and structures — and stuffed them raw into a JS array. Those cells are not valid JS values, so the first generic operation on one (e.g. Object.prototype.toString.call) type-confused inside JSC and aborted the process. The fix simply drops non-object cells, which matches the API name and is strictly narrowing.
Security risks
None introduced; if anything this is a hardening. Exposing private symbols to user JS would leak the engine's private property names, and handing out non-JSValue cells is a type-confusion footgun. Filtering to JSObject removes both. There is no new user-controlled input, no allocation change, and no new surface.
Level of scrutiny
Low. bun:jsc is a debugging/introspection module, not a hot production path. The native change is a one-line predicate using a standard JSC accessor (JSCell::isObject()), the semantics are unambiguous, and the change can only remove elements from the result — it cannot introduce wrong ones. The existing getProtectedObjects().length > 0 test still holds because the global object is always protected.
Other factors
The new test follows repo conventions well: it spawns a child via bunExe()/bunEnv, drains stdout/stderr/exited concurrently, asserts a combined { stdout, exitCode } object (avoiding brittle empty-stderr checks), and verifies the list remains non-empty. The PR description confirms the test fails under USE_SYSTEM_BUN=1 (child aborts with 134) and passes on the debug build. The C++ comment is concise and within the 3-line limit. No CODEOWNERS cover this path and there are no outstanding reviewer comments.
There was a problem hiding this comment.
LGTM — one-line isObject() guard on a debugging API, well-tested; the inline nit is diagnostics-only.
Extended reasoning...
Overview
This PR adds a single if (cell->isObject()) guard inside functionGetProtectedObjects in src/jsc/modules/BunJSCModule.h, so bun:jsc's getProtectedObjects() no longer returns engine-internal cells (unlinked code blocks, private symbols, structures) that are not valid JavaScript values. It adds one subprocess test in test/js/bun/jsc/bun-jsc.test.ts that exercises Object(value) and Object.prototype.toString.call(value) on every returned element — the exact operations that abort on the unfixed build.
Security risks
None introduced; if anything this reduces exposure. Before the fix, a caller could receive private-symbol cells from the protected set (which the PR description correctly notes would hand user code engine-private property names). Filtering to JSObject closes that off. The change is strictly a narrowing filter on a read-only introspection API and cannot expose anything new.
Level of scrutiny
Low. bun:jsc is an explicitly-imported debugging/introspection module, not a hot path or a Node/Web compat surface. The change is one predicate inside a heap-iteration lambda, using the canonical JSCell::isObject() check. It cannot break any caller that wasn't already crashing — the only values removed from the array are ones that abort the process on first touch. The pre-existing getProtectedObjects().length > 0 test still passes because the global object remains in the set, and the new test asserts that too (list.length > 0).
Other factors
The PR description traces the crash to JSCell::toObject's fall-through jsCast<Symbol> on a non-string/non-BigInt/non-symbol cell, which is the real mechanism. The author explicitly considered and rejected the alternative of wrapping non-object cells, giving a security rationale (private symbols). The one finding from the bug hunt is a diagnostics nit (assert stderr alongside stdout/exitCode so regression failures are self-diagnosing) — it does not affect test correctness and is not a blocker. No prior human reviews or outstanding comments; CI is building.
There was a problem hiding this comment.
LGTM — the stderr assertion nit is addressed, and the one-line isObject() filter is the right fix for the crash.
Extended reasoning...
Overview
This PR changes functionGetProtectedObjects in src/jsc/modules/BunJSCModule.h to filter the heap's protected-cell set down to JSObject cells before returning them to JS, and adds a subprocess regression test in test/js/bun/jsc/bun-jsc.test.ts. The native change is a single if (cell->isObject()) guard inside the existing forEachProtectedCell lambda; everything else in the function is untouched.
Security risks
None introduced. The change strictly narrows what is exposed to user JS. The PR description correctly notes that the previous behavior could hand engine-internal cells (unlinked code blocks, private symbols, structures) to userland, which is both a crash vector and — for private symbols — an information-disclosure concern. Filtering to JSObject removes both. JSCell::isObject() is the standard JSC predicate for this and does not run user code, so no new exception-handling is needed.
Level of scrutiny
Low-to-moderate. bun:jsc is a debugging/introspection module, not a hot path, and getProtectedObjects is a diagnostic helper. The fix is defensive (drops elements rather than transforming them), so the worst-case failure mode is an under-populated result array — not a new crash. The existing getProtectedObjects().length > 0 test continues to pass because the global object is always protected and is a JSObject, and the new test verifies every returned element survives Object() and Object.prototype.toString.call().
Other factors
I left one inline nit on the previous revision (include stderr in the combined assertion so a regression's abort message shows in the diff); that was addressed in 909b76c and the thread is resolved. The new test follows the exact subprocess pattern of the three neighboring tests in the same file (bunExe()/bunEnv, piped stdout+stderr drained concurrently, combined-object assertion). The bug-hunting pass on the latest revision found nothing. No CODEOWNERS entry covers this path.
|
CI status: the change and its test are green on every lane that ran them; the failing jobs are unrelated to this diff. Buildkite build 71800 (head d2774cd):
GitHub Actions "TypeScript types": failing on main independently of this PR ( |
909b76c to
b72bbd4
Compare
There was a problem hiding this comment.
LGTM — one-line guard that stops getProtectedObjects() from returning non-JSObject cells and crashing on first use; my earlier stderr-assertion nit was addressed in 909b76c. The remaining inline note is a docs-only nit and doesn't block.
Extended reasoning...
Overview
The PR adds a single if (cell->isObject()) guard inside the forEachProtectedCell lambda in functionGetProtectedObjects (src/jsc/modules/BunJSCModule.h), plus a subprocess regression test in test/js/bun/jsc/bun-jsc.test.ts. Before the change, internal JSC cells (UnlinkedProgramCodeBlock, private symbols, structures) were placed into the returned JS array; touching one via Object.prototype.toString.call fell through JSCell::toObject's string/BigInt/Symbol switch into a bad jsCast<Symbol*>, aborting the process.
Security risks
None introduced. The change strictly narrows what is exposed to JS. If anything it is a mild hardening: private symbols in the protected set are no longer handed to user code.
Level of scrutiny
Low-to-moderate. bun:jsc is a diagnostic/debug module, not a hot production path. The fix is a well-understood JSC idiom (JSCell::isObject() gates on the JSType range for JSObject), and the semantics match the API name. The pre-existing getProtectedObjects().length > 0 test still passes because the global object is always protected, and the new test independently asserts list.length > 0.
Other factors
- My prior inline comment (assert
{stdout, stderr, exitCode}) was addressed in 909b76c and the thread is resolved. - CI:
test/js/bun/jsc/bun-jsc.test.tspassed on every lane in builds 67553 and 67578; the failing jobs are known flakes unrelated to this diff. - The one finding from this run is a stale JSDoc warning in
packages/bun-types/jsc.d.ts— docs-only, no runtime effect, fine to address as a follow-up. - I considered whether filtering also drops legitimately-protected strings/BigInts/public Symbols that are valid JS values: it does, but the function is named
getProtectedObjects, and the PR argues (correctly) that private symbols must not leak, so filtering to objects is the conservative name-matching choice.
There was a problem hiding this comment.
Looks good — the cell->isObject() filter is the right fix and both earlier nits are addressed. Not auto-approving only because packages/bun-types/ has a CODEOWNER; the .d.ts change is a trivial JSDoc reword + any[] → object[] tightening.
What was reviewed:
forEachProtectedCelllambda now skips non-object cells;MarkedArgumentBufferstill roots survivors acrossconstructArray.- Confirmed the existing
getProtectedObjects().length > 0test still holds (global object is always protected), and the new subprocess test exercisesObject()/toString.callon every element. object[]return type matches the post-filter contract; no callers in-tree depend on the oldany[].
Extended reasoning...
Overview
Three files: a one-condition filter in functionGetProtectedObjects (src/jsc/modules/BunJSCModule.h) so only JSCells where isObject() is true are appended to the returned array; a subprocess regression test in test/js/bun/jsc/bun-jsc.test.ts; and a JSDoc/return-type update in packages/bun-types/jsc.d.ts. The last file was added in d2774cd in response to my previous review comment.
Security risks
None introduced. The change strictly narrows what is exposed to JS — previously the array could contain engine-internal cells (unlinked code blocks, private symbols, structures) that type-confused on first touch and, in the private-symbol case, would have leaked engine-private property names. Dropping them is a strict reduction in attack surface. bun:jsc is a debugging module, not on any untrusted-input path.
Level of scrutiny
Low-to-medium. The C++ change is a single well-understood predicate (JSCell::isObject()) inside an existing iteration; no new allocation, no new GC interaction (the MarkedArgumentBuffer was already there). The API is documented as a Bun-internal debugging tool. The pre-existing getProtectedObjects().length > 0 test guards against over-filtering.
Other factors
Both of my earlier inline nits (assert stderr in the combined object; update the stale JSDoc warning) were addressed in 909b76c and d2774cd respectively. CI on 909b76c passed bun-jsc.test.ts on every lane per the author's build summary; the d2774cd commit is types-only. The one thing keeping me from auto-approving is that packages/bun-types/ is CODEOWNER-gated, and the return-type tightening (any[] → object[]) — while accurate and low-risk for a debug-only API — is a public type surface change that owner should sign off on.
|
The "TypeScript types" check failure is not caused by this PR's |
…not fixed delays (#33983) "should handle partial writes and buffering" in `node-http-connect.node.mts` wrote `"Client data"` from the client at a fixed `t=35ms` while the server wrote `"Test data"`+`end()` at `t=40ms` relative to its own `'connect'` event. Under CPU load on Windows the client's 35ms timer fires late enough that the server's FIN reaches the client first, the awaited promise resolves on the client's `'end'`, and the assertion on `bufferReceived` runs before the server's data handler has seen the bytes. Seen red on Windows 2019 x64-baseline in two unrelated PR builds: [71915](https://buildkite.com/bun/bun/builds/71915) (#33974, epoll-only change) and [71800](https://buildkite.com/bun/bun/builds/71800) (#33183, a types-only change), both with: ``` AssertionError: false == true at toContain (...\node-http-connect.node.mts:18:14) at ...\node-http-connect.node.mts:351:28 (fail) HTTP server CONNECT > should handle partial writes and buffering ``` Reproduced locally on Windows with 16 background spinners: 8/50 runs fail. After this change, 0/100 under the same load. The test no longer uses `setTimeout`. A `writeChunked` helper writes each chunk, awaits its write callback, then yields one `setImmediate` so the peer still sees a fragmented stream (verified: 1 `'data'` event with a bare write-callback chain vs 3 with this helper, both Node and Bun). The client sends `"Client data"` once it has seen the full `"Connection established"` response, the server ends only once it has received it (from the `"Test data"` write callback so the FIN cannot outrun it), and both sockets have `'error'` wired to reject the awaited promise. Passes under Node and Bun on Linux and Windows. The test landed in #22756 and has had this race since then. <!-- robobun:evidence:begin --> --- **no test proof** · iteration 1 · docs-only change; test-proof not applicable <!-- robobun:evidence:end -->
|
Still reproduces on current main (9a543cc) and on the 1.4.0 canary: This branch needs a rebase: #35849 changed |
1 participant
What does this PR do?
require("bun:jsc").getProtectedObjects()copies every cell in the heap's protected set into a JS array. That set is not limited to JavaScript objects: at startup it already contains the entry point'sUnlinkedProgramCodeBlock, and depending on what the process did it can also hold private symbols, structures, and other engine-internal cells. Those cells are not valid JavaScript values, so the array ends up with elements that reporttypeof x === "object"but are not objects, and the first generic operation on one of them type-confuses inside JSC.Repro on Bun 1.4.0 (release):
On a debug build the same line asserts:
JSCell::toObjectassumes a non-object cell is a string, heap BigInt, or symbol, so for anUnlinkedProgramCodeBlockit falls through tojsCast<Symbol*>on a cell that is not aSymbol.bun:jsc's owndescribe()shows the two cells protected at startup before this change:The fix is in
functionGetProtectedObjects(src/jsc/modules/BunJSCModule.h): onlyJSObjectcells are appended to the result, which is what the API name promises. Non-object cells are dropped rather than wrapped; exposing them is never safe (private symbols in particular would hand user code the engine's private property names).How did you verify your code works?
Added
getProtectedObjects returns only objectstotest/js/bun/jsc/bun-jsc.test.ts. It spawns a child process that callsObject(value)andObject.prototype.toString.call(value)on every element returned bygetProtectedObjects().USE_SYSTEM_BUN=1 bun test test/js/bun/jsc/bun-jsc.test.ts -t "returns only objects"fails: the child aborts with exit code 134.bun bd test test/js/bun/jsc/bun-jsc.test.tspasses the new test and the existinggetProtectedObjectstest (the list is still non-empty, since the global object is protected).The pre-existing
profile can be called multiple timestest in the same file exceeds its 5s timeout under a debug ASAN build on the (slow) machine used here; it is unrelated to this change and passes with a release build.no test proof · iteration 4 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/bun/jsc/bun-jsc.test.ts