Re: [PATCH] Correct refcount issues when using client certs in NSS
Date: Sun, 31 May 2009 15:43:30 +0200
On 30 maj 2009, at 18.01, Kamil Dudka wrote:
> Thanks for digging this. In that case we should let NSS libraries to
> CERT_DestroyCertificate() function on the certificate. Then we
> actually no
> more need to hold the reference within the ssl_connect_data. Calling
> of the CERT_DestroyCertificate() function was the only reason to
> hold the
My pleasure =)
I've attached a new patch against CVS that removes the client_cert
member from ssl_connect_data and all its uses. All tests pass as does
my script that uses smartcard certs. Could you test to see that it
doesn't break anything for you?
>> What kind of certificates are leaking memory for you - those loaded
>> from a PEM file or ones that are already in a NSS store?
> PEM file. I'll play with it a bit more next week to find out why.
Ok, I'll do some memory leak checking as well.
>> It's the second connection when it finds the certificate in the
>> session cache that it seg. faults because the cert has been destroyed
>> when we closed the connection last time which is why it's actually
>> CERT_DupCertificate that fails and not CERT_DestroyCertificate.
> Please try the following patch instead of yours if it helps:
> --- nss.c.orig 2009-05-30 17:51:57.857984631 +0200
> +++ nss.c 2009-05-30 17:52:26.569499370 +0200
> @@ -912,8 +912,6 @@ void Curl_nss_close(struct connectdata *
> connssl->client_nickname = NULL;
> - if(connssl->client_cert)
> - CERT_DestroyCertificate(connssl->client_cert);
Yes, it would help. See attached patch.
> The debug log is for the not-working variant? Please make sure the
> function is called in the working variant. If not, it's possible it
> memory in your case as well.
The debug log was for the working variant. The reason it didn't close
the sessions was a bug in the perl bindings that I've fixed so that
curl is properly shutdown when exit - a patch for this has been sent
- application/octet-stream attachment: refcount-client-certs-in-nss-revised.patch