From f5305f5595c98ec26b0eb0f71ff7bad066019074 Mon Sep 17 00:00:00 2001
From: Christos Tsantilas <christos@chtsanti.net>
Date: Wed, 27 Nov 2019 18:20:27 +0200
Subject: [PATCH] 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
---
 src/FwdState.cc    | 30 ++++++++++++++++++++++++++++++
 src/FwdState.h     |  5 +++++
 src/peer_select.cc | 11 +++++++++++
 3 files changed, 46 insertions(+)

diff --git a/src/FwdState.cc b/src/FwdState.cc
index 1724c64ace..c51d26fdeb 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 da8f8a477e..bfefe21f48 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 4c7624d4d6..3f8317367a 100644
--- 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.
-- 
2.47.2