From: Alex Rousskov Date: Sat, 19 Mar 2022 21:05:32 +0000 (+0000) Subject: Bug 4946: client_side_request.cc: "request != newRequest" (#1000) X-Git-Tag: SQUID_6_0_1~222 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=05add813e4baef0d8640a871c53d2fa5ef2a6ffb;p=thirdparty%2Fsquid.git Bug 4946: client_side_request.cc: "request != newRequest" (#1000) ... assertion when preceded by the following error message: ERROR: Inconsistent service method ... in dynamic adaptation chain The assertion is triggered by the following chain of events. During SslBump step1, a REQMOD adaptation service returns a dynamic (X-Next-Services) plan containing a RESPMOD service P. Then, during SslBump step2 (after obtaining TLS client SNI): * Adaptation::AccessCheck::start() discovers P in the "future services" storage (Adaptation::History::theFutureServices) and returns it. * The adaptation routing code correctly concludes that P is not applicable to the current vectoring point, logs the above ERROR, and returns the untouched virgin message object to the adaptation initiator. See thePlan.exhausted() in Adaptation::Iterator::step(). * ClientHttpRequest asserts because it expects a new message object. Fixed Adaptation::AccessCheck code no longer assumes that it cannot be activated twice for the same vectoring point. It leaves services applicable to future vectoring points in theFutureServices instead of always suggesting them for the current vectoring point. TODO: We can and should optimize adaptation requesting code to stop requiring a new message object when no adaptation is necessary, but that change is difficult (we tried!) and independent from the bug fixed here. --- diff --git a/src/adaptation/AccessCheck.cc b/src/adaptation/AccessCheck.cc index e32b507843..e3b0db5d77 100644 --- a/src/adaptation/AccessCheck.cc +++ b/src/adaptation/AccessCheck.cc @@ -55,8 +55,7 @@ Adaptation::AccessCheck::AccessCheck(const ServiceFilter &aFilter, h->start("ACL"); #endif - debugs(93, 5, "AccessCheck constructed for " << - methodStr(filter.method) << " " << vectPointStr(filter.point)); + debugs(93, 5, "AccessCheck constructed for " << filter); } Adaptation::AccessCheck::~AccessCheck() @@ -85,10 +84,10 @@ Adaptation::AccessCheck::usedDynamicRules() if (!ah) return false; // dynamic rules not enabled or not triggered - DynamicGroupCfg services; - if (!ah->extractFutureServices(services)) { // clears history - debugs(85,9, "no service-proposed rules stored"); - return false; // earlier service did not plan for the future + const auto services = ah->extractCurrentServices(filter); // updates history + if (services.empty()) { + debugs(85, 5, "no service-proposed rules for " << filter); + return false; } debugs(85,3, "using stored service-proposed rules: " << services); diff --git a/src/adaptation/DynamicGroupCfg.h b/src/adaptation/DynamicGroupCfg.h index 1bb1163698..51eb55d601 100644 --- a/src/adaptation/DynamicGroupCfg.h +++ b/src/adaptation/DynamicGroupCfg.h @@ -27,6 +27,10 @@ public: Store services; ///< services in the group bool empty() const { return services.empty(); } ///< no services added + + /// configured service IDs in X-Next-Services value (comma-separated) format + const String &serviceIds() const { return id; } + void add(const String &item); ///< updates group id and services void clear(); ///< makes the config empty }; diff --git a/src/adaptation/History.cc b/src/adaptation/History.cc index 1381cf4a13..e5f4be2b44 100644 --- a/src/adaptation/History.cc +++ b/src/adaptation/History.cc @@ -9,6 +9,7 @@ #include "squid.h" #include "adaptation/Config.h" #include "adaptation/History.h" +#include "adaptation/ServiceGroups.h" #include "base/TextException.h" #include "debug/Stream.h" #include "globals.h" @@ -160,13 +161,12 @@ Adaptation::History::setFutureServices(const DynamicGroupCfg &services) theFutureServices = services; // may be empty } -bool Adaptation::History::extractFutureServices(DynamicGroupCfg &value) +Adaptation::DynamicGroupCfg +Adaptation::History::extractCurrentServices(const ServiceFilter &filter) { - if (theFutureServices.empty()) - return false; - - value = theFutureServices; - theFutureServices.clear(); - return true; + DynamicGroupCfg current, future; + DynamicServiceChain::Split(filter, theFutureServices.serviceIds(), current, future); + theFutureServices = future; // may already be the same + return current; // may be empty } diff --git a/src/adaptation/History.h b/src/adaptation/History.h index 045dd01a7e..27f8903780 100644 --- a/src/adaptation/History.h +++ b/src/adaptation/History.h @@ -10,6 +10,7 @@ #define SQUID_ADAPT_HISTORY_H #include "adaptation/DynamicGroupCfg.h" +#include "adaptation/forward.h" #include "base/RefCount.h" #include "HttpHeader.h" #include "Notes.h" @@ -70,8 +71,8 @@ public: /// sets future services for the Adaptation::AccessCheck to notice void setFutureServices(const DynamicGroupCfg &services); - /// returns true, fills the value, and resets iff future services were set - bool extractFutureServices(DynamicGroupCfg &services); + /// returns and forgets planned/future services matching the given filter + DynamicGroupCfg extractCurrentServices(const ServiceFilter &); private: /// single Xaction stats (i.e., a historical record entry) diff --git a/src/adaptation/ServiceFilter.cc b/src/adaptation/ServiceFilter.cc index 3d24133b07..1c425842d7 100644 --- a/src/adaptation/ServiceFilter.cc +++ b/src/adaptation/ServiceFilter.cc @@ -63,3 +63,10 @@ Adaptation::ServiceFilter &Adaptation::ServiceFilter::operator =(const ServiceFi return *this; } +std::ostream & +Adaptation::operator <<(std::ostream &os, const ServiceFilter &filter) +{ + os << methodStr(filter.method) << ' ' << vectPointStr(filter.point); + return os; +} + diff --git a/src/adaptation/ServiceFilter.h b/src/adaptation/ServiceFilter.h index 8af65ed6da..7c16c979d7 100644 --- a/src/adaptation/ServiceFilter.h +++ b/src/adaptation/ServiceFilter.h @@ -36,6 +36,8 @@ public: AccessLogEntry::Pointer al; ///< info for the future access.log entry }; +std::ostream &operator <<(std::ostream &, const ServiceFilter &); + } // namespace Adaptation #endif /* SQUID_ADAPTATION__SERVICE_FILTER_H */