Fix memory leak of AccessLogentry::url

 ... created by ACLFilledChecklist::syncAle().

::syncAle() is the only place in the codebase that assigns a URL that
AccessLogEntry is expected to free(), which AccessLogEntry doesn't do.
This results in a memory leak.

  This is submitted on behalf of Bloomberg L.P.

diff --git a/src/AccessLogEntry.h b/src/AccessLogEntry.h
index 5968ab2f0f17b97eae8e8b1d768c45e1d344fd0e..6665bd887e892c8a67a8964ee93cd59f73c395a6 100644 (file)
--- a/src/AccessLogEntry.h
+++ b/src/AccessLogEntry.h
@@ -21,6 +21,7 @@
 #include "LogTags.h"
 #include "MessageSizes.h"
 #include "Notes.h"
+#include "sbuf/SBuf.h"
 #if ICAP_CLIENT
 #include "adaptation/icap/Elements.h"
 #endif
@@ -56,7 +57,7 @@ public:
     /// Fetch the transaction method string (ICP opcode, HTCP opcode or HTTP method)
     SBuf getLogMethod() const;
 
-    const char *url;
+    SBuf url;
 
     /// TCP/IP level details about the client connection
     Comm::ConnectionPointer tcpClient;

diff --git a/src/acl/FilledChecklist.cc b/src/acl/FilledChecklist.cc
index dd5973407a500b9da9037f433dcad7b40a9b87f2..271c6bc2f6e56683d557b251247c5195cfb47ec6 100644 (file)
--- a/src/acl/FilledChecklist.cc
+++ b/src/acl/FilledChecklist.cc
@@ -103,9 +103,9 @@ ACLFilledChecklist::syncAle() const
             HTTPMSGLOCK(al->adapted_request);
         }
 
-        if (!al->url) {
+        if (al->url.isEmpty()) {
             showDebugWarning("URL");
-            al->url = xstrdup(request->url.absolute().c_str());
+            al->url = request->url.absolute();
         }
     }
 

diff --git a/src/format/Format.cc b/src/format/Format.cc
index c096aa4ee33b322ebdcbb030c98bb50d528ad4a5..7ca4f53036ae9e32abcd974ced56c68750b54b9b 100644 (file)
--- a/src/format/Format.cc
+++ b/src/format/Format.cc
@@ -34,6 +34,8 @@
 /// Convert a string to NULL pointer if it is ""
 #define strOrNull(s) ((s)==NULL||(s)[0]=='\0'?NULL:(s))
 
+const SBuf Format::Dash("-");
+
 Format::Format::Format(const char *n) :
     format(NULL),
     next(NULL)
