From e2bbd3b3f1b5d11f375189a94d41070c52fa4d2c Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Wed, 13 Apr 2022 03:22:18 +0000 Subject: [PATCH] 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. --- src/FwdState.cc | 18 +++++++----- src/tunnel.cc | 73 ++++++++++++++++++++++++++++++++----------------- 2 files changed, 59 insertions(+), 32 deletions(-) diff --git a/src/FwdState.cc b/src/FwdState.cc index 3f2eedacc6..6448595493 100644 --- 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 0bcae01152..20bde63ef0 100644 --- 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::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) -- 2.39.5