Fix X509 memory leak in connTLSGetPeerCert#14855
Merged
sundb merged 1 commit intoredis:unstablefrom Mar 10, 2026
Merged
Conversation
`connTLSGetPeerCert()` calls `SSL_get_peer_certificate()` which increments the X509 reference count, but never calls `X509_free()` to release it. This leaks an X509 object on each call to `RM_GetClientCertificate()`. This PR adds `X509_free(cert)` before returns. The existing `test module getclientcert api` test in `tests/unit/moduleapi/misc.tcl` exercises this code path. But the leak was not caught because the ASAN CI job does not enable TLS.
🤖 Augment PR SummarySummary: Fixes an X509 reference-count leak in Changes: Adds 🤖 Was this summary useful? React with 👍 or 👎 |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
sundb
approved these changes
Mar 9, 2026
Collaborator
sundb
left a comment
sundb
left a comment
There was a problem hiding this comment.
we can reproduce this leak using the following commands.
make BUILD_TLS=yes valgrind
./runtest --single unit/moduleapi/misc --valgrind --tls
Collaborator
|
It's been there for a long time, so only backport to 8.6. |
YaacovHazan
pushed a commit
to YaacovHazan/redis
that referenced
this pull request
Mar 19, 2026
`connTLSGetPeerCert()` calls `SSL_get_peer_certificate()` which increments the X509 reference count, but never calls `X509_free()` to release it. This leaks an X509 object on each call to `RM_GetClientCertificate()`. This PR adds `X509_free(cert)` before returns. The existing `test module getclientcert api` test in `tests/unit/moduleapi/misc.tcl` exercises this code path. But the leak was not caught because the ASAN CI job does not enable TLS.
Merged
YaacovHazan
pushed a commit
that referenced
this pull request
Mar 23, 2026
`connTLSGetPeerCert()` calls `SSL_get_peer_certificate()` which increments the X509 reference count, but never calls `X509_free()` to release it. This leaks an X509 object on each call to `RM_GetClientCertificate()`. This PR adds `X509_free(cert)` before returns. The existing `test module getclientcert api` test in `tests/unit/moduleapi/misc.tcl` exercises this code path. But the leak was not caught because the ASAN CI job does not enable TLS.
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
connTLSGetPeerCert()callsSSL_get_peer_certificate()which increments the X509 reference count, but never callsX509_free()to release it. This leaks an X509 object on each call toRM_GetClientCertificate(). This PR addsX509_free(cert)before returns.The existing
test module getclientcert apitest intests/unit/moduleapi/misc.tclexercises this code path. But the leak was not caught because the ASAN CI job does not enable TLS.Note
Low Risk
Low risk: adds missing
X509_free()calls on both success and error paths, with no behavioral change beyond preventing a leak.Overview
Prevents a per-call memory leak when exporting the client certificate (e.g., via
RM_GetClientCertificate) by freeing theX509 *certobtained fromSSL_get_peer_certificate()inconnTLSGetPeerCert()on both the failure path and after building the PEM string.Written by Cursor Bugbot for commit d962210. This will update automatically on new commits. Configure here.