Re: [PATCH 6/8] reconfiguration leaks: SSL certificate context cache

[Alex Rousskov]

On 06/15/2014 05:00 AM, Tsantilas Christos wrote:
> On 06/13/2014 10:46 PM, Alex Rousskov wrote:
>> On 04/25/2014 01:46 AM, Amos Jeffries wrote:
>>> On 25/04/2014 12:56 p.m., Alex Rousskov wrote:
>>>> Do not leak fake SSL certificate context cache when reconfigure
>>>> changes port addresses.

>>> This requires the guarantee that all connections using the storage are
>>> closed right?


>> Hi Christos,
>>
>>    My understanding is that deleting a cached LocalContextStorage object
>> does not actually affect connections that use the corresponding SSL_CTX
>> and certificate because any SSL object using those things increments
>> their sharing counter and deleting LocalContextStorage only decrements
>> that counter. The [cached] SSL_CTX object is not destroyed by
>> SSL_CTX_free until that sharing counter reaches zero. Is my
>> understanding flawed?


> This is true. The SSL_CTX objects are not destroyed.



>> Do we have any code that stores SSL_CTX pointers for asyncrhonous use
>> (i.e., across many main loop iterations) but does not increment the
>> sharing counter?


> Nope.
> I hope I am not loosing anything. In any case if such case found it
> should be considered as bug, and must fixed...


Hi Amos,

    Does the above exchange resolve your concerns regarding that 6/8
leak patch? I have re-attached the same patch here for your convenience.


Thank you,

Alex.



Do not leak fake SSL certificate context cache when reconfigure
changes port addresses.

=== modified file 'src/ssl/context_storage.cc'
--- src/ssl/context_storage.cc	2014-03-30 12:00:34 +0000
+++ src/ssl/context_storage.cc	2014-04-24 20:35:45 +0000
@@ -73,36 +73,37 @@ Ssl::LocalContextStorage *Ssl::GlobalCon
         return NULL;
     else
         return i->second;
 }
 
 void Ssl::GlobalContextStorage::reconfigureStart()
 {
     configureStorage.clear();
     reconfiguring = true;
 }
 
 void Ssl::GlobalContextStorage::reconfigureFinish()
 {
     if (reconfiguring) {
         reconfiguring = false;
 
         // remove or change old local storages.
         for (std::map<Ip::Address, LocalContextStorage *>::iterator i = storage.begin(); i != storage.end(); ++i) {
             std::map<Ip::Address, size_t>::iterator conf_i = configureStorage.find(i->first);
             if (conf_i == configureStorage.end() || conf_i->second <= 0) {
+                delete i->second;
                 storage.erase(i);
             } else {
                 i->second->setMemLimit(conf_i->second);
             }
         }
 
         // add new local storages.
         for (std::map<Ip::Address, size_t>::iterator conf_i = configureStorage.begin(); conf_i != configureStorage.end(); ++conf_i ) {
             if (storage.find(conf_i->first) == storage.end() && conf_i->second > 0) {
                 storage.insert(std::pair<Ip::Address, LocalContextStorage *>(conf_i->first, new LocalContextStorage(-1, conf_i->second)));
             }
         }
     }
 }
 
 Ssl::GlobalContextStorage Ssl::TheGlobalContextStorage;