From b585dc78bb17e344645c680aec1faba4b8e57679 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Mon, 16 Mar 2026 14:12:48 +0000 Subject: [PATCH] fix: Fix memory leaks in HTTPClient (#6370) The `HTTPClient` class initializes a global SSL context via `initializeSSLContext()`. However, it had no way to release it, which caused memory leaks flagged by the LeakSanitizer. Multiple LSAN suppressions in the sanitizers' suppressions file were masking these leaks. Our test code also manually called `initializeSSLContext()` in each test without guaranteed cleanup on failure paths. This change fixes these memory leaks by adding a `cleanupSSLContext()` method to properly release the global SSL context, and removes the corresponding LSAN suppressions. The change further refactors the `HTTPClient` tests to use a Google Test fixture (`HTTPClientTest`) that manages the SSL context lifecycle via RAII (SetUp/TearDown), making it impossible for tests to leak the context. --- include/xrpl/net/HTTPClient.h | 12 +++ sanitizers/suppressions/lsan.supp | 11 --- src/libxrpl/net/HTTPClient.cpp | 6 ++ src/tests/libxrpl/net/HTTPClient.cpp | 138 ++++++++++++++++----------- 4 files changed, 99 insertions(+), 68 deletions(-) diff --git a/include/xrpl/net/HTTPClient.h b/include/xrpl/net/HTTPClient.h index 9ea5f98a37..c176e2d2e8 100644 --- a/include/xrpl/net/HTTPClient.h +++ b/include/xrpl/net/HTTPClient.h @@ -29,6 +29,18 @@ public: bool sslVerify, beast::Journal j); + /** Destroys the global SSL context created by initializeSSLContext(). + * + * This releases the underlying boost::asio::ssl::context and any + * associated OpenSSL resources. Must not be called while any + * HTTPClient requests are in flight. + * + * @note Currently only called from tests during teardown. In production, + * the SSL context lives for the lifetime of the process. + */ + static void + cleanupSSLContext(); + static void get(bool bSSL, boost::asio::io_context& io_context, diff --git a/sanitizers/suppressions/lsan.supp b/sanitizers/suppressions/lsan.supp index a81d7d89fa..30d01a2bfa 100644 --- a/sanitizers/suppressions/lsan.supp +++ b/sanitizers/suppressions/lsan.supp @@ -1,16 +1,5 @@ # The idea is to empty this file gradually by fixing the underlying issues and removing suppresions. -# Suppress leaks detected by asan in rippled code. -leak:src/libxrpl/net/HTTPClient.cpp -leak:src/libxrpl/net/RegisterSSLCerts.cpp -leak:src/tests/libxrpl/net/HTTPClient.cpp -leak:xrpl/net/AutoSocket.h -leak:xrpl/net/HTTPClient.h -leak:xrpl/net/HTTPClientSSLContext.h -leak:xrpl/net/RegisterSSLCerts.h -leak:ripple::HTTPClient -leak:ripple::HTTPClientImp - # Suppress leaks detected by asan in boost code. leak:boost::asio leak:boost/asio diff --git a/src/libxrpl/net/HTTPClient.cpp b/src/libxrpl/net/HTTPClient.cpp index 91a64a1ca9..cbae927feb 100644 --- a/src/libxrpl/net/HTTPClient.cpp +++ b/src/libxrpl/net/HTTPClient.cpp @@ -26,6 +26,12 @@ HTTPClient::initializeSSLContext( httpClientSSLContext.emplace(sslVerifyDir, sslVerifyFile, sslVerify, j); } +void +HTTPClient::cleanupSSLContext() +{ + httpClientSSLContext.reset(); +} + //------------------------------------------------------------------------------ // // Fetch a web page via http or https. diff --git a/src/tests/libxrpl/net/HTTPClient.cpp b/src/tests/libxrpl/net/HTTPClient.cpp index 7b17891568..1f648cced4 100644 --- a/src/tests/libxrpl/net/HTTPClient.cpp +++ b/src/tests/libxrpl/net/HTTPClient.cpp @@ -185,61 +185,86 @@ private: } }; -// Helper function to run HTTP client test -bool -runHTTPTest( - TestHTTPServer& server, - std::string const& path, - bool& completed, - int& resultStatus, - std::string& resultData, - boost::system::error_code& resultError) -{ - // Create a null journal for testing - beast::Journal j{TestSink::instance()}; - - // Initialize HTTPClient SSL context - HTTPClient::initializeSSLContext("", "", false, j); - - HTTPClient::get( - false, // no SSL - server.ioc(), - "127.0.0.1", - server.port(), - path, - 1024, // max response size - std::chrono::seconds(5), - [&](boost::system::error_code const& ec, int status, std::string const& data) -> bool { - resultError = ec; - resultStatus = status; - resultData = data; - completed = true; - return false; // don't retry - }, - j); - - // Run the IO context until completion - auto start = std::chrono::steady_clock::now(); - while (server.ioc().run_one() != 0) - { - if (std::chrono::steady_clock::now() - start >= std::chrono::seconds(10) || - server.finished()) - { - break; - } - - if (completed) - { - server.stop(); - } - } - - return completed; -} - } // anonymous namespace -TEST(HTTPClient, case_insensitive_content_length) +// Test fixture that manages the SSL context lifecycle via RAII. +// SetUp() initializes the context before each test and TearDown() +// cleans it up afterwards, so individual tests don't need to worry +// about resource management. +class HTTPClientTest : public ::testing::Test +{ +protected: + // Shared journal for SSL context initialization and HTTP requests. + beast::Journal j_{TestSink::instance()}; + + // Initialize the global SSL context used by HTTPClient. + void + SetUp() override + { + HTTPClient::initializeSSLContext( + "" /* sslVerifyDir*/, "" /*sslVerifyFile */, false /* sslVerify */, j_ /* journal */); + } + + // Release the global SSL context to prevent memory leaks. + void + TearDown() override + { + HTTPClient::cleanupSSLContext(); + } + + // Issue an HTTP GET to the given test server and drive the + // io_context until a response arrives or a timeout is reached. + // Returns true when the completion handler was invoked. + bool + runHTTPTest( + TestHTTPServer& server, + std::string const& path, + bool& completed, + int& resultStatus, + std::string& resultData, + boost::system::error_code& resultError) + { + HTTPClient::get( + false, // no SSL + server.ioc(), + "127.0.0.1", + server.port(), + path, + 1024, // max response size + std::chrono::seconds(5), + [&](boost::system::error_code const& ec, int status, std::string const& data) -> bool { + resultError = ec; + resultStatus = status; + resultData = data; + completed = true; + return false; // don't retry + }, + j_); + + // Run the IO context until completion + auto start = std::chrono::steady_clock::now(); + while (server.ioc().run_one() != 0) + { + if (std::chrono::steady_clock::now() - start >= std::chrono::seconds(10) || + server.finished()) + { + break; + } + + if (completed) + { + server.stop(); + } + } + + // Drain any remaining handlers to ensure proper cleanup of HTTPClientImp + server.ioc().poll(); + + return completed; + } +}; + +TEST_F(HTTPClientTest, case_insensitive_content_length) { // Test different cases of Content-Length header std::vector headerCases = { @@ -264,7 +289,6 @@ TEST(HTTPClient, case_insensitive_content_length) bool testCompleted = runHTTPTest(server, "/test", completed, resultStatus, resultData, resultError); - // Verify results EXPECT_TRUE(testCompleted); EXPECT_FALSE(resultError); @@ -273,7 +297,7 @@ TEST(HTTPClient, case_insensitive_content_length) } } -TEST(HTTPClient, basic_http_request) +TEST_F(HTTPClientTest, basic_http_request) { TestHTTPServer server; std::string testBody = "Test response body"; @@ -294,7 +318,7 @@ TEST(HTTPClient, basic_http_request) EXPECT_EQ(resultData, testBody); } -TEST(HTTPClient, empty_response) +TEST_F(HTTPClientTest, empty_response) { TestHTTPServer server; server.setResponseBody(""); // Empty body @@ -314,7 +338,7 @@ TEST(HTTPClient, empty_response) EXPECT_TRUE(resultData.empty()); } -TEST(HTTPClient, different_status_codes) +TEST_F(HTTPClientTest, different_status_codes) { std::vector statusCodes = {200, 404, 500};