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 90ea6238d9..798ce5b317 100644
--- 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 f5f94695d4..f35017df7a 100644
--- 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 19a2704305..4febd48689 100644
--- a/src/peer_select.cc
+++ b/src/peer_select.cc
@@ -274,6 +274,20 @@ 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;
+        // Caller requires to check for pinned connections through
+        // CachePeer object:
+        p->setPeer(fs->_peer.get());
+        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.