CachedTypes: reject out-of-range offsets instead of reading past the buffer - #368
CachedTypes: reject out-of-range offsets instead of reading past the buffer#368robobun wants to merge 1 commit into
Conversation
…buffer Bun's node:vm exposes decodeCodeBlock() to user-provided bytes via the cachedData option. The decoder derives pointers from self-relative offsets stored in the payload and bounds-checks them only via debug ASSERTs, so a truncated or corrupted buffer reads wild heap memory in release (SIGSEGV) and trips Decoder::offsetOf in debug. Under USE(BUN_JSC_ADDITIONS): - Decoder gains a latched m_failed flag. - offsetOf() and ptrForOffsetFromBase() bounds-check against the span and set m_failed instead of asserting. - CachedPtr::decode() bails out once the decoder has failed instead of dereferencing an out-of-range buffer(). - GenericCacheEntry::decode() returns false for an unknown/eval tag rather than RELEASE_ASSERT_NOT_REACHED. - decodeCodeBlockImpl / decodeSourceCodeKey / isCachedBytecodeStillValid reject spans shorter than a CacheEntry before the bit_cast, and check decoder->failed() before trusting the result. This is defense in depth behind the envelope Bun wraps around node:vm cachedData; it does not attempt to harden every decode path against arbitrary corruption.
|
Warning Review limit reached
Next review available in: 6 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: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I didn't find any bugs, but this is security-sensitive hardening of the bytecode decoder against untrusted node:vm cachedData input, and the PR explicitly scopes which paths are hardened vs. left to the Bun-side hash envelope — a human should sign off on that boundary.
Checked: the new bounds checks in offsetOf/ptrForOffsetFromBase mirror the existing ASSERT conditions and return in-span sentinel values; CachedPtr::decode bails before dereferencing get() when failed() latches; the three entry points reject short spans before the bit_cast and re-check failed() after decode. All changes are gated behind USE(BUN_JSC_ADDITIONS).
Extended reasoning...
Overview
The PR adds defensive bounds-checking to JSC's CachedTypes bytecode decoder under USE(BUN_JSC_ADDITIONS). It introduces a latched m_failed flag on Decoder, converts debug-only ASSERTs in offsetOf() and ptrForOffsetFromBase() into runtime checks that set the flag and return safe sentinel values, adds a bail-out in CachedPtr::decode(), replaces two RELEASE_ASSERT_NOT_REACHED() sites in GenericCacheEntry::decode() with return false, and adds minimum-size checks plus post-decode failed() checks to the three top-level entry points (decodeCodeBlockImpl, decodeSourceCodeKey, isCachedBytecodeStillValid).
Security risks
This is explicitly security-relevant: it hardens a code path that Bun exposes to user-provided bytes via node:vm's cachedData option, where the current release build reads wild heap memory on a corrupted/truncated buffer. The PR is a partial hardening — the description notes that other VariableLengthObject::buffer() callers still rely on Bun's hash-verified envelope. Whether the chosen boundary (harden CachedPtr::decode and the entry points, leave CachedArray and direct buffer() reads alone) is the right defense-in-depth line is a design judgment that should be confirmed by a maintainer familiar with the Bun-side envelope in oven-sh/bun#32839.
Level of scrutiny
High. This touches JavaScriptCore's bytecode cache decoder — memory-safety code on a path reachable from untrusted input. Even though every change is behind #if USE(BUN_JSC_ADDITIONS) and the individual edits are small and mechanical (mirror the existing assert condition, latch a flag, return early), the correctness of the sentinel returns (e.g. ptrForOffsetFromBase returning span().data() on failure, offsetOf returning 0) depends on every caller either checking failed() before using the result or being unreachable with untrusted data. That's the kind of invariant a human reviewer should confirm.
Other factors
No prior human reviews on the PR (only a rate-limited CodeRabbit stub). The bug-hunting system found no issues. The changes look internally consistent — minimumCacheEntrySize uses std::max over the two concrete CacheEntry<...> sizes, the failed() checks are placed after each decode call, and the non-Bun build is unaffected. But given the security surface and the explicit "does not attempt to harden every decode path" caveat, deferring to a human is the right call.
1 participant
Bun's
node:vmexposesdecodeCodeBlock()to user-provided bytes via thecachedDataoption. The decoder derives pointers from self-relative offsets stored in the payload and bounds-checks them only via debugASSERTs, so a truncated or corrupted buffer reads wild heap memory in release (SIGSEGVat a wild address) and tripsDecoder::offsetOfin debug:Under
USE(BUN_JSC_ADDITIONS):Decodergains a latchedm_failedflag.offsetOf()andptrForOffsetFromBase()bounds-check against the span and setm_failedinstead of asserting.CachedPtr::decode()bails out once the decoder has failed instead of dereferencing an out-of-rangebuffer().GenericCacheEntry::decode()returnsfalsefor an unknown or eval tag rather thanRELEASE_ASSERT_NOT_REACHED.decodeCodeBlockImpl/decodeSourceCodeKey/isCachedBytecodeStillValidreject spans shorter than aCacheEntrybefore thebit_cast, and checkdecoder->failed()before trusting the result.This is defense in depth behind the envelope Bun wraps around
node:vmcachedData(oven-sh/bun#32839). It does not attempt to harden every decode path against arbitrary corruption; the remainingVariableLengthObject::buffer()callers that read payload bytes directly are still only reached with a hash-verified payload on the Bun side. It also tightens the short-buffer behavior for the other callers of these entry points (bun build --bytecodecache files).