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 53774cb6da746d6b46b69102ae38e5cec4a515a2..ea67d77285f0c1544c6fdfea6b43a22f15205598 100644 (file)
--- a/src/ssl/PeerConnector.cc
+++ b/src/ssl/PeerConnector.cc
@@ -213,7 +213,17 @@ void
 Ssl::PeekingPeerConnector::cbCheckForPeekAndSpliceDone(allow_t answer, void *data)
 {
     Ssl::PeekingPeerConnector *peerConnect = (Ssl::PeekingPeerConnector *) data;
-    peerConnect->checkForPeekAndSpliceDone((Ssl::BumpMode)answer.kind);
+    // Use job calls to add done() checks and other job logic/protections.
+    CallJobHere1(83, 7, CbcPointer<PeekingPeerConnector>(peerConnect), Ssl::PeekingPeerConnector, checkForPeekAndSpliceDone, answer);
+}
+
+void
+Ssl::PeekingPeerConnector::checkForPeekAndSpliceDone(allow_t answer)
+{
+    const Ssl::BumpMode finalAction = (answer.code == ACCESS_ALLOWED) ?
+        static_cast<Ssl::BumpMode>(answer.kind):
+        checkForPeekAndSpliceGuess();
+    checkForPeekAndSpliceMatched(finalAction);
 }
 
 void
@@ -247,7 +257,7 @@ Ssl::PeekingPeerConnector::checkForPeekAndSplice()
 }
 
 void
-Ssl::PeekingPeerConnector::checkForPeekAndSpliceDone(Ssl::BumpMode const action)
+Ssl::PeekingPeerConnector::checkForPeekAndSpliceMatched(const Ssl::BumpMode action)
 {
     SSL *ssl = fd_table[serverConn->fd].ssl;
     BIO *b = SSL_get_rbio(ssl);
@@ -281,6 +291,23 @@ Ssl::PeekingPeerConnector::checkForPeekAndSpliceDone(Ssl::BumpMode const action)
     }
 }
 
+Ssl::BumpMode
+Ssl::PeekingPeerConnector::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)
 {
@@ -532,7 +559,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 *
@@ -789,7 +822,7 @@ Ssl::PeekingPeerConnector::noteSslNegotiationError(const int result, const int s
         // we currently splice all resumed sessions unconditionally
         if (const bool spliceResumed = true) {
             bypassCertValidator();
-            checkForPeekAndSpliceDone(Ssl::bumpSplice);
+            checkForPeekAndSpliceMatched(Ssl::bumpSplice);
             return;
         } // else fall through to find a matching ssl_bump action (with limited info)
     }

diff --git a/src/ssl/PeerConnector.h b/src/ssl/PeerConnector.h
index fc1dcdc638c1d1089dfa03febe80f2d6766fdada..e70fc175b1b146110ecc328a7c587544658d9bc6 100644 (file)
--- a/src/ssl/PeerConnector.h
+++ b/src/ssl/PeerConnector.h
@@ -238,8 +238,13 @@ public:
     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;
 
     /// Runs after the server certificate verified to update client
     /// connection manager members