Stop HttpRequest and AccessLogEntry memory leaks.

The leaks were introduced by recent request_header_add improvements (r12213)
which needed server-side access to client-side information like client
certificate so that it can be stuffed into the outgoing request headers.
Stuffing was done via Format API that uses AccessLogEntry as the source of
information.

This change breaks the HttpRequest->AccessLogEntry->HttpRequest refcounting
loop that prevented both request and ale objects from being destroyed. The ale
object is now delivered to the server side using FwdState::Start() API.  Only
HTTP code currently takes advantage of ale availability on the server side.

Also had to modify httpHdrAdd() API because ale is no longer available via
the request pointer.

diff --git a/src/HttpHeaderTools.cc b/src/HttpHeaderTools.cc
index 1dfaca43ddc4f5971637e6b13709edb9558576dc..4373dabbf1415d66ad66bda038fbff17f28c1a9b 100644 (file)
--- a/src/HttpHeaderTools.cc
+++ b/src/HttpHeaderTools.cc
@@ -628,7 +628,7 @@ HeaderManglers::find(const HttpHeaderEntry &e) const
 }
 
 void
-httpHdrAdd(HttpHeader *heads, HttpRequest *request, HeaderWithAclList &headersAdd)
+httpHdrAdd(HttpHeader *heads, HttpRequest *request, const AccessLogEntryPointer &al, HeaderWithAclList &headersAdd)
 {
     ACLFilledChecklist checklist(NULL, request, NULL);
 
@@ -637,9 +637,9 @@ httpHdrAdd(HttpHeader *heads, HttpRequest *request, HeaderWithAclList &headersAd
             const char *fieldValue = NULL;
             MemBuf mb;
             if (hwa->quoted) {
-                if (request->al != NULL) {
+                if (al != NULL) {
                     mb.init();
-                    hwa->valueFormat->assemble(mb, request->al, 0);
+                    hwa->valueFormat->assemble(mb, al, 0);
                     fieldValue = mb.content();
                 }
             } else {

diff --git a/src/HttpRequest.cc b/src/HttpRequest.cc
index 41347024df9b37e552e2e2e5b4e729cadc628bcb..df439e77454fa4fd34bcd0995167f8eda4e0e76a 100644 (file)
--- a/src/HttpRequest.cc
+++ b/src/HttpRequest.cc
@@ -264,7 +264,6 @@ HttpRequest::inheritProperties(const HttpMsg *aMsg)
     // main property is which connection the request was received on (if any)
     clientConnectionManager = aReq->clientConnectionManager;
 
-    al = aReq->al;
     return true;
 }
 

diff --git a/src/HttpRequest.h b/src/HttpRequest.h
index 0d95f6d864cb2f30adb02e1db4b613dc4873d45b..35a3a443a9d8d2a48f7dd1ceb7ad1126737b403d 100644 (file)
--- a/src/HttpRequest.h
+++ b/src/HttpRequest.h
@@ -55,8 +55,6 @@ extern void httpRequestPack(void *obj, Packer *p);
 
 class HttpHdrRange;
 class DnsLookupDetails;
-class AccessLogEntry;
-typedef RefCount<AccessLogEntry> AccessLogEntryPointer;
 
 class HttpRequest: public HttpMsg
 {
@@ -244,12 +242,6 @@ public:
      */
     CbcPointer<ConnStateData> clientConnectionManager;
 
-    /**
-     * The AccessLogEntry for the current ClientHttpRequest/Server HttpRequest
-     * pair, if known;
-     */
-    AccessLogEntryPointer al;
-
     int64_t getRangeOffsetLimit(); /* the result of this function gets cached in rangeOffsetLimit */
 
 private:

diff --git a/src/client_side.cc b/src/client_side.cc
index 547256ba09a92d478acb0056bc1782301693c834..39131c790d67236490272a2caaf3f0bf70259e94 100644 (file)
--- a/src/client_side.cc
+++ b/src/client_side.cc
@@ -2634,7 +2634,6 @@ clientProcessRequest(ConnStateData *conn, HttpParser *hp, ClientSocketContext *c
     }
 
     request->clientConnectionManager = conn;
-    request->al = http->al;
 
     request->flags.accelerated = http->flags.accel;
     request->flags.sslBumped = conn->switchedToHttps();

diff --git a/src/client_side_reply.cc b/src/client_side_reply.cc
index 6d9753dcc0401d039f9d25992f4312bf4067478a..24155320a28a53334f5394a8014f944b228c392c 100644 (file)
--- a/src/client_side_reply.cc
+++ b/src/client_side_reply.cc
@@ -295,7 +295,7 @@ clientReplyContext::processExpired()
      * this clientReplyContext does
      */
     Comm::ConnectionPointer conn = http->getConn() != NULL ? http->getConn()->clientConnection : NULL;
-    FwdState::fwdStart(conn, http->storeEntry(), http->request);
+    FwdState::Start(conn, http->storeEntry(), http->request, http->al);
 
     /* Register with storage manager to receive updates when data comes in. */
 
@@ -680,7 +680,7 @@ clientReplyContext::processMiss()
 
         /** Start forwarding to get the new object from network */
         Comm::ConnectionPointer conn = http->getConn() != NULL ? http->getConn()->clientConnection : NULL;
-        FwdState::fwdStart(conn, http->storeEntry(), r);
+        FwdState::Start(conn, http->storeEntry(), r, http->al);
     }
 }
 

diff --git a/src/forward.cc b/src/forward.cc
index b37a9995fbc5390c689d09eded5795d8c1d8f256..1e46ec1bb145009724e03f61c643432b3ca88be6 100644 (file)
--- a/src/forward.cc
+++ b/src/forward.cc
@@ -33,6 +33,7 @@
 
 #include "squid-old.h"
 #include "forward.h"
+#include "AccessLogEntry.h"
 #include "acl/FilledChecklist.h"
 #include "acl/Gadgets.h"
 #include "anyp/PortCfg.h"
@@ -98,7 +99,8 @@ FwdState::abort(void* d)
 
 /**** PUBLIC INTERFACE ********************************************************/
 
-FwdState::FwdState(const Comm::ConnectionPointer &client, StoreEntry * e, HttpRequest * r)
+FwdState::FwdState(const Comm::ConnectionPointer &client, StoreEntry * e, HttpRequest * r, const AccessLogEntryPointer &alp):
+                   al(alp)
 {
     debugs(17, 2, HERE << "Forwarding client request " << client << ", url=" << e->url() );
     entry = e;
@@ -243,7 +245,7 @@ FwdState::~FwdState()
  * allocate a FwdState.
  */
 void
-FwdState::fwdStart(const Comm::ConnectionPointer &clientConn, StoreEntry *entry, HttpRequest *request)
+FwdState::Start(const Comm::ConnectionPointer &clientConn, StoreEntry *entry, HttpRequest *request, const AccessLogEntryPointer &al)
 {
     /** \note
      * client_addr == no_addr indicates this is an "internal" request
@@ -305,7 +307,7 @@ FwdState::fwdStart(const Comm::ConnectionPointer &clientConn, StoreEntry *entry,
         return;
 
     default:
-        FwdState::Pointer fwd = new FwdState(clientConn, entry, request);
+        FwdState::Pointer fwd = new FwdState(clientConn, entry, request, al);
         fwd->start(fwd);
         return;
     }
@@ -313,6 +315,13 @@ FwdState::fwdStart(const Comm::ConnectionPointer &clientConn, StoreEntry *entry,
     /* NOTREACHED */
 }
 
+void
+FwdState::fwdStart(const Comm::ConnectionPointer &clientConn, StoreEntry *entry, HttpRequest *request)
+{
+    // Hides AccessLogEntry.h from code that does not supply ALE anyway.
+    Start(clientConn, entry, request, NULL);
+}
+
 void
 FwdState::startConnectionOrFail()
 {

diff --git a/src/forward.h b/src/forward.h
index 691167a4fd6f6ffc83d8c3a494fe215233fc5da5..fde9b75bfc6dba7afb7936efc148ad7d88e4b2b5 100644 (file)
--- a/src/forward.h
+++ b/src/forward.h
@@ -3,6 +3,8 @@
 
 /* forward decls */
 
+class AccessLogEntry;
+typedef RefCount<AccessLogEntry> AccessLogEntryPointer;
 class ErrorState;
 class HttpRequest;
 
@@ -32,6 +34,9 @@ public:
     ~FwdState();
     static void initModule();
 
+    /// Initiates request forwarding to a peer or origin server.
+    static void Start(const Comm::ConnectionPointer &client, StoreEntry *, HttpRequest *, const AccessLogEntryPointer &alp);
+    /// Same as Start() but no master xaction info (AccessLogEntry) available.
     static void fwdStart(const Comm::ConnectionPointer &client, StoreEntry *, HttpRequest *);
 
     /// This is the real beginning of server connection. Call it whenever
@@ -66,7 +71,7 @@ public:
 
 private:
     // hidden for safer management of self; use static fwdStart
-    FwdState(const Comm::ConnectionPointer &client, StoreEntry *, HttpRequest *);
+    FwdState(const Comm::ConnectionPointer &client, StoreEntry *, HttpRequest *, const AccessLogEntryPointer &alp);
     void start(Pointer aSelf);
 
     void selectPeerForIntercepted();
@@ -80,6 +85,8 @@ private:
 public:
     StoreEntry *entry;
     HttpRequest *request;
+    AccessLogEntryPointer al; ///< info for the future access.log entry
+
     static void abort(void*);
 
 private:

diff --git a/src/htcp.cc b/src/htcp.cc
index c4e0b08a5896f8d54ba31833521ba0e763c809a5..3ff14f6bdf3f9c7bd0cdc96614184ed916144b3d 100644 (file)
--- a/src/htcp.cc
+++ b/src/htcp.cc
@@ -1574,7 +1574,7 @@ htcpQuery(StoreEntry * e, HttpRequest * req, peer * p)
     stuff.S.method = (char *) RequestMethodStr(req->method);
     stuff.S.uri = (char *) e->url();
     stuff.S.version = vbuf;
-    HttpStateData::httpBuildRequestHeader(req, e, &hdr, flags);
+    HttpStateData::httpBuildRequestHeader(req, e, NULL, &hdr, flags);
     mb.init();
     packerToMemInit(&pa, &mb);
     hdr.packInto(&pa);
@@ -1645,7 +1645,7 @@ htcpClear(StoreEntry * e, const char *uri, HttpRequest * req, const HttpRequestM
     }
     stuff.S.version = vbuf;
     if (reason != HTCP_CLR_INVALIDATION) {
-        HttpStateData::httpBuildRequestHeader(req, e, &hdr, flags);
+        HttpStateData::httpBuildRequestHeader(req, e, NULL, &hdr, flags);
         mb.init();
         packerToMemInit(&pa, &mb);
         hdr.packInto(&pa);

diff --git a/src/http.cc b/src/http.cc
index fc6582b76b3bce6ff534a62eda004ab9ef736948..a37dd69e255a2ec5bf0f1a18085ed8a4650f70fd 100644 (file)
--- a/src/http.cc
+++ b/src/http.cc
@@ -89,7 +89,7 @@ static void httpMaybeRemovePublic(StoreEntry *, http_status);
 static void copyOneHeaderFromClientsideRequestToUpstreamRequest(const HttpHeaderEntry *e, const String strConnection, const HttpRequest * request,
         HttpHeader * hdr_out, const int we_do_ranges, const http_state_flags);
 //Declared in HttpHeaderTools.cc
-void httpHdrAdd(HttpHeader *heads, HttpRequest *request, HeaderWithAclList &headers_add);
+void httpHdrAdd(HttpHeader *heads, HttpRequest *request, const AccessLogEntryPointer &al, HeaderWithAclList &headers_add);
 
 HttpStateData::HttpStateData(FwdState *theFwdState) : AsyncJob("HttpStateData"), ServerStateData(theFwdState),
         lastChunk(0), header_bytes_read(0), reply_bytes_read(0),
@@ -1608,6 +1608,7 @@ httpFixupAuthentication(HttpRequest * request, const HttpHeader * hdr_in, HttpHe
 void
 HttpStateData::httpBuildRequestHeader(HttpRequest * request,
                                       StoreEntry * entry,
+                                      const AccessLogEntryPointer &al,
                                       HttpHeader * hdr_out,
                                       const http_state_flags flags)
 {
@@ -1785,7 +1786,7 @@ HttpStateData::httpBuildRequestHeader(HttpRequest * request,
         httpHdrMangleList(hdr_out, request, ROR_REQUEST);
 
     if (Config.request_header_add && !Config.request_header_add->empty())
-        httpHdrAdd(hdr_out, request, *Config.request_header_add);
+        httpHdrAdd(hdr_out, request, al, *Config.request_header_add);
 
     strConnection.clean();
 }
@@ -2008,7 +2009,7 @@ HttpStateData::buildRequestPrefix(MemBuf * mb)
     {
         HttpHeader hdr(hoRequest);
         Packer p;
-        httpBuildRequestHeader(request, entry, &hdr, flags);
+        httpBuildRequestHeader(request, entry, fwd->al, &hdr, flags);
 
         if (request->flags.pinned && request->flags.connection_auth)
             request->flags.auth_sent = 1;

diff --git a/src/http.h b/src/http.h
index ee17c3408dfd2fa2edf249d79d0ab2d50134029a..4cae62d5390de16311b73565a344b30381b2904f 100644 (file)
--- a/src/http.h
+++ b/src/http.h
@@ -50,6 +50,7 @@ public:
 
     static void httpBuildRequestHeader(HttpRequest * request,
                                        StoreEntry * entry,
+                                       const AccessLogEntryPointer &al,
                                        HttpHeader * hdr_out,
                                        const http_state_flags flags);
 

diff --git a/src/tunnel.cc b/src/tunnel.cc
index 0a54855b98586054eba322b819ab078f5fea3886..453defc1543eb8f3be60b4b25512a06b2e2067c2 100644 (file)
--- a/src/tunnel.cc
+++ b/src/tunnel.cc
@@ -694,6 +694,7 @@ tunnelRelayConnectRequest(const Comm::ConnectionPointer &srv, void *data)
     mb.Printf("CONNECT %s HTTP/1.1\r\n", tunnelState->url);
     HttpStateData::httpBuildRequestHeader(tunnelState->request,
                                           NULL,                        /* StoreEntry */
+                                          NULL,                        /* AccessLogEntry */
                                           &hdr_out,
                                           flags);                      /* flags */
     packerToMemInit(&p, &mb);