MOD-18192 - (LEAK) free ssl in case of redisInitiateSSL error - #124
Conversation
0092d4e
gabsow
left a comment
gabsow
left a comment
There was a problem hiding this comment.
LGTM. SSL_free(ssl) correctly plugs the leak on the redisInitiateSSL failure path — SSL_CTX_free on the context doesn't free the SSL object created via SSL_new, and it was never attached to redisContext since redisInitiateSSL failed, so it was previously leaked on every failed inter-shard TLS handshake. CI split (build without sudo, then sudo make install ... SKIP_BUILD=1) makes sense to keep BUILD_TLS=yes honored consistently, and all matrix legs are green.
| git checkout ${{ matrix.redis_version }} | ||
| BUILD_TLS=yes make valgrind install | ||
| BUILD_TLS=yes make valgrind | ||
| sudo make install BUILD_TLS=yes SKIP_BUILD=1 |
There was a problem hiding this comment.
What does this weird combo (BUILD_TLS=yes SKIP_BUILD=1) do?
| git clone https://github.com/redis/redis | ||
| cd redis | ||
| git checkout ${{ matrix.redis_version }} | ||
| BUILD_TLS=yes make -j8 |
There was a problem hiding this comment.
Why the hardcoded -j8? We have no idea what machine will build in ci so cannot assume anything about its number of cores.
4 participants
Note
Low Risk
Small resource-cleanup fix on a TLS failure path plus CI Redis install sequencing; no change to successful TLS or cluster messaging behavior.
Overview
Fixes an OpenSSL leak on inter-shard TLS setup: when
redisInitiateSSLfails inMR_OnConnectCallback, the code now callsSSL_free(ssl)before scheduling async disconnect/retry. Previously theSSLcreated withSSL_newwas left allocated after the context was already freed.CI install redis steps on Linux and macOS are split into a normal TLS build (
make valgrind/make -j8) and a separatesudo make install ... SKIP_BUILD=1, instead of a single combined build+install target.Reviewed by Cursor Bugbot for commit cf5196e. Bugbot is set up for automated code reviews on this repo. Configure here.