Gracefully shut down TLS session before closing connection - #1353
Conversation
Terminate the TLS session with SSL_shutdown() and free the SSL context before closing the socket in redisFree(), so servers see a clean protocol termination instead of an abrupt disconnect. Ported from redis/redis@4a7dd65.
ABI compatibility check✅
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f19f45ddbb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| /* Should not be called if a previous fatal error has occurred on a connection; | ||
| * i.e., if SSL_get_error(3) has returned SSL_ERROR_SYSCALL or SSL_ERROR_SSL. | ||
| * (https://docs.openssl.org/3.3/man3/SSL_shutdown/#description). */ | ||
| SSL_shutdown(rsc->ssl); |
There was a problem hiding this comment.
When SSL_read or SSL_write returns SSL_ERROR_SSL or SSL_ERROR_SYSCALL—for example after a protocol error or connection reset—the read/write paths only set the context error and do not preserve that fatal state in redisSSL. A subsequent synchronous free or asynchronous error-driven auto-disconnect therefore unconditionally calls SSL_shutdown() despite the OpenSSL precondition cited in the adjacent comment, potentially attempting further TLS I/O on an invalid connection. Record fatal SSL errors and call only SSL_free() on those teardown paths.
Useful? React with 👍 / 👎.
| if (c->funcs && c->funcs->free_privctx) { | ||
| c->funcs->free_privctx(c->privctx); | ||
| c->privctx = NULL; |
There was a problem hiding this comment.
For custom redisContextFuncs transports whose close callback accesses c->privctx—the function table supports complex transports where the descriptor and private transport state are closed together—this now invokes free_privctx and clears the pointer before calling close. Such transports can no longer release or shut down their connection resources correctly during the universally used redisFree() path. Keep the established close-before-free ordering for generic transports and perform the TLS shutdown through a TLS-specific close path instead.
Useful? React with 👍 / 👎.
| /* Should not be called if a previous fatal error has occurred on a connection; | ||
| * i.e., if SSL_get_error(3) has returned SSL_ERROR_SYSCALL or SSL_ERROR_SSL. | ||
| * (https://docs.openssl.org/3.3/man3/SSL_shutdown/#description). */ | ||
| SSL_shutdown(rsc->ssl); |
There was a problem hiding this comment.
For a default blocking TLS context with no command timeout, SSL_shutdown() writes the close_notify through a blocking socket. If the peer has stopped consuming data and the socket transmit buffer is full, redisFree() and redisReconnect() can now wait indefinitely inside this destructor, whereas they previously closed the descriptor immediately. Make the notification best-effort without blocking, or apply a bounded shutdown timeout before entering this call.
Useful? React with 👍 / 👎.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit f19f45d. Configure here.
| /* Should not be called if a previous fatal error has occurred on a connection; | ||
| * i.e., if SSL_get_error(3) has returned SSL_ERROR_SYSCALL or SSL_ERROR_SSL. | ||
| * (https://docs.openssl.org/3.3/man3/SSL_shutdown/#description). */ | ||
| SSL_shutdown(rsc->ssl); |
There was a problem hiding this comment.
Unsafe SSL_shutdown after fatal errors
Medium Severity
redisSSLFree always calls SSL_shutdown whenever an SSL object exists, including after prior SSL_ERROR_SYSCALL / SSL_ERROR_SSL failures and while a handshake may still be unfinished. OpenSSL forbids that, and the new comment documents the restriction without enforcing it. redisSSL does not record fatal SSL state, so teardown cannot skip shutdown after failed reads/writes. That often runs on the error path into redisFree, and can yield SIGPIPE, hangs, or other undefined OpenSSL behavior instead of a clean close.
Reviewed by Cursor Bugbot for commit f19f45d. Configure here.
1 participant
Backport of the hiredis portion of redis/redis@4a7dd65 (redis/redis#14721) to the upstream hiredis repo.
Changes
redisFree(): free the connection'sprivctx(which for TLS tears down the session) before closing the file descriptor, instead of after, so the TLS shutdown can still be sent over the socket.redisSSLFree(): callSSL_shutdown()beforeSSL_free()so the server sees a cleanclose_notifyinstead of an abrupt disconnect.The
redis-cliportions of the original commit are not applicable here.🤖 Generated with Claude Code
Note
Low Risk
Small change to connection teardown order for TLS only; no impact on normal request/response paths or non-TLS connections.
Overview
TLS clients now tear down the session in the right order when a
redisContextis freed, so Redis (and other peers) see a proper TLS close_notify instead of an abrupt socket close.redisFree()now callsfree_privctx(and clearsprivctx) beforeclose, so SSL shutdown can still write over the open fd.redisSSLFree()invokesSSL_shutdown()beforeSSL_free()to complete the handshake side of shutdown.Reviewed by Cursor Bugbot for commit f19f45d. Bugbot is set up for automated code reviews on this repo. Configure here.