Polish from final audit

diff --git a/src/security/LockingPointer.h b/src/security/LockingPointer.h
index 7e736211e8772ce56c6be286a120d5c7e40e166e..3fd547bc114a65b4d6fc84fe5d6b2f0bfd28278a 100644 (file)
--- a/src/security/LockingPointer.h
+++ b/src/security/LockingPointer.h
@@ -43,11 +43,6 @@ namespace Security
  * Normally, reset() would lock(), but libraries like OpenSSL
  * pre-lock objects before they are fed to LockingPointer, necessitating
  * this resetWithoutLocking() customization hook.
- *
- * The lock() method increments Object's reference counter.
- *
- * The unlock() method decrements Object's reference counter and destroys
- * the object when the counter reaches zero.
  */
 template <typename T, void (*UnLocker)(T *t), int lockId>
 class LockingPointer
@@ -62,7 +57,10 @@ public:
      * created one reference lock for the object pointed to.
      * Our destructor will do the matching unlock.
      */
-    explicit LockingPointer(T *t = nullptr): raw(t) {}
+    explicit LockingPointer(T *t = nullptr): raw(nullptr) {
+        // de-optimized for clarity about non-locking
+        resetWithoutLocking(t);
+    }
 
     /// use the custom UnLocker to unlock any value still stored.
     ~LockingPointer() { unlock(); }
@@ -101,6 +99,9 @@ public:
         }
     }
 
+    /// Forget the raw pointer - unlock if any value was set. Become a nil pointer.
+    void reset() { unlock(); }
+
     /// Forget the raw pointer without unlocking it. Become a nil pointer.
     T *release() {
         T *ret = raw;
@@ -109,6 +110,7 @@ public:
     }
 
 private:
+    /// The lock() method increments Object's reference counter.
     void lock(T *t) {
 #if USE_OPENSSL
             if (t)
@@ -120,14 +122,28 @@ private:
 #endif
     }
 
-    /// Unlock the raw pointer. Become a nil pointer.
+    /// Become a nil pointer. Decrements any pointed-to Object's reference counter
+    /// using UnLocker which ideally destroys the object when the counter reaches zero.
     void unlock() {
-        if (raw)
+        if (raw) {
             UnLocker(raw);
-        raw = nullptr;
+            raw = nullptr;
+        }
     }
 
-    T *raw; ///< pointer to T object or nullptr
+    /**
+     * Normally, no other code will have this raw pointer.
+     *
+     * However, OpenSSL does some strange and not always consistent things.
+     * OpenSSL library may keep its own internal raw pointers and manage
+     * their reference counts independently, or it may not. This varies between
+     * API functions, though it is usually documented.
+     *
+     * This means the caller code needs to be carefuly written to use the correct
+     * reset method and avoid the raw-pointer constructor unless OpenSSL function
+     * producing the pointer is clearly documented as incrementing a lock for it.
+     */
+    T *raw;
 };
 
 } // namespace Security

diff --git a/src/ssl/gadgets.cc b/src/ssl/gadgets.cc
index 3fc5f5d6559cbef88a0d3fd8dde16af21621e8e9..4abff40b385680446762372fa76d78f6924d776c 100644 (file)
--- a/src/ssl/gadgets.cc
+++ b/src/ssl/gadgets.cc
@@ -679,8 +679,8 @@ void Ssl::readCertAndPrivateKeyFromFiles(Security::CertPointer & cert, Ssl::EVP_
     pkey.resetWithoutLocking(readSslPrivateKey(keyFilename));
     cert.resetWithoutLocking(readSslX509Certificate(certFilename));
     if (!pkey || !cert || !X509_check_private_key(cert.get(), pkey.get())) {
-        pkey.resetWithoutLocking(nullptr);
-        cert.resetWithoutLocking(nullptr);
+        pkey.reset();
+        cert.reset();
     }
 }
 

diff --git a/src/ssl/support.cc b/src/ssl/support.cc
index c79275f769e895f2b3a906cea1f514b55cb2c799..637ae3e5fe0cf014d8d6bce317f4fe6d1680210a 100644 (file)
--- a/src/ssl/support.cc
+++ b/src/ssl/support.cc
@@ -315,7 +315,7 @@ ssl_verify_cb(int ok, X509_STORE_CTX * ctx)
                 }
                 delete filledCheck->sslErrors;
                 filledCheck->sslErrors = NULL;
-                filledCheck->serverCert.resetWithoutLocking(nullptr);
+                filledCheck->serverCert.reset();
             }
             // If the certificate validator is used then we need to allow all errors and
             // pass them to certficate validator for more processing
@@ -1270,8 +1270,8 @@ void Ssl::readCertChainAndPrivateKeyFromFiles(Security::CertPointer & cert, EVP_
     pkey.resetWithoutLocking(readSslPrivateKey(keyFilename, cb));
     cert.resetWithoutLocking(readSslX509CertificatesChain(certFilename, chain.get()));
     if (!pkey || !cert || !X509_check_private_key(cert.get(), pkey.get())) {
-        pkey.resetWithoutLocking(nullptr);
-        cert.resetWithoutLocking(nullptr);
+        pkey.reset();
+        cert.reset();
     }
 }