From: Alex Rousskov Date: Sun, 22 Jul 2012 03:15:02 +0000 (-0600) Subject: Stop HttpRequest and AccessLogEntry memory leaks. X-Git-Tag: sourceformat-review-1~166 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=4bf68cfabb8a0db84bb0070541ca727179855edc;p=thirdparty%2Fsquid.git 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 1dfaca43dd..4373dabbf1 100644 --- 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 41347024df..df439e7745 100644 --- 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 0d95f6d864..35a3a443a9 100644 --- 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 AccessLogEntryPointer; class HttpRequest: public HttpMsg { @@ -244,12 +242,6 @@ public: */ CbcPointer 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 547256ba09..39131c790d 100644 --- 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 6d9753dcc0..24155320a2 100644 --- 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 b37a9995fb..1e46ec1bb1 100644 --- 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 691167a4fd..fde9b75bfc 100644 --- a/src/forward.h +++ b/src/forward.h @@ -3,6 +3,8 @@ /* forward decls */ +class AccessLogEntry; +typedef RefCount 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 c4e0b08a58..3ff14f6bdf 100644 --- 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 fc6582b76b..a37dd69e25 100644 --- 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 ee17c3408d..4cae62d539 100644 --- 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 0a54855b98..453defc154 100644 --- 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);