Re: [PATCH 6/8] reconfiguration leaks: SSL certificate context cache
On 08/20/2014 01:09 AM, Amos Jeffries wrote: > On 20/08/2014 9:27 a.m., Alex Rousskov wrote: >> 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. > > It does, yes. +1. Committed to trunk as r13537. Thank you, Alex.
Re: [PATCH 6/8] reconfiguration leaks: SSL certificate context cache
On 20/08/2014 9:27 a.m., Alex Rousskov wrote: > 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. It does, yes. +1. Amos > > > Thank you, > > Alex. > >
Re: [PATCH 6/8] reconfiguration leaks: SSL certificate context cache
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 +
+++ src/ssl/context_storage.cc 2014-04-24 20:35:45 +
@@ -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::iterator i = storage.begin(); i != storage.end(); ++i) {
std::map::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::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(conf_i->first, new LocalContextStorage(-1, conf_i->second)));
}
}
}
}
Ssl::GlobalContextStorage Ssl::TheGlobalContextStorage;
Re: [PATCH 6/8] reconfiguration leaks: SSL certificate context cache
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... Thank you, Alex.
Re: [PATCH 6/8] reconfiguration leaks: SSL certificate context cache
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? 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? Thank you, Alex.
Re: [PATCH 6/8] reconfiguration leaks: SSL certificate context cache
On 25/04/2014 7:46 p.m., 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. >> >> Alex. >> > > This requires the guarantee that all connections using the storage are > closed right? > That is a false assertion on reconfigure. I mean "false assumption". There are almost certainly open client connections using it. Amos
Re: [PATCH 6/8] reconfiguration leaks: SSL certificate context cache
On 25/04/2014 12:56 p.m., Alex Rousskov wrote: > Do not leak fake SSL certificate context cache when reconfigure > changes port addresses. > > Alex. > This requires the guarantee that all connections using the storage are closed right? That is a false assertion on reconfigure. Amos
