You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@trafficserver.apache.org by so...@apache.org on 2015/06/02 17:19:05 UTC
trafficserver git commit: TS-3554: * Memory load reloading
ssl_multicert.config. * Fix coverity discovered memory leak. * Another memory
leak fix in the SSL cert loading. Specifically the ticket key blocks. * Had
to rearrange functions so the tes
Repository: trafficserver
Updated Branches:
refs/heads/5.3.x 62be18134 -> 258afd347
TS-3554:
* Memory load reloading ssl_multicert.config.
* Fix coverity discovered memory leak.
* Another memory leak fix in the SSL cert loading. Specifically the ticket key blocks.
* Had to rearrange functions so the test_certlookup program would link with additional release method.
(cherry picked from commit 98c87ee51b2ad91787b7a9fa2827cab1c03b3d22)
(cherry picked from commit 86a3a1464c32b24f02e2633906b93916f0080536)
(cherry picked from commit 88c5531b2dccbf2378b7e518aa946ddf5f88c72a)
(cherry picked from commit 29d72d393aa31950a173e43c26f0798efdc77127)
Project: http://git-wip-us.apache.org/repos/asf/trafficserver/repo
Commit: http://git-wip-us.apache.org/repos/asf/trafficserver/commit/258afd34
Tree: http://git-wip-us.apache.org/repos/asf/trafficserver/tree/258afd34
Diff: http://git-wip-us.apache.org/repos/asf/trafficserver/diff/258afd34
Branch: refs/heads/5.3.x
Commit: 258afd3479710bdbe55eca6a75d638c6fa0cb936
Parents: 62be181
Author: shinrich <sh...@yahoo-inc.com>
Authored: Thu Apr 23 14:42:39 2015 -0500
Committer: Phil Sorber <so...@apache.org>
Committed: Tue Jun 2 09:14:22 2015 -0600
----------------------------------------------------------------------
CHANGES | 2 ++
iocore/net/P_SSLCertLookup.h | 4 ++++
iocore/net/SSLCertLookup.cc | 41 +++++++++++++++++++++++++++++++++-
iocore/net/SSLConfig.cc | 11 +++++++--
iocore/net/SSLUtils.cc | 47 ++++++++++-----------------------------
mgmt/ProxyConfig.cc | 4 ++++
6 files changed, 71 insertions(+), 38 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/trafficserver/blob/258afd34/CHANGES
----------------------------------------------------------------------
diff --git a/CHANGES b/CHANGES
index 1fbea51..18a0c80 100644
--- a/CHANGES
+++ b/CHANGES
@@ -1,6 +1,8 @@
-*- coding: utf-8 -*-
Changes with Apache Traffic Server 5.3.1
+ *) [TS-3554] Memory leak on ssl_multicert.config reload.
+
*) [TS-3649] url_sig: fix for crasher related to key index.
Author: Gancho Tenev <gt...@gmail.com>
http://git-wip-us.apache.org/repos/asf/trafficserver/blob/258afd34/iocore/net/P_SSLCertLookup.h
----------------------------------------------------------------------
diff --git a/iocore/net/P_SSLCertLookup.h b/iocore/net/P_SSLCertLookup.h
index a6c3629..b3591ec 100644
--- a/iocore/net/P_SSLCertLookup.h
+++ b/iocore/net/P_SSLCertLookup.h
@@ -66,6 +66,7 @@ struct SSLCertContext {
explicit SSLCertContext(SSL_CTX *c) : ctx(c), opt(OPT_NONE), keyblock(NULL) {}
SSLCertContext(SSL_CTX *c, Option o) : ctx(c), opt(o), keyblock(NULL) {}
SSLCertContext(SSL_CTX *c, Option o, ssl_ticket_key_block *kb) : ctx(c), opt(o), keyblock(kb) {}
+ void release();
SSL_CTX *ctx; ///< openSSL context.
Option opt; ///< Special handling option.
@@ -108,4 +109,7 @@ struct SSLCertLookup : public ConfigInfo {
virtual ~SSLCertLookup();
};
+void ticket_block_free(void *ptr);
+ssl_ticket_key_block *ticket_block_alloc(unsigned count);
+
#endif /* __P_SSLCERTLOOKUP_H__ */
http://git-wip-us.apache.org/repos/asf/trafficserver/blob/258afd34/iocore/net/SSLCertLookup.cc
----------------------------------------------------------------------
diff --git a/iocore/net/SSLCertLookup.cc b/iocore/net/SSLCertLookup.cc
index 28755ba..d6b76bc 100644
--- a/iocore/net/SSLCertLookup.cc
+++ b/iocore/net/SSLCertLookup.cc
@@ -133,6 +133,45 @@ private:
int store(SSLCertContext const &cc);
};
+// Zero out and free the heap space allocated for ticket keys to avoid leaking secrets.
+// The first several bytes stores the number of keys and the rest stores the ticket keys.
+void
+ticket_block_free(void *ptr)
+{
+ if (ptr) {
+ ssl_ticket_key_block *key_block_ptr = (ssl_ticket_key_block *)ptr;
+ unsigned num_ticket_keys = key_block_ptr->num_keys;
+ memset(ptr, 0, sizeof(ssl_ticket_key_block) + num_ticket_keys * sizeof(ssl_ticket_key_t));
+ }
+ ats_free(ptr);
+}
+
+ssl_ticket_key_block *
+ticket_block_alloc(unsigned count)
+{
+ ssl_ticket_key_block *ptr;
+ size_t nbytes = sizeof(ssl_ticket_key_block) + count * sizeof(ssl_ticket_key_t);
+
+ ptr = (ssl_ticket_key_block *)ats_malloc(nbytes);
+ memset(ptr, 0, nbytes);
+ ptr->num_keys = count;
+
+ return ptr;
+}
+
+void
+SSLCertContext::release()
+{
+ if (keyblock) {
+ ticket_block_free(keyblock);
+ keyblock = NULL;
+ }
+ if (ctx) {
+ SSL_CTX_free(ctx);
+ ctx = NULL;
+ }
+}
+
SSLCertLookup::SSLCertLookup() : ssl_storage(new SSLContextStorage()), ssl_default(NULL), is_valid(true)
{
}
@@ -265,7 +304,7 @@ SSLContextStorage::~SSLContextStorage()
for (unsigned i = 0; i < this->ctx_store.length(); ++i) {
if (this->ctx_store[i].ctx != last_ctx) {
last_ctx = this->ctx_store[i].ctx;
- SSLReleaseContext(this->ctx_store[i].ctx);
+ this->ctx_store[i].release();
}
}
http://git-wip-us.apache.org/repos/asf/trafficserver/blob/258afd34/iocore/net/SSLConfig.cc
----------------------------------------------------------------------
diff --git a/iocore/net/SSLConfig.cc b/iocore/net/SSLConfig.cc
index 669e1c1..0c8bd56 100644
--- a/iocore/net/SSLConfig.cc
+++ b/iocore/net/SSLConfig.cc
@@ -333,7 +333,7 @@ SSLCertificateConfig::startup()
// Exit if there are problems on the certificate loading and the
// proxy.config.ssl.server.multicert.exit_on_load_fail is true
- SSLConfigParams *params = SSLConfig::acquire();
+ SSLConfig::scoped_config params;
if (!reconfigure() && params->configExitOnLoadError) {
Error("Problems loading ssl certificate file, %s. Exiting.", params->configFilePath);
_exit(1);
@@ -357,10 +357,17 @@ SSLCertificateConfig::reconfigure()
}
SSLParseCertificateConfiguration(params, lookup);
- configid = configProcessor.set(configid, lookup);
+
if (!lookup->is_valid) {
retStatus = false;
}
+ // If there are errors in the certificate configs and we had wanted to exit on error
+ // we won't want to reset the config
+ if (lookup->is_valid || !params->configExitOnLoadError) {
+ configid = configProcessor.set(configid, lookup);
+ } else {
+ delete lookup;
+ }
return retStatus;
}
http://git-wip-us.apache.org/repos/asf/trafficserver/blob/258afd34/iocore/net/SSLUtils.cc
----------------------------------------------------------------------
diff --git a/iocore/net/SSLUtils.cc b/iocore/net/SSLUtils.cc
index 75a44a7..76727c5 100644
--- a/iocore/net/SSLUtils.cc
+++ b/iocore/net/SSLUtils.cc
@@ -116,35 +116,9 @@ static int ssl_callback_session_ticket(SSL *, unsigned char *, unsigned char *,
#if HAVE_OPENSSL_SESSION_TICKETS
static int ssl_session_ticket_index = -1;
+#endif
-// Zero out and free the heap space allocated for ticket keys to avoid leaking secrets.
-// The first several bytes stores the number of keys and the rest stores the ticket keys.
-static void
-ticket_block_free(void *ptr)
-{
- if (ptr) {
- ssl_ticket_key_block *key_block_ptr = (ssl_ticket_key_block *)ptr;
- unsigned num_ticket_keys = key_block_ptr->num_keys;
- memset(ptr, 0, sizeof(ssl_ticket_key_block) + num_ticket_keys * sizeof(ssl_ticket_key_t));
- }
- ats_free(ptr);
-}
-
-static ssl_ticket_key_block *
-ticket_block_alloc(unsigned count)
-{
- ssl_ticket_key_block *ptr;
- size_t nbytes = sizeof(ssl_ticket_key_block) + count * sizeof(ssl_ticket_key_t);
-
- ptr = (ssl_ticket_key_block *)ats_malloc(nbytes);
- memset(ptr, 0, nbytes);
- ptr->num_keys = count;
-
- return ptr;
-}
-
-#endif
static pthread_mutex_t *mutex_buf = NULL;
static bool open_ssl_initialized = false;
@@ -267,7 +241,7 @@ set_context_cert(SSL *ssl)
{
SSL_CTX *ctx = NULL;
SSLCertContext *cc = NULL;
- SSLCertLookup *lookup = SSLCertificateConfig::acquire();
+ SSLCertificateConfig::scoped_config lookup;
const char *servername = SSL_get_servername(ssl, TLSEXT_NAMETYPE_host_name);
SSLNetVConnection *netvc = (SSLNetVConnection *)SSL_get_app_data(ssl);
bool found = true;
@@ -326,7 +300,6 @@ set_context_cert(SSL *ssl)
goto done;
}
done:
- SSLCertificateConfig::release(lookup);
return retval;
}
@@ -1682,6 +1655,14 @@ ssl_store_ssl_context(const SSLConfigParams *params, SSLCertLookup *lookup, cons
}
}
}
+ if (!inserted) {
+#if HAVE_OPENSSL_SESSION_TICKETS
+ if (keyblock != NULL) {
+ ticket_block_free(keyblock);
+ }
+#endif
+ }
+
#if defined(SSL_OP_NO_TICKET)
// Session tickets are enabled by default. Disable if explicitly requested.
@@ -1721,11 +1702,6 @@ ssl_store_ssl_context(const SSLConfigParams *params, SSLCertLookup *lookup, cons
}
}
if (!inserted) {
-#if HAVE_OPENSSL_SESSION_TICKETS
- if (keyblock != NULL) {
- ticket_block_free(keyblock);
- }
-#endif
if (ctx != NULL) {
SSL_CTX_free(ctx);
ctx = NULL;
@@ -1886,7 +1862,7 @@ static int
ssl_callback_session_ticket(SSL *ssl, unsigned char *keyname, unsigned char *iv, EVP_CIPHER_CTX *cipher_ctx, HMAC_CTX *hctx,
int enc)
{
- SSLCertLookup *lookup = SSLCertificateConfig::acquire();
+ SSLCertificateConfig::scoped_config lookup;
SSLNetVConnection *netvc = (SSLNetVConnection *)SSL_get_app_data(ssl);
// Get the IP address to look up the keyblock
@@ -1901,6 +1877,7 @@ ssl_callback_session_ticket(SSL *ssl, unsigned char *keyname, unsigned char *iv,
if (cc == NULL || cc->keyblock == NULL) {
// No, key specified. Must fail out at this point.
// Alternatively we could generate a random key
+
return -1;
}
ssl_ticket_key_block *keyblock = cc->keyblock;
http://git-wip-us.apache.org/repos/asf/trafficserver/blob/258afd34/mgmt/ProxyConfig.cc
----------------------------------------------------------------------
diff --git a/mgmt/ProxyConfig.cc b/mgmt/ProxyConfig.cc
index bd06063..09545e2 100644
--- a/mgmt/ProxyConfig.cc
+++ b/mgmt/ProxyConfig.cc
@@ -154,6 +154,9 @@ ConfigProcessor::set(unsigned int id, ConfigInfo *info, unsigned timeout_secs)
old_info = infos[idx];
} while (!ink_atomic_cas(&infos[idx], old_info, info));
+ Debug("config", "Set for slot %d 0x%" PRId64 " was 0x%" PRId64 " with ref count %d", id, (int64_t)info, (int64_t)old_info,
+ (old_info) ? old_info->refcount() : 0);
+
if (old_info) {
// The ConfigInfoReleaser now takes our refcount, but
// someother thread might also have one ...
@@ -204,6 +207,7 @@ ConfigProcessor::release(unsigned int id, ConfigInfo *info)
if (info->refcount_dec() == 0) {
// When we release, we should already have replaced this object in the index.
+ Debug("config", "Release config %d 0x%" PRId64, id, (int64_t)info);
ink_release_assert(info != this->infos[idx]);
delete info;
}