@@ -1039,7 +1041,11 @@ Format::Format::assemble(MemBuf &mb, const AccessLogEntry::Pointer &al, int logS
         break;
 
         case LFT_REQUEST_URI:
-            out = al->url;
+            if (!al->url.isEmpty()) {
+                const SBuf &s = al->url;
+                sb.append(s.rawContent(), s.length());
+                out = sb.termedBuf();
+            }
             break;
 
         case LFT_REQUEST_VERSION_OLD_2X:

diff --git a/src/format/Format.h b/src/format/Format.h
index 4e764dace88f458c2e4dc4d2d024ba099e93a27c..08249ecc9d720443ebc472b75d22600ca48cd4e3 100644 (file)
--- a/src/format/Format.h
+++ b/src/format/Format.h
@@ -11,6 +11,7 @@
 
 #include "base/RefCount.h"
 #include "ConfigParser.h"
+#include "sbuf/SBuf.h"
 
 /*
  * Squid configuration allows users to define custom formats in
@@ -32,6 +33,8 @@ class StoreEntry;
 namespace Format
 {
 
+extern const SBuf Dash;
+
 class Token;
 
 // XXX: inherit from linked list

diff --git a/src/log/FormatHttpdCombined.cc b/src/log/FormatHttpdCombined.cc
index 6af7e7400517212a6a6ecd207c70d9975950fe4c..a063f8928b875e12c01ffa40f2f00ce3c4a3e535 100644 (file)
--- a/src/log/FormatHttpdCombined.cc
+++ b/src/log/FormatHttpdCombined.cc
@@ -47,13 +47,13 @@ Log::Format::HttpdCombined(const AccessLogEntry::Pointer &al, Logfile * logfile)
 
     const SBuf method(al->getLogMethod());
 
-    logfilePrintf(logfile, "%s %s %s [%s] \"" SQUIDSBUFPH " %s %s/%d.%d\" %d %" PRId64 " \"%s\" \"%s\" %s:%s%s",
+    logfilePrintf(logfile, "%s %s %s [%s] \"" SQUIDSBUFPH " " SQUIDSBUFPH " %s/%d.%d\" %d %" PRId64 " \"%s\" \"%s\" %s:%s%s",
                   clientip,
                   user_ident ? user_ident : dash_str,
                   user_auth ? user_auth : dash_str,
                   Time::FormatHttpd(squid_curtime),
                   SQUIDSBUFPRINT(method),
-                  al->url,
+                  SQUIDSBUFPRINT(al->url),
                   AnyP::ProtocolType_str[al->http.version.protocol],
                   al->http.version.major, al->http.version.minor,
                   al->http.code,

diff --git a/src/log/FormatHttpdCommon.cc b/src/log/FormatHttpdCommon.cc
index 0d191c174025c898e287d4a2bde0ceb4b44a58f1..b14036ec031e9d7b827d35c9aa21f522f0e209c8 100644 (file)
--- a/src/log/FormatHttpdCommon.cc
+++ b/src/log/FormatHttpdCommon.cc
@@ -34,13 +34,13 @@ Log::Format::HttpdCommon(const AccessLogEntry::Pointer &al, Logfile * logfile)
 
     const SBuf method(al->getLogMethod());
 
-    logfilePrintf(logfile, "%s %s %s [%s] \"" SQUIDSBUFPH " %s %s/%d.%d\" %d %" PRId64 " %s:%s%s",
+    logfilePrintf(logfile, "%s %s %s [%s] \"" SQUIDSBUFPH " " SQUIDSBUFPH " %s/%d.%d\" %d %" PRId64 " %s:%s%s",
                   clientip,
                   user_ident ? user_ident : dash_str,
                   user_auth ? user_auth : dash_str,
                   Time::FormatHttpd(squid_curtime),
                   SQUIDSBUFPRINT(method),
-                  al->url,
+                  SQUIDSBUFPRINT(al->url),
                   AnyP::ProtocolType_str[al->http.version.protocol],
                   al->http.version.major, al->http.version.minor,
                   al->http.code,

diff --git a/src/log/FormatSquidNative.cc b/src/log/FormatSquidNative.cc
index 562631ed19dbd42477e29e059e2fe0c72695f7cf..594e11b75d11d68e7564345230f07c883e62845c 100644 (file)
--- a/src/log/FormatSquidNative.cc
+++ b/src/log/FormatSquidNative.cc
@@ -50,7 +50,7 @@ Log::Format::SquidNative(const AccessLogEntry::Pointer &al, Logfile * logfile)
 
     const SBuf method(al->getLogMethod());
 
-    logfilePrintf(logfile, "%9ld.%03d %6ld %s %s/%03d %" PRId64 " " SQUIDSBUFPH " %s %s %s%s/%s %s%s",
+    logfilePrintf(logfile, "%9ld.%03d %6ld %s %s/%03d %" PRId64 " " SQUIDSBUFPH " " SQUIDSBUFPH " %s %s%s/%s %s%s",
                   (long int) current_time.tv_sec,
                   (int) current_time.tv_usec / 1000,
                   tvToMsec(al->cache.trTime),
@@ -59,7 +59,7 @@ Log::Format::SquidNative(const AccessLogEntry::Pointer &al, Logfile * logfile)
                   al->http.code,
                   al->http.clientReplySz.messageTotal(),
                   SQUIDSBUFPRINT(method),
-                  al->url,
+                  SQUIDSBUFPRINT(al->url),
                   user ? user : dash_str,
                   al->hier.ping.timedout ? "TIMEOUT_" : "",
                   hier_code_str[al->hier.code],

diff --git a/src/log/FormatSquidReferer.cc b/src/log/FormatSquidReferer.cc
index 0ccadcfb1bfcbd15de1404c299e9fbf71d1a7104..1efc2b0f182a791d574454624b3bd330d7469a27 100644 (file)
--- a/src/log/FormatSquidReferer.cc
+++ b/src/log/FormatSquidReferer.cc
@@ -28,11 +28,13 @@ Log::Format::SquidReferer(const AccessLogEntry::Pointer &al, Logfile *logfile)
     char clientip[MAX_IPSTRLEN];
     al->getLogClientIp(clientip, MAX_IPSTRLEN);
 
-    logfilePrintf(logfile, "%9ld.%03d %s %s %s\n",
+    const SBuf url = !al->url.isEmpty() ? al->url : ::Format::Dash;
+
+    logfilePrintf(logfile, "%9ld.%03d %s %s " SQUIDSBUFPH "\n",
                   (long int) current_time.tv_sec,
                   (int) current_time.tv_usec / 1000,
                   clientip,
                   referer,
-                  al->url ? al->url : "-");
+                  SQUIDSBUFPRINT(url));
 }
 

diff --git a/src/log/access_log.cc b/src/log/access_log.cc
index 8c8cb8f6499727be5c7b00262631054f0c05c90b..a3fc35a4b0c41e2c39f5762f62492f0a8e84237d 100644 (file)
--- a/src/log/access_log.cc
+++ b/src/log/access_log.cc
@@ -76,8 +76,8 @@ void
 accessLogLogTo(CustomLog* log, AccessLogEntry::Pointer &al, ACLChecklist * checklist)
 {
 
-    if (al->url == NULL)
-        al->url = dash_str;
+    if (al->url.isEmpty())
+        al->url = Format::Dash;
 
     if (!al->http.content_type || *al->http.content_type == '\0')
         al->http.content_type = dash_str;
@@ -160,8 +160,8 @@ accessLogLog(AccessLogEntry::Pointer &al, ACLChecklist * checklist)
     else {
         unsigned int ibuf[365];
         size_t isize;
-        xstrncpy((char *) ibuf, al->url, 364 * sizeof(int));
-        isize = ((strlen(al->url) + 8) / 8) * 2;
+        xstrncpy((char *) ibuf, al->url.c_str(), 364 * sizeof(int));
+        isize = ((al->url.length() + 8) / 8) * 2;
 
         if (isize > 364)
             isize = 364;