On Sun, Feb 12, 2017 at 06:31:59PM +0100, Matthias Andree wrote:
> 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.I think it depends whether PEM_read_X509() frees the cert for us in the EOF case. check_certificate_by_digest() has a memory leak if that's not the case, but the new version would double-free if it did. Unfortunately, I can't figure out where that function is defined in the OpenSSL source to verify, and there is scant documentation. I think the safest approach is your first version of the fix, so I'll apply that for now. -- Kevin J. McCarthy GPG Fingerprint: 8975 A9B3 3AA3 7910 385C 5308 ADEF 7684 8031 6BDA
signature.asc
Description: PGP signature
