[squid-dev] [PATCH] shuffle SessionCacheRunner to libsecurity

[Amos Jeffries]

On 12/02/2016 7:21 a.m., Alex Rousskov wrote:
> On 02/11/2016 10:20 AM, Amos Jeffries wrote:
> 
>> One issue was uncovered during this:
>>
>> * While ssl/support.h was defining a destruct_session_cache() function
>> that appeared to release the cache memory, it was not actually being
>> used anywhere. Which unless a fortuitous sequence of events is happening
>> means that OpenSSL locks we create for the cache entries may not be
>> released properly. On the other hand the cache should only be erased on
>> shutdown so the effects of this are minor.
>>
>> The unused function has been removed and the issue is now expicitly
>> noted in the Runner shutdown handling method.
> 
> 
>> +SharedSessionCacheRr::~SharedSessionCacheRr()
>> +{
>> +#if 0
>> +    // XXX: the Ssl::SessionCache memory (if any) is leaked.
>> +    // at least technically. OpenSSL is never informed about this memory
>> +    // being no longer used and the bulk SHM chunk being released.
>> +    // Any locks it is holding for the sessions may stay set.
>> +//      delete Ssl::SessionCache;
>> +#endif
>> +
>> +    delete owner;
>> +}
> 
> 
> Why not delete Ssl::SessionCache?
> 
> I am not intimately familiar with the specifics of the OpenSSL session
> locks you are discussing, so I cannot comment about that aspect, but
> after "delete owner" above, any Ssl::SessionCache memory that OpenSSL
> might have been pointing at would be gone anyway and must not be
> accessed, right?

Well. I hoped the XXX explained that. The cache is a SHM block of memory
- so far so good. but...

> 
> AFAICT, Ssl::SessionCache itself does not know anything about OpenSSL
> because it is just a [shared memory] key:opaque_value cache. With its
> memory gone and no SessionCache-->OpenSSL connection, it feels like
> deleting SessionCache cannot make things worse (and will free a few
> otherwise "technically leaking" Ipc::MemMap bytes that are not shared).


... using delete in kid1 would unset/nul/unlock all the Pointer in it
even if kid2/kid3 were still going to be active for a while. AFAIK the
SHM locking parts only ensure the SHM block itself (as a blob) remains
open, not the individual objects in it.

(we have the problem that mgr:shutdown can target individual workers.
and maybe exceptino handling will cycle one worker cleanly. So no
guarantees the current one shutting down means the others are too.)


To get around all that I think we will need an atomic counter in the
session cache which tells any kid how many other kids are currently
accessing that SHM block. But there may be a better way.

> 
> I am guessing you have tried deleting Ssl::SessionCache and something
> broke. What was it?
> 

No. I chose to leave this bug-compatible with existing trunk, since I
cant test that change properly myself.

Amos