Hello,
The attached patch is a demonstration of a memory leak on
AccessLogentry::url, created by ACLFilledChecklist::syncAle(). The
patch itself is not good enough quality to warrant acceptance, and no
doubt introduces bugs.
::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.
As mentioned, the patch is of questionable quality, as the chunk in
src/client_side.cc shows. This sort of change would have to be made
everywhere that url is assigned to, which isn't practical.
It would be good to modify ::syncAle() so that it doesn't allocate a
new URL, but I'm not sure how that could be achieved. I think the only
workable alternative is to change AccessLogEntry::url to a more
intelligent string type. There is also the possibility of making the
member private and using a setter to ensure it is always xstrdup'd
from another URL.
If someone with better knowledge than me would like to make a
recommendation on what to do here, I'd be happy to work towards a
better solution.
Thank you,
Nathan.
Demonstration of a memory leak in Squid via an incomplete patch.
This is submitted on behalf of Bloomberg L.P.
=== modified file 'src/AccessLogEntry.cc'
--- src/AccessLogEntry.cc 2016-01-01 00:12:18 +0000
+++ src/AccessLogEntry.cc 2016-03-10 23:06:37 +0000
@@ -72,6 +72,10 @@
safe_free(headers.reply);
+ if (free_url) {
+ safe_free(url);
+ }
+
safe_free(headers.adapted_request);
HTTPMSGUNLOCK(adapted_request);
=== modified file 'src/AccessLogEntry.h'
--- src/AccessLogEntry.h 2016-01-01 00:12:18 +0000
+++ src/AccessLogEntry.h 2016-03-10 23:06:59 +0000
@@ -41,6 +41,7 @@
AccessLogEntry() :
url(nullptr),
+ free_url(false),
lastAclName(nullptr),
lastAclData(nullptr),
reply(nullptr),
@@ -58,6 +59,7 @@
SBuf getLogMethod() const;
const char *url;
+ bool free_url;
/// TCP/IP level details about the client connection
Comm::ConnectionPointer tcpClient;
=== modified file 'src/acl/FilledChecklist.cc'
--- src/acl/FilledChecklist.cc 2016-01-31 12:05:30 +0000
+++ src/acl/FilledChecklist.cc 2016-03-10 23:06:08 +0000
@@ -106,6 +106,7 @@
if (!al->url) {
showDebugWarning("URL");
al->url = xstrdup(request->url.absolute().c_str());
+ al->free_url = true;
}
}
=== modified file 'src/client_side.cc'
--- src/client_side.cc 2016-02-13 07:51:20 +0000
+++ src/client_side.cc 2016-03-10 23:06:36 +0000
@@ -383,6 +383,10 @@
debugs(33, 5, "logging half-baked transaction: " << log_uri);
al->icp.opcode = ICP_INVALID;
+ if (al->url && al->free_url) {
+ safe_free(al->url);
+ al->free_url = false;
+ }
al->url = log_uri;
debugs(33, 9, "clientLogRequest: al.url='" << al->url << "'");
_______________________________________________
squid-dev mailing list
[email protected]
http://lists.squid-cache.org/listinfo/squid-dev
