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.
This commit is contained in:
Pratik Mankawde
2026-03-16 14:12:48 +00:00
committed by GitHub
parent 918185e18f
commit b585dc78bb
4 changed files with 99 additions and 68 deletions

View File

@@ -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,

View File

@@ -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

View File

@@ -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.

View File

@@ -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<std::string> 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<unsigned int> statusCodes = {200, 404, 500};