Bug 5186: noteDestinationsEnd check failed: transportWait (#985)

When the "no more destinations to try" notification comes after the last
forwarding/tunneling attempt has failed, the !destinationsFound block
does not run (because destinations were found), the usingDestination()
block does not run (because we are done with that last/failed
destination), but transportWait is false (for the same reason).

Also applied Bug 5090 (master/v6 commit 15bde30) FwdState protections to
tunnel.cc code. Tunnels may continue writing to the client while the
to-server connection is closing or closed, so TunnelStateData can be
potentially exposed to bug 5090 "no server connection but still
transporting" concerns. TunnelStateData never _retries_ successfully
established tunnels and, hence, can (and now does) stop receiving spare
destinations after committedToServer becomes true, but future tunnels
may start reforwarding in many cases, and most of the code does not need
to know about this (temporary?) simplification.

Also re-unified and polished related FwdState and TunnelStateData code,
including fixing lying source code comments and debug messages.

diff --git a/src/FwdState.cc b/src/FwdState.cc
index 3f2eedacc65a02a9c6c0117b9188fd390d805ad3..6448595493bb96b8ebb52661b37cdc7145ec1a67 100644 (file)
--- a/src/FwdState.cc
+++ b/src/FwdState.cc
@@ -641,7 +641,6 @@ FwdState::noteDestination(Comm::ConnectionPointer path)
     if (transporting())
         return; // and continue to receive destinations for backup
 
-    // This is the first path candidate we have seen. Use it.
     useDestinations();
 }
 
@@ -657,12 +656,8 @@ FwdState::noteDestinationsEnd(ErrorState *selectionError)
             Must(!err); // if we tried to connect, then path selection succeeded
             fail(selectionError);
         }
-        else if (err)
-            debugs(17, 3, "Will abort forwarding because all found paths have failed.");
-        else
-            debugs(17, 3, "Will abort forwarding because path selection found no paths.");
 
-        useDestinations(); // will detect and handle the lack of paths
+        stopAndDestroy("path selection found no paths");
         return;
     }
     // else continue to use one of the previously noted destinations;
@@ -675,7 +670,16 @@ FwdState::noteDestinationsEnd(ErrorState *selectionError)
         return; // and continue to wait for FwdState::noteConnection() callback
     }
 
-    Must(transporting()); // or we would be stuck with nothing to do or wait for
+    if (transporting()) {
+        // We are already using a previously opened connection (but were also
+        // receiving more destinations in case we need to re-forward).
+        debugs(17, 7, "keep transporting");
+        return;
+    }
+
+    // destinationsFound, but none of them worked, and we were waiting for more
+    assert(err);
+    stopAndDestroy("all found paths have failed");
 }
 
 /// makes sure connection opener knows that the destinations have changed

diff --git a/src/tunnel.cc b/src/tunnel.cc
index 0bcae01152762fe40e6acf62a1bbd9a2bd36af3b..20bde63ef07c73d94782f7cf637caf14d0f2cf58 100644 (file)
--- a/src/tunnel.cc
+++ b/src/tunnel.cc
@@ -99,6 +99,10 @@ public:
         return (server.conn != NULL && server.conn->getPeer() ? server.conn->getPeer()->host : request->url.host());
     };
 
