redis: fix heap-use-after-free from refcount over-release

Merged
RO
4mo ago
+185-115

Problem

Fuzzing found a heap-use-after-free in JSValkeyClient: the native Box<JSValkeyClient> (880 bytes) is freed while the JS wrapper and armed event-loop timers still point at it. The fuzzer surfaced several faces of the same over-release: subscribe reading flags.needs_to_open_socket on a freed allocation, on_connection_timeout's deref_guard freeing the box ahead of GC finalize, and a RefCount canary underflow during close-while-disconnecting.

==ERROR: AddressSanitizer: heap-use-after-free on address 0x... (READ of size 1)
    #0 <valkey::Status as PartialEq>::eq valkey.rs:77
    #1 JSValkeyClient::get_connected js_valkey.rs
    ...
freed by:
    #10 JSValkeyClient::deinit js_valkey.rs
    #14 JSValkeyClient::deref js_valkey.rs
    #15 deref_guard::drop_fn
    #19 JSValkeyClient::do_connect js_valkey.rs

Cause

connect() took the socket keep-alive ref (released later by on_valkey_close/on_valkey_reconnect) after the custom-TLS context check. When SSL_CTX creation failed, the tls_ctx_failed branch called on_valkey_close() anyway, whose scope guard unconditionally derefs that socket ref. Since the ref had never been taken, this ate into the JS wrapper's +1 instead:

  • start: count = 1 (JS wrapper)
  • do_connect/connect scoped ref_() bumps
  • on_valkey_close guard derefs the not-yet-taken socket ref: net -1
  • scoped deref_guards drop: count hits 0, deinit() frees the Box
  • JS wrapper's ext ptr now dangles; the next c.connected / c.close() / GC finalize reads or writes freed memory

The host-fn shim (host_fn_this_shared) passes a bare &self with no ref of its own, so once a re-entrant fault path spends the wrapper's ref there is nothing keeping *self alive for the rest of the call. The fuzzer's non-TLS repro hits the same shape through subscribe(): upsert_receive_handler's exit guard runs on_writable()/update_poll_ref() before send() is reached, and a dying socket's close path can release the remaining strong refs under the live borrow.

Fix

  • Move the socket keep-alive ref_() (and its errdefer_deref) to before the TLS-context check so on_valkey_close() always has a ref to consume. Disarm the errdefer before that call so the ref is released exactly once.
  • Hold a scoped ref across JSValkeyClient::send(), subscribe(), unsubscribe() and js_disconnect() (the host-fn entry points that can synchronously re-enter connect/close/fail), so *self stays live for the duration of the call even if a re-entrant path releases every other ref.

Verification

New tests in test/js/valkey/valkey-gc.test.ts:

  • RedisClient survives a failed custom-TLS context without freeing the live client reproduces the over-release deterministically (ASAN heap-use-after-free on every run before the fix, clean after).
  • RedisClient survives subscribe() + close() against a server that resets the connection exercises the fuzzer's RST-during-subscribe scenario.
$ bun bd test test/js/valkey/valkey-gc.test.ts
 7 pass
 0 fail