From 235c0077b8648a3b941090991a0ce6ac24d681ab Mon Sep 17 00:00:00 2001 From: Daniel Stenberg Date: Mon, 13 Apr 2009 17:42:10 +0000 Subject: [PATCH] - Toshio Kuratomi reported a memory leak problem with libcurl+NSS that turned out to be leaking cacerts. Kamil Dudka helped me complete the fix. The issue is found in Redhat's bug tracker: https://bugzilla.redhat.com/show_bug.cgi?id=453612 There are still memory leaks present, but they seem to have other reasons. --- CHANGES | 8 ++++++++ RELEASE-NOTES | 3 ++- lib/nss.c | 45 +++++++++++++++++++++++++++++---------------- lib/urldata.h | 5 +++++ 4 files changed, 44 insertions(+), 17 deletions(-) diff --git a/CHANGES b/CHANGES index 369c6642a..8df917b5f 100644 --- a/CHANGES +++ b/CHANGES @@ -6,6 +6,14 @@ Changelog +Daniel Stenberg (13 Apr 2009) +- Toshio Kuratomi reported a memory leak problem with libcurl+NSS that turned + out to be leaking cacerts. Kamil Dudka helped me complete the fix. The issue + is found in Redhat's bug tracker: + https://bugzilla.redhat.com/show_bug.cgi?id=453612 + + There are still memory leaks present, but they seem to have other reasons. + Daniel Fandrich (11 Apr 2009) - Added new libcurl source files to Symbian OS build files. - Improved Symbian support for SSL. diff --git a/RELEASE-NOTES b/RELEASE-NOTES index 2d67dda86..8d42a4665 100644 --- a/RELEASE-NOTES +++ b/RELEASE-NOTES @@ -26,6 +26,7 @@ This release includes the following bugfixes: o properly return an error code in curl_easy_recv o Sun compilers specific preprocessor block removed from curlbuild.h.dist o allow creation of four way fat libcurl Mac OS X Framework + o memory leaks in libcurl+NSS This release includes the following known bugs: @@ -36,6 +37,6 @@ advice from friends like these: Daniel Fandrich, Yang Tse, David James, Chris Deidun, Bill Egert, Andre Guibert de Bruet, Andreas Farber, Frank Hempel, Pierre Brico, - Kamil Dudka, Jim Freeman, Daniel Johnson + Kamil Dudka, Jim Freeman, Daniel Johnson, Toshio Kuratomi Thanks! (and sorry if I forgot to mention someone) diff --git a/lib/nss.c b/lib/nss.c index 373c28390..6e0a91bfd 100644 --- a/lib/nss.c +++ b/lib/nss.c @@ -282,13 +282,12 @@ static int is_file(const char *filename) return 0; } -static int -nss_load_cert(const char *filename, PRBool cacert) +static int nss_load_cert(struct ssl_connect_data *ssl, + const char *filename, PRBool cacert) { #ifdef HAVE_PK11_CREATEGENERICOBJECT CK_SLOT_ID slotID; PK11SlotInfo * slot = NULL; - PK11GenericObject *rv; CK_ATTRIBUTE *attrs; CK_ATTRIBUTE theTemplate[20]; CK_BBOOL cktrue = CK_TRUE; @@ -363,11 +362,12 @@ nss_load_cert(const char *filename, PRBool cacert) /* This load the certificate in our PEM module into the appropriate * slot. */ - rv = PK11_CreateGenericObject(slot, theTemplate, 4, PR_FALSE /* isPerm */); + ssl->cacert[slotID] = PK11_CreateGenericObject(slot, theTemplate, 4, + PR_FALSE /* isPerm */); PK11_FreeSlot(slot); - if(rv == NULL) { + if(ssl->cacert == NULL) { free(nickname); return 0; } @@ -474,11 +474,10 @@ static int nss_load_crl(const char* crlfilename, PRBool ascii) return 1; } -static int nss_load_key(struct connectdata *conn, char *key_file) +static int nss_load_key(struct connectdata *conn, int sockindex, char *key_file) { #ifdef HAVE_PK11_CREATEGENERICOBJECT PK11SlotInfo * slot = NULL; - PK11GenericObject *rv; CK_ATTRIBUTE *attrs; CK_ATTRIBUTE theTemplate[20]; CK_BBOOL cktrue = CK_TRUE; @@ -486,6 +485,7 @@ static int nss_load_key(struct connectdata *conn, char *key_file) CK_SLOT_ID slotID; pphrase_arg_t *parg = NULL; char slotname[SLOTSIZE]; + struct ssl_connect_data *sslconn = &conn->ssl[sockindex]; attrs = theTemplate; @@ -505,8 +505,9 @@ static int nss_load_key(struct connectdata *conn, char *key_file) strlen(key_file)+1); attrs++; /* When adding an encrypted key the PKCS#11 will be set as removed */ - rv = PK11_CreateGenericObject(slot, theTemplate, 3, PR_FALSE /* isPerm */); - if(rv == NULL) { + sslconn->key = PK11_CreateGenericObject(slot, theTemplate, 3, + PR_FALSE /* isPerm */); + if(sslconn->key == NULL) { PR_SetError(SEC_ERROR_BAD_KEY, 0); return 0; } @@ -554,13 +555,14 @@ static int display_error(struct connectdata *conn, PRInt32 err, return 0; /* The caller will print a generic error */ } -static int cert_stuff(struct connectdata *conn, char *cert_file, char *key_file) +static int cert_stuff(struct connectdata *conn, + int sockindex, char *cert_file, char *key_file) { struct SessionHandle *data = conn->data; int rv = 0; if(cert_file) { - rv = nss_load_cert(cert_file, PR_FALSE); + rv = nss_load_cert(&conn->ssl[sockindex], cert_file, PR_FALSE); if(!rv) { if(!display_error(conn, PR_GetError(), cert_file)) failf(data, "Unable to load client cert %d.", PR_GetError()); @@ -569,10 +571,10 @@ static int cert_stuff(struct connectdata *conn, char *cert_file, char *key_file) } if(key_file || (is_file(cert_file))) { if(key_file) - rv = nss_load_key(conn, key_file); + rv = nss_load_key(conn, sockindex, key_file); else /* In case the cert file also has the key */ - rv = nss_load_key(conn, cert_file); + rv = nss_load_key(conn, sockindex, cert_file); if(!rv) { if(!display_error(conn, PR_GetError(), key_file)) failf(data, "Unable to load client key %d.", PR_GetError()); @@ -938,6 +940,12 @@ void Curl_nss_close(struct connectdata *conn, int sockindex) free(connssl->client_nickname); connssl->client_nickname = NULL; } + if(connssl->key) + (void)PK11_DestroyGenericObject(connssl->key); + if(connssl->cacert[1]) + (void)PK11_DestroyGenericObject(connssl->cacert[1]); + if(connssl->cacert[0]) + (void)PK11_DestroyGenericObject(connssl->cacert[0]); connssl->handle = NULL; } } @@ -973,6 +981,10 @@ CURLcode Curl_nss_connect(struct connectdata *conn, int sockindex) if (connssl->state == ssl_connection_complete) return CURLE_OK; + connssl->cacert[0] = NULL; + connssl->cacert[1] = NULL; + connssl->key = NULL; + /* FIXME. NSS doesn't support multiple databases open at the same time. */ PR_Lock(nss_initlock); if(!initialized) { @@ -1100,7 +1112,8 @@ CURLcode Curl_nss_connect(struct connectdata *conn, int sockindex) /* skip the verifying of the peer */ ; else if(data->set.ssl.CAfile) { - int rc = nss_load_cert(data->set.ssl.CAfile, PR_TRUE); + int rc = nss_load_cert(&conn->ssl[sockindex], data->set.ssl.CAfile, + PR_TRUE); if(!rc) { curlerr = CURLE_SSL_CACERT_BADFILE; goto error; @@ -1128,7 +1141,7 @@ CURLcode Curl_nss_connect(struct connectdata *conn, int sockindex) snprintf(fullpath, sizeof(fullpath), "%s/%s", data->set.ssl.CApath, entry->name); - rc = nss_load_cert(fullpath, PR_TRUE); + rc = nss_load_cert(&conn->ssl[sockindex], fullpath, PR_TRUE); /* FIXME: check this return value! */ } /* This is purposefully tolerant of errors so non-PEM files @@ -1178,7 +1191,7 @@ CURLcode Curl_nss_connect(struct connectdata *conn, int sockindex) free(nickname); goto error; } - if(!cert_stuff(conn, data->set.str[STRING_CERT], + if(!cert_stuff(conn, sockindex, data->set.str[STRING_CERT], data->set.str[STRING_KEY])) { /* failf() is already done in cert_stuff() */ if(nickname_alloc) diff --git a/lib/urldata.h b/lib/urldata.h index a50ede005..60742e5b9 100644 --- a/lib/urldata.h +++ b/lib/urldata.h @@ -93,6 +93,7 @@ #ifdef USE_NSS #include +#include #endif #ifdef USE_QSOSSL @@ -210,6 +211,10 @@ struct ssl_connect_data { #ifdef USE_NSS PRFileDesc *handle; char *client_nickname; +#ifdef HAVE_PK11_CREATEGENERICOBJECT + PK11GenericObject *key; + PK11GenericObject *cacert[2]; +#endif #endif /* USE_NSS */ #ifdef USE_QSOSSL SSLHandle *handle;