redis: gate the socket keep-alive adopt and defer destruction to finalize

Closed
RO
3mo ago
+72-8

Follow-up to #34760. The RefCountedTimer wrapper closed the timer over-release path, but the fuzzer still produces the same heap-use-after-free on b18d5df35e75 (10/10 on a release-asan build, 2/2 under the exact-fault plan).

Problem

access:  state js_valkey.rs:394 <- disarm :428 <- stop_timers :1536
         <- JSValkeyClient::finalize :1524 <- Heap::finalize
freed:   JSValkeyClient::deinit :1726
         <- rc_deref <- drop ScopedRef<JSValkeyClient>
         <- JSValkeyClient::on_connection_timeout :1199
         <- __bun_fire_timer <- All::drain_timers

on_valkey_close / on_valkey_reconnect adopt connect()'s socket keep-alive ref unconditionally. That adopt is the only release on the teardown path that is not tracked against its acquire. The close/fail paths can re-enter: on_data parse fail (or on_connection_timeout -> client_fail) -> fail_with_js_value -> close() dispatches on_close synchronously, and the enter_event_loop_scope drain inside on_valkey_close runs the connect() rejection while the outer socket-handler frames are still on the stack, so user code (or a second dispatch in the same batch) can reach on_valkey_close again with the ref already released. The second adopt spends the JS wrapper's +1; the ScopedRef drops at the end of on_connection_timeout then bring the count to 0 and deinit frees the box while the wrapper is live, and GC finalize -> stop_timers reads the freed allocation.

Fix

take_socket_ref() mirrors RefCountedTimer: connect() records each socket_ref.forget() in a Cell<u32>, and the adopt is a checked_sub so a re-entrant caller with no outstanding ref returns None instead of over-releasing. A counter (not a bool) because a stale reconnect can leave more than one us_socket_t in flight for the same client.

Verification

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

The new test drives on_data -> parse fail -> close() deterministically with a server that replies !garbage to HELLO. The fuzzer interleaving under on_connection_timeout is timing-dependent (0/35 runs on the CI container vs 10/10 on the reporting hardware), so fuzzer re-verification against this branch is the intended proof for that face.