[squid-dev] [PATCH] LockingPointer API update

[Christos Tsantilas]

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.

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

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.