Bytecode cache: hand out looked-up symbols with the reference the decoder's callers adopt

[dylan-conway]

CachedUniquedStringImplBase::decode() returns a raw UniquedStringImpl* that its callers take ownership of: CachedRefPtr::decode() does adoptRef(decodedPtr) and registers a Decoder finalizer that derefs it once more, CachedJSValue::decode() does jsString(vm, adoptRef(*impl)), and the lean-decoder path adopts it directly. So every branch of the create lambda has to return the impl at +1, and they do — AtomStringImpl::add(...).leakRef(), symbolForKey(...).leakRef(), createNullSymbol().leakRef() — except the branch that resolves well-known symbols and private names through BuiltinNames:

} else if (m_isWellKnownSymbol)
    symbol = vm.propertyNames->builtinNames().lookUpWellKnownSymbol(buffer);
else
    symbol = vm.propertyNames->builtinNames().lookUpPrivateName(buffer);
RELEASE_ASSERT(symbol);
String str = symbol;                              // +1
StringImpl* impl = str.releaseImpl().unsafeGet(); // temporary RefPtr dies: -1
return static_cast<UniquedStringImpl*>(impl);     // +0, adopted by the caller

Each Decoder that materializes such a symbol therefore releases one more reference than it took. In stock JSC those lookups resolve to StaticSymbolImpls, whose static flag keeps deref() from ever destroying them, so nothing is observable. Bun registers its builtin private names as heap-allocated PrivateSymbolImpls via BuiltinNames::appendExternalName() (BunBuiltinNames.cpp), so decoding cached builtin-mode code that references them (the builtin bytecode cache work in #502 / oven-sh/bun#33316) would free them underneath BuiltinNames once enough decoded code blocks are collected.

The lookup branch now takes the reference explicitly; the registered-symbol branch already had its +1 from leakRef() and no longer round-trips through a String.

Tested: Release build; a script using generators/async functions (JSC private-name identifiers), for-of (well-known symbol identifiers) and class private fields round-trips through JSC_diskCachePath + JSC_forceDiskCache=1.

[coderabbitai]

Warning

Review limit reached

• Run on-demand review

_{On-demand reviews are free for the next 28 days. After that, they cost $0.25 per reviewed file.}

Or wait 17 minutes for your next included review.

View limit details

Limit details: You’ve used the included review currently available. Your 63 included PR review attempts over the past 7 days set your current allowance at 1 review per hour.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 9d3312c0-3990-40e4-a475-de0860fe8340

📥 Commits

Reviewing files that changed from the base of the PR and between fc1a8df and 9718f36.

📒 Files selected for processing (1)

• Source/JavaScriptCore/runtime/CachedTypes.cpp

---

_{Comment @coderabbitai help to get the list of available commands.}

[claude]

I reviewed this PR and didn't find any bugs. The refcount analysis checks out, but since this touches ref-counting on the bytecode-cache decode path (memory-safety-adjacent in the engine runtime), a human look would still be worthwhile.

What was reviewed:

• Verified lookUpWellKnownSymbol/lookUpPrivateName return raw +0 pointers and that the old String str = symbol; str.releaseImpl().unsafeGet() round-trip nets to +0 (temporary RefPtr destructs after unsafeGet), so callers that adoptRef were over-releasing; the added symbol->ref() restores the +1 contract.
• Checked the m_isRegistered branch still returns +1 via symbolForKey(...).leakRef() now that the String round-trip is dropped.
• Confirmed the collapsed ASSERT(m_isWellKnownSymbol != symbol->isPrivate()) is equivalent to the old two-armed assert and holds for the registered-symbol paths as well.
• RELEASE_ASSERT(symbol) moving inside the else is fine — symbolForKey returns a reference, so the registered branch can't be null.

Extended reasoning...

Overview

This PR touches a single function, the create lambda inside CachedUniquedStringImplBase::decode() in Source/JavaScriptCore/runtime/CachedTypes.cpp. It fixes a reference-counting mismatch: callers of decode() adopt the returned pointer (i.e., expect it at +1), and every branch returned +1 except the well-known-symbol / private-name lookup branch, which returned +0. The fix adds an explicit symbol->ref() on that branch and drops a now-redundant String round-trip on the registered-symbol branch (which already had its +1 from leakRef()).

I traced the old sequence against WTF headers: String str = symbol refs (+1), releaseImpl() returns a by-value RefPtr<StringImpl>, unsafeGet() (RefPtr.h:119) returns the raw pointer without leaking, and the temporary RefPtr then destructs (-1) — net 0. BuiltinNames::lookUpWellKnownSymbol/lookUpPrivateName return raw pointers, so that branch was indeed handing out a +0 pointer to be adopted. The description's claim that stock JSC masks this because those are StaticSymbolImpls (whose deref is a no-op) while Bun's appendExternalName names are heap-allocated is consistent with BuiltinNames.h.

Security risks

This is a memory-safety fix (under-ref → potential UAF of Bun's heap-allocated private-name symbols after enough decoded code blocks are collected). The change itself introduces no new attack surface; it only adds a ref() on one branch and removes a no-op round-trip on another. No input parsing, auth, or crypto involved.

Level of scrutiny

Higher than a typical mechanical change: ref-counting bugs in a JS engine's decoder can turn into use-after-free, and the correctness hinges on the caller contract (adoptRef + Decoder finalizer) described in the PR body rather than being visible in the diff itself. The change is small and I believe correct, but a maintainer who owns the bytecode-cache series should confirm the +1 contract at all three call sites named in the description.

Other factors

• The collapsed ASSERT(m_isWellKnownSymbol != symbol->isPrivate()) is logically identical to the previous two-armed form and, per the Shape encode logic, also holds for both registered-symbol sub-branches.
• Moving RELEASE_ASSERT(symbol) inside the lookup else is safe because symbolForKey returns a reference (&...leakRef() cannot yield null).
• This is part of an active series of bytecode-cache PRs (#490/#494/#497/#499/#502) from the same area; a quick human sign-off keeps that series consistent.

[github-actions]

Preview Builds

Commit	Release	Date
9718f361	autobuild-preview-pr-504-9718f361	2026-08-23 23:27:26 UTC