[xmlsec] Memory leak in xmlSecMSCryptoAppKeysMngrCertLoadMemory

[Aleksey Sanin]

No worries. Thanks a lot!

Aleksey

On 9/20/11 1:56 PM, Satoshi Ito wrote:
> Hi Aleksey,
>
> Our company has a policy against outbound attachments, so I’ll put the
> unified diff in the email body; apologies in advance for any
> inconvenience that causes.
>
> Patch for the memory leak:
>
> --- C:/SVN/3rdparty/xmlsec/xmlsec1-1.2.18/src/mscrypto/app.c Wed May 11
> 16:02:02 2011
>
> +++ C:/SVN/3rdparty/xmlsec/xmlsec1-1.2.18-fix/src/mscrypto/app.c Tue Sep
> 20 12:01:24 2011
>
> @@ -921,6 +921,7 @@
>
> return(-1);
>
> }
>
> + CertFreeCertificateContext(pCert);
>
> return(0);
>
> }
>
> Also, while I’m at it, here is a patch for the HCRYPTPROV handling in
> mscrypto/certkeys.c. One of the cleanup checks for hProv was checking
> for hProv == 0 instead of hProv != 0, and the cleanup needed reordering
> (according to MSDN, CryptDestroyKey should be called before
> CryptReleaseContext).
>
> --- C:/SVN/3rdparty/xmlsec/xmlsec1-1.2.18/src/mscrypto/certkeys.c Wed
> May 11 16:02:02 2011
>
> +++ C:/SVN/3rdparty/xmlsec/xmlsec1-1.2.18-fix/src/mscrypto/certkeys.c
> Tue Sep 20 12:04:37 2011
>
> @@ -1275,12 +1275,12 @@
>
> res = 0;
>
> done:
>
> - if (hProv == 0) {
>
> - CryptReleaseContext(hProv, 0);
>
> - }
>
> if (hKey != 0) {
>
> CryptDestroyKey(hKey);
>
> }
>
> + if (hProv != 0) {
>
> + CryptReleaseContext(hProv, 0);
>
> + }
>
> if (data != 0) {
>
> xmlSecKeyDataDestroy(data);
>
> }
>
> @@ -1519,14 +1519,12 @@
>
> res = 0;
>
> done:
>
> - if (hProv != 0) {
>
> - CryptReleaseContext(hProv, 0);
>
> - }
>
> -
>
> if (hKey != 0) {
>
> CryptDestroyKey(hKey);
>
> }
>
> -
>
> + if (hProv != 0) {
>
> + CryptReleaseContext(hProv, 0);
>
> + }
>
> return(res);
>
> }
>
> @@ -2417,14 +2415,12 @@
>
> res = 0;
>
> done:
>
> - if (hProv != 0) {
>
> - CryptReleaseContext(hProv, 0);
>
> - }
>
> -
>
> if (hKey != 0) {
>
> CryptDestroyKey(hKey);
>
> }
>
> -
>
> + if (hProv != 0) {
>
> + CryptReleaseContext(hProv, 0);
>
> + }
>
> return(res);
>
> }
>
> *From:*Aleksey Sanin [mailto:aleksey at aleksey.com]
> *Sent:* Monday, September 19, 2011 7:47 PM
> *To:* Satoshi Ito
> *Cc:* xmlsec at aleksey.com
> *Subject:* Re: [xmlsec] Memory leak in
> xmlSecMSCryptoAppKeysMngrCertLoadMemory
>
> Hi Satoshi,
>
> This sounds reasonable. Do you mind creating a patch?
>
> Thanks in advance,
>
> Aleksey
>
> On 9/19/11 11:13 AM, Satoshi Ito wrote:
>
> Hello,
>
> I noticed that xmlSecMSCryptoAppKeysMngrCertLoadMemory (v1.2.18
> mscrypto\app.c:867 ) leaks memory on each successful call. This seems to
> be because the PCCERT_CONTEXT constructed from the buffer is freed only
> when xmlSecMSCryptoX509StoreAdoptCert (v1.2.18 mscrypto\x509vfy.c:582)
> fails (according to MSDN, CertAddCertificateContextToStore creates a
> copy of the context and adds the copy to the store, so the original
> PCCERT_CONTEXT is leaked on success). I think you can safely get around
> this by adding CertFreeCertificateContext(pCert) before the return(0) on
> mscrypto\app.c:924, but please correct me if I’m misunderstanding something.
>
> Sincerely,
>
> Satoshi Ito
>
>
>
>
> _______________________________________________
>
> xmlsec mailing list
>
> xmlsec at aleksey.com  <mailto:xmlsec at aleksey.com>
>
> http://www.aleksey.com/mailman/listinfo/xmlsec
>