wasphin commented on code in PR #3657:
URL: https://github.com/apache/brpc/pull/3657#discussion_r4379245396
##########
src/brpc/details/ssl_helper.cpp:
##########
@@ -953,14 +953,15 @@ void Print(std::ostream& os, SSL* ssl, const char* sep) {
<< "protocol=" << SSL_get_version(ssl) << sep
<< "verify=" << (SSL_get_verify_mode(ssl) & SSL_VERIFY_PEER
? "success" : "none");
- X509* cert = SSL_get_peer_certificate(ssl);
+ std::unique_ptr<X509, decltype(&X509_free)> cert(
+ SSL_get_peer_certificate(ssl), X509_free);
if (cert) {
Review Comment:
Keeping the existing API for compatibility with older OpenSSL releases.
SSL_get_peer_certificate() returns an owning reference, equivalent to
SSL_get1_peer_certificate(), and the X509_free deleter here releases it
correctly. SSL_get0_peer_certificate() and SSL_get1_peer_certificate() were
added in 3.0; using get0 would require a different ownership model. This PR
fixes resource cleanup while retaining the existing API compatibility. See the
[OpenSSL notes and
history](https://docs.openssl.org/3.0/man3/SSL_get_peer_certificate/#notes).
##########
test/brpc_ssl_unittest.cpp:
##########
@@ -88,6 +90,75 @@ class SSLTest : public ::testing::Test{
virtual void TearDown() {};
};
+#ifndef USE_MESALINK
+static void MarkCertificateFreed(void*, void* ptr, CRYPTO_EX_DATA*,
+ int, long, void*) {
+ if (ptr != nullptr) {
+ *static_cast<bool*>(ptr) = true;
+ }
+}
+
+TEST_F(SSLTest, diagnostic_print_releases_peer_certificate) {
+ bool certificate_freed = false;
+ std::unique_ptr<SSL_CTX, decltype(&SSL_CTX_free)> server_ctx(
+ SSL_CTX_new(SSLv23_method()), SSL_CTX_free);
+ std::unique_ptr<SSL_CTX, decltype(&SSL_CTX_free)> client_ctx(
+ SSL_CTX_new(SSLv23_method()), SSL_CTX_free);
Review Comment:
This repeats the [earlier
discussion](https://github.com/apache/brpc/pull/3657#discussion_r4378890129).
Current OpenSSL headers map SSLv23_method to TLS_method through a macro; the
call compiles with OpenSSL 3 and 4 under -Werror=deprecated-declarations.
TLS_method was added in 1.1.0, so an unconditional replacement would exclude
older compatibility paths. Keeping the existing call here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
