Avoid ssl/helper.cc "ssl_crtd" assertions on reconfiguration (#186)

Reconfiguration process consists of mainReconfigureStart() and
mainReconfigureFinish() steps separated by at least one main loop
iteration. Clearing a Squid global variable in mainReconfigureStart()
creates two problems for transactions that were started before
reconfiguration:

1. Transactions accessing that global _during_ reconfiguration loop
   iteration(s) may be confused by the variable sudden disappearance.

2. Transactions accessing that global _after_ mainReconfigureFinish()
   may be confused by the variable disappearance if reconfiguration
   resulted in the global variable becoming nil.

To remove the first problem for ssl_crtd, external_acl, and redirecting
helpers, all of them are now reconfigured "instantly", during
mainReconfigureFinish().

To prevent crashes due to the second problem, Squid now generates helper
errors if the disappeared ssl_crtd or external_acl helpers are accessed
after reconfiguration. The admin is warned about such problems via
level-1 cache.log ERROR messages.

The second problem cannot be fully solved without storing (refcounted)
configuration globals inside each transaction that uses them. Such
serious changes are outside this small assertion-fixing project scope.

This is a Measurement Factory project.

diff --git a/src/client_side.cc b/src/client_side.cc
index d2cbf30862c070d995482ba0b939c6059b238198..c4465aaeb9e6547c6b932237affdfff42ad31caa 100644 (file)
--- a/src/client_side.cc
+++ b/src/client_side.cc
@@ -3024,7 +3024,7 @@ ConnStateData::getSslContextStart()
             request_message.setCode(Ssl::CrtdMessage::code_new_certificate);
             request_message.composeRequest(certProperties);
             debugs(33, 5, HERE << "SSL crtd request: " << request_message.compose().c_str());
-            Ssl::Helper::GetInstance()->sslSubmit(request_message, sslCrtdHandleReplyWrapper, this);
+            Ssl::Helper::Submit(request_message, sslCrtdHandleReplyWrapper, this);
             return;
         } catch (const std::exception &e) {
             debugs(33, DBG_IMPORTANT, "ERROR: Failed to compose ssl_crtd " <<

diff --git a/src/external_acl.cc b/src/external_acl.cc
index 4e2deebb99842ea3b23b30dad9dee6a36f9330b0..1b3ea43a88ab3f0739c6495475998103317bd951 100644 (file)
--- a/src/external_acl.cc
+++ b/src/external_acl.cc
@@ -991,6 +991,8 @@ externalAclHandleReply(void *data, const Helper::Reply &reply)
         entryData.password = label;
 #endif
 
+    // XXX: This state->def access conflicts with the cbdata validity check
+    // below.
     dlinkDelete(&state->list, &state->def->queue);
 
     ExternalACLEntryPointer entry;

diff --git a/src/main.cc b/src/main.cc
index 91ed2d4b864df5c0da3d532b20d59ecdafec2c65..869c76abef6e7cb58867efe1c5d0e6d0b7eb0287 100644 (file)
--- a/src/main.cc
+++ b/src/main.cc
@@ -880,15 +880,9 @@ mainReconfigureStart(void)
 #if USE_HTCP
     htcpClosePorts();
 #endif
-#if USE_SSL_CRTD
-    Ssl::Helper::GetInstance()->Shutdown();
-#endif
 #if USE_OPENSSL
-    if (Ssl::CertValidationHelper::GetInstance())
-        Ssl::CertValidationHelper::GetInstance()->Shutdown();
     Ssl::TheGlobalContextStorage.reconfigureStart();
 #endif
-    redirectShutdown();
 #if USE_AUTH
     authenticateReset();
 #endif
@@ -976,14 +970,13 @@ mainReconfigureFinish(void *)
     storeLogOpen();
     Dns::Init();
 #if USE_SSL_CRTD
-    Ssl::Helper::GetInstance()->Init();
+    Ssl::Helper::Reconfigure();
 #endif
 #if USE_OPENSSL
-    if (Ssl::CertValidationHelper::GetInstance())
-        Ssl::CertValidationHelper::GetInstance()->Init();
+    Ssl::CertValidationHelper::Reconfigure();
 #endif
 
-    redirectInit();
+    redirectReconfigure();
 #if USE_AUTH
     authenticateInit(&Auth::TheConfig.schemes);
 #endif
@@ -1186,12 +1179,11 @@ mainInitialize(void)
     Dns::Init();
 
 #if USE_SSL_CRTD
-    Ssl::Helper::GetInstance()->Init();
+    Ssl::Helper::Init();
 #endif
 
 #if USE_OPENSSL
-    if (Ssl::CertValidationHelper::GetInstance())
-        Ssl::CertValidationHelper::GetInstance()->Init();
+    Ssl::CertValidationHelper::Init();
 #endif
 
     redirectInit();
@@ -2063,11 +2055,10 @@ SquidShutdown()
 
     debugs(1, DBG_IMPORTANT, "Shutting down...");
 #if USE_SSL_CRTD
-    Ssl::Helper::GetInstance()->Shutdown();
+    Ssl::Helper::Shutdown();
 #endif
 #if USE_OPENSSL
-    if (Ssl::CertValidationHelper::GetInstance())
-        Ssl::CertValidationHelper::GetInstance()->Shutdown();
+    Ssl::CertValidationHelper::Shutdown();
 #endif
     redirectShutdown();
     externalAclShutdown();

diff --git a/src/redirect.cc b/src/redirect.cc
index 4e16e03f41e8863a0c5129136d6a68d10649f939..3f741622f63302178ceb7b434f5897a9c370c6bc 100644 (file)
--- a/src/redirect.cc
+++ b/src/redirect.cc
@@ -434,3 +434,9 @@ redirectShutdown(void)
     storeIdExtrasFmt = NULL;
 }
 
+void
+redirectReconfigure()
+{
+    redirectShutdown();
+    redirectInit();
+}

diff --git a/src/redirect.h b/src/redirect.h
index 8c204c624d9427539bc139f6285c29ac5b45f8fd..edc2d33bae4e4423ff395ce223fcf0962e3cd080 100644 (file)
--- a/src/redirect.h
+++ b/src/redirect.h
@@ -19,6 +19,7 @@ class ClientHttpRequest;
 
 void redirectInit(void);
 void redirectShutdown(void);
+void redirectReconfigure();
 void redirectStart(ClientHttpRequest *, HLPCB *, void *);
 void storeIdStart(ClientHttpRequest *, HLPCB *, void *);
 

diff --git a/src/security/PeerConnector.cc b/src/security/PeerConnector.cc
index bfa82c1adc692bd26cbdcb2fc5b046e96966f81a..d3d99d5e65e4af7a082f7d5422c61a1e1fe44e96 100644 (file)
--- a/src/security/PeerConnector.cc
+++ b/src/security/PeerConnector.cc
@@ -241,7 +241,7 @@ Security::PeerConnector::sslFinalized()
         try {
             debugs(83, 5, "Sending SSL certificate for validation to ssl_crtvd.");
             AsyncCall::Pointer call = asyncCall(83,5, "Security::PeerConnector::sslCrtvdHandleReply", Ssl::CertValidationHelper::CbDialer(this, &Security::PeerConnector::sslCrtvdHandleReply, nullptr));
-            Ssl::CertValidationHelper::GetInstance()->sslSubmit(validationRequest, call);
+            Ssl::CertValidationHelper::Submit(validationRequest, call);
             return false;
         } catch (const std::exception &e) {
             debugs(83, DBG_IMPORTANT, "ERROR: Failed to compose ssl_crtvd " <<

diff --git a/src/ssl/helper.cc b/src/ssl/helper.cc
index 2763bad3048444e0afeb65c3b29f02f63fa65be7..e6aa41ddfa16eb0841e7f631e617aa5665e44d68 100644 (file)
--- a/src/ssl/helper.cc
+++ b/src/ssl/helper.cc
@@ -67,20 +67,7 @@ operator <<(std::ostream &os, const Ssl::GeneratorRequest &gr)
 /// pending Ssl::Helper requests (to all certificate generator helpers combined)
 static Ssl::GeneratorRequests TheGeneratorRequests;
 
-Ssl::Helper * Ssl::Helper::GetInstance()
-{
-    static Ssl::Helper sslHelper;
-    return &sslHelper;
-}
-
-Ssl::Helper::Helper() : ssl_crtd(NULL)
-{
-}
-
-Ssl::Helper::~Helper()
-{
-    Shutdown();
-}
+helper *Ssl::Helper::ssl_crtd = nullptr;
 
 void Ssl::Helper::Init()
 {
@@ -123,10 +110,15 @@ void Ssl::Helper::Shutdown()
     ssl_crtd = NULL;
 }
 
-void Ssl::Helper::sslSubmit(CrtdMessage const & message, HLPCB * callback, void * data)
+void
+Ssl::Helper::Reconfigure()
 {
-    assert(ssl_crtd);
+    Shutdown();
+    Init();
+}
 
+void Ssl::Helper::Submit(CrtdMessage const & message, HLPCB * callback, void * data)
+{
     SBuf rawMessage(message.compose().c_str()); // XXX: helpers cannot use SBuf
     rawMessage.append("\n", 1);
 
@@ -142,7 +134,9 @@ void Ssl::Helper::sslSubmit(CrtdMessage const & message, HLPCB * callback, void
     request->emplace(callback, data);
     TheGeneratorRequests.emplace(request->query, request);
     debugs(83, 5, "request from " << data << " as " << *request);
-    if (ssl_crtd->trySubmit(request->query.c_str(), HandleGeneratorReply, request))
+    // ssl_crtd becomes nil if Squid is reconfigured without SslBump or
+    // certificate generation disabled in the new configuration
+    if (ssl_crtd && ssl_crtd->trySubmit(request->query.c_str(), HandleGeneratorReply, request))
         return;
 
     ::Helper::Reply failReply(::Helper::BrokenHelper);
@@ -168,25 +162,13 @@ Ssl::HandleGeneratorReply(void *data, const ::Helper::Reply &reply)
 }
 #endif //USE_SSL_CRTD
 
-Ssl::CertValidationHelper * Ssl::CertValidationHelper::GetInstance()
-{
-    static Ssl::CertValidationHelper sslHelper;
-    if (!Ssl::TheConfig.ssl_crt_validator)
-        return NULL;
-    return &sslHelper;
-}
-
-Ssl::CertValidationHelper::CertValidationHelper() : ssl_crt_validator(NULL)
-{
-}
-
-Ssl::CertValidationHelper::~CertValidationHelper()
-{
-    Shutdown();
-}
+helper *Ssl::CertValidationHelper::ssl_crt_validator = nullptr;
 
 void Ssl::CertValidationHelper::Init()
 {
+    if (!Ssl::TheConfig.ssl_crt_validator)
+        return;
+
     assert(ssl_crt_validator == NULL);
 
     // we need to start ssl_crtd only if some port(s) need to bump SSL
@@ -249,6 +231,13 @@ void Ssl::CertValidationHelper::Shutdown()
     HelperCache = NULL;
 }
 
+void
+Ssl::CertValidationHelper::Reconfigure()
+{
+    Shutdown();
+    Init();
+}
+
 class submitData
 {
     CBDATA_CLASS(submitData);
@@ -298,10 +287,8 @@ sslCrtvdHandleReplyWrapper(void *data, const ::Helper::Reply &reply)
     delete crtdvdData;
 }
 
-void Ssl::CertValidationHelper::sslSubmit(Ssl::CertValidationRequest const &request, AsyncCall::Pointer &callback)
+void Ssl::CertValidationHelper::Submit(Ssl::CertValidationRequest const &request, AsyncCall::Pointer &callback)
 {
-    assert(ssl_crt_validator);
-
     Ssl::CertValidationMsg message(Ssl::CrtdMessage::REQUEST);
     message.setCode(Ssl::CertValidationMsg::code_cert_validate);
     message.composeRequest(request);
@@ -325,15 +312,18 @@ void Ssl::CertValidationHelper::sslSubmit(Ssl::CertValidationRequest const &requ
         return;
     }
 
-    if (!ssl_crt_validator->trySubmit(crtdvdData->query.c_str(), sslCrtvdHandleReplyWrapper, crtdvdData)) {
-        Ssl::CertValidationResponse::Pointer resp = new Ssl::CertValidationResponse(crtdvdData->ssl);
-        resp->resultCode = ::Helper::BrokenHelper;
-        Ssl::CertValidationHelper::CbDialer *dialer = dynamic_cast<Ssl::CertValidationHelper::CbDialer*>(callback->getDialer());
-        Must(dialer);
-        dialer->arg1 = resp;
-        ScheduleCallHere(callback);
-        delete crtdvdData;
+    // ssl_crt_validator becomes nil if Squid is reconfigured with cert
+    // validator disabled in the new configuration
+    if (ssl_crt_validator && ssl_crt_validator->trySubmit(crtdvdData->query.c_str(), sslCrtvdHandleReplyWrapper, crtdvdData))
         return;
-    }
+
+    Ssl::CertValidationResponse::Pointer resp = new Ssl::CertValidationResponse(crtdvdData->ssl);
+    resp->resultCode = ::Helper::BrokenHelper;
+    Ssl::CertValidationHelper::CbDialer *dialer = dynamic_cast<Ssl::CertValidationHelper::CbDialer*>(callback->getDialer());
+    Must(dialer);
+    dialer->arg1 = resp;
+    ScheduleCallHere(callback);
+    delete crtdvdData;
+    return;
 }
 

diff --git a/src/ssl/helper.h b/src/ssl/helper.h
index 217021047c5b9b805ffecb4711af3f26da326244..5f958ad95ebac8ff4de5ee0dfcdd2ca0bd596a26 100644 (file)
--- a/src/ssl/helper.h
+++ b/src/ssl/helper.h
@@ -22,23 +22,19 @@ namespace Ssl
 {
 #if USE_SSL_CRTD
 /**
- * Set of thread for ssl_crtd. This class is singleton. Use this class only
- * over GetIntance() static method. This class use helper structure
- * for threads management.
+ * Set of thread for ssl_crtd. This class is singleton.
+ * This class use helper structure for threads management.
  */
 class Helper
 {
 public:
-    static Helper * GetInstance(); ///< Instance class.
-    void Init(); ///< Init helper structure.
-    void Shutdown(); ///< Shutdown helper structure.
+    static void Init(); ///< Init helper structure.
+    static void Shutdown(); ///< Shutdown helper structure.
+    static void Reconfigure(); ///< Reconfigure helper structure.
     /// Submit crtd message to external crtd server.
-    void sslSubmit(CrtdMessage const & message, HLPCB * callback, void *data);
+    static void Submit(CrtdMessage const & message, HLPCB * callback, void *data);
 private:
-    Helper();
-    ~Helper();
-
-    helper * ssl_crtd; ///< helper for management of ssl_crtd.
+    static helper * ssl_crtd; ///< helper for management of ssl_crtd.
 };
 #endif
 
@@ -50,16 +46,13 @@ public:
     typedef UnaryMemFunT<Security::PeerConnector, CertValidationResponse::Pointer> CbDialer;
 
     typedef void CVHCB(void *, Ssl::CertValidationResponse const &);
-    static CertValidationHelper * GetInstance(); ///< Instance class.
-    void Init(); ///< Init helper structure.
-    void Shutdown(); ///< Shutdown helper structure.
+    static void Init(); ///< Init helper structure.
+    static void Shutdown(); ///< Shutdown helper structure.
+    static void Reconfigure(); ///< Reconfigure helper structure
     /// Submit crtd request message to external crtd server.
-    void sslSubmit(Ssl::CertValidationRequest const & request, AsyncCall::Pointer &);
+    static void Submit(Ssl::CertValidationRequest const & request, AsyncCall::Pointer &);
 private:
-    CertValidationHelper();
-    ~CertValidationHelper();
-
-    helper * ssl_crt_validator; ///< helper for management of ssl_crtd.
+    static helper * ssl_crt_validator; ///< helper for management of ssl_crtd.
 public:
     typedef LruMap<SBuf, Ssl::CertValidationResponse::Pointer, sizeof(Ssl::CertValidationResponse::Pointer) + sizeof(Ssl::CertValidationResponse)> LruCache;
     static LruCache *HelperCache; ///< cache for cert validation helper

diff --git a/src/ssl/stub_libsslutil.cc b/src/ssl/stub_libsslutil.cc
index e8cedaa4ed0253d16823431e827b01257f278605..58728a5589b08cda5d24a003013b44e226c25f08 100644 (file)
--- a/src/ssl/stub_libsslutil.cc
+++ b/src/ssl/stub_libsslutil.cc
@@ -37,8 +37,7 @@ void Ssl::readCertAndPrivateKeyFromFiles(Security::CertPointer &, Security::Priv
 bool Ssl::sslDateIsInTheFuture(char const *) STUB_RETVAL(false)
 
 #include "ssl/helper.h"
-Ssl::Helper * Ssl::Helper::GetInstance() STUB_RETVAL(NULL)
 void Ssl::Helper::Init() STUB
 void Ssl::Helper::Shutdown() STUB
-void Ssl::Helper::sslSubmit(Ssl::CrtdMessage const & message, HLPCB * callback, void *data) STUB
+void Ssl::Helper::Submit(Ssl::CrtdMessage const & message, HLPCB * callback, void *data) STUB