You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@trafficserver.apache.org by persiaAziz <gi...@git.apache.org> on 2016/09/15 17:56:39 UTC
[GitHub] trafficserver pull request #1024: TS-4858: fix memory leak of global_default...
GitHub user persiaAziz opened a pull request:
https://github.com/apache/trafficserver/pull/1024
TS-4858: fix memory leak of global_default_keyblock
fix memory leak of global_default_keyblock.
global_defualt_keyblock will be freed and reassigned on each reload
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/persiaAziz/trafficserver TS-4858
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/trafficserver/pull/1024.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #1024
----
commit 58b325b0ab8861dbb3e388b044010eb6bcab4756
Author: Persia Aziz <pe...@yahoo-inc.com>
Date: 2016-09-15T17:46:15Z
TS-4858: fix memory leak of global_default_keyblock
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #1024: TS-4858: fix memory leak of global_default_keyblo...
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/1024
Linux build *successful*! See https://ci.trafficserver.apache.org/job/Github-Linux/709/ for details.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #1024: TS-4858: fix memory leak of global_default_keyblo...
Posted by shinrich <gi...@git.apache.org>.
Github user shinrich commented on the issue:
https://github.com/apache/trafficserver/pull/1024
[approve ci]
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #1024: TS-4858: fix memory leak of global_default_keyblo...
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/1024
FreeBSD build *failed*! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/1016/ for details.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #1024: TS-4858: fix memory leak of global_default_keyblo...
Posted by persiaAziz <gi...@git.apache.org>.
Github user persiaAziz commented on the issue:
https://github.com/apache/trafficserver/pull/1024
pointer check removed
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request #1024: TS-4858: fix memory leak of global_default...
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/1024#discussion_r82047142
--- Diff: iocore/net/SSLCertLookup.cc ---
@@ -160,7 +160,40 @@ ticket_block_alloc(unsigned count)
return ptr;
}
+ssl_ticket_key_block *
+ssl_create_ticket_key_block_buffer(char *ticket_key_data, int ticket_key_len)
+{
+ ssl_ticket_key_block *keyblock = NULL;
+ int 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;
+ }
+
+ // 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);
+ }
--- End diff --
Loading a ticket key should not have any side-effects on metrics. Since this was existing code, please file a new Jira to clean this up.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request #1024: TS-4858: fix memory leak of global_default...
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/1024#discussion_r82048013
--- Diff: iocore/net/SSLUtils.cc ---
@@ -2149,6 +2112,7 @@ ssl_callback_session_ticket(SSL *ssl, unsigned char *keyname, unsigned char *iv,
int enc)
{
SSLCertificateConfig::scoped_config lookup;
+ SSLConfig::scoped_config params;
--- End diff --
Move this inside keyblock check so that we don't try to acquire the config unless we need it.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #1024: TS-4858: fix memory leak of global_default_keyblo...
Posted by shinrich <gi...@git.apache.org>.
Github user shinrich commented on the issue:
https://github.com/apache/trafficserver/pull/1024
Looks good. Persia addressed Leif and James' comments.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #1024: TS-4858: fix memory leak of global_default_keyblo...
Posted by shinrich <gi...@git.apache.org>.
Github user shinrich commented on the issue:
https://github.com/apache/trafficserver/pull/1024
[approve ci]
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request #1024: TS-4858: fix memory leak of global_default...
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/1024#discussion_r82046421
--- Diff: iocore/net/SSLConfig.cc ---
@@ -243,6 +245,21 @@ SSLConfigParams::initialize()
ats_free(ssl_server_ca_cert_filename);
ats_free(CACertRelativePath);
+ REC_ReadConfigStringAlloc(ticket_key_filename, "proxy.config.ssl.server.ticket_key.filename");
--- End diff --
Add a newline to separate logically distinct code.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #1024: TS-4858: fix memory leak of global_default_keyblo...
Posted by shinrich <gi...@git.apache.org>.
Github user shinrich commented on the issue:
https://github.com/apache/trafficserver/pull/1024
[approve ci]
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #1024: TS-4858: fix memory leak of global_default_keyblo...
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/1024
Linux build *failed*! See https://ci.trafficserver.apache.org/job/Github-Linux/830/ for details.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #1024: TS-4858: fix memory leak of global_default_keyblo...
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/1024
FreeBSD build *failed*! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/1018/ for details.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request #1024: TS-4858: fix memory leak of global_default...
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/1024#discussion_r82625751
--- Diff: iocore/net/SSLCertLookup.cc ---
@@ -160,7 +160,40 @@ ticket_block_alloc(unsigned count)
return ptr;
}
+ssl_ticket_key_block *
+ssl_create_ticket_key_block_buffer(char *ticket_key_data, int ticket_key_len)
+{
+ ssl_ticket_key_block *keyblock = NULL;
+ int num_ticket_keys = ticket_key_len / sizeof(ssl_ticket_key_t);
--- End diff --
As per the compiler warning ``num_ticket_keys`` and ``ticket_key_len`` should both be ``size_t``.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #1024: TS-4858: fix memory leak of global_default_keyblo...
Posted by persiaAziz <gi...@git.apache.org>.
Github user persiaAziz commented on the issue:
https://github.com/apache/trafficserver/pull/1024
Please review @shinrich
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #1024: TS-4858: fix memory leak of global_default_keyblo...
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/1024
FreeBSD build *failed*! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/938/ for details.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request #1024: TS-4858: fix memory leak of global_default...
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/1024#discussion_r82044347
--- Diff: iocore/net/P_SSLCertLookup.h ---
@@ -110,5 +109,5 @@ struct SSLCertLookup : public ConfigInfo {
void ticket_block_free(void *ptr);
ssl_ticket_key_block *ticket_block_alloc(unsigned count);
-
+ssl_ticket_key_block *ssl_create_ticket_key_block_buffer(char *ticket_key_data, int ticket_key_len);
--- End diff --
Please use a consistent naming convention. We have``ticket_block_free()``, ``ticket_block_alloc()``, so consider ``ticket_block_create()`` or something.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #1024: TS-4858: fix memory leak of global_default_keyblo...
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/1024
Linux build *failed*! See https://ci.trafficserver.apache.org/job/Github-Linux/908/ for details.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #1024: TS-4858: fix memory leak of global_default_keyblo...
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/1024
Linux build *failed*! See https://ci.trafficserver.apache.org/job/Github-Linux/910/ for details.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #1024: TS-4858: fix memory leak of global_default_keyblo...
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/1024
FreeBSD build *successful*! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/813/ for details.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #1024: TS-4858: fix memory leak of global_default_keyblo...
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/1024
Linux build *successful*! See https://ci.trafficserver.apache.org/job/Github-Linux/926/ for details.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request #1024: TS-4858: fix memory leak of global_default...
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/1024#discussion_r79039044
--- Diff: iocore/net/SSLUtils.cc ---
@@ -2052,6 +2052,7 @@ SSLParseCertificateConfiguration(const SSLConfigParams *params, SSLCertLookup *l
// Load the global ticket key for later use.
REC_ReadConfigStringAlloc(ticket_key_filename, "proxy.config.ssl.server.ticket_key.filename");
+ ticket_block_free(global_default_keyblock);
--- End diff --
Unfortunately this is not going to work correctly. At the same time you have frees the ticket block here, other threads are accessing it to perform SSL handshaking, so they will read a stale pointer.
One way to fix this is to make the ticket block part of the ``SSLCertLookup`` object and then point the global pointer that that object. That might get a little nasty since updating the certificate lookup can fail after you assign the global pointer. You might need to find a different way.
In general, I think what needs to happen here is:
- Only update the ticket block if it has changed. Updating is each time will reduce the possibility of SSL session resumption.
- If you update the ticket block, swap the pointers to the old and new blocks atomically, then schedule the old block for deletion
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request #1024: TS-4858: fix memory leak of global_default...
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/1024#discussion_r82625369
--- Diff: iocore/net/SSLConfig.cc ---
@@ -243,6 +245,21 @@ SSLConfigParams::initialize()
ats_free(ssl_server_ca_cert_filename);
ats_free(CACertRelativePath);
+ REC_ReadConfigStringAlloc(ticket_key_filename, "proxy.config.ssl.server.ticket_key.filename");
--- End diff --
The purpose of the newline is to break the code into "paragraphs" that make it easier to read. Getting a value from config is distinct from using that value for a particular purpose, so it deserves a new paragraph. Make it easy for the reader.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #1024: TS-4858: fix memory leak of global_default_keyblo...
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/1024
FreeBSD build *successful*! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/1034/ for details.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #1024: TS-4858: fix memory leak of global_default_keyblo...
Posted by persiaAziz <gi...@git.apache.org>.
Github user persiaAziz commented on the issue:
https://github.com/apache/trafficserver/pull/1024
Please review @shinrich
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request #1024: TS-4858: fix memory leak of global_default...
Posted by shinrich <gi...@git.apache.org>.
Github user shinrich closed the pull request at:
https://github.com/apache/trafficserver/pull/1024
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request #1024: TS-4858: fix memory leak of global_default...
Posted by persiaAziz <gi...@git.apache.org>.
Github user persiaAziz commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/1024#discussion_r82054996
--- Diff: iocore/net/SSLCertLookup.cc ---
@@ -160,7 +160,40 @@ ticket_block_alloc(unsigned count)
return ptr;
}
+ssl_ticket_key_block *
+ssl_create_ticket_key_block_buffer(char *ticket_key_data, int ticket_key_len)
+{
+ ssl_ticket_key_block *keyblock = NULL;
+ int 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;
+ }
+
+ // 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);
+ }
--- End diff --
Sure
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request #1024: TS-4858: fix memory leak of global_default...
Posted by shinrich <gi...@git.apache.org>.
Github user shinrich commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/1024#discussion_r82625741
--- Diff: iocore/net/SSLUtils.cc ---
@@ -2159,7 +2123,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;
--- End diff --
Not sure I'm following your used just once comment. The keyblock is used only within this function. As long as we have acquired the SSLConfig, the keyblock value it refers to will not disappear. It seems that in most cases, the overlap on config reload will be at most one, but it could be multiple safely as long as we have a referece to the SSLConfig before we start and hold it the entire time we are using the keyblock value.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request #1024: TS-4858: fix memory leak of global_default...
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/1024#discussion_r82049092
--- Diff: iocore/net/SSLUtils.cc ---
@@ -2159,7 +2123,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;
--- End diff --
OK, so as long as ``ssl_callback_session_ticket`` is called just once before the SSL config is destroyed we are OK. I'd feel more comfortable if there was a way to remove the keyblock from the SSL context after use so that we have less chance of a dangling pointer.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #1024: TS-4858: fix memory leak of global_default_keyblo...
Posted by zwoop <gi...@git.apache.org>.
Github user zwoop commented on the issue:
https://github.com/apache/trafficserver/pull/1024
[approve ci]
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request #1024: TS-4858: fix memory leak of global_default...
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/1024#discussion_r82046288
--- Diff: iocore/net/SSLConfig.cc ---
@@ -243,6 +245,21 @@ SSLConfigParams::initialize()
ats_free(ssl_server_ca_cert_filename);
ats_free(CACertRelativePath);
+ REC_ReadConfigStringAlloc(ticket_key_filename, "proxy.config.ssl.server.ticket_key.filename");
+ int ticket_key_len;
+ ats_scoped_str ticket_key_path(Layout::relative_to(this->serverCertPathOnly, this->ticket_key_filename));
+ ats_scoped_str ticket_key_data;
+ if (ticket_key_filename != NULL) {
+ ticket_key_data = readIntoBuffer(ticket_key_path, __func__, &ticket_key_len);
+ } 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);
+ }
--- End diff --
Simplify to this:
```C
if (ticket_key_filename) {
int len;
ats_scoped_str path(Layout::relative_to(this->serverCertPathOnly, this->ticket_key_filename));
ats_scoped_str data = readIntoBuffer(path, __func__, &len);
// XXX error checking?
default_global_keyblock = ticket_block_XXX(data, len);
} else {
ssl_ticket_key_t key;
RAND_bytes(&key, sizeof(key));
default_global_keyblock = ticket_block_XXX(&key, sizeof(key));
}
```
*or*
Add additional ticket block APIs:
```C
ssl_ticket_key_block *ticket_block_alloc_random(unsigned count);
ssl_ticket_key_block *ticket_block_read(const char *path);
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request #1024: TS-4858: fix memory leak of global_default...
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/1024#discussion_r82625392
--- Diff: iocore/net/SSLCertLookup.cc ---
@@ -160,7 +160,40 @@ ticket_block_alloc(unsigned count)
return ptr;
}
+ssl_ticket_key_block *
+ssl_create_ticket_key_block_buffer(char *ticket_key_data, int ticket_key_len)
+{
+ ssl_ticket_key_block *keyblock = NULL;
+ int 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;
+ }
+
+ // 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);
+ }
--- End diff --
Sure, but the single responsibility of this function is to load a ticket block. If the caller needs a metric to track how many times it is called, the caller should do it. By pushing higher level responsibilities into library code, we are breaking the single responsibility principle and making the code less reusable and harder to reason about.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #1024: TS-4858: fix memory leak of global_default_keyblo...
Posted by shinrich <gi...@git.apache.org>.
Github user shinrich commented on the issue:
https://github.com/apache/trafficserver/pull/1024
[approve ci]
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---