From db8b598f02b6adbdf0acc26770b3ab34282dcdfa Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Mon, 1 Apr 2024 02:53:39 +0000 Subject: [PATCH] Bug 5360: FwdState::noteDestinationsEnd() assertion "err" (#1767) FATAL: assertion failed: FwdState.cc:660: "err" When FwdState decides to re-forward a request, it forgets the original response[^1] but does not create an error object. Since commit e2bbd3b, FwdState::noteDestinationsEnd() correctly assumed that we only idly wait for additional destinations after encountering a problem, but incorrectly asserted that past problems imply error object existence. Now Squid generates an HTTP 502 (Bad Gateway) response while setting %err_code/%err_detail to ERR_CANNOT_FORWARD/REFORWARD_TO_NONE. TunnelStateData::noteDestinationsEnd() code is similar, but it probably does not suffer from the same bug because an error object is created before every retryOrBail() call, and there is no re-forwarding code that forgets an HTTP error response without creating an error. Those invariants are not well tracked, and this change mimics FwdState changes in TunnelStateData just in case and to keep the two methods in sync. In those hypothetical cases, ERR_CANNOT_FORWARD/RETRY_TO_NONE is logged. [^1]: Long-term we probably want to preserve that original response so that we do not have to replace it with a generated error, but doing so requires significant refactoring and is outside this minimal fix scope. --- src/FwdState.cc | 8 +++++++- src/tunnel.cc | 10 +++++++++- 2 files changed, 16 insertions(+), 2 deletions(-) diff --git a/src/FwdState.cc b/src/FwdState.cc index abae1d6a5a..34cbd873ca 100644 --- a/src/FwdState.cc +++ b/src/FwdState.cc @@ -647,7 +647,13 @@ FwdState::noteDestinationsEnd(ErrorState *selectionError) } // destinationsFound, but none of them worked, and we were waiting for more - assert(err); + debugs(17, 7, "no more destinations to try after " << n_tries << " failed attempts"); + if (!err) { + const auto finalError = new ErrorState(ERR_CANNOT_FORWARD, Http::scBadGateway, request, al); + static const auto d = MakeNamedErrorDetail("REFORWARD_TO_NONE"); + finalError->detailError(d); + fail(finalError); + } // else use actual error from last forwarding attempt stopAndDestroy("all found paths have failed"); } diff --git a/src/tunnel.cc b/src/tunnel.cc index dca34bdc5d..81cd72cbb7 100644 --- a/src/tunnel.cc +++ b/src/tunnel.cc @@ -1364,7 +1364,15 @@ TunnelStateData::noteDestinationsEnd(ErrorState *selectionError) } // destinationsFound, but none of them worked, and we were waiting for more - assert(savedError); + debugs(17, 7, "no more destinations to try after " << n_tries << " failed attempts"); + if (!savedError) { + // retryOrBail() must be preceded by saveError(), but in case we forgot: + const auto finalError = new ErrorState(ERR_CANNOT_FORWARD, Http::scBadGateway, request.getRaw(), al); + static const auto d = MakeNamedErrorDetail("RETRY_TO_NONE"); + finalError->detailError(d); + saveError(finalError); + } // else use actual error from last forwarding attempt + // XXX: Honor clientExpectsConnectResponse() before replying. sendError(savedError, "all found paths have failed"); } -- 2.47.2