Bytecode cache: return null from Encoder::release when the payload allocation fails - #512
Bytecode cache: return null from Encoder::release when the payload allocation fails#512robobun wants to merge 1 commit into
Conversation
…location fails Encoder::release allocated the final contiguous payload with MallocSpan<uint8_t, VMMalloc>::malloc, which is crash-on-failure by contract. On Windows release builds that contract does not hold: bmalloc's BCRASH() is compiled as a call through a null function pointer, clang treats that as unreachable and deletes the null check in bmalloc::api::malloc, so fastMalloc returns null on OOM. The encoder then memcpy'd every page into a null buffer (bun 1.4.0 Windows x64, Sentry BUN-4RGJ, fault address 0x20 inside memcpy). The payload is the largest single allocation the encoder makes, and a bytecode cache is optional. Allocate it with tryMalloc and, if that fails, set BytecodeCacheError::StandardError(ENOMEM) and return null. Every encodeCodeBlock, encodeFunctionCodeBlock and encodeBuiltinFunction caller already handles a null result (release already returns null for a missing page and for a failed mapped write), so an embedder skips the cache for that module instead of crashing. The BCRASH() miscompile itself is fixed separately (#316).
|
Warning Review limit reached
On-demand reviews are free for the next 27 days. After that, they cost $0.25 per reviewed file. Or wait 6 minutes for your next included review. View limit detailsLimit details: You’ve used all 5 included reviews currently available. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — small defensive change that swaps a crash-on-fail allocation for a fallible one and reports ENOMEM through the existing error path.
What was reviewed:
MallocSpan::tryMallocreturns an empty span on failure andAllocSpanMixin::operator!testsdata(), soif (!buffer)is correct.release()already returnsnullptrfor the no-page andreleaseMappedfailure cases; all three call sites (encodeCodeBlock,encodeFunctionCodeBlock,encodeBuiltinFunction) propagate a nullRefPtrsafely.BytecodeCacheError::StandardError(int)matches the pattern already used a few lines below inreleaseMapped.
Extended reasoning...
Overview
The PR changes one allocation in Encoder::release (Source/JavaScriptCore/runtime/CachedTypes.cpp) from MallocSpan<uint8_t, VMMalloc>::malloc(size) to tryMalloc(size), and adds a null check that sets error = BytecodeCacheError::StandardError(ENOMEM) and returns nullptr. Net diff is +6/-1 lines including a comment.
Security risks
None. This is a defensive OOM guard on the bytecode cache's final payload buffer. The bytecode cache is an optional optimization; skipping it on allocation failure has no correctness or security impact. No user-controlled data flows change.
Level of scrutiny
Low. The change is mechanical and mirrors the existing failure path in the same function: releaseMapped at line ~742 already does error = BytecodeCacheError::StandardError(errno); return nullptr;. I verified in MallocSpan.h that tryMalloc returns a default-constructed span on failure, and in AllocSpanMixin.h that operator! checks m_span.data(), so the if (!buffer) test is sound. BytecodeCacheError has an operator=(const StandardError&) overload, and errno.h constants are already in scope (used a few lines below).
Other factors
All three callers of encoder.release(error) — encodeCodeBlock, encodeFunctionCodeBlock, encodeBuiltinFunction — return the RefPtr<CachedBytecode> directly or explicitly null-check it (line 4197), so no caller will dereference the new null return. The motivating Sentry crash (Windows OOM → memcpy into null) is plausibly addressed by bailing out before the copy loop. No outstanding reviewer comments; the only prior timeline entry is a rate-limited CodeRabbit notice.
Preview Builds
|
2 participants
Problem
Encoder::releaseallocates the final contiguous payload withMallocSpan<uint8_t, VMMalloc>::malloc(size)and copies every page into it (runtime/CachedTypes.cpp).VMMalloc::mallocis crash-on-failure by contract, so the result is never checked.BCRASH()compiles as a call through a null function pointer, clang treats it as unreachable and deletes the null check inbmalloc::api::malloc, sofastMallocreturns null on OOM (BAssert.h: use __builtin_trap for BCRASH on clang-cl #316 has the disassembly). The encoder then doesmemcpyinto a null buffer. Sentry BUN-4RGJ: bun 1.4.0 Windows x64,JSC::Encoder::release->WTF::memcpySpan->memcpy, fault address0x20, reached from the embedder's compile-cache thread.Fix
tryMalloc. If it fails, seterror = BytecodeCacheError::StandardError(ENOMEM)and return null.releasealready returns null in two cases (no page, failed mapped write), and every caller handles it:encodeCodeBlock,encodeFunctionCodeBlock,encodeBuiltinFunctionreturn theRefPtras is, andjsc.cppplus the embedder check for null. A module that cannot be cached is skipped. The payload is the largest single allocation the encoder makes, so it is the one most likely to fail under memory pressure. The page allocations keep the crash-on-failuremalloc.CachedTypes.cppcompiles with the change (clang++ -fsyntax-only -std=c++23against the JSC headers). An OOM path cannot be exercised by a unit test here.Background
Encoderwrites into page-sized chunks andreleaseconcatenates them.VMMallocisFastMalloc.FastMalloc::malloccrashes on failure,FastMalloc::tryMallocreturns null.MallocSpan::tryMallocwraps the latter and yields an empty span on failure.BytecodeCacheError::StandardError(int errno)is the existing wayreleasereports an OS failure to the caller.BCRASH()miscompile itself. With both changes, a failed payload allocation skips the cache, and any other OOM inside JSC on Windows crashes at the allocator (0xbbadbeef) instead of at a random null write.