Logformat %lp expands to "-" in wildcard listening port configs (#997)

FindListeningPortAddress() and its helpers look for "local" address of
an accepted connection. When FindListeningPortAddress() is called in %la
context, we must skip Ip::Address objects with INADDR_ANY IP addresses
because we are looking for a specific IP address, not a wildcard.
However, when called in %lp context, skipping those Ip::Address objects
may, in some cases, result in skipping the only object that actually
contains the port information, resulting in %lp expanding to "-".

Similarly, zero-port Ip::Address objects (with non-any IPs) could be, in
theory, returned instead of continuing the search for an object with a
non-zero port number, although this case was not observed in tests.

Now we configure the address searching helpers with a "good Ip::Address
object" filter so that each %code code path can customize its search.

The problem was introduced in commit ea35939 that expanded INADDR_ANY
check scope from %la to %lp.

diff --git a/src/HttpRequest.cc b/src/HttpRequest.cc
index 5d43aee163f03b5b4ecec7a7eaf4c6d7cb61c032..08d43019ff00129eb1c0e13dae4fccd2d13ecfd6 100644 (file)
--- a/src/HttpRequest.cc
+++ b/src/HttpRequest.cc
@@ -824,30 +824,25 @@ HttpRequest::canonicalCleanUrl() const
     return urlCanonicalCleanWithoutRequest(effectiveRequestUri(), method, url.getScheme());
 }
 
-/// a helper for validating FindListeningPortAddress()-found address candidates
-static const Ip::Address *
-FindListeningPortAddressInAddress(const Ip::Address *ip)
-{
-    // FindListeningPortAddress() callers do not want INADDR_ANY addresses
-    return (ip && !ip->isAnyAddr()) ? ip : nullptr;
-}
-
 /// a helper for handling PortCfg cases of FindListeningPortAddress()
+template <typename Filter>
 static const Ip::Address *
-FindListeningPortAddressInPort(const AnyP::PortCfgPointer &port)
+FindGoodListeningPortAddressInPort(const AnyP::PortCfgPointer &port, const Filter isGood)
 {
-    return port ? FindListeningPortAddressInAddress(&port->s) : nullptr;
+    return (port && isGood(port->s)) ? &port->s : nullptr;
 }
 
 /// a helper for handling Connection cases of FindListeningPortAddress()
+template <typename Filter>
 static const Ip::Address *
-FindListeningPortAddressInConn(const Comm::ConnectionPointer &conn)
+FindGoodListeningPortAddressInConn(const Comm::ConnectionPointer &conn, const Filter isGood)
 {
-    return conn ? FindListeningPortAddressInAddress(&conn->local) : nullptr;
+    return (conn && isGood(conn->local)) ? &conn->local : nullptr;
 }
 
+template <typename Filter>
 const Ip::Address *
-FindListeningPortAddress(const HttpRequest *callerRequest, const AccessLogEntry *ale)
+FindGoodListeningPortAddress(const HttpRequest *callerRequest, const AccessLogEntry *ale, const Filter filter)
 {
     // Check all sources of usable listening port information, giving
     // HttpRequest and masterXaction a preference over ALE.
@@ -858,18 +853,35 @@ FindListeningPortAddress(const HttpRequest *callerRequest, const AccessLogEntry
     if (!request)
         return nullptr; // not enough information
 
-    const Ip::Address *ip = FindListeningPortAddressInPort(request->masterXaction->squidPort);
+    auto ip = FindGoodListeningPortAddressInPort(request->masterXaction->squidPort, filter);
     if (!ip && ale)
-        ip = FindListeningPortAddressInPort(ale->cache.port);
+        ip = FindGoodListeningPortAddressInPort(ale->cache.port, filter);
 
     // XXX: also handle PROXY protocol here when we have a flag to identify such request
     if (ip || request->flags.interceptTproxy || request->flags.intercepted)
         return ip;
 
     /* handle non-intercepted cases that were not handled above */
-    ip = FindListeningPortAddressInConn(request->masterXaction->tcpClient);
+    ip = FindGoodListeningPortAddressInConn(request->masterXaction->tcpClient, filter);
     if (!ip && ale)
-        ip = FindListeningPortAddressInConn(ale->tcpClient);
+        ip = FindGoodListeningPortAddressInConn(ale->tcpClient, filter);
     return ip; // may still be nil
 }
 
+const Ip::Address *
+FindListeningPortAddress(const HttpRequest *callerRequest, const AccessLogEntry *ale)
+{
+    return FindGoodListeningPortAddress(callerRequest, ale, [](const Ip::Address &address) {
+        // FindListeningPortAddress() callers do not want INADDR_ANY addresses
+        return !address.isAnyAddr();
+    });
+}
+
+unsigned short
+FindListeningPortNumber(const HttpRequest *callerRequest, const AccessLogEntry *ale)
+{
+    const auto ip = FindGoodListeningPortAddress(callerRequest, ale, [](const Ip::Address &address) {
+        return address.port() > 0;
+    });
+    return ip ? ip->port() : 0;
+}

diff --git a/src/HttpRequest.h b/src/HttpRequest.h
index 3eaf2ac5e720b647c269f9e2fb8308cbc0a16cf8..98aeea9b56edde9c2f3471cc785614b36e268d8f 100644 (file)
--- a/src/HttpRequest.h
+++ b/src/HttpRequest.h
@@ -285,5 +285,9 @@ void UpdateRequestNotes(ConnStateData *csd, HttpRequest &request, NotePairs cons
 /// nil parameter(s) indicate missing caller information and are handled safely
 const Ip::Address *FindListeningPortAddress(const HttpRequest *, const AccessLogEntry *);
 
+/// \returns listening/*_port port number used by the client connection (or zero)
+/// nil parameter(s) indicate missing caller information and are handled safely
+unsigned short FindListeningPortNumber(const HttpRequest *, const AccessLogEntry *);
+
 #endif /* SQUID_HTTPREQUEST_H */
 

diff --git a/src/format/Format.cc b/src/format/Format.cc
index 968fb235a4a3bafbdc2a686f18147bf2f22b7992..4d23236e7bd01e1cbe410427545049bf8151c209 100644 (file)
--- a/src/format/Format.cc
+++ b/src/format/Format.cc
@@ -507,8 +507,8 @@ Format::Format::assemble(MemBuf &mb, const AccessLogEntry::Pointer &al, int logS
             break;
 
         case LFT_LOCAL_LISTENING_PORT:
-            if (const auto addr = FindListeningPortAddress(nullptr, al.getRaw())) {
-                outint = addr->port();
+            if (const auto port = FindListeningPortNumber(nullptr, al.getRaw())) {
+                outint = port;
                 doint = 1;
             }
             break;