From: Alex Rousskov <rousskov@measurement-factory.com>
Date: Sun, 27 Sep 2015 08:18:53 +0000 (-0700)
Subject: Bug 4304: PeerConnector.cc:743 "!callback" assertion.
X-Git-Tag: SQUID_3_5_10~4
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=16336e74604384313b05331480fbf2fad0d78bc1;p=thirdparty%2Fsquid.git

Bug 4304: PeerConnector.cc:743 "!callback" assertion.

When no ssl_bump rules match, Squid may throw a "a rule with the final
action must have matched" exception:

Must(finalAction == Ssl::bumpSplice || finalAction == Ssl::bumpBump ||
     finalAction == Ssl::bumpTerminate);

After the exception is thrown, Squid attempts to wind down the affected
transaction (as it should), but the code either quits with an unhandled
exception error or hits the !callback assertion, depending on whether
the async job processing was in place when the exception was hit (which
depends on whether non-blocking/slow ssl_bump ACLs were active).

The attached patch does three things:

1. Teaches Squid to guess the final ssl_bump action when no ssl_bump
rules match. The final guessed action is "bump" if the last non-final
action was "stare" and "splice" otherwise. I suspect that the older
Squid code attempted to do something like that, but that code may have
been lost when we taught Squid to ignore impossible ssl_bump actions.

2. Protects ssl_bump-checking code from quitting with an unhandled
exception error.

3. Converts the fatal !callback assertion into [hopefully less damaging]
transaction error, with a BUG message logged to cache.log.

More work may be needed to investigate other exceptions, especially
Must(!csd->serverBump() || csd->serverBump()->step <= Ssl::bumpStep2);

This is a Measurement Factory project
---

diff --git a/src/ssl/PeerConnector.cc b/src/ssl/PeerConnector.cc
index 8abae3fb67..bc88cf6b87 100644
--- a/src/ssl/PeerConnector.cc
+++ b/src/ssl/PeerConnector.cc
@@ -370,7 +370,17 @@ void
 Ssl::PeerConnector::cbCheckForPeekAndSpliceDone(allow_t answer, void *data)
 {
     Ssl::PeerConnector *peerConnect = (Ssl::PeerConnector *) data;
-    peerConnect->checkForPeekAndSpliceDone((Ssl::BumpMode)answer.kind);
+    // Use job calls to add done() checks and other job logic/protections.
+    CallJobHere1(83, 7, CbcPointer<PeerConnector>(peerConnect), Ssl::PeerConnector, checkForPeekAndSpliceDone, answer);
+}
+
+void
+Ssl::PeerConnector::checkForPeekAndSpliceDone(allow_t answer)
+{
+    const Ssl::BumpMode finalAction = (answer.code == ACCESS_ALLOWED) ?
+                                      static_cast<Ssl::BumpMode>(answer.kind):
+                                      checkForPeekAndSpliceGuess();
+    checkForPeekAndSpliceMatched(finalAction);
 }
 
 void
@@ -404,7 +414,7 @@ Ssl::PeerConnector::checkForPeekAndSplice()
 }
 
 void
-Ssl::PeerConnector::checkForPeekAndSpliceDone(Ssl::BumpMode const action)
+Ssl::PeerConnector::checkForPeekAndSpliceMatched(const Ssl::BumpMode action)
 {
     SSL *ssl = fd_table[serverConn->fd].ssl;
     BIO *b = SSL_get_rbio(ssl);
@@ -437,6 +447,23 @@ Ssl::PeerConnector::checkForPeekAndSpliceDone(Ssl::BumpMode const action)
     }
 }
 
+Ssl::BumpMode
+Ssl::PeerConnector::checkForPeekAndSpliceGuess() const
+{
+    if (const ConnStateData *csd = request->clientConnectionManager.valid()) {
+        const Ssl::BumpMode currentMode = csd->sslBumpMode;
+        if (currentMode == Ssl::bumpStare) {
+            debugs(83,5, "default to bumping after staring");
+            return Ssl::bumpBump;
+        }
+        debugs(83,5, "default to splicing after " << currentMode);
+    } else {
+        debugs(83,3, "default to splicing due to missing info");
+    }
+
+    return Ssl::bumpSplice;
+}
+
 void
 Ssl::PeerConnector::sslCrtvdHandleReplyWrapper(void *data, Ssl::CertValidationResponse const &validationResponse)
 {
@@ -609,7 +636,7 @@ Ssl::PeerConnector::handleNegotiateError(const int ret)
         if (srvBio->bumpMode() == Ssl::bumpPeek && (resumingSession = srvBio->resumingSession())) {
             // we currently splice all resumed sessions unconditionally
             if (const bool spliceResumed = true) {
-                checkForPeekAndSpliceDone(Ssl::bumpSplice);
+                checkForPeekAndSpliceMatched(Ssl::bumpSplice);
                 return;
             } // else fall through to find a matching ssl_bump action (with limited info)
         }
@@ -744,7 +771,13 @@ Ssl::PeerConnector::swanSong()
 {
     // XXX: unregister fd-closure monitoring and CommSetSelect interest, if any
     AsyncJob::swanSong();
-    assert(!callback); // paranoid: we have not left the caller waiting
+    if (callback != NULL) { // paranoid: we have left the caller waiting
+        debugs(83, DBG_IMPORTANT, "BUG: Unexpected state while connecting to a cache_peer or origin server");
+        ErrorState *anErr = new ErrorState(ERR_GATEWAY_FAILURE, Http::scInternalServerError, request.getRaw());
+        bail(anErr);
+        assert(!callback);
+        return;
+    }
 }
 
 const char *
diff --git a/src/ssl/PeerConnector.h b/src/ssl/PeerConnector.h
index 88d04489c7..44e95c08d1 100644
--- a/src/ssl/PeerConnector.h
+++ b/src/ssl/PeerConnector.h
@@ -126,8 +126,13 @@ protected:
     void checkForPeekAndSplice();
 
     /// Callback function for ssl_bump acl check in step3  SSL bump step.
+    void checkForPeekAndSpliceDone(allow_t answer);
+
     /// Handles the final bumping decision.
-    void checkForPeekAndSpliceDone(Ssl::BumpMode const);
+    void checkForPeekAndSpliceMatched(const Ssl::BumpMode finalMode);
+
+    /// Guesses the final bumping decision when no ssl_bump rules match.
+    Ssl::BumpMode checkForPeekAndSpliceGuess() const;
 
     /// Called when the SSL negotiation step aborted because data needs to
     /// be transferred to/from SSL server or on error. In the first case