From: Alex Rousskov Date: Sat, 11 Sep 2010 23:59:07 +0000 (-0600) Subject: Partial bug #2964 fix: Prevent memory leaks when ICAP transactions fail. X-Git-Tag: take1~274 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=c7d51c86241d69feb80d219eebc56f08c88464fb;p=thirdparty%2Fsquid.git Partial bug #2964 fix: Prevent memory leaks when ICAP transactions fail. We now make sure that heap-allocated objects are deleted if an exception is thrown before the object pointers are saved/registered in a safe location like a data member. Assigning state.serviceWaiting=true after calling callWhenReady() in ModXact prevents ModXact leak when callWhenReady() throws. This may need more work to mark ModXact state appropriately for the adaptation log. TODO: Convert other HttpMsg pointer members to use safe HttpMsg::Pointer. --- diff --git a/src/adaptation/icap/ModXact.cc b/src/adaptation/icap/ModXact.cc index 93b4da74c9..2bdb1820f0 100644 --- a/src/adaptation/icap/ModXact.cc +++ b/src/adaptation/icap/ModXact.cc @@ -62,7 +62,7 @@ Adaptation::Icap::ModXact::ModXact(HttpMsg *virginHeader, // nothing to do because we are using temporary buffers // parsing; TODO: do not set until we parse, see ICAPOptXact - icapReply = HTTPMSGLOCK(new HttpReply); + icapReply = new HttpReply; icapReply->protoPrefix = "ICAP/"; // TODO: make an IcapReply class? debugs(93,7, HERE << "initialized." << status()); @@ -94,11 +94,11 @@ void Adaptation::Icap::ModXact::waitForService() { Must(!state.serviceWaiting); debugs(93, 7, HERE << "will wait for the ICAP service" << status()); - state.serviceWaiting = true; typedef NullaryMemFunT Dialer; AsyncCall::Pointer call = JobCallback(93,5, Dialer, this, Adaptation::Icap::ModXact::noteServiceReady); service().callWhenReady(call); + state.serviceWaiting = true; // after callWhenReady() which may throw } void Adaptation::Icap::ModXact::noteServiceReady() @@ -873,32 +873,34 @@ void Adaptation::Icap::ModXact::prepEchoing() // allocate the adapted message and copy metainfo Must(!adapted.header); - HttpMsg *newHead = NULL; + { + HttpMsg::Pointer newHead; if (const HttpRequest *oldR = dynamic_cast(oldHead)) { - HttpRequest *newR = new HttpRequest; + HttpRequest::Pointer newR(new HttpRequest); newR->canonical = oldR->canonical ? xstrdup(oldR->canonical) : NULL; // parse() does not set it newHead = newR; } else if (dynamic_cast(oldHead)) { - HttpReply *newRep = new HttpReply; - newHead = newRep; + newHead = new HttpReply; } - Must(newHead); + Must(newHead != NULL); + newHead->inheritProperties(oldHead); adapted.setHeader(newHead); + } // parse the buffer back http_status error = HTTP_STATUS_NONE; - Must(newHead->parse(&httpBuf, true, &error)); + Must(adapted.header->parse(&httpBuf, true, &error)); - Must(newHead->hdr_sz == httpBuf.contentSize()); // no leftovers + Must(adapted.header->hdr_sz == httpBuf.contentSize()); // no leftovers httpBuf.clean(); debugs(93, 7, HERE << "cloned virgin message " << oldHead << " to " << - newHead); + adapted.header); // setup adapted body pipe if needed if (oldHead->body_pipe != NULL) { @@ -1151,6 +1153,11 @@ void Adaptation::Icap::ModXact::noteBodyConsumerAborted(BodyPipe::Pointer) mustStop("adapted body consumer aborted"); } +Adaptation::Icap::ModXact::~ModXact() +{ + delete bodyParser; +} + // internal cleanup void Adaptation::Icap::ModXact::swanSong() { @@ -1401,21 +1408,20 @@ void Adaptation::Icap::ModXact::encapsulateHead(MemBuf &icapBuf, const char *sec icapBuf.Printf("%s=%d, ", section, (int) httpBuf.contentSize()); // begin cloning - HttpMsg *headClone = NULL; + HttpMsg::Pointer headClone; if (const HttpRequest* old_request = dynamic_cast(head)) { - HttpRequest* new_request = new HttpRequest; - assert(old_request->canonical); + HttpRequest::Pointer new_request(new HttpRequest); + Must(old_request->canonical); urlParse(old_request->method, old_request->canonical, new_request); new_request->http_ver = old_request->http_ver; headClone = new_request; } else if (const HttpReply *old_reply = dynamic_cast(head)) { - HttpReply* new_reply = new HttpReply; + HttpReply::Pointer new_reply(new HttpReply); new_reply->sline = old_reply->sline; headClone = new_reply; } - - Must(headClone); + Must(headClone != NULL); headClone->inheritProperties(head); HttpHeaderPos pos = HttpHeaderInitPos; @@ -1432,7 +1438,7 @@ void Adaptation::Icap::ModXact::encapsulateHead(MemBuf &icapBuf, const char *sec // pack polished HTTP header packHead(httpBuf, headClone); - delete headClone; + // headClone unlocks and, hence, deletes the message we packed } void Adaptation::Icap::ModXact::packHead(MemBuf &httpBuf, const HttpMsg *head) diff --git a/src/adaptation/icap/ModXact.h b/src/adaptation/icap/ModXact.h index 61bf8130af..0cad76a905 100644 --- a/src/adaptation/icap/ModXact.h +++ b/src/adaptation/icap/ModXact.h @@ -137,6 +137,7 @@ class ModXact: public Xaction, public BodyProducer, public BodyConsumer public: ModXact(HttpMsg *virginHeader, HttpRequest *virginCause, ServiceRep::Pointer &s); + virtual ~ModXact(); // BodyProducer methods virtual void noteMoreBodySpaceAvailable(BodyPipe::Pointer); diff --git a/src/adaptation/icap/OptXact.cc b/src/adaptation/icap/OptXact.cc index 6903ca9555..be06693e01 100644 --- a/src/adaptation/icap/OptXact.cc +++ b/src/adaptation/icap/OptXact.cc @@ -68,11 +68,10 @@ void Adaptation::Icap::OptXact::handleCommWrote(size_t size) // comm module read a portion of the ICAP response for us void Adaptation::Icap::OptXact::handleCommRead(size_t) { - if (HttpMsg *r = parseResponse()) { + if (parseResponse()) { icap_tio_finish = current_time; setOutcome(xoOpt); - sendAnswer(r); - icapReply = HTTPMSGLOCK(dynamic_cast(r)); + sendAnswer(icapReply); Must(done()); // there should be nothing else to do return; } @@ -80,24 +79,23 @@ void Adaptation::Icap::OptXact::handleCommRead(size_t) scheduleRead(); } -HttpMsg *Adaptation::Icap::OptXact::parseResponse() +bool Adaptation::Icap::OptXact::parseResponse() { debugs(93, 5, HERE << "have " << readBuf.contentSize() << " bytes to parse" << status()); debugs(93, 5, HERE << "\n" << readBuf.content()); - HttpReply *r = HTTPMSGLOCK(new HttpReply); + HttpReply::Pointer r(new HttpReply); r->protoPrefix = "ICAP/"; // TODO: make an IcapReply class? - if (!parseHttpMsg(r)) { // throws on errors - HTTPMSGUNLOCK(r); - return 0; - } + if (!parseHttpMsg(r)) // throws on errors + return false; if (httpHeaderHasConnDir(&r->header, "close")) reuseConnection = false; - return r; + icapReply = r; + return true; } void Adaptation::Icap::OptXact::swanSong() diff --git a/src/adaptation/icap/OptXact.h b/src/adaptation/icap/OptXact.h index 35561996dc..0fee9a78cc 100644 --- a/src/adaptation/icap/OptXact.h +++ b/src/adaptation/icap/OptXact.h @@ -60,7 +60,7 @@ protected: virtual void handleCommRead(size_t size); void makeRequest(MemBuf &buf); - HttpMsg *parseResponse(); + bool parseResponse(); void startReading(); diff --git a/src/adaptation/icap/Xaction.cc b/src/adaptation/icap/Xaction.cc index 9c2784eec0..8c4bfe9df9 100644 --- a/src/adaptation/icap/Xaction.cc +++ b/src/adaptation/icap/Xaction.cc @@ -52,7 +52,6 @@ Adaptation::Icap::Xaction::~Xaction() debugs(93,3, typeName << " destructed, this=" << this << " [icapx" << id << ']'); // we should not call virtual status() here HTTPMSGUNLOCK(icapRequest); - HTTPMSGUNLOCK(icapReply); } Adaptation::Icap::ServiceRep & diff --git a/src/adaptation/icap/Xaction.h b/src/adaptation/icap/Xaction.h index 4e81dfca91..7eb8d010c2 100644 --- a/src/adaptation/icap/Xaction.h +++ b/src/adaptation/icap/Xaction.h @@ -40,8 +40,8 @@ #include "adaptation/icap/ServiceRep.h" #include "adaptation/Initiate.h" #include "AccessLogEntry.h" +#include "HttpReply.h" -class HttpMsg; class CommConnectCbParams; namespace Adaptation @@ -80,7 +80,7 @@ public: // TODO: create these only when actually sending/receiving HttpRequest *icapRequest; ///< sent (or at least created) ICAP request - HttpReply *icapReply; ///< received ICAP reply, if any + HttpReply::Pointer icapReply; ///< received ICAP reply, if any /// the number of times we tried to get to the service, including this time int attempts;