Fix memory leak due caused due to X509_STORE #671
Conversation
|
@omjego, thanks for the pull request. How did you detect memory leak? Can I reproduce it easily on my Mac? Also I wonder if you can add a unit test to test/test.cc. |
|
Hey @yhirose the memory leak was detected with address sanitiser. Have added a test for repro and address sanitizer flag to the test Makefile which will be useful for finding such bugs in future. |
|
@omjego, thanks for adding the unit test. I now fully understand the situation. I reviewed the pull request, but it seems to be not enough. Here is an example: {
httplib::SSLClient cli("www.google.com");
auto cs1 = X509_STORE_new();
X509_STORE_load_locations(cs1, "/etc/ssl/certs/ca-certificates.crt", nullptr);
cli.set_ca_cert_store(cs1);
auto cs2 = X509_STORE_new();
X509_STORE_load_locations(cs2, "/etc/ssl/certs/ca-certificates.crt", nullptr);
cli.set_ca_cert_store(cs2);
// When the destructor of `cli` gets executed, only `cs2` will be freed...
}So, I am thinking to remove inline void SSLClient::set_ca_cert_store(X509_STORE *ca_cert_store) {
if (ca_cert_store) {
if (ctx_) {
if (SSL_CTX_get_cert_store(ctx_) != ca_cert_store) {
// The previous cert store pointer will be freed, and the current one
// wiil be freed in `~SSLClient()`.
SSL_CTX_set_cert_store(ctx_, ca_cert_store);
}
} else {
X509_STORE_free(ca_cert_store);
}
}
}
inline bool SSLClient::load_certs() {
bool ret = true;
std::call_once(initialize_cert_, [&]() {
std::lock_guard<std::mutex> guard(ctx_mutex_);
if (!ca_cert_file_path_.empty()) {
if (!SSL_CTX_load_verify_locations(ctx_, ca_cert_file_path_.c_str(),
nullptr)) {
ret = false;
}
} else if (!ca_cert_dir_path_.empty()) {
if (!SSL_CTX_load_verify_locations(ctx_, nullptr,
ca_cert_dir_path_.c_str())) {
ret = false;
}
} else {
#ifdef _WIN32
detail::load_system_certs_on_windows(SSL_CTX_get_cert_store(ctx_));
#else
SSL_CTX_set_default_verify_paths(ctx_);
#endif
}
});
return ret;
}What do you think? |
|
Hey @yhirose, thanks for the example, I kind of missed that. This approach sounds good to me! |
|
@omjego, I just added the unit test and tested it with TEST(SSLClientTest, UpdateCAStore) {
httplib::SSLClient httplib_client("www.google.com");
auto ca_store_1 = X509_STORE_new();
X509_STORE_load_locations(ca_store_1, "/etc/ssl/certs/ca-certificates.crt",
nullptr);
httplib_client.set_ca_cert_store(ca_store_1);
auto ca_store_2 = X509_STORE_new();
X509_STORE_load_locations(ca_store_2, "/etc/ssl/certs/ca-certificates.crt",
nullptr);
httplib_client.set_ca_cert_store(ca_store_2);
} |
|
@yhirose
[ RUN ] SSLClientTest.UpdateCAStore [ OK ] SSLClientServerTest.TrustDirOptional (14 ms) [----------] Global test environment tear-down ==1926963==ERROR: LeakSanitizer: detected memory leaks Indirect leak of 589008 byte(s) in 12692 object(s) allocated from: Indirect leak of 198314 byte(s) in 260 object(s) allocated from: Indirect leak of 175438 byte(s) in 520 object(s) allocated from: Indirect leak of 65292 byte(s) in 3928 object(s) allocated from: Indirect leak of 53466 byte(s) in 520 object(s) allocated from: Indirect leak of 11328 byte(s) in 708 object(s) allocated from: Indirect leak of 10368 byte(s) in 168 object(s) allocated from: Indirect leak of 7488 byte(s) in 72 object(s) allocated from: Indirect leak of 720 byte(s) in 36 object(s) allocated from: Indirect leak of 640 byte(s) in 6 object(s) allocated from: Indirect leak of 214 byte(s) in 24 object(s) allocated from: UMMARY: AddressSanitizer: 1186412 byte(s) leaked in 19454 allocation(s). |
|
@omjego, thanks for the info. It seems like clang does't work with |
|
@omjego, could you also fix the conflicts in your pull request? Then, I'll merge it to the master. Thanks! |
|
@yhirose oh I'm using linux, seems address sanitizer isn't supported on mac. Should I remove the sanitizer option from the makefile or make it conditional based on os? |
|
@omjego, I am ok with the current Makefile, since it doesn't cause any side effect. Thanks for your contribution! |
* Fix memory leak due caused due to X509_STORE * Add test for repro and address sanitizer to compiler flags * Add comment * Sync * Associate ca_store with ssl context within set_ca_cert_store() * Split SlowPost test * Fix yhirose#674 Co-authored-by: yhirose <yuji.hirose.bug@gmail.com>
When X509_STORE objects are associated with a SSL_CTX (ctx_), they will automatically be deleted when the parent SSL_CTX is deleted (in the destructor). However, library lazily sets these in load_certs(), only doing it on a valid HTTP request (after the socket connects inside the call to initialize_ssl()). It causes a memory leak when connecting to an invalid URL, since it can't establish the socket.
The current PR has quick fix for the issue, although not sure if this is the ideal solution. Should we associate the X509_STORE with the ctx_t immediately in set_ca_cert_store() ?