Bug 4864: !Comm::MonitorsRead assertion in maybeReadVirginBody() (#351)

This assertion is probably triggered when Squid retries/reforwards
server-first or step2+ bumped connections (after they fail).
Retrying/reforwarding such pinned connections is wrong because the
corresponding client-to-Squid TLS connection was negotiated based on the
now-failed Squid-to-server TLS connection, and there is no mechanism to
ensure that the new Squid-to-server TLS connection will have exactly the
same properties. Squid should forward the error to client instead.

Also fixed peer selection code that could return more than one PINNED
paths with only the first path having the destination of the actual
pinned connection.

This is a Measurement Factory project

This is a limited equivalent to master branch commit 3dde9e52

diff --git a/src/FwdState.cc b/src/FwdState.cc
index 1724c64ace72eb95917e662c29d29202f920950c..c51d26fdeba92484e40a9f3c3595ea13f36bb907 100644 (file)
--- a/src/FwdState.cc
+++ b/src/FwdState.cc
@@ -587,6 +587,9 @@ FwdState::checkRetry()
     if (!entry->isEmpty())
         return false;
 
+    if (request->flags.pinned && !pinnedCanRetry())
+        return false;
+
     if (exhaustedTries())
         return false;
 
@@ -1068,6 +1071,11 @@ FwdState::reforward()
 
     debugs(17, 3, HERE << e->url() << "?" );
 
+    if (request->flags.pinned && !pinnedCanRetry()) {
+        debugs(17, 3, "pinned connection; cannot retry");
+        return 0;
+    }
+
     if (!EBIT_TEST(e->flags, ENTRY_FWD_HDR_WAIT)) {
         debugs(17, 3, HERE << "No, ENTRY_FWD_HDR_WAIT isn't set");
         return 0;
@@ -1229,6 +1237,28 @@ FwdState::exhaustedTries() const
     return n_tries >= Config.forward_max_tries;
 }
 
+bool
+FwdState::pinnedCanRetry() const
+{
+    assert(request->flags.pinned);
+
+    // pconn race on pinned connection: Currently we do not have any mechanism
+    // to retry current pinned connection path.
+    if (pconnRace == raceHappened)
+        return false;
+
+    // If a bumped connection was pinned, then the TLS client was given our peer
+    // details. Do not retry because we do not ensure that those details stay
+    // constant. Step1-bumped connections do not get our TLS peer details, are
+    // never pinned, and, hence, never reach this method.
+    if (request->flags.sslBumped)
+        return false;
+
+    // The other pinned cases are FTP proxying and connection-based HTTP
+    // authentication. TODO: Do these cases have restrictions?
+    return true;
+}
+
 /**** PRIVATE NON-MEMBER FUNCTIONS ********************************************/
 
 /*

diff --git a/src/FwdState.h b/src/FwdState.h
index da8f8a477e5414402688258d0beb3c7fc7621660..bfefe21f4806ca97cbd2edc9a32ef923f3055431 100644 (file)
--- a/src/FwdState.h
+++ b/src/FwdState.h
@@ -117,6 +117,11 @@ private:
     void doneWithRetries();
     void completed();
     void retryOrBail();
+
+    /// whether a pinned to-peer connection can be replaced with another one
+    /// (in order to retry or reforward a failed request)
+    bool pinnedCanRetry() const;
+
     ErrorState *makeConnectingError(const err_type type) const;
     void connectedToPeer(Security::EncryptorAnswer &answer);
     static void RegisterWithCacheManager(void);

diff --git a/src/peer_select.cc b/src/peer_select.cc
index 4c7624d4d6a0644d1f2fdac089f4beb5f8b37823..3f8317367a80abbc939a6ee1be61552402e24420 100644 (file)
--- a/src/peer_select.cc
+++ b/src/peer_select.cc
@@ -274,6 +274,17 @@ peerSelectDnsPaths(ps_state *psstate)
         return;
     }
 
+    if (fs && fs->code == PINNED) {
+        // Send an empty IP address marked as PINNED
+        const Comm::ConnectionPointer p = new Comm::Connection();
+        p->peerType = PINNED;
+        psstate->paths->push_back(p);
+        psstate->servers = fs->next;
+        delete fs;
+        peerSelectDnsPaths(psstate);
+        return;
+    }
+
     // convert the list of FwdServer destinations into destinations IP addresses
     if (fs && psstate->paths->size() < (unsigned int)Config.forward_max_tries) {
         // send the next one off for DNS lookup.