[squid-dev] [PATCH] LockingPointer API update

[Alex Rousskov]

On 06/21/2016 04:00 AM, Amos Jeffries wrote:
> With GnuTLS support it is sometimes more useful to use a TidyPointer
> where a LockingPointer is used by OpenSSL.

Are there examples of such code in Squid trunk already? If yes, please
point to one or two to set the context. I could not quickly identify
them while looking at the patch... If not, perhaps you can cut-and-paste
them from your GnuTLS branch?


IMHO, the following rule of thumb is relevant to this discussion:
Library-agnostic APIs (e.g., Security) must be the same, regardless of
the library Squid is being compiled with.

The implementation of those Library-agnostic APIs may depend on the
library, of course. It is also perfectly fine for GnuTLS-specific code
to use types that are different from types used by OpenSSL-specific
code. However, the library-agnostic APIs must remain the same.

For example, do not add and/or rely on something like this:

    namespace Security {
        #if USE_OPENSSL
            typedef LockingPointer<...> SessionPointer;
        #elif USE_GNUTLS
            typedef TidyPointer<...> SessionPointer;
        #endif
    };

unless LockingPointer and TidyPointer have the same API.

Violating this rule will lead to library-agnostic code that compiles and
works fine with one library but either does not compile or, worse, does
not work with the other library.

Please note that I am not trying to accuse the proposed patch of
violating the above rule! I just feel it is important to mention that
rule here because trunk code already violates that rule, the patch might
be trying to work around existing violations, and future patch
modifications may have to work with this rule.


Now to the specific change being proposed:

> This patch converts the LockingPointer resetAndLock() to a virtual
> reset() so callers can use the right one without needing to care which
> type of pointer they are handling.

The two LockingPointer methods, inherited reset(X) and "direct"
resetAndLock(X) give different results: The former does not lock X while
the latter does. Proposed renaming essentially removes one of the
methods. That can be done only if either

  A) nobody outside LockingPointer calls its inherited reset() method OR
  B) all such calls are buggy.

(A) is false -- there are ~15 external LockingPointer::reset() calls,
without even counting those that simply reset the pointer to nil. You
have not asserted (B) AFAICT, and I am worried that it might also be false.

I have attached a list of relevant trunk calls. It may be incomplete.


> Doing this has already uncovered two instances of the
> TidyPointer::reset() being wrongly used on cert LockingPointer objects.

AFAIK, a LockingPointer::reset(X) call is legitimate if something locked
X for us and/or the previous owner is losing ownership without
decreasing the lock count. Can you point to the two instances you found
please? If those two are indeed bugs, they should be fixed separately,
before your changes go in IMO -- we should record where those bugs were.

There are also LockingPointer::reset(nil) calls but those are benign and
should be replaced with clear() calls that are faster and, well, clearer.


> There may be more hidden away

Indeed! The ability to _accidentally_ call LockingPointer::reset() is a
major LockingPointer API bug IMO. And the fact that we cannot be sure
whether the existing external LockingPointer::reset() calls are
accidental is a lesser LockingPointer API bug.


There are two sane ways to proceed AFAICT:

1. Assert that all external LockingPointer::reset() calls are bugs.
   Hide LockingPointer::reset() from external callers and then
   fix "LockingPointer<>::reset(T*) is private" compilation errors,
   arriving at LockingPointer with a single reset method.
   Revisit your patch: Do other differences in two classes APIs matter?

2. Assert that some external LockingPointer::reset() calls are needed.
   Provide LockingPointer::resetWithoutLocking() instead,
   hide LockingPointer::reset() from external callers, and
   fix "LockingPointer<>::reset(T*) is private" compilation errors.
   Reject your patch (because LockingPointer has two reset methods) and
   study Security/GnuTLS needs to propose a different solution.

If it turns out that some external LockingPointer::reset() calls are
needed, I would be happy to try to help you find a different solution.


HTH,

Alex.

-------------- next part --------------
src/security/cert_generators/file/certificate_db.cc:313:             cert.reset(findCert.release());
src/security/cert_generators/file/certificate_db.cc:314:             pkey.reset(findPkey.release());
src/client_side_request.cc:180:                 al->cache.sslClientCert.reset(SSL_get_peer_certificate(ssl));
src/ssl/gadgets.cc:133:     cert.reset(PEM_read_bio_X509(bio.get(), &certPtr, 0, 0));
src/ssl/gadgets.cc:138:     pkey.reset(PEM_read_bio_PrivateKey(bio.get(), &pkeyPtr, 0, 0));
src/ssl/gadgets.cc:151:     cert.reset(PEM_read_bio_X509(bio.get(), &certPtr, 0, 0));
src/ssl/gadgets.cc:514:         pkey.reset(Ssl::createSslPrivateKey());
src/ssl/gadgets.cc:553:     certToStore.reset(cert.release());
src/ssl/gadgets.cc:554:     pkeyToStore.reset(pkey.release());
src/ssl/gadgets.cc:679:     pkey.reset(readSslPrivateKey(keyFilename));
src/ssl/gadgets.cc:680:     cert.reset(readSslX509Certificate(certFilename));
src/ssl/PeekingPeerConnector.cc:342:             serverBump->serverCert.reset(serverCert.release());
src/ssl/PeekingPeerConnector.cc:357:             serverCert.reset(SSL_get_peer_certificate(ssl));
src/anyp/PortCfg.cc:146:     secure.staticContext.reset(secure.createStaticServerContext(*this));
src/security/ServerOptions.cc:162:     parsedDhParams.reset(dhp);
src/ssl/support.cc:1276:     pkey.reset(readSslPrivateKey(keyFilename, cb));
src/ssl/support.cc:1277:     cert.reset(readSslX509CertificatesChain(certFilename, chain.get()));
src/ssl/support.cc:1322:             fd_table[fd].ssl.reset(ssl);

The above suspects were detected by compiling with this unpolished patch:

=== modified file 'src/security/LockingPointer.h'
--- src/security/LockingPointer.h	2016-03-31 23:33:45 +0000
+++ src/security/LockingPointer.h	2016-06-21 21:27:50 +0000
@@ -64,26 +64,29 @@ public:
     LockingPointer<T, DeAllocator, lock> &operator =(LockingPointer<T, DeAllocator, lock> &&o) {
         if (o.get() != this->get())
             this->reset(o.release());
         return *this;
     }
 #endif
 
     void resetAndLock(T *t) {
         if (t != this->get()) {
             this->reset(t);
 #if USE_OPENSSL
             if (t)
                 CRYPTO_add(&t->references, 1, lock);
 #elif USE_GNUTLS
             // XXX: GnuTLS does not provide locking ?
 #else
             assert(false);
 #endif
         }
     }
+
+private:
+    void reset(T *t) { Parent::reset(t); } // hide Parent's method
 };
 
 } // namespace Security
 
 #endif /* SQUID_SRC_SECURITY_LOCKINGPOINTER_H */