Browse Source

Fix memory leak due caused due to X509_STORE (#671)

* 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 #674

Co-authored-by: yhirose <[email protected]>
Omkar Jadhav 5 years ago
parent
commit
143b2dd15a
3 changed files with 24 additions and 7 deletions
  1. 10 6
      httplib.h
  2. 1 1
      test/Makefile
  3. 13 0
      test/test.cc

+ 10 - 6
httplib.h

@@ -1171,7 +1171,6 @@ private:
 
   std::string ca_cert_file_path_;
   std::string ca_cert_dir_path_;
-  X509_STORE *ca_cert_store_ = nullptr;
   long verify_result_ = 0;
 
   friend class ClientImpl;
@@ -5844,7 +5843,16 @@ inline void SSLClient::set_ca_cert_path(const char *ca_cert_file_path,
 }
 
 inline void SSLClient::set_ca_cert_store(X509_STORE *ca_cert_store) {
-  if (ca_cert_store) { ca_cert_store_ = ca_cert_store; }
+  if (ca_cert_store) {
+    if(ctx_) {
+      if (SSL_CTX_get_cert_store(ctx_) != ca_cert_store) {
+        // Free memory allocated for old cert and use new store `ca_cert_store`
+        SSL_CTX_set_cert_store(ctx_, ca_cert_store);
+      }
+    } else {
+      X509_STORE_free(ca_cert_store);
+    }
+  }
 }
 
 inline long SSLClient::get_openssl_verify_result() const {
@@ -5922,10 +5930,6 @@ inline bool SSLClient::load_certs() {
                                          ca_cert_dir_path_.c_str())) {
         ret = false;
       }
-    } else if (ca_cert_store_ != nullptr) {
-      if (SSL_CTX_get_cert_store(ctx_) != ca_cert_store_) {
-        SSL_CTX_set_cert_store(ctx_, ca_cert_store_);
-      }
     } else {
 #ifdef _WIN32
       detail::load_system_certs_on_windows(SSL_CTX_get_cert_store(ctx_));

+ 1 - 1
test/Makefile

@@ -1,6 +1,6 @@
 
 #CXX = clang++
-CXXFLAGS = -ggdb -O0 -std=c++11 -DGTEST_USE_OWN_TR1_TUPLE -I.. -I. -Wall -Wextra -Wtype-limits -Wconversion
+CXXFLAGS = -ggdb -O0 -std=c++11 -DGTEST_USE_OWN_TR1_TUPLE -I.. -I. -Wall -Wextra -Wtype-limits -Wconversion -fsanitize=address
 
 OPENSSL_DIR = /usr/local/opt/[email protected]
 OPENSSL_SUPPORT = -DCPPHTTPLIB_OPENSSL_SUPPORT -I$(OPENSSL_DIR)/include -L$(OPENSSL_DIR)/lib -lssl -lcrypto

+ 13 - 0
test/test.cc

@@ -3125,6 +3125,19 @@ TEST_F(PayloadMaxLengthTest, ExceedLimit) {
 }
 
 #ifdef CPPHTTPLIB_OPENSSL_SUPPORT
+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);
+}
+
 TEST(SSLClientTest, ServerNameIndication) {
   SSLClient cli("httpbin.org", 443);
   auto res = cli.Get("/get");