redis: encapsulate EventLoopTimer keep-alive refs in RefCountedTimer

Merged
RO
4mo ago
+189-115

Follow-up to #34714, which fixed the TLS-context over-release path but left the timer path with an unconditional ScopedRef::adopt.

Problem

The fuzzer still produces heap-use-after-free on 99fc2f892c23: on_connection_timeout's ScopedRef::adopt drops as the final deref, freeing the Box<JSValkeyClient> while the JS wrapper and the other armed EventLoopTimer still point at it; GC finalize then reaches stop_timers on freed memory.

freed by:
    ScopedRef::<JSValkeyClient>::drop
    JSValkeyClient::on_connection_timeout js_valkey.rs:1195
    ...
accessed by:
    <State as PartialEq>::eq
    JSValkeyClient::stop_timers
    JSValkeyClient::finalize

The fire callbacks adopt what they assume is add_timer's +1 unconditionally. When the close/reconnect/fail paths re-enter each other and the timer's ref has already been released (or was never taken for that arm), the adopt spends a phantom ref and the count reaches zero under live references.

Fix

Wrap each timer in a RefCountedTimer that tracks whether its keep-alive ref is currently held:

  • arm() inserts into the timer heap and ref_()s the owner iff ref_held was false.
  • disarm() unlinks and deref()s iff ref_held was true.
  • take_fire_ref() marks the timer FIRED and returns Some(ScopedRef::adopt(..)) iff ref_held was true, otherwise None.

So a fire (or stop) path can never release a ref that was not taken, regardless of how the connect/close/timeout callbacks interleave. deinit() asserts both flags are clear at count==0. The wrapper is #[repr(C)] with the EventLoopTimer first so the from_field_ptr! container-of recovery in dispatch.rs is unchanged (offset_of const-asserted).

This also folds away the open-coded state == ACTIVE checks and the separate remove-then-add at every call site, so the net diff is -115/+188 with most of the addition being the wrapper itself.

Verification

$ bun bd test test/js/valkey/valkey-gc.test.ts
 8 pass
 0 fail

The new test ("connection-timeout + reconnect churn against an under-replying server") exercises the fire/disarm/re-arm interleaving under ASAN but does not reproduce the fuzzer crash deterministically; I could not construct a local sequence that over-releases on the parent commit. The fix is structural: the flag makes phantom release impossible by construction, so fuzzer re-verification against this branch is the intended proof.

[review] gate passed · iteration 0 · 2 files touched

fails on main (without fix) 
ASAN without fix: all passed
$ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" test/js/valkey/valkey-gc.test.ts
info: syncing channel updates for nightly-2026-05-06-x86_64-unknown-linux-gnu
info: latest update on 2026-05-06 for version 1.97.0-nightly (e95e73209 2026-05-05)
info: component rust-src is up to date
info: checking for self-update (current version: 1.29.0)
bun test v1.4.0 (cddda957a)

test/js/valkey/valkey-gc.test.ts:
(pass) RedisClient survives a failed custom-TLS context without freeing the live client [522.76ms]
(pass) custom setter with a foreign receiver throws instead of corrupting the heap [493.89ms]
(pass) RedisClient survives GC after a command throws during argument validation [640.52ms]
(pass) rejects a RESP simple-string reply whose line terminator never arrives [497.23ms]
(pass) RedisClient survives GC across many short-lived instances [738.72ms]
(pass) RedisClient survives connection-timeout + reconnect churn against an under-replying server [1406.40ms]
(pass) RedisClient survives subscribe() + close() against a server that resets the connection [2589.52ms]
(pass) getBuffer replies surviv
... (truncated)

release without fix: 1 FAILED
bun test v1.4.0-canary.1 (1498d7b77)

