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

[Alex Rousskov]

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.

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.



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

There are already counters like that, on the cache slot level. They seem
irrelevant here because Ssl::SessionCache does not export them to
OpenSSL (AFAICT). AFAICT, Ssl::SessionCache does this:

openForReading
  ... read/parse ...
closeForReading

or this:

openForWriting
  ... write/store ...
closeForWriting


without leaving those open/close blocks open outside the corresponding
function call.


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


Thank you,

Alex.