Copilot commented on code in PR #3657:
URL: https://github.com/apache/brpc/pull/3657#discussion_r4378369435
##########
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);
+ ASSERT_TRUE(server_ctx);
+ ASSERT_TRUE(client_ctx);
+ ASSERT_EQ(1, SSL_CTX_use_certificate_chain_file(server_ctx.get(),
"cert1.crt"));
+ ASSERT_EQ(1, SSL_CTX_use_PrivateKey_file(
+ server_ctx.get(), "cert1.key", SSL_FILETYPE_PEM));
+ SSL_CTX_set_session_cache_mode(server_ctx.get(), SSL_SESS_CACHE_OFF);
+ SSL_CTX_set_session_cache_mode(client_ctx.get(), SSL_SESS_CACHE_OFF);
+ std::unique_ptr<SSL, decltype(&SSL_free)> server(
+ SSL_new(server_ctx.get()), SSL_free);
+ std::unique_ptr<SSL, decltype(&SSL_free)> client(
+ SSL_new(client_ctx.get()), SSL_free);
+ ASSERT_TRUE(server);
+ ASSERT_TRUE(client);
+ BIO* server_bio = nullptr;
+ BIO* client_bio = nullptr;
+ ASSERT_EQ(1, BIO_new_bio_pair(&server_bio, 0, &client_bio, 0));
+ SSL_set_bio(server.get(), server_bio, server_bio);
+ SSL_set_bio(client.get(), client_bio, client_bio);
+ SSL_set_accept_state(server.get());
+ SSL_set_connect_state(client.get());
+ for (int i = 0; i < 100 &&
+ (!SSL_is_init_finished(client.get()) ||
+ !SSL_is_init_finished(server.get())); ++i) {
+ for (SSL* ssl : {client.get(), server.get()}) {
+ const int rc = SSL_do_handshake(ssl);
+ if (rc != 1) {
+ const int error = SSL_get_error(ssl, rc);
+ ASSERT_TRUE(error == SSL_ERROR_WANT_READ ||
+ error == SSL_ERROR_WANT_WRITE) << error;
+ }
+ }
+ }
+ ASSERT_TRUE(SSL_is_init_finished(client.get()));
+ ASSERT_TRUE(SSL_is_init_finished(server.get()));
+ std::unique_ptr<X509, decltype(&X509_free)> peer(
+ SSL_get_peer_certificate(client.get()), X509_free);
+ ASSERT_TRUE(peer);
+ const int index = X509_get_ex_new_index(
Review Comment:
`X509_get_ex_new_index()` allocates a new global ex-data index each time
it’s called; doing this per test invocation can lead to unbounded growth when
tests are run repeatedly in the same process (e.g., stress / fuzz / sharded
runners). Consider making `index` a function-local static initialized once (or
using `std::once_flag`) so only one index is allocated for the process.
##########
src/brpc/details/ssl_helper.cpp:
##########
@@ -969,7 +970,8 @@ void Print(std::ostream& os, SSL* ssl, const char* sep) {
}
void Print(std::ostream& os, X509* cert, const char* sep) {
- BIO* buf = BIO_new(BIO_s_mem());
+ std::unique_ptr<BIO, FreeBIO> bio(BIO_new(BIO_s_mem()));
+ BIO* buf = bio.get();
if (buf == nullptr) {
return;
}
Review Comment:
This can be simplified by checking the owning pointer directly (e.g., `if
(!bio) return;`) and then using `bio.get()` afterward. It keeps
ownership/validity checks in one place and avoids a redundant raw-pointer null
check.
##########
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:
`SSLv23_method()` has long been deprecated in OpenSSL (kept as a
compatibility alias in many builds) and can trigger
`-Werror=deprecated-declarations` in stricter toolchains. Prefer `TLS_method()`
here to avoid deprecation warnings and better reflect intent.
--
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]
