From: Alex Rousskov <rousskov@measurement-factory.com>
Date: Fri, 21 May 2021 18:47:36 +0000 (+0000)
Subject: Bug 4528: ICAP transactions quit on async DNS lookups (#795)
X-Git-Tag: 4.15-20210525-snapshot
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=26cf7c3c01c3e58178054eb99b323471e40e9703;p=thirdparty%2Fsquid.git

Bug 4528: ICAP transactions quit on async DNS lookups (#795)

The bug directly affected some ICAP OPTIONS transactions and indirectly
affected some ICAP REQMOD/RESPMOD transactions:

* OPTIONS: When a transaction needed to look up an IP address of the
  ICAP service, and that address was not cached by Squid, it ended
  prematurely because Adaptation::Icap::Xaction::doneAll() was unaware
  of ipcache_nbgethostbyname()'s async nature. This bug is fixed now.

* REQMOD/RESPMOD: Adaptation::Icap::ModXact masked the _direct_ effects
  of the bug: ModXact::startWriting() sets state.writing before calling
  openConnection() which schedules the DNS lookup. That "I am still
  writing" state makes ModXact::doneAll() false while a REQMOD or
  RESPMOD transaction waits for the DNS lookup.

  However, REQMOD and RESPMOD transactions that require an OPTIONS
  transaction (because the service options have never been fetched
  before or have expired) could still fail because the OPTIONS
  transaction they trigger could fail as described in the first bullet.
  For example, the first few REQMOD and RESPMOD transactions for a given
  service -- all those started before the DNS lookup completes and Squid
  caches its result -- could fail this way. With the OPTIONS now fixed,
  these REQMOD and RESPMOD transactions should work correctly.

Broken since inception (commit fb505fa).
---

diff --git a/src/adaptation/icap/Xaction.cc b/src/adaptation/icap/Xaction.cc
index 9bbc75dba7..09b2835836 100644
--- a/src/adaptation/icap/Xaction.cc
+++ b/src/adaptation/icap/Xaction.cc
@@ -86,6 +86,7 @@ Adaptation::Icap::Xaction::Xaction(const char *aTypeName, Adaptation::Icap::Serv
     isRetriable(true),
     isRepeatable(true),
     ignoreLastWrite(false),
+    waitingForDns(false),
     stopReason(NULL),
     connector(NULL),
     reader(NULL),
@@ -187,12 +188,17 @@ Adaptation::Icap::Xaction::openConnection()
     debugs(93,3, typeName << " opens connection to " << s.cfg().host.termedBuf() << ":" << s.cfg().port);
 
     // Locate the Service IP(s) to open
+    assert(!waitingForDns);
+    waitingForDns = true; // before the possibly-synchronous ipcache_nbgethostbyname()
     ipcache_nbgethostbyname(s.cfg().host.termedBuf(), icapLookupDnsResults, this);
 }
 
 void
 Adaptation::Icap::Xaction::dnsLookupDone(const ipcache_addrs *ia)
 {
+    assert(waitingForDns);
+    waitingForDns = false;
+
     Adaptation::Icap::ServiceRep &s = service();
 
     if (ia == NULL) {
@@ -418,7 +424,8 @@ void Adaptation::Icap::Xaction::callEnd()
 
 bool Adaptation::Icap::Xaction::doneAll() const
 {
-    return !connector && !securer && !reader && !writer && Adaptation::Initiate::doneAll();
+    return !waitingForDns && !connector && !securer && !reader && !writer &&
+           Adaptation::Initiate::doneAll();
 }
 
 void Adaptation::Icap::Xaction::updateTimeout()
@@ -690,6 +697,9 @@ void Adaptation::Icap::Xaction::fillPendingStatus(MemBuf &buf) const
 
         buf.append(";", 1);
     }
+
+    if (waitingForDns)
+        buf.append("D", 1);
 }
 
 void Adaptation::Icap::Xaction::fillDoneStatus(MemBuf &buf) const
diff --git a/src/adaptation/icap/Xaction.h b/src/adaptation/icap/Xaction.h
index d20933d35e..8fcceaa0ac 100644
--- a/src/adaptation/icap/Xaction.h
+++ b/src/adaptation/icap/Xaction.h
@@ -137,6 +137,7 @@ protected:
     bool isRetriable;  ///< can retry on persistent connection failures
     bool isRepeatable; ///< can repeat if no or unsatisfactory response
     bool ignoreLastWrite;
+    bool waitingForDns; ///< expecting a ipcache_nbgethostbyname() callback
 
     const char *stopReason;