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 93b4da74c9b67b57f633eca5fa5ac22a71cd72c0..2bdb1820f04a4655d52abb683cda616c061e4cb5 100644 (file)
--- 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<ModXact> 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<const HttpRequest*>(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<const HttpReply*>(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<const HttpRequest*>(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<const HttpReply*>(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 61bf8130af5899bd6d0404c8ef2d8f514e41e930..0cad76a905764116e288a39e50f419bd3342da9b 100644 (file)
--- 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 6903ca955576ed3e198e6d0dde5ace5743bd7db7..be06693e0189614da9c46580603b44ff76d001e4 100644 (file)
--- 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<HttpReply*>(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 35561996dc5f3f8855741b3f91a6b59bac1206e2..0fee9a78cc9f9c05738ef750e16bfe8300e40c4d 100644 (file)
--- 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 9c2784eec02e07fca1c58036cba5c9a9a375394b..8c4bfe9df9054d42e8d07aa3902aa4849581288c 100644 (file)
--- 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 4e81dfca917ac1ee96e0ced391fc76f5fc2b2a45..7eb8d010c26d67a96cc8a54b2b8084092553a38c 100644 (file)
--- 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;