From: Alex Rousskov <rousskov@measurement-factory.com>
Date: Sat, 12 Aug 2023 15:40:12 +0000 (+0000)
Subject: Bug 5294: ERR_CANNOT_FORWARD returned instead of ERR_DNS_FAIL (#1453)
X-Git-Tag: SQUID_6_3~4
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=9bb687ad0c07679d10b2293bbf9002b04fea9b76;p=thirdparty%2Fsquid.git

Bug 5294: ERR_CANNOT_FORWARD returned instead of ERR_DNS_FAIL (#1453)

Since 2017 commit fd9c47d, peer selection code stopped reporting
ERR_DNS_FAIL cases because PeerSelector::noteIps() treated DNS answers
without IP addresses as if at least one IP address was received. Without
seeing a DNS resolution error, the ultimate recipient of the DNS
resolution results (e.g., CONNECT tunneling or regular forwarding code)
used ERR_CANNOT_FORWARD to indicate a failure to find a forwarding path.

PeerSelector::noteIps() code mimicked legacy IPH code with regard to
handling of the addresses parameter. However, IPH caller had a special
emptyIsNil adjustment that was missing from the noteIps() call! We now
apply that adjustment to both noteIps() and IPH code paths.

Long-term, we should probably remove nil address container pointers.
Having two different ways to signal lack of IPs is dangerous. Currently,
there is only one known supplier of nil address container:
IpcacheStats.invalid code that validates ipcache_nbgethostbyname() name
parameter. Either the corresponding nil/empty name check should be
converted into an assertion (blaming the ipcache_nbgethostbyname()
caller for not validating the name) OR that checking code should supply
an empty address container to finalCallback().
---

diff --git a/src/ipcache.cc b/src/ipcache.cc
index 0c2518c4c5..8957c671af 100644
--- a/src/ipcache.cc
+++ b/src/ipcache.cc
@@ -227,18 +227,20 @@ IpCacheLookupForwarder::IpCacheLookupForwarder(IPH *fun, void *data):
 }
 
 void
-IpCacheLookupForwarder::finalCallback(const Dns::CachedIps *addrs, const Dns::LookupDetails &details)
+IpCacheLookupForwarder::finalCallback(const Dns::CachedIps * const possiblyEmptyAddrs, const Dns::LookupDetails &details)
 {
+    // TODO: Consider removing nil-supplying IpcacheStats.invalid code and refactoring accordingly.
+    // may be nil but is never empty
+    const auto addrs = (possiblyEmptyAddrs && possiblyEmptyAddrs->empty()) ? nullptr : possiblyEmptyAddrs;
+
     debugs(14, 7, addrs << " " << details);
     if (receiverObj.set()) {
         if (auto receiver = receiverObj.valid())
             receiver->noteIps(addrs, details);
         receiverObj.clear();
     } else if (receiverFun) {
-        if (receiverData.valid()) {
-            const Dns::CachedIps *emptyIsNil = (addrs && !addrs->empty()) ? addrs : nullptr;
-            receiverFun(emptyIsNil, details, receiverData.validDone());
-        }
+        if (receiverData.valid())
+            receiverFun(addrs, details, receiverData.validDone());
         receiverFun = nullptr;
     }
 }
diff --git a/src/ipcache.h b/src/ipcache.h
index 89137baca5..b4eb20b394 100644
--- a/src/ipcache.h
+++ b/src/ipcache.h
@@ -199,6 +199,7 @@ public:
     /// Called when nbgethostbyname() fully resolves the name.
     /// The `ips` may contain both bad and good IP addresses, but each good IP
     /// (if any) is guaranteed to had been previously reported via noteIp().
+    /// When no IPs were obtained, `ips` is nil.
     virtual void noteIps(const CachedIps *ips, const LookupDetails &details) = 0;
 
     /// Called when/if nbgethostbyname() discovers a new good IP address.