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

[Amos Jeffries]

On 12/02/2016 4:48 p.m., Alex Rousskov wrote:
> On 02/11/2016 08:14 PM, Amos Jeffries wrote:
>> 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
> 
> Sorry, the XXX comment did not help me understand. I _do_ know that the
> cache is in shared memory, but the comment talks about things that do
> not match my understanding of reality.
> 
> 
>>> 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
> 
> I do not follow. Ssl::SessionCache is Ipc::MemMap. Ipc::MemMap does not
> know anything about OpenSSL. Deleting Ipc::MemMap cannot
> unset/nul/unlock any OpenSSL pointers AFAICT.

Does it initiate the destructor chain for the objects stored in the
SessionCache ?


> 
> Moreover, correctly deleting Ipc::MemMap in kid1 should have no effect
> on IpcMemMap in other kids -- this is shared memory, and its users may
> come and go at any time (by design!).
> 
> 
> If your argument is that some OpenSSL structures somewhere are pointing
> to the shared memory maintained by Ssl::SessionCache, then
> 
> (a) IIRC, deleting the memory owner (which we do!) ought to invalidate
> that memory anyway, and
> 
> (b) please point me to those structures because I only see SessionCache
> managing opaque blobs that OpenSSL either parses or writes to within the
> scope of a given function call -- no long-term sharing of that opaque
> data area with the rest of Squid.
> 
> 
> Again, I am not intimate with that area of the code, so please correct
> me if I am wrong.
> 

Sorry. I mucked myself up confusing this piece with my previous
(artially failed) experiments.

It is a side-track, but since they are related:

GnuTLS does things slightly different to OpenSSL. It requires that the
memory allocated belong to the library (otherwise fetched and linked the
same to a session).
 BUT, the crucial thing there is that our shared memory stores the
pointer to  rather than a fully separate copy. So it appears the next
stages might need the cache to store a Pointer. Which means the Pointer
deallocation will manage whatever locking is (or not) needed. So the
destructors need to be run from each worker, but not free from each worker.

Anyhow since as you say thats out of scope for this patch. And the
dependency issue I ran aground on means this wont be relevant for
anything soon anyway. We might have a better solution by then.

> 
>>> 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.
> 
> If we were never deleting Ssl::SessionCache before, and you do not want
> to deal with that problem, then it is acceptable to leave it as is, of
> course. However, please replace the proposed XXX comment with something
> less assertive about scary thing that may not be true. For example:
> 
>   // TODO: Enable after testing to improve at-exit memory cleanup.
>   // delete Ssl::SessionCache
> 
> or
> 
>   // XXX: Enable after testing to reduce at-exit memory "leaks".
>   // delete Ssl::SessionCache
> 

Okay, done.

If there are no objections to the code itself, in particular the
initialization change. Then I will apply later today.

Amos