+    /// store the given to-server connection; prohibit retries and do not look
+    /// for any other destinations
+    void commitToServer(const Comm::ConnectionPointer &);
+
     /// Whether the client sent a CONNECT request to us.
     bool clientExpectsConnectResponse() const {
         // If we are forcing a tunnel after receiving a client CONNECT, then we
@@ -187,6 +191,10 @@ public:
     /// whether another destination may be still attempted if the TCP connection
     /// was unexpectedly closed
     bool retriable;
+
+    /// whether the decision to tunnel to a particular destination was final
+    bool committedToServer;
+
     // TODO: remove after fixing deferred reads in TunnelStateData::copyRead()
     CodeContext::Pointer codeContext; ///< our creator context
 
@@ -264,9 +272,8 @@ private:
 
     /// \returns whether the request should be retried (nil) or the description why it should not
     const char *checkRetry();
-    /// whether the successfully selected path destination or the established
-    /// server connection is still in use
-    bool usingDestination() const;
+
+    bool transporting() const;
 
     /// details of the "last tunneling attempt" failure (if it failed)
     ErrorState *savedError = nullptr;
@@ -363,6 +370,7 @@ TunnelStateData::TunnelStateData(ClientHttpRequest *clientRequest) :
     destinations(new ResolvedPeers()),
     destinationsFound(false),
     retriable(true),
+    committedToServer(false),
     codeContext(CodeContext::Current())
 {
     debugs(26, 3, "TunnelStateData constructed this=" << this);
@@ -1006,8 +1014,7 @@ void
 TunnelStateData::notePeerReadyToShovel(const Comm::ConnectionPointer &conn)
 {
     assert(!client.dirty);
-    retriable = false;
-    server.initConnection(conn, tunnelServerClosed, "tunnelServerClosed", this);
+    commitToServer(conn);
 
     if (!clientExpectsConnectResponse())
         tunnelStartShoveling(this); // ssl-bumped connection, be quiet
@@ -1022,6 +1029,15 @@ TunnelStateData::notePeerReadyToShovel(const Comm::ConnectionPointer &conn)
     }
 }
 
+void
+TunnelStateData::commitToServer(const Comm::ConnectionPointer &conn)
+{
+    committedToServer = true;
+    retriable = false; // may already be false
+    PeerSelectionInitiator::subscribed = false; // may already be false
+    server.initConnection(conn, tunnelServerClosed, "tunnelServerClosed", this);
+}
+
 static void
 tunnelErrorComplete(int fd/*const Comm::ConnectionPointer &*/, void *data, size_t)
 {
@@ -1249,18 +1265,15 @@ TunnelStateData::noteDestination(Comm::ConnectionPointer path)
 
     destinations->addPath(path);
 
-    if (usingDestination()) {
-        // We are already using a previously opened connection but also
-        // receiving destinations in case we need to re-forward.
-        Must(!transportWait);
-        return;
-    }
-
     if (transportWait) {
+        assert(!transporting());
         notifyConnOpener();
         return; // and continue to wait for tunnelConnectDone() callback
     }
 
+    if (transporting())
+        return; // and continue to receive destinations for backup
+
     startConnecting();
 }
 
@@ -1276,8 +1289,9 @@ TunnelStateData::noteDestinationsEnd(ErrorState *selectionError)
         if (selectionError)
             return sendError(selectionError, "path selection has failed");
 
+        // TODO: Merge with FwdState and remove this likely unnecessary check.
         if (savedError)
-            return sendError(savedError, "all found paths have failed");
+            return sendError(savedError, "path selection found no paths (with an impossible early error)");
 
         return sendError(new ErrorState(ERR_CANNOT_FORWARD, Http::scInternalServerError, request.getRaw(), al),
                          "path selection found no paths");
@@ -1286,21 +1300,32 @@ TunnelStateData::noteDestinationsEnd(ErrorState *selectionError)
     // if all of them fail, tunneling as whole will fail
     Must(!selectionError); // finding at least one path means selection succeeded
 
-    if (usingDestination()) {
-        // We are already using a previously opened connection but also
-        // receiving destinations in case we need to re-forward.
-        Must(!transportWait);
+    if (transportWait) {
+        assert(!transporting());
+        notifyConnOpener();
+        return; // and continue to wait for the noteConnection() callback
+    }
+
+    if (transporting()) {
+        // We are already using a previously opened connection (but were also
+        // receiving more destinations in case we need to re-forward).
+        debugs(17, 7, "keep transporting");
         return;
     }
 
-    Must(transportWait); // or we would be stuck with nothing to do or wait for
-    notifyConnOpener();
+    // destinationsFound, but none of them worked, and we were waiting for more
+    assert(savedError);
+    // XXX: Honor clientExpectsConnectResponse() before replying.
+    sendError(savedError, "all found paths have failed");
 }
 
+/// Whether a tunneling attempt to some selected destination X is in progress
+/// (after successfully opening/reusing a transport connection to X).
+/// \sa transportWait
 bool
-TunnelStateData::usingDestination() const
+TunnelStateData::transporting() const
 {
-    return encryptionWait || peerWait || Comm::IsConnOpen(server.conn);
+    return encryptionWait || peerWait || committedToServer;
 }
 
 /// remembers an error to be used if there will be no more connection attempts
@@ -1359,7 +1384,7 @@ TunnelStateData::startConnecting()
         request->hier.startPeerClock();
 
     assert(!destinations->empty());
-    assert(!usingDestination());
+    assert(!transporting());
     AsyncCall::Pointer callback = asyncCall(17, 5, "TunnelStateData::noteConnection", HappyConnOpener::CbDialer<TunnelStateData>(&TunnelStateData::noteConnection, this));
     const auto cs = new HappyConnOpener(destinations, callback, request, startTime, 0, al);
     cs->setHost(request->url.host());
@@ -1454,12 +1479,10 @@ switchToTunnel(HttpRequest *request, const Comm::ConnectionPointer &clientConn,
     debugs(26, 3, request->method << " " << context->http->uri << " " << request->http_ver);
 
     TunnelStateData *tunnelState = new TunnelStateData(context->http);
-    tunnelState->retriable = false;
+    tunnelState->commitToServer(srvConn);
 
     request->hier.resetPeerNotes(srvConn, tunnelState->getHost());
 
-    tunnelState->server.initConnection(srvConn, tunnelServerClosed, "tunnelServerClosed", tunnelState);
-
 #if USE_DELAY_POOLS
     /* no point using the delayIsNoDelay stuff since tunnel is nice and simple */
     if (!srvConn->getPeer() || !srvConn->getPeer()->options.no_delay)