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

[Alex Rousskov]

On 02/12/2016 08:30 AM, Amos Jeffries wrote:
> 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 ?


AFAICT, besides various metadata PODs (locks, offsets, and such), there
are no C++ objects stored in the SessionCache, just opaque blobs.



> 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.

I hope that we will not have to store [smart] pointers in shared memory
caches. AFAICT, the current SessionCache does not do that. I hope GnuTLS
does not require us to do it either. The session cache is similar to a
cache_dir: We do not store pointers in any cache_dirs I am familiar
with. Instead, we serialize/parse objects to store/load them.

In SessionCache context, you can see that serialization and parsing by
looking for d2i_SSL_SESSION() and i2d_SSL_SESSION() calls in ssl/support.cc


HTH,

Alex.