redis: give SubscriptionCtx an explicit BackRef to fix the subscribe() refcount over-release - #35787
redis: give SubscriptionCtx an explicit BackRef to fix the subscribe() refcount over-release#35787robobun wants to merge 5 commits into
Conversation
…ibe() refcount over-release SubscriptionCtx previously recovered its owning JSValkeyClient via impl_field_parent! on &self. SubscriptionCtx is three plain bools (a Freeze type), so the &self argument is lowered as noalias readonly and its provenance covers only those three bytes. The refcount .set() in on_new_subscription_callback_insert (reached through that pointer) is a store LLVM may discard at O2+, while the paired ScopedRef drop survives as an opaque call. Net: each subscribe() releases one more ref than it takes; after close() + connection-timeout the last legitimate release frees the Box under the live JS wrapper. SubscriptionCtx now holds an Option<BackRef<JSValkeyClient>> built from the heap::into_raw pointer at init time, so every parent access carries the allocation's full provenance. Also: - RefCount::ref_ re-reads the count via a volatile load under debug_assertions and asserts it advanced, so this bug class trips on release-assertions/release-asan lanes at the store site instead of surfacing as a heap-use-after-free on whichever releaser runs last. - ByteBlobLoader::to_any_blob now reaches is_closed via the &mut-self accessor (same audit finding; ByteBlobLoader is Freeze). - impl_field_parent! docs warn that the ref-only form is read-only when the child is Freeze.
|
Status: diff ready; needs a maintainer merge on the strength of the analysis. All review threads resolved, clippy/miri/format/lint green. No valkey/refcount/stream failures on any lane. Reproduced how: connect → CI: builds #81369 and #81477 both hit PR: #35787 |
|
Updated 9:21 PM PT - Jul 25th, 2026
❌ @robobun, your commit 3767356 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 35787That installs a local version of the PR into your bun-35787 --bun |
WalkthroughChangesThe PR replaces recovered subscription-context parent references with explicit Subscription context ownership
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/js/valkey/valkey-gc.test.ts`:
- Around line 140-142: Update the regression tests around the healthy-server
subscribe/close/GC flow and the cases at the referenced sections so they run the
reproducer with an optimized ASAN binary rather than bunExe()’s debug build.
Replace timing-only waits with an awaited observable timeout or close condition,
and assert that condition so the test deterministically exercises the pre-fix
failure and verifies every required timeout callback completes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d41331c5-09b0-495e-aac3-fa0b335be0ea
📒 Files selected for processing (8)
src/bun_core/lib.rssrc/ptr/ref_count.rssrc/runtime/api/BunObject.rssrc/runtime/valkey_jsc/js_valkey.rssrc/runtime/valkey_jsc/js_valkey_functions.rssrc/runtime/webcore/ByteBlobLoader.rssrc/runtime/webcore/ReadableStream.rstest/js/valkey/valkey-gc.test.ts
|
Found 2 issues this PR may fix:
🤖 Generated with Claude Code |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Thanks for fixing the test-comment placement in 3932982. I didn't find any bugs, but this is a subtle memory-safety fix (readonly-provenance dead-store elimination in the intrusive refcount path) touching RefCount::ref_, the impl_field_parent! contract, Valkey client lifecycle, and ByteBlobLoader — worth a human look, especially on the audit table's "load-bearing on one Cell" entries for ValkeyClient/MySQLConnection.
What was reviewed:
- All three
SubscriptionCtx::initcall sites (create,get_valkey_default_client,duplicate) pass the freshheap::into_rawpointer;#[derive(Default)]still holds viaOption<BackRef>. parent_backref()/parent()cover all five formerimpl_field_parent!consumers;BackRef: Copyso the struct-literal move + subsequent reads ininitare sound.ByteBlobLoader::to_any_blobalready had&mut self, so the switch to the raw-mutparent()accessor is available;ByteStream::to_any_blob(which takes&self) was left alone becauseByteStreamis non-Freeze.- The volatile tripwire in
RefCount::ref_isdebug_assertions-gated and reads a liveCellpointer.
Extended reasoning...
Overview
Fixes a heap-UAF in RedisClient.subscribe() where SubscriptionCtx (three plain bools → Freeze) recovered &JSValkeyClient via impl_field_parent! container-of on a noalias readonly &self. At O2+ LLVM elides the raw_count.set() store reached through that pointer while the paired ScopedRef drop (opaque call) survives, netting −1 on the intrusive refcount per subscribe(). The fix stores an explicit BackRef<JSValkeyClient> on SubscriptionCtx constructed from the heap::into_raw pointer (full-allocation provenance), removes the macro invocation, and updates all three construction sites plus five consumers. A parallel fix in ByteBlobLoader::to_any_blob switches from parent_const() to the &mut self raw-mut accessor. A debug_assertions volatile-re-read tripwire is added to RefCount::ref_, and the macro docs gain a provenance note.
Security risks
None new. This closes a UAF reachable from user JS (subscribe()), which is a security-relevant improvement. No new attack surface.
Level of scrutiny
High. This is a pointer-provenance / optimizer-interaction bug in intrusive refcounting — the most-blocked category in this repo's review guidelines. The root-cause analysis (disassembly-verified dead-store, numbered refcount trace) is thorough, but the fix touches shared infrastructure (RefCount::ref_, impl_field_parent! docs) and the audit table explicitly flags two other users (ValkeyClient, MySQLConnection) as correct only because they happen to contain a Cell field today. A maintainer should decide whether that's acceptable or whether those need the same treatment now.
Other factors
- The new test only reproduces on optimized-ASAN lanes (opt-level 0 doesn't elide the store); the PR description and test comment both state this, and the release-build evidence in the PR body shows a pre-fix failure (albeit on a different test in the file, which is a bit odd — the healthy-server test itself shows as passing on release-without-fix in the truncated output, though the description says it's the ASAN lane that catches it).
- All comment-cop and CodeRabbit threads are resolved; my prior placement nit was fixed.
- The
unsafe fn initsignature change with its# Safetydoc is appropriate given clippy'snot_unsafe_ptr_arg_deref. ByteBlobLoaderchange is defensible independent of whether the specific store was actually being elided today — it removes a same-class hazard.
There was a problem hiding this comment.
No bugs found, and the test-comment placement I flagged earlier is fixed in 3932982. Deferring to a human reviewer: this is a memory-safety fix in intrusive refcounting with a subtle optimizer-provenance root cause, and the audit table names two other impl_field_parent! users (ValkeyClient, MySQLConnection) that are one Cell-removal away from the same fault — worth a human deciding whether that's acceptable to leave.
What was reviewed:
SubscriptionCtx::initnow takes the rawheap::into_rawpointer at all three construction sites (create,Bun.redisdefault,duplicate);BackRefisCopyso the struct-literal field ordering (parent: Some(parent)before laterparent.client.get()reads) is fine.- The removed
impl_field_parent!invocation had no other consumers; all fiveparent()uses now go through the stored back-ref. ByteBlobLoader::to_any_blobswitch to the&mut selfaccessor writes a disjoint field (is_closed), andByteStream::to_any_blob(the other writer named in theis_closeddoc comment) already goes through a non-Freeze child so is unaffected.- The
RefCount::ref_volatile re-read tripwire is#[cfg(debug_assertions)]-gated and reads through the same livecountreference.
Extended reasoning...
Overview
Fixes a heap-use-after-free in RedisClient.subscribe() where the intrusive refcount's .set() store was elided at O2+ because it was reached through a pointer derived from a Freeze shared-ref (&SubscriptionCtx, three plain bools) lowered as noalias readonly. The fix stores an explicit BackRef<JSValkeyClient> on SubscriptionCtx (constructed from the full-provenance heap::into_raw pointer) instead of recovering the parent via container_of. Also: a debug-only volatile-re-read tripwire in RefCount::ref_, a doc warning on the impl_field_parent! macro, a same-class fix in ByteBlobLoader::to_any_blob, and a new GC regression test.
Security risks
None in the traditional sense — this is internal lifetime bookkeeping. The pre-fix state is a UAF (memory safety), which the PR closes on the SubscriptionCtx and ByteBlobLoader paths.
Level of scrutiny
High. This is exactly the "memory safety (the most-blocked category)" bucket in REVIEW.md: intrusive refcounts, pointer provenance, and an optimizer-dependent fault. The root-cause claim — that LLVM drops a Cell::set reached through a readonly-provenance pointer while the paired ScopedRef drop survives as an opaque call — is plausible and well-argued (with disassembly cited), but it's the kind of theory a maintainer should sign off on. The audit table also documents that ValkeyClient::parent() and MySQLConnection are "OK today; load-bearing on one Cell" — i.e., they avoid this bug only because an incidental interior-mutable field makes the child non-Freeze. A human should decide whether that latent risk is acceptable or whether those should also move to stored back-refs now.
Other factors
- My earlier inline comment (RST-race test comment glued to the wrong test) was addressed in 3932982; verified the comment now sits above its own test at line 212.
- All CodeRabbit and comment-cop threads are resolved; the remaining doc comments are legitimate (
# Safetyonunsafe fn, macro invariant docs). - The new test only reproduces on optimized-ASAN lanes (documented in the test header and PR body); this matches how the rest of
valkey-gc.test.tsworks and is a property of the bug, not the test. SubscriptionCtxgained a field butfinalize()'s "nothing to release" comment was correctly updated:BackRefis non-owning.- CI (#81369) is still building at the time of this review.
2 participants
Root cause
RedisClient.subscribe()releases exactly one more intrusive ref than it takes, which frees theBox<JSValkeyClient>under its still-live JS wrapper once the connection-timeout timer (or any other last legitimate holder) lets go. The crash surfaces as whichever path next touches the allocation:RefCountedTimer::state/disarmfromstop_timersin GC finalize,bufferedAmount, the!ref_heldasserts, etc., which is why the face kept moving across #34714 and #34760.The missing increment is a codegen artifact of writing through a
Freeze-typed shared reference:SubscriptionCtxwas three plainbools, so&SubscriptionCtxis aFreezeshared ref that rustc lowers asnoalias readonly.parent()derives&JSValkeyClientfromcore::ptr::from_ref(self), andupsert_receive_handler's exit guard reacheson_new_subscription_callback_insert→ref_scope()→RefCount::ref_through it. At O2+ LLVM is entitled to treat theraw_count.set()store as dead (it's a write through a pointer derived from areadonlyargument) while the pairedScopedRefdrop survives as an opaque call toderef_with_context. Disassembly of the optimized build shows both predecessor blocks ofupsert_receive_handlerexecutingassert_single_threaded→ compute&raw_count→ overflow-probeget()+1→ no store →call on_writable.Numbered trace on a healthy server (no faults):
finalizelater adopts)this_valueis still attached)on_valkey_closereleases the socket ref → 1 (only the armed timer)deinitfrees the Box under the live wrapperThe free stack is always the last legitimate release; the defect is a missing increment, which appears in no stack trace.
Fix
SubscriptionCtxnow stores an explicitOption<BackRef<JSValkeyClient>>constructed from theheap::into_rawpointer inSubscriptionCtx::init, and theimpl_field_parent!invocation is removed. All fiveparent()consumers (subscription-map lookup, theupsert_receive_handlerexit guard, theinvoke_callbackspoll-ref guard,is_deletable,close) read through the stored back-reference, so writes toref_count/poll_ref/clientcarry the allocation's full provenance instead of a readonly-derived one.initis called from all three construction paths (create,Bun.redis's default-client accessor,duplicate()).Tripwire
RefCount::ref_re-reads the count viaptr::read_volatileunderdebug_assertionsand asserts it advanced. On optimized builds with assertions enabled this converts the class of bug from "heap-UAF at a distance" into an assertion at the dropped store itself.impl_field_parent!auditSubscriptionCtxref_scope,update_poll_ref)ByteBlobLoaderis_closed.set)&mut selfaccessor)ValkeyClientAutoFlusher.registered: Cell<bool>)ref_,add_subscription, …)CellMySQLConnectionMySQLRequestQueueCell/JsCell)ref_guard,stop_timers, …)queueAssetsCapturedWriterByteStreamExecutionSourceMapStoreFileReaderAdded a safety note to the macro's docs so the next refactor of
ValkeyClient/MySQLConnectiondoesn't silently reintroduce this.Verification
Healthy-server subscribe isolation (shape A): connect →
subscribe()→close()→ wait for the connection-timeout timer →Bun.gc(true)→ touchbufferedAmount/connected, 40 clients per process. Added totest/js/valkey/valkey-gc.test.ts.The defect is an LLVM dead-store at O2+ on a
Cell::setreached through anoalias readonlyargument, so it is only observable on a build that both (a) compiles the Rust side at O2 or higher and (b) instruments the heap so the post-free read faults. Neither of the mechanical fail-before configurations has both:--profile=debug(ASAN)--profile=release--profile=release-asanDisassembly of the optimized build shows both predecessor blocks of
upsert_receive_handlerreachingassert_single_threaded→ compute&raw_count→ overflow-probeget()+1→ no store →call on_writable; the same site in thedevbuild is an out-of-linecall ref_that executes the store. The addedRefCount::ref_volatile tripwire turns the elision into an assertion on any O2+debug-assertions build.Locally against the debug build: 9/9 in
valkey-gc.test.ts,body.test.ts348/348,serve.test.tsunchanged from main.[review] gate passed · iteration 0 · 8 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