test/js/valkey/valkey-gc.test.ts:
(pass) RedisClient survives a failed custom-TLS context without freeing the live client [16.40ms]
352 |       });
353 |       try {
354 |         await client.send("PING", []);
355 |         expect.unreachable();
356 |       } catch (error: any) {
357 |         expect(error.code).toBe("ERR_REDIS_CONNECTION_CLOSED");
                                 ^
error: expect(received).toBe(expected)

Expected: "ERR_REDIS_CONNECTION_CLOSED"
Received: "ERR_REDIS_INVALID_RESPONSE"

      at <anonymous> (/workspace/bun/test/js/valkey/valkey-gc.test.ts:357:28)
(fail) rejects a RESP simple-string reply whose line terminator never arrives [12.75ms]
(pass) RedisClient survives GC after a command throws during argument validation [15.55ms]
(pass) custom setter with a foreign receiver throws instead of corrupting the heap [14.95ms]
(pass) RedisClient survives GC across many short-lived instances [14.96ms]
(pass) getBuffer replies survive GC with adopted backing stores intact [53.11ms]
(pass) RedisClient survives subscribe() + close() against a server that resets the connection [436.48ms]
(pass) RedisClient survives 
... (truncated)
passes on PR (with fix) 
ASAN with fix: all passed
$ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" test/js/valkey/valkey-gc.test.ts
info: syncing channel updates for nightly-2026-05-06-x86_64-unknown-linux-gnu
info: latest update on 2026-05-06 for version 1.97.0-nightly (e95e73209 2026-05-05)
info: component rust-src is up to date
info: checking for self-update (current version: 1.29.0)
bun test v1.4.0 (cddda957a)

test/js/valkey/valkey-gc.test.ts:
(pass) RedisClient survives a failed custom-TLS context without freeing the live client [516.07ms]
(pass) custom setter with a foreign receiver throws instead of corrupting the heap [500.60ms]
(pass) RedisClient survives GC after a command throws during argument validation [645.94ms]
(pass) rejects a RESP simple-string reply whose line terminator never arrives [483.82ms]
(pass) RedisClient survives GC across many short-lived instances [735.06ms]
(pass) RedisClient survives connection-timeout + reconnect churn against an under-replying server [1383.79ms]
(pass) RedisClient survives subscribe() + close() against a server that resets the connection [2606.69ms]
(pass) getBuffer replies surviv
... (truncated)

release with fix: all passed
$ bun scripts/build.ts --profile=release
info: syncing channel updates for nightly-2026-05-06-x86_64-unknown-linux-gnu
info: latest update on 2026-05-06 for version 1.97.0-nightly (e95e73209 2026-05-05)
info: component rust-src is up to date
info: checking for self-update (current version: 1.29.0)
[configured] bun-profile → bun (stripped) in 711ms (unchanged)
ninja: Entering directory `/workspace/bun/build/release'
[1/5] gen generated_host_exports.rs
generated_host_exports.rs: 91 exports (host=3, lazy=10, generic=78, rust=0); 244 extern-C blocks audited
[1/5] cargo bun_bin → libbun_rust.a (--target x86_64-unknown-linux-gnu)
info: syncing channel updates for nightly-2026-05-06-x86_64-unknown-linux-gnu
info: latest update on 2026-05-06 for version 1.97.0-nightly (e95e73209 2026-05-05)
info: component rust-src is up to date
info: component rust-std is up to date

  nightly-2026-05-06-x86_64-unknown-linux-gnu unchanged - rustc 1.97.0-nightly (e95e73209 2026-05-05)

info: checking for self-update (current version: 1.29.0)
    Blocking waiting for file lock on build directory
   Compiling bun_runtime v0.0.0 (/workspace/bun/src/runtime)
[
... (truncated)
diff hotspot 
src/runtime/valkey_jsc/js_valkey.rs | 222 ++++++++++++++++++------------------
 test/js/valkey/valkey-gc.test.ts    |  82 ++++++++++++-
 2 files changed, 189 insertions(+), 115 deletions(-)

gate history · 1 passed · 0 rejected · iteration 0

evidence per changed file 
file                                 reads  edits  tests
src/runtime/valkey_jsc/js_valkey.rs     14     15      0
test/js/valkey/valkey-gc.test.ts         1      2      0