Re: PATCH: plug memory leaks in new code to filter expired certs from $certificate_file for OpenSSL verification

[Kevin J. McCarthy]

On Sun, Feb 12, 2017 at 01:42:02PM +0100, Matthias Andree wrote:
> Am 12.02.2017 um 13:23 schrieb Matthias Andree:
> > All this certificate handling apparently introduces memory leaks. I
> > first tried to get a hold of them with clang's address sanitizer, which
> > seems somehow handicapped on Ubuntu 16.04, but valgrind seems useful
> > enough even if it hogs down performance even more.
> 
> Got it. The attached patch plugs the leak. If you use
> X509_STORE_add_cert(), it makes a copy of the certificate we are
> offering it, so we need to X509_free it afterwards.

Excellent.  Thanks for tracking that down - I should have looked more
deeply to see if it was copying the cert or not.

The PEM_read_X509() actually seems to be able to reuse the cert if we
pass it as the second parameter: see check_certificate_by_digest().
Using that same logic, how about if fold this into the original patch
instead:

diff --git a/mutt_ssl.c b/mutt_ssl.c
--- a/mutt_ssl.c
+++ b/mutt_ssl.c
@@ -90,43 +90,45 @@
  * certs loaded into the trusted store.  This function filters out expired
  * certs.
  * Previously the code used this form:
  *     SSL_CTX_load_verify_locations (ssldata->ctx, SslCertFile, NULL);
  */
 static int ssl_load_certificates (SSL_CTX *ctx)
 {
   FILE *fp;
-  X509 *cert;
+  X509 *cert = NULL;
   X509_STORE *store;
   char buf[STRING];
 
   dprint (2, (debugfile, "ssl_load_certificates: loading trusted 
certificates\n"));
   store = SSL_CTX_get_cert_store (ctx);
   if (!store)
   {
     store = X509_STORE_new ();
     SSL_CTX_set_cert_store (ctx, store);
   }
 
   if ((fp = fopen (SslCertFile, "rt")) == NULL)
     return 0;
 
-  while ((cert = PEM_read_X509 (fp, NULL, NULL, NULL)) != NULL)
+  while ((cert = PEM_read_X509 (fp, &cert, NULL, NULL)) != NULL)
   {
     if ((X509_cmp_current_time (X509_get_notBefore (cert)) >= 0) ||
         (X509_cmp_current_time (X509_get_notAfter (cert)) <= 0))
     {
       dprint (2, (debugfile, "ssl_load_certificates: filtering expired cert: 
%s\n",
               X509_NAME_oneline (X509_get_subject_name (cert), buf, sizeof 
(buf))));
-      X509_free (cert);
     }
     else
+    {
       X509_STORE_add_cert (store, cert);
+    }
   }
+  X509_free (cert);
   safe_fclose (&fp);
 
   return 1;
 }
 
 /* mutt_ssl_starttls: Negotiate TLS over an already opened connection.
  *   TODO: Merge this code better with ssl_socket_open. */
 int mutt_ssl_starttls (CONNECTION* conn)

-- 
Kevin J. McCarthy
GPG Fingerprint: 8975 A9B3 3AA3 7910 385C  5308 ADEF 7684 8031 6BDA

signature.asc
Description: PGP signature