> On Oct 17, 2016, at 3:53 PM, shinr...@apache.org wrote:
> 
> This is an automated email from the ASF dual-hosted git repository.
> 
> shinrich pushed a commit to branch master
> in repository https://git-dual.apache.org/repos/asf/trafficserver.git
> 
> The following commit(s) were added to refs/heads/master by this push:
>       new  34b5535   TS-4858: fix memory leak problem of 
> global_default_keyblock
> 34b5535 is described below
> 
> commit 34b55356d1cfb38744bb4b4c080fc758e96c6381
> Author: Persia Aziz <per...@yahoo-inc.com>
> AuthorDate: Tue Sep 20 14:40:28 2016 -0500
> 
>    TS-4858: fix memory leak problem of global_default_keyblock
> ---
> iocore/net/P_SSLCertLookup.h |  4 +--
> iocore/net/P_SSLConfig.h     |  6 +++-
> iocore/net/SSLCertLookup.cc  | 70 ++++++++++++++++++++++++++++++++++++++++
> iocore/net/SSLConfig.cc      | 23 ++++++++++++--
> iocore/net/SSLUtils.cc       | 76 +++-----------------------------------------
> proxy/InkAPI.cc              |  1 -
> 6 files changed, 103 insertions(+), 77 deletions(-)
> 
> diff --git a/iocore/net/P_SSLCertLookup.h b/iocore/net/P_SSLCertLookup.h
> index b408626..97c11ff 100644
> --- a/iocore/net/P_SSLCertLookup.h
> +++ b/iocore/net/P_SSLCertLookup.h
> @@ -40,7 +40,6 @@ struct ssl_ticket_key_block {
>   unsigned num_keys;
>   ssl_ticket_key_t keys[];
> };
> -
> /** A certificate context.
> 
>     This holds data about a certificate and how it is used by the SSL logic. 
> Current this is mainly
> @@ -110,5 +109,6 @@ struct SSLCertLookup : public ConfigInfo {
> 
> void ticket_block_free(void *ptr);
> ssl_ticket_key_block *ticket_block_alloc(unsigned count);
> -
> +ssl_ticket_key_block *ticket_block_create(char *ticket_key_data, int 
> ticket_key_len);
> +ssl_ticket_key_block *ssl_create_ticket_keyblock(const char 
> *ticket_key_path);

As noted in the original review, this should be called ticket_block_read() to 
be consistent with the other APIs above.

> #endif /* __P_SSLCERTLOOKUP_H__ */
> diff --git a/iocore/net/P_SSLConfig.h b/iocore/net/P_SSLConfig.h
> index 35fdff1..6adc2b5 100644
> --- a/iocore/net/P_SSLConfig.h
> +++ b/iocore/net/P_SSLConfig.h
> @@ -34,9 +34,11 @@
> #include "ProxyConfig.h"
> #include "SSLSessionCache.h"
> #include "ts/ink_inet.h"
> +#include <openssl/rand.h>
> +#include "P_SSLCertLookup.h"
> 
> struct SSLCertLookup;
> -
> +struct ssl_ticket_key_block;
> /////////////////////////////////////////////////////////////
> //
> // struct SSLConfigParams
> @@ -67,6 +69,8 @@ struct SSLConfigParams : public ConfigInfo {
>   char *dhparamsFile;
>   char *cipherSuite;
>   char *client_cipherSuite;
> +  char *ticket_key_filename;
> +  ssl_ticket_key_block *default_global_keyblock;
>   int configExitOnLoadError;
>   int clientCertLevel;
>   int verify_depth;
> diff --git a/iocore/net/SSLCertLookup.cc b/iocore/net/SSLCertLookup.cc
> index b9d7381..972071d 100644
> --- a/iocore/net/SSLCertLookup.cc
> +++ b/iocore/net/SSLCertLookup.cc
> @@ -28,10 +28,18 @@
> #include "P_SSLConfig.h"
> #include "I_EventSystem.h"
> #include "ts/I_Layout.h"
> +#include "ts/MatcherUtils.h"
> #include "ts/Regex.h"
> #include "ts/Trie.h"
> #include "ts/TestBox.h"
> 
> +// Check if the ticket_key callback #define is available, and if so, enable 
> session tickets.
> +#ifdef SSL_CTX_set_tlsext_ticket_key_cb
> +
> +#define HAVE_OPENSSL_SESSION_TICKETS 1
> +
> +#endif /* SSL_CTX_set_tlsext_ticket_key_cb */
> +
> struct SSLAddressLookupKey {
>   explicit SSLAddressLookupKey(const IpEndpoint &ip) : sep(0)
>   {
> @@ -160,7 +168,69 @@ ticket_block_alloc(unsigned count)
> 
>   return ptr;
> }
> +ssl_ticket_key_block *
> +ticket_block_create(char *ticket_key_data, int ticket_key_len)
> +{
> +  ssl_ticket_key_block *keyblock = NULL;
> +  unsigned num_ticket_keys       = ticket_key_len / sizeof(ssl_ticket_key_t);
> +  if (num_ticket_keys == 0) {
> +    Error("SSL session ticket key is too short (>= 48 bytes are required)");
> +    goto fail;
> +  }
> +
> +  keyblock = ticket_block_alloc(num_ticket_keys);
> 
> +  // Slurp all the keys in the ticket key file. We will encrypt with the 
> first key, and decrypt
> +  // with any key (for rotation purposes).
> +  for (unsigned i = 0; i < num_ticket_keys; ++i) {
> +    const char *data = (const char *)ticket_key_data + (i * 
> sizeof(ssl_ticket_key_t));
> +
> +    memcpy(keyblock->keys[i].key_name, data, 
> sizeof(keyblock->keys[i].key_name));
> +    memcpy(keyblock->keys[i].hmac_secret, data + 
> sizeof(keyblock->keys[i].key_name), sizeof(keyblock->keys[i].hmac_secret));
> +    memcpy(keyblock->keys[i].aes_key, data + 
> sizeof(keyblock->keys[i].key_name) + sizeof(keyblock->keys[i].hmac_secret),
> +           sizeof(keyblock->keys[i].aes_key));
> +  }
> +
> +  return keyblock;
> +
> +fail:
> +  ticket_block_free(keyblock);
> +  return NULL;
> +}
> +
> +ssl_ticket_key_block *
> +ssl_create_ticket_keyblock(const char *ticket_key_path)
> +{
> +#if HAVE_OPENSSL_SESSION_TICKETS
> +  ats_scoped_str ticket_key_data;
> +  int ticket_key_len;
> +  ssl_ticket_key_block *keyblock = NULL;
> +
> +  if (ticket_key_path != NULL) {
> +    ticket_key_data = readIntoBuffer(ticket_key_path, __func__, 
> &ticket_key_len);
> +    if (!ticket_key_data) {
> +      Error("failed to read SSL session ticket key from %s", (const char 
> *)ticket_key_path);
> +      goto fail;
> +    }
> +    keyblock = ticket_block_create(ticket_key_data, ticket_key_len);
> +  } else {
> +    // Generate a random ticket key
> +    ssl_ticket_key_t key;
> +    RAND_bytes(reinterpret_cast<unsigned char *>(&key), sizeof(key));
> +    keyblock = ticket_block_create(reinterpret_cast<char *>(&key), 
> sizeof(key));
> +  }
> +
> +  return keyblock;
> +
> +fail:
> +  ticket_block_free(keyblock);
> +  return NULL;
> +
> +#else  /* !HAVE_OPENSSL_SESSION_TICKETS */
> +  (void)ticket_key_path;
> +  return NULL;
> +#endif /* HAVE_OPENSSL_SESSION_TICKETS */
> +}

Please leave a newline between functions.

> void
> SSLCertContext::release()
> {
> diff --git a/iocore/net/SSLConfig.cc b/iocore/net/SSLConfig.cc
> index 13b6921..f52a1b0 100644
> --- a/iocore/net/SSLConfig.cc
> +++ b/iocore/net/SSLConfig.cc
> @@ -64,12 +64,19 @@ char *SSLConfigParams::ssl_wire_trace_server_name = NULL;
> 
> static ConfigUpdateHandler<SSLCertificateConfig> *sslCertUpdate;
> 
> +// Check if the ticket_key callback #define is available, and if so, enable 
> session tickets.
> +#ifdef SSL_CTX_set_tlsext_ticket_key_cb
> +
> +#define HAVE_OPENSSL_SESSION_TICKETS 1
> +
> +#endif /* SSL_CTX_set_tlsext_ticket_key_cb */
> +
> SSLConfigParams::SSLConfigParams()
> {
>   serverCertPathOnly = serverCertChainFilename = configFilePath = 
> serverCACertFilename = serverCACertPath = clientCertPath =
>     clientKeyPath = clientCACertFilename = clientCACertPath = cipherSuite = 
> client_cipherSuite = dhparamsFile = serverKeyPathOnly =
> -      NULL;
> -
> +      ticket_key_filename                                                    
>                                  = NULL;
> +  default_global_keyblock                                                    
>                                  = NULL;
>   clientCertLevel = client_verify_depth = verify_depth = clientVerify = 0;
> 
>   ssl_ctx_options               = SSL_OP_NO_SSLv2 | SSL_OP_NO_SSLv3;
> @@ -105,6 +112,8 @@ SSLConfigParams::cleanup()
>   ats_free_null(client_cipherSuite);
>   ats_free_null(dhparamsFile);
>   ats_free_null(ssl_wire_trace_ip);
> +  ats_free_null(ticket_key_filename);
> +  ticket_block_free(default_global_keyblock);
> 
>   clientCertLevel = client_verify_depth = verify_depth = clientVerify = 0;
> }
> @@ -243,6 +252,16 @@ SSLConfigParams::initialize()
>   ats_free(ssl_server_ca_cert_filename);
>   ats_free(CACertRelativePath);
> 
> +#if HAVE_OPENSSL_SESSION_TICKETS
> +  REC_ReadConfigStringAlloc(ticket_key_filename, 
> "proxy.config.ssl.server.ticket_key.filename");
> +  if (this->ticket_key_filename != NULL) {
> +    ats_scoped_str 
> ticket_key_path(Layout::relative_to(this->serverCertPathOnly, 
> this->ticket_key_filename));
> +    default_global_keyblock = ssl_create_ticket_keyblock(ticket_key_path);
> +  } else {
> +    default_global_keyblock = ssl_create_ticket_keyblock(NULL);
> +  }
> +#endif
> +
>   // SSL session cache configurations
>   REC_ReadConfigInteger(ssl_session_cache, "proxy.config.ssl.session_cache");
>   REC_ReadConfigInteger(ssl_session_cache_size, 
> "proxy.config.ssl.session_cache.size");
> diff --git a/iocore/net/SSLUtils.cc b/iocore/net/SSLUtils.cc
> index ef44181..ce96222 100644
> --- a/iocore/net/SSLUtils.cc
> +++ b/iocore/net/SSLUtils.cc
> @@ -133,8 +133,7 @@ static int ssl_callback_session_ticket(SSL *, unsigned 
> char *, unsigned char *,
> #endif /* SSL_CTX_set_tlsext_ticket_key_cb */
> 
> #if HAVE_OPENSSL_SESSION_TICKETS
> -static int ssl_session_ticket_index                  = -1;
> -static ssl_ticket_key_block *global_default_keyblock = NULL;
> +static int ssl_session_ticket_index = -1;
> #endif
> 
> static int ssl_vc_index = -1;
> @@ -562,70 +561,15 @@ ssl_context_enable_ecdh(SSL_CTX *ctx)
> }
> 
> static ssl_ticket_key_block *
> -ssl_create_ticket_keyblock(const char *ticket_key_path)
> +ssl_context_enable_tickets(SSL_CTX *ctx, const char *ticket_key_path)
> {
> #if HAVE_OPENSSL_SESSION_TICKETS
> -  ats_scoped_str ticket_key_data;
> -  int ticket_key_len;
> -  unsigned num_ticket_keys;
>   ssl_ticket_key_block *keyblock = NULL;
> -
> -  if (ticket_key_path != NULL) {
> -    ticket_key_data = readIntoBuffer(ticket_key_path, __func__, 
> &ticket_key_len);
> -    if (!ticket_key_data) {
> -      Error("failed to read SSL session ticket key from %s", (const char 
> *)ticket_key_path);
> -      goto fail;
> -    }
> -  } else {
> -    // Generate a random ticket key
> -    ticket_key_len  = 48;
> -    ticket_key_data = (char *)ats_malloc(ticket_key_len);
> -    char *tmp_ptr   = ticket_key_data;
> -    RAND_bytes(reinterpret_cast<unsigned char *>(tmp_ptr), ticket_key_len);
> -  }
> -
> -  num_ticket_keys = ticket_key_len / sizeof(ssl_ticket_key_t);
> -  if (num_ticket_keys == 0) {
> -    Error("SSL session ticket key from %s is too short (>= 48 bytes are 
> required)", (const char *)ticket_key_path);
> -    goto fail;
> -  }
> -
> +  keyblock                       = 
> ssl_create_ticket_keyblock(ticket_key_path);
>   // Increase the stats.
>   if (ssl_rsb != NULL) { // ssl_rsb is not initialized during the first run.
>     SSL_INCREMENT_DYN_STAT(ssl_total_ticket_keys_renewed_stat);
>   }
> -
> -  keyblock = ticket_block_alloc(num_ticket_keys);
> -
> -  // Slurp all the keys in the ticket key file. We will encrypt with the 
> first key, and decrypt
> -  // with any key (for rotation purposes).
> -  for (unsigned i = 0; i < num_ticket_keys; ++i) {
> -    const char *data = (const char *)ticket_key_data + (i * 
> sizeof(ssl_ticket_key_t));
> -
> -    memcpy(keyblock->keys[i].key_name, data, 
> sizeof(keyblock->keys[i].key_name));
> -    memcpy(keyblock->keys[i].hmac_secret, data + 
> sizeof(keyblock->keys[i].key_name), sizeof(keyblock->keys[i].hmac_secret));
> -    memcpy(keyblock->keys[i].aes_key, data + 
> sizeof(keyblock->keys[i].key_name) + sizeof(keyblock->keys[i].hmac_secret),
> -           sizeof(keyblock->keys[i].aes_key));
> -  }
> -
> -  return keyblock;
> -
> -fail:
> -  ticket_block_free(keyblock);
> -  return NULL;
> -
> -#else  /* !HAVE_OPENSSL_SESSION_TICKETS */
> -  (void)ticket_key_path;
> -  return NULL;
> -#endif /* HAVE_OPENSSL_SESSION_TICKETS */
> -}
> -
> -static ssl_ticket_key_block *
> -ssl_context_enable_tickets(SSL_CTX *ctx, const char *ticket_key_path)
> -{
> -#if HAVE_OPENSSL_SESSION_TICKETS
> -  ssl_ticket_key_block *keyblock = NULL;
> -  keyblock                       = 
> ssl_create_ticket_keyblock(ticket_key_path);
>   // Setting the callback can only fail if OpenSSL does not recognize the
>   // SSL_CTRL_SET_TLSEXT_TICKET_KEY_CB constant. we set the callback first
>   // so that we don't leave a ticket_key pointer attached if it fails.
> @@ -2054,22 +1998,11 @@ SSLParseCertificateConfiguration(const 
> SSLConfigParams *params, SSLCertLookup *l
>   char *tok_state = NULL;
>   char *line      = NULL;
>   ats_scoped_str file_buf;
> -  ats_scoped_str ticket_key_filename;
>   unsigned line_num = 0;
>   matcher_line line_info;
> 
>   const matcher_tags sslCertTags = {NULL, NULL, NULL, NULL, NULL, NULL, 
> false};
> 
> -  // Load the global ticket key for later use.
> -  REC_ReadConfigStringAlloc(ticket_key_filename, 
> "proxy.config.ssl.server.ticket_key.filename");
> -
> -  if (ticket_key_filename != NULL) {
> -    ats_scoped_str 
> ticket_key_path(Layout::relative_to(params->serverCertPathOnly, 
> ticket_key_filename));
> -    global_default_keyblock = ssl_create_ticket_keyblock(ticket_key_path);
> -  } else {
> -    global_default_keyblock = ssl_create_ticket_keyblock(NULL);
> -  }
> -
>   Note("loading SSL certificate configuration from %s", 
> params->configFilePath);
> 
>   if (params->configFilePath) {
> @@ -2153,6 +2086,7 @@ ssl_callback_session_ticket(SSL *ssl, unsigned char 
> *keyname, unsigned char *iv,
>                             int enc)
> {
>   SSLCertificateConfig::scoped_config lookup;
> +  SSLConfig::scoped_config params;
>   SSLNetVConnection *netvc = SSLNetVCAccess(ssl);
> 
>   // Get the IP address to look up the keyblock
> @@ -2163,7 +2097,7 @@ ssl_callback_session_ticket(SSL *ssl, unsigned char 
> *keyname, unsigned char *iv,
>   ssl_ticket_key_block *keyblock = NULL;
>   if (cc == NULL || cc->keyblock == NULL) {
>     // Try the default
> -    keyblock = global_default_keyblock;
> +    keyblock = params->default_global_keyblock;
>   } else {
>     keyblock = cc->keyblock;
>   }
> diff --git a/proxy/InkAPI.cc b/proxy/InkAPI.cc
> index 52f8c00..2c92c42 100644
> --- a/proxy/InkAPI.cc
> +++ b/proxy/InkAPI.cc
> @@ -5440,7 +5440,6 @@ TSHttpSsnIncomingAddrGet(TSHttpSsn ssnp)
>   if (vc == NULL) {
>     return 0;
>   }
> -
>   return vc->get_local_addr();
> }
> sockaddr const *
> 
> -- 
> To stop receiving notification emails like this one, please contact
> ['"commits@trafficserver.apache.org" <commits@trafficserver.apache.org>'].

Reply via email to