[squid-dev] [PATCH] LockingPointer API update

[Amos Jeffries]

On 22/06/2016 10:42 p.m., Christos Tsantilas wrote:
> On 06/22/2016 07:32 AM, Amos Jeffries wrote:
>> On 22/06/2016 1:02 p.m., Alex Rousskov wrote:
>>> On 06/21/2016 04:00 AM, Amos Jeffries wrote:
>>
>> The two I saw were:
>>
>> 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.

LockingPointer has a move assignment operator for doing that in clearer
way now in trunk - and 3.5 when built with C++11 compiler.



>>
>> 2) PeekingPeerConnector::serverCertificateVerified() doing two different
>> reset() vs resetAndLock() depending on the data source.
>>
>> The OpenSSL docs do say the call increments a reference count itself.
>> But I still think this is leaking. Since we explicitly unlock on
>> destruct as well as calling the necessary free API method in OpenSSL.
> 
> I do not think that this is leaking here.
> The code is the following:
> 
>   Security::CertPointer serverCert;
>    if (...) {
>            // We need to lock here because the destructor of
>        // serverCert will call X509_free and will
>            // decrease the lock of the
>            serverCert.resetAndLock(serverBump->serverCert.get());
>    } 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.
>            serverCert.reset(SSL_get_peer_certificate(ssl));
>    }
>    // serverCert::~serverCert() called on function
>    // leave, calling X509_free.
> 
> 
> For LockingCounter we need both a reset and a resetAndLock method to
> correctly using it with openSSL library. There are cases where an
> openSSL call increases the locking counter for the object it returns and
> other cases where it doesn't.

In the patch I'm working on now its looking very much like we can drop
the reset() method for these instances and just use a temporary local
object. Like so:

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.


So long as we only construct LockingPointer from freshly minted OpenSSL
raw-pointers that have an OpenSSL set lock. Then we do not need the
non-locking reset() operation.
We do need the locking reset*() method for the cases where the raw
pointer is not locked by the library.

> 
> The LockingPointer is not perfect. But it is a class used and adjusted
> to be used for openSSL related objects. OpenSSL is a C library and it is
> not easy to convert openSSL objects to equivalent C++ objects.
> 
> I must also note that the locking pointer is not used only for X509
> objects but for other objects too.
> 
> In a point of view the LockingPointer should never moved under the
> Security and should be used only for openSSL related code.

Yes and no.

It got moved to src/security/ so objects of its type could be used by
the src/security/ code in builds where the src/ssl/ path is not compiled.

If this attempt to get TidyPointer able to be used as a seamless
replacement in the generic libsecurity API then LockingPointer can
probably go back there. But not until that is true.

But, the GnuTLS builds will need an equivalent Pointer with a different
lock action to handle session pointers.

Amos