--------------------- PatchSet 1633 Date: 2005/08/31 20:03:39 Author: dwsquid Branch: squid3-icap Tag: (none) Log: fixed ICAP-related memory leaks and other bugs. Note that MsgPipe is now refcounted. Members: src/ICAPAnchor.cc:1.1.2.21->1.1.2.22 src/ICAPAnchor.h:1.1.2.7->1.1.2.8 src/ICAPClient.cc:1.1.2.6->1.1.2.7 src/ICAPClient.h:1.1.2.5->1.1.2.6 src/ICAPXaction.cc:1.1.2.13->1.1.2.14 src/ICAPXaction.h:1.1.2.8->1.1.2.9 src/MemBuf.cci:1.2.20.2->1.2.20.3 src/MemBuf.h:1.3.16.6->1.3.16.7 src/MsgPipe.h:1.1.2.5->1.1.2.6 src/MsgPipeData.h:1.1.2.8->1.1.2.9 src/MsgPipeEnd.h:1.1.2.2->1.1.2.3 src/http.cc:1.49.2.20->1.49.2.21 src/http.h:1.11.4.8->1.11.4.9 Index: squid3/src/ICAPAnchor.cc =================================================================== RCS file: /cvsroot/squid-sf//squid3/src/Attic/ICAPAnchor.cc,v retrieving revision 1.1.2.21 retrieving revision 1.1.2.22 diff -u -r1.1.2.21 -r1.1.2.22 --- squid3/src/ICAPAnchor.cc 31 Aug 2005 19:39:10 -0000 1.1.2.21 +++ squid3/src/ICAPAnchor.cc 31 Aug 2005 20:03:39 -0000 1.1.2.22 @@ -2,6 +2,8 @@ #include "http.h" #include "MsgPipe.h" #include "MsgPipeData.h" +#include "MsgPipeSource.h" +#include "MsgPipeSink.h" #include "HttpRequest.h" #include "HttpReply.h" #include "ICAPAnchor.h" @@ -15,7 +17,7 @@ ICAPAnchor::ICAPAnchor(): httpState(NULL), virgin(NULL), adapted(NULL) { - debug(93,0)("ICAPAnchor constructed, this=%p\n", this); + debug(93,5)("ICAPAnchor constructed, this=%p\n", this); } ICAPAnchor::~ICAPAnchor() @@ -24,18 +26,11 @@ cbdataReferenceDone(httpState); debug(93,5)("ICAPAnchor destructed, this=%p\n", this); - if (virgin) { - delete virgin->data->body; - delete virgin->data; - leakFree(virgin, leaky); - delete virgin; - } + if (virgin != NULL) + freeVirgin(); - if (adapted) { - delete adapted->data; - leakFree(adapted, leaky); - delete adapted; - } + if (adapted != NULL) + freeAdapted(); leaky->dump(); } @@ -45,16 +40,16 @@ httpState = cbdataReference(anHttpState); virgin = new MsgPipe("virgin"); // this is the place to create a refcount ptr - leakAdd(virgin, leaky); + leakAdd(virgin.getRaw(), leaky); virgin->source = this; virgin->data = new MsgPipeData; - virgin->data->cause = request; + virgin->data->cause = requestLink(request); virgin->data->header = reply; virgin->data->body = new MemBuf; memBufInit(virgin->data->body, ICAPMsgPipeBufSizeMin, ICAPMsgPipeBufSizeMax); adapted = new MsgPipe("adapted"); - leakAdd(adapted, leaky); + leakAdd(adapted.getRaw(), leaky); adapted->sink = this; #if ICAP_ANCHOR_LOOPBACK @@ -78,7 +73,7 @@ * than will fit in body MemBuf. Caller should use * potentialSpaceSize() to find out how much we can hold. */ - leakTouch(virgin, leaky); + leakTouch(virgin.getRaw(), leaky); virgin->data->body->append(buf.data, buf.length); virgin->sendSourceProgress(); } @@ -86,7 +81,7 @@ int ICAPAnchor::potentialSpaceSize() { - leakTouch(virgin, leaky); + leakTouch(virgin.getRaw(), leaky); return (int) virgin->data->body->potentialSpaceSize(); } @@ -103,7 +98,7 @@ return; #else - leakTouch(virgin, leaky); + leakTouch(virgin.getRaw(), leaky); virgin->sendSourceFinish(); #endif } @@ -120,7 +115,7 @@ { debug(93,5)("ICAPAnchor::noteSinkNeed() called\n"); - leakTouch(virgin, leaky); + leakTouch(virgin.getRaw(), leaky); if (virgin->data->body->potentialSpaceSize()) httpState->icapSpaceAvailable(); @@ -138,7 +133,7 @@ void ICAPAnchor::noteSourceStart(MsgPipe *p) { debug(93,5)("ICAPAnchor::noteSourceStart() called\n"); - leakTouch(adapted, leaky); + leakTouch(adapted.getRaw(), leaky); httpState->takeAdaptedHeaders(adapted->data->header); noteSourceProgress(p); } @@ -173,36 +168,26 @@ // internal cleanup void ICAPAnchor::stop(Notify notify) { - if (virgin) { + if (virgin != NULL) { + leakTouch(virgin.getRaw(), leaky); + if (notify == notifyIcap) virgin->sendSourceAbort(); else virgin->source = NULL; - // this is the place to decrement refcount ptr - delete virgin->data->body; - - delete virgin->data; - - leakFree(virgin, leaky); - - delete virgin; - - virgin = NULL; + freeVirgin(); } - if (adapted) { + if (adapted != NULL) { + leakTouch(adapted.getRaw(), leaky); + if (notify == notifyIcap) adapted->sendSinkAbort(); else adapted->sink = NULL; - // this is the place to decrement refcount ptr - delete adapted->data; - - leakFree(adapted, leaky); - - adapted = NULL; + freeAdapted(); } if (httpState) { @@ -212,7 +197,22 @@ cbdataReferenceDone(httpState); - // httpSTate is now NULL, will not call it any more + // httpState is now NULL, will not call it any more } } +void ICAPAnchor::freeVirgin() +{ + requestUnlink(virgin->data->cause); + virgin->data->cause = NULL; + virgin->data->header = NULL; + leakFree(virgin.getRaw(), leaky); + virgin = NULL; // refcounted +} + +void ICAPAnchor::freeAdapted() +{ + adapted->data->header = NULL; // we don't own it + leakFree(adapted.getRaw(), leaky); + adapted = NULL; // refcounted +} Index: squid3/src/ICAPAnchor.h =================================================================== RCS file: /cvsroot/squid-sf//squid3/src/Attic/ICAPAnchor.h,v retrieving revision 1.1.2.7 retrieving revision 1.1.2.8 diff -u -r1.1.2.7 -r1.1.2.8 --- squid3/src/ICAPAnchor.h 26 Aug 2005 20:28:41 -0000 1.1.2.7 +++ squid3/src/ICAPAnchor.h 31 Aug 2005 20:03:39 -0000 1.1.2.8 @@ -1,6 +1,6 @@ /* - * $Id: ICAPAnchor.h,v 1.1.2.7 2005/08/26 20:28:41 dwsquid Exp $ + * $Id: ICAPAnchor.h,v 1.1.2.8 2005/08/31 20:03:39 dwsquid Exp $ * * * SQUID Web Proxy Cache http://www.squid-cache.org/ @@ -34,6 +34,7 @@ #ifndef SQUID_ICAPANCHOR_H #define SQUID_ICAPANCHOR_H +#include "MsgPipe.h" #include "MsgPipeSource.h" #include "MsgPipeSink.h" @@ -75,12 +76,14 @@ public: HttpStateData *httpState; - MsgPipe *virgin; - MsgPipe *adapted; + MsgPipe::Pointer virgin; + MsgPipe::Pointer adapted; private: typedef enum { notifyNone, notifyOwner, notifyIcap } Notify; void stop(Notify notify); + void freeVirgin(); + void freeAdapted(); CBDATA_CLASS2(ICAPAnchor); }; Index: squid3/src/ICAPClient.cc =================================================================== RCS file: /cvsroot/squid-sf//squid3/src/Attic/ICAPClient.cc,v retrieving revision 1.1.2.6 retrieving revision 1.1.2.7 diff -u -r1.1.2.6 -r1.1.2.7 --- squid3/src/ICAPClient.cc 26 Aug 2005 20:28:41 -0000 1.1.2.6 +++ squid3/src/ICAPClient.cc 31 Aug 2005 20:03:39 -0000 1.1.2.7 @@ -18,7 +18,7 @@ {} // initialize ICAP-specific ends of message pipes -void ICAPInitXaction(MsgPipe *virgin, MsgPipe *adapted) +void ICAPInitXaction(MsgPipe::Pointer virgin, MsgPipe::Pointer adapted) { ICAPXaction::Pointer x = new ICAPXaction; debugs(93,5, "ICAPInitXaction: " << x.getRaw()); Index: squid3/src/ICAPClient.h =================================================================== RCS file: /cvsroot/squid-sf//squid3/src/Attic/ICAPClient.h,v retrieving revision 1.1.2.5 retrieving revision 1.1.2.6 diff -u -r1.1.2.5 -r1.1.2.6 --- squid3/src/ICAPClient.h 26 Aug 2005 20:28:41 -0000 1.1.2.5 +++ squid3/src/ICAPClient.h 31 Aug 2005 20:03:39 -0000 1.1.2.6 @@ -1,6 +1,6 @@ /* - * $Id: ICAPClient.h,v 1.1.2.5 2005/08/26 20:28:41 dwsquid Exp $ + * $Id: ICAPClient.h,v 1.1.2.6 2005/08/31 20:03:39 dwsquid Exp $ * * * SQUID Web Proxy Cache http://www.squid-cache.org/ @@ -45,7 +45,7 @@ // let ICAP initialize ICAP-specific ends of message pipes class MsgPipe; -extern void ICAPInitXaction(MsgPipe *virgin, MsgPipe *adapted); +extern void ICAPInitXaction(MsgPipe::Pointer virgin, MsgPipe::Pointer adapted); // recommended initial size and max capacity for MsgPipe buffer enum { ICAPMsgPipeBufSizeMin = (4*1024), ICAPMsgPipeBufSizeMax = SQUID_TCP_SO_RCVBUF }; Index: squid3/src/ICAPXaction.cc =================================================================== RCS file: /cvsroot/squid-sf//squid3/src/Attic/ICAPXaction.cc,v retrieving revision 1.1.2.13 retrieving revision 1.1.2.14 diff -u -r1.1.2.13 -r1.1.2.14 --- squid3/src/ICAPXaction.cc 26 Aug 2005 20:28:41 -0000 1.1.2.13 +++ squid3/src/ICAPXaction.cc 31 Aug 2005 20:03:39 -0000 1.1.2.14 @@ -107,15 +107,12 @@ { notify = notifyNone; doStop(); - - if (icapReply) - httpReplyDestroy(icapReply); } -void ICAPXaction::init(MsgPipe *aVirgin, MsgPipe *anAdapted, Pointer &aSelf) +void ICAPXaction::init(MsgPipe::Pointer aVirgin, MsgPipe::Pointer anAdapted, Pointer &aSelf) { - assert(!self.getRaw() && !virgin && !adapted); - assert(aSelf.getRaw() && aVirgin && anAdapted); + assert(!self.getRaw() && !virgin.getRaw() && !adapted.getRaw()); + assert(aSelf.getRaw() && aVirgin.getRaw() && anAdapted.getRaw()); self = aSelf; virgin = aVirgin; @@ -266,6 +263,8 @@ // comm will free the chunk state.isWriting = &ICAPXaction_noteCommWroteBody; comm_old_write_mbuf(connection, writeBuf, state.isWriting, this); + } else { + memBufClean(&writeBuf); } } @@ -326,8 +325,11 @@ void ICAPXaction::startReading() { - Must(connection >= 0 && !state.isReading && - adapted && adapted->data && adapted->data->body); + Must(connection >= 0); + Must(!state.isReading); + Must(adapted.getRaw()); + Must(adapted->data); + Must(adapted->data->body); readMore(); } @@ -453,7 +455,7 @@ state.doneSending = true; adapted->sendSourceFinish(); delete bodyParser; - bodyParser = 0; + bodyParser = NULL; } bool ICAPXaction::parsePresentBody() @@ -554,28 +556,31 @@ debugs(93, 5, "ICAPXaction::doStop"); memBufClean(&readBuf); - memBufClean(&requestBuf); + // don't clean requestBuf because comm_old_write_mbuf() does it. + + if (icapReply) + httpReplyDestroy(icapReply); + + icapReply = NULL; closeConnection(); // TODO: pconn support: close unless notifyService ... - if (virgin) { + if (virgin != NULL) { if (notify == notifyHttp || notify == notifyAll) virgin->sendSinkAbort(); else virgin->sink = NULL; - // this is the place to decrement refcount ptr - virgin = NULL; + virgin = NULL; // refcounted } - if (adapted) { + if (adapted != NULL) { if (notify == notifyHttp || notify == notifyAll) adapted->sendSourceAbort(); else adapted->source = NULL; - // this is the place to decrement refcount ptr - adapted = NULL; + adapted = NULL; // refcounted } if (self != NULL) { // even if notify is notifyNone @@ -630,6 +635,7 @@ /* ICAP request body */ buf->append(httpBuf.content(), httpBuf.contentSize()); + memBufClean(&httpBuf); } bool ICAPXaction::callStart(const char *method) Index: squid3/src/ICAPXaction.h =================================================================== RCS file: /cvsroot/squid-sf//squid3/src/Attic/ICAPXaction.h,v retrieving revision 1.1.2.8 retrieving revision 1.1.2.9 diff -u -r1.1.2.8 -r1.1.2.9 --- squid3/src/ICAPXaction.h 26 Aug 2005 20:28:41 -0000 1.1.2.8 +++ squid3/src/ICAPXaction.h 31 Aug 2005 20:03:39 -0000 1.1.2.9 @@ -1,6 +1,6 @@ /* - * $Id: ICAPXaction.h,v 1.1.2.8 2005/08/26 20:28:41 dwsquid Exp $ + * $Id: ICAPXaction.h,v 1.1.2.9 2005/08/31 20:03:39 dwsquid Exp $ * * * SQUID Web Proxy Cache http://www.squid-cache.org/ @@ -35,6 +35,7 @@ #define SQUID_ICAPXACTION_H #include "MemBuf.h" +#include "MsgPipe.h" #include "MsgPipeSource.h" #include "MsgPipeSink.h" @@ -59,7 +60,7 @@ virtual ~ICAPXaction(); // called by ICAPClient - void init(MsgPipe *aVirgin, MsgPipe *anAdapted, Pointer &aSelf); + void init(MsgPipe::Pointer aVirgin, MsgPipe::Pointer anAdapted, Pointer &aSelf); void ownerAbort(); // pipe source methods; called by Anchor while receiving the adapted msg @@ -116,8 +117,8 @@ bool gotEncapsulated(const char *section) const; Pointer self; - MsgPipe *virgin; - MsgPipe *adapted; + MsgPipe::Pointer virgin; + MsgPipe::Pointer adapted; HttpReply *icapReply; Index: squid3/src/MemBuf.cci =================================================================== RCS file: /cvsroot/squid-sf//squid3/src/MemBuf.cci,v retrieving revision 1.2.20.2 retrieving revision 1.2.20.3 diff -u -r1.2.20.2 -r1.2.20.3 --- squid3/src/MemBuf.cci 31 Aug 2005 19:39:10 -0000 1.2.20.2 +++ squid3/src/MemBuf.cci 31 Aug 2005 20:03:39 -0000 1.2.20.3 @@ -1,6 +1,6 @@ /* - * $Id: MemBuf.cci,v 1.2.20.2 2005/08/31 19:39:10 dwsquid Exp $ + * $Id: MemBuf.cci,v 1.2.20.3 2005/08/31 20:03:39 dwsquid Exp $ * * DEBUG: section 59 auto-growing Memory Buffer with printf * AUTHOR: Robert Collins @@ -38,5 +38,4 @@ MemBuf::~MemBuf() { - valid = 0; } Index: squid3/src/MemBuf.h =================================================================== RCS file: /cvsroot/squid-sf//squid3/src/MemBuf.h,v retrieving revision 1.3.16.6 retrieving revision 1.3.16.7 diff -u -r1.3.16.6 -r1.3.16.7 --- squid3/src/MemBuf.h 26 Aug 2005 20:28:41 -0000 1.3.16.6 +++ squid3/src/MemBuf.h 31 Aug 2005 20:03:39 -0000 1.3.16.7 @@ -1,6 +1,7 @@ + /* - * $Id: MemBuf.h,v 1.3.16.6 2005/08/26 20:28:41 dwsquid Exp $ + * $Id: MemBuf.h,v 1.3.16.7 2005/08/31 20:03:39 dwsquid Exp $ * * * SQUID Web Proxy Cache http://www.squid-cache.org/ Index: squid3/src/MsgPipe.h =================================================================== RCS file: /cvsroot/squid-sf//squid3/src/Attic/MsgPipe.h,v retrieving revision 1.1.2.5 retrieving revision 1.1.2.6 diff -u -r1.1.2.5 -r1.1.2.6 --- squid3/src/MsgPipe.h 26 Aug 2005 20:28:41 -0000 1.1.2.5 +++ squid3/src/MsgPipe.h 31 Aug 2005 20:03:39 -0000 1.1.2.6 @@ -1,6 +1,6 @@ /* - * $Id: MsgPipe.h,v 1.1.2.5 2005/08/26 20:28:41 dwsquid Exp $ + * $Id: MsgPipe.h,v 1.1.2.6 2005/08/31 20:03:39 dwsquid Exp $ * * * SQUID Web Proxy Cache http://www.squid-cache.org/ @@ -41,19 +41,25 @@ // MsgPipe also provides convenience wrappers for asynchronous calls to // recepient's and sender's note*() methods. -class MsgPipeData; +#include "MsgPipeData.h" +#include "MsgPipeSource.h" +#include "MsgPipeSink.h" class MsgPipeEnd; -class MsgPipeSource; - -class MsgPipeSink; - -class MsgPipe +class MsgPipe : public RefCountable { public: + typedef RefCount Pointer; + MsgPipe(const char *aName = "anonym"); + ~MsgPipe() + { + delete data; + delete source; + delete sink; + }; // the pipe source calls these to notify the sink void sendSourceStart(); Index: squid3/src/MsgPipeData.h =================================================================== RCS file: /cvsroot/squid-sf//squid3/src/Attic/MsgPipeData.h,v retrieving revision 1.1.2.8 retrieving revision 1.1.2.9 diff -u -r1.1.2.8 -r1.1.2.9 --- squid3/src/MsgPipeData.h 26 Aug 2005 23:25:59 -0000 1.1.2.8 +++ squid3/src/MsgPipeData.h 31 Aug 2005 20:03:39 -0000 1.1.2.9 @@ -1,6 +1,6 @@ /* - * $Id: MsgPipeData.h,v 1.1.2.8 2005/08/26 23:25:59 dwsquid Exp $ + * $Id: MsgPipeData.h,v 1.1.2.9 2005/08/31 20:03:39 dwsquid Exp $ * * * SQUID Web Proxy Cache http://www.squid-cache.org/ @@ -51,7 +51,16 @@ public: MsgPipeData(): header(0), body(0), cause(0) {}; - ~MsgPipeData() {}; + ~MsgPipeData() + { + assert(NULL == cause); + assert(NULL == header); + + if (body) { + memBufClean(body); + delete body; + } + }; public: typedef HttpReply Header; Index: squid3/src/MsgPipeEnd.h =================================================================== RCS file: /cvsroot/squid-sf//squid3/src/Attic/MsgPipeEnd.h,v retrieving revision 1.1.2.2 retrieving revision 1.1.2.3 diff -u -r1.1.2.2 -r1.1.2.3 --- squid3/src/MsgPipeEnd.h 26 Aug 2005 20:28:41 -0000 1.1.2.2 +++ squid3/src/MsgPipeEnd.h 31 Aug 2005 20:03:39 -0000 1.1.2.3 @@ -1,6 +1,6 @@ /* - * $Id: MsgPipeEnd.h,v 1.1.2.2 2005/08/26 20:28:41 dwsquid Exp $ + * $Id: MsgPipeEnd.h,v 1.1.2.3 2005/08/31 20:03:39 dwsquid Exp $ * * * SQUID Web Proxy Cache http://www.squid-cache.org/ @@ -34,8 +34,6 @@ #ifndef SQUID_MSGPIPEEND_H #define SQUID_MSGPIPEEND_H -#include "RefCount.h" - // MsgPipeEnd is a common part of the MsgPipeSource and MsgPipeSink interfaces. // Mesage pipe ends must be refcounted so that the recepient does not disappear // while a message is being [asynchoronously] delivered to it. Index: squid3/src/http.cc =================================================================== RCS file: /cvsroot/squid-sf//squid3/src/http.cc,v retrieving revision 1.49.2.20 retrieving revision 1.49.2.21 diff -u -r1.49.2.20 -r1.49.2.21 --- squid3/src/http.cc 26 Aug 2005 23:27:05 -0000 1.49.2.20 +++ squid3/src/http.cc 31 Aug 2005 20:03:39 -0000 1.49.2.21 @@ -1,6 +1,6 @@ /* - * $Id: http.cc,v 1.49.2.20 2005/08/26 23:27:05 dwsquid Exp $ + * $Id: http.cc,v 1.49.2.21 2005/08/31 20:03:39 dwsquid Exp $ * * DEBUG: section 11 Hypertext Transfer Protocol (HTTP) * AUTHOR: Harvest Derived @@ -104,13 +104,19 @@ delete httpState->reply_hdr; /* why clean? */ requestUnlink(httpState->request); + requestUnlink(httpState->orig_request); + httpState->request = NULL; + httpState->orig_request = NULL; + #if ICAP_CLIENT delete httpState->icap; + cbdataReferenceDone(httpState->icap); + #endif cbdataFree(httpState); @@ -2035,6 +2041,7 @@ HttpStateData::doIcap() { debug(11,5)("HttpStateData::doIcap() called\n"); + assert(NULL == icap); icap = new ICAPAnchor; (void) cbdataReference(icap); return 0; @@ -2061,13 +2068,24 @@ return; } + /* + * We gave the virgin reply to ICAP, but ICAP doesn't + * need it now, so destroy it. + */ + httpReplyDestroy(reply); + + reply = NULL; + storeEntryReplaceObject(entry, rep); + /* * After calling storeEntryReplaceObject() we give up control * of the rep and this->reply pointers. */ - rep = reply = NULL; + rep = NULL; + haveParsedReplyHeaders(); + debug(11,5)("HttpStateData::takeAdaptedHeaders() finished\n"); } @@ -2097,10 +2115,10 @@ if (!entry->isAccepting()) { debug(11,5)("\toops, entry is not Accepting!\n"); icap->ownerAbort(); - return; + } else { + fwdComplete(fwd); } - fwdComplete(fwd); assert(fd == -1); httpStateFree(-1, this); } Index: squid3/src/http.h =================================================================== RCS file: /cvsroot/squid-sf//squid3/src/http.h,v retrieving revision 1.11.4.8 retrieving revision 1.11.4.9 diff -u -r1.11.4.8 -r1.11.4.9 --- squid3/src/http.h 26 Aug 2005 20:28:42 -0000 1.11.4.8 +++ squid3/src/http.h 31 Aug 2005 20:03:40 -0000 1.11.4.9 @@ -1,6 +1,6 @@ /* - * $Id: http.h,v 1.11.4.8 2005/08/26 20:28:42 dwsquid Exp $ + * $Id: http.h,v 1.11.4.9 2005/08/31 20:03:40 dwsquid Exp $ * * * SQUID Web Proxy Cache http://www.squid-cache.org/ @@ -43,6 +43,8 @@ { public: + ~HttpStateData() { assert(NULL == reply); }; + static CWCB SendComplete; /* should be private */ void processReplyHeader(const char *buf, int size);