[squid-dev] [PATCH] LockingPointer API update

[Alex Rousskov]

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


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

If you are switching to approach #2, then I agree with Christos that we
should remove TidyPointer as a LockingPointer parent. LockingPointer
should use TidyPointer as the type of its pointer data member instead:

  template <...>
  class LockingPointer {
  ...
  private:
      TidyPointer<...> pointer; ///< owns the object we lock
  };


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


HTH,

Alex.