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

[Matthias Andree]

Am 12.02.2017 um 17:35 schrieb Kevin J. McCarthy:
>
> -  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);

This won't work, you'll need to rewrite the while loop:

while (NULL != PEM_read_X509 (fp, &cert, NULL, NULL)) { ... }

because otherwise you'll clobber the old value of 'cert' the moment
PEM_read_X509 returns NULL,
and then you've trashed the only pointer you hold to cert, leaking
memory again.

The attached patch, incremental to my earlier one, achieves this.





exporting patch:
# HG changeset patch
# User Matthias Andree <matthias.and...@gmx.de>
# Date 1486920664 -3600
#      Sun Feb 12 18:31:04 2017 +0100
# Node ID 445386dcae5a017dfbc2a915977a5968c8d44c0a
# Parent  3bf43219f533d517161ca7649d2391c01f329bbf
Reuse cert to avoid free/reallocation overhead.

diff --git a/mutt_ssl.c b/mutt_ssl.c
--- a/mutt_ssl.c
+++ b/mutt_ssl.c
@@ -96,7 +96,7 @@
 static int ssl_load_certificates (SSL_CTX *ctx)
 {
   FILE *fp;
-  X509 *cert;
+  X509 *cert = NULL;
   X509_STORE *store;
   char buf[STRING];
 
@@ -111,7 +111,7 @@
   if ((fp = fopen (SslCertFile, "rt")) == NULL)
     return 0;
 
-  while ((cert = PEM_read_X509 (fp, NULL, NULL, NULL)) != NULL)
+  while (NULL != PEM_read_X509 (fp, &cert, NULL, NULL))
   {
     if ((X509_cmp_current_time (X509_get_notBefore (cert)) >= 0) ||
         (X509_cmp_current_time (X509_get_notAfter (cert)) <= 0))
@@ -123,8 +123,8 @@
     {
       X509_STORE_add_cert (store, cert);
     }
-    X509_free (cert);
   }
+  X509_free (cert);
   safe_fclose (&fp);
 
   return 1;