[squid-dev] [PATCH] LockingPointer API update

[Amos Jeffries]

On 23/06/2016 4:58 a.m., Alex Rousskov wrote:
> On 06/22/2016 05:29 AM, Amos Jeffries wrote:
>> On 22/06/2016 10:42 p.m., Christos Tsantilas wrote:
>>> On 06/22/2016 07:32 AM, Amos Jeffries wrote:
>>>> 1) PeekingPeerConnector::handleServerCertificate() doing
>>>> serverBump->serverCert.reset(serverCert.release())
>>>>
>>>> On much closer inspection it appears not a bug. But is doing move
>>>> semantics without a move operator relying heavily on specific
>>>> implementation detail of TidyPointer API exposure.
> 
>>> This is not bug. The call
>>> newServerCertPointer.reset(oldServerCertPointer.release())
>>> just moves the X509 object with the current locks to the
>>> newServerCertPointer.
> 
>> Yeah. Thats move semantics.
> 
> Agreed.
> 
> 
>> LockingPointer has a move assignment operator for doing that in clearer
>> way now in trunk - and 3.5 when built with C++11 compiler.
> 
> At the risk of sounding like a broken record, I have to note that a move
> assignment operator is an _optimization_. It is _not_ a clearer way of
> doing anything. It is a cheaper way. It is not triggered except in some
> special circumstances [that are usually absent in the current
> LockingPointer code]. A move method alone is neither necessary nor
> usually sufficient for move semantics support.
> 
> Even if all non-clearing LockingPointer::reset() calls are there to
> _move_ things, we still cannot merge reset() and resetAndLock() because
> it would be trivial for the code to accidentally call the assignment
> operator (that locks) instead of the move assignment operator (that does
> not lock).

As you said the actual move is an optimization. If something breaks by
doing a copy and extra lock - then its not suitabel for using
LockingPointer in the first place.

The lines (quoted below here) which sparked this side discussion can
clearly be implemented with either assignment operator and both work
properly.

* All that happens on the copy-assign "slow" path is one extra
lock+unlock action.

* the use of constructor and assign [ X = construct(Y); ] is one of the
patterns the compiler deduces to be a move operator.

So the below simple logic should compile to the same thing the arcane
mix of reset+release did.

> 
>> The above code becomes:
>>
>>   Security::CertPointer serverCert;
>>   if (...) {
>>     // We need to lock here because the destructor of
>>     // serverCert will call X509_free and decrease the lock
>>
>>     // operator =() copy-assign does the lock stuff
>>
>>     serverCert = serverBump->serverCert;
>>
>>   } else {
>>     // We do not need to lock here, the SSL_get_peer_certificate
>>     // already increases the clock counter and we have to
>>     // free with X509_free.
>>
>>     // CertPointer() constructor builds object without locking it
>>     // assuming SSL_get_peer_certificate() did the lock for it.
>>
>>     // operator =() move-assign or copy-assign does the lock stuff
>>
>>     serverCert = Security::CertPointer(SSL_get_peer_certificate(ssl)));
>>   }
>>   // serverCert::~serverCert() called on function
>>   // leave, calling X509_free.
> 
> As Christos has said or implied, this approach assumes that
> Security::CertPointer is an assignable pointer (e.g., LockingPointer)
> and not a TidyPointer that does not have an assignment operator (by
> design). There is nothing wrong with that approach per se, but this is
> not what your patch (and approach known as #1) was about.
> 

Nod. I am taking advantage of the fact that currently GnuTLS does not do
bumping so this code is wrapped in USE_OPENSSL.

I dont't see that as a problem since the CertPointer type will need to
be converted to GnuTLS-enabled LockingPointer in order for GnuTLS to use
the feature this code is about.

My experimental build the past few hours has confirmed that all current
LockingPointer uses are OpenSSL-specific or already using a temporary
'-1' LockingPointer for GnuTLS.


> If you are switching to approach #2,

I'm currently experimenting with the ideas to see what a transition
needs to look like.

> then I agree with Christos that we
> should remove TidyPointer as a LockingPointer parent.

Doing some experimental builds of that now. Not quite a full split, but
with TidyPointer as protected rather than public inheritence.

> LockingPointer
> should use TidyPointer as the type of its pointer data member instead:
> 
>   template <...>
>   class LockingPointer {
>   ...
>   private:
>       TidyPointer<...> pointer; ///< owns the object we lock
>   };
> 

Hmm. TidyPointer alone is essentially a std::unique_ptr with custom
delete. We can probably drop TidyPointer from trunk entirely.

> 
>> But, the GnuTLS builds will need an equivalent Pointer with a different
>> lock action to handle session pointers.
> 
> Besides the TidyPointer disassociation sketched above, I suggest
> replacing the "int lock" LockingPointer template parameter with a
> locking function parameter. With some [trivial] wrappers for OpenSSL's
> "CRYPTO_add(..., lock)", it might work well for both GnuTLS and OpenSSL
> needs.
> 

Was just giving that some serious consideration before your mail came
in. Its on my list of experiments to try.

Amos