[squid-dev] Broken trunk after r14735, r14726

[Christos Tsantilas]

On 07/16/2016 03:56 PM, Amos Jeffries wrote:
> On 16/07/2016 7:02 a.m., Alex Rousskov wrote:
>> Hello,
>>
>>      There are two more recent changes that broke trunk:
>>
>> * After r14735 (Replaced TidyPointer with std::unique_ptr), Squid cannot
>> start due to an "std::bad_function_call" exception.
>>
>> * After r14726 (GnuTLS: support for TLS session resume): Squid segfaults
>> when attempting to connect to a Secure ICAP service. Official Squid
>> v4.0.12 suffers from this bug.
>>
>> Stack traces from both crashes are quoted at the end of this email.
>>
>> Please fix these regressions or undo the changes that created or exposed
>> them.
>>
>
> <snip>
>> * segfault when attempting to connect to a Secure ICAP REQMOD service
>> (tested with r14726, r14734):
>>
>
> Does this patch fix the session issue ?
>
> === modified file 'src/security/Session.cc'
> --- src/security/Session.cc     2016-07-07 19:03:02 +0000
> +++ src/security/Session.cc     2016-07-16 12:43:38 +0000
> @@ -53,7 +53,7 @@
>   void
>   Security::SetSessionResumeData(const Security::SessionPtr &s, const
> Security::SessionStatePointer &data)
>   {
> -    if (s) {
> +    if (data) {
>   #if USE_OPENSSL
>           (void)SSL_set_session(s, data.get());
>   #elif USE_GNUTLS
>
>
> I'm a little worried about the code calling SetSessionResumeData.
> OpenSSL documentation states:
>    "If there is already a session set inside ssl (because it was set with
> SSL_set_session() before or because the same ssl was already used for a
> connection), SSL_SESSION_free() will be called for that session."
>
> But our SetSessionResumeData() is called after setting up the sessions
> host data, etc. So I'm thinking all that setup in
> Ssl::BlindPeerConnector::initializeTls() may be thrown away by the
> resume action being called.
>

Squid crashes at the first TLS connection trying to establish to the 
ICAP server.  There is not any session to resume so the SSL session 
related methods should not called at all.

It is a strange crash. Is it a corrupted SSL object?

I must say that I am worrying a lot for all of these changes.
It is very difficult for me to follow them, and already I have 
difficulties to read and debug squid openSSL relate code.

We are using our own naming scheme for openSSL structures, eg 
"Security::SessionPtr" instead of "SSL *" or 
"Security::SessionStatePointer" instead of "SSL_SESSION *" and this is 
make it very difficult to follow Squid/openSSL code.

It is difficult to read an example openSSL code and trying convert it to 
squid, or reading a reference implementation and trying check against 
squid code. Someone is obligated to search for definitions and 
equivalents types all the time.

I have not ever study for a good way to support both GnuTLS and openSSL 
for squid, to know the problems,  but probably I would start to 
implement openSSLAcceptor/gnuTlsAcceptor and 
openSSLPeerConnector/gnuTLSPeerConnector classes.
Internally gnuTLS and openSSL should use their own types. This is will 
help current and future squid developers.

I am just expressing my worries here, I do not want to impose anything 
and if there was a related discussion in squid-dev, sorry that I do not 
participate and I did not express my concerns earlier.

Regards,
    Christos