From: Christos Tsantilas <chtsanti@users.sourceforge.net>
Date: Fri, 7 Oct 2016 08:16:42 +0000 (+0300)
Subject: Do not leak Downloader-related objects.
X-Git-Tag: SQUID_4_0_15~4
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=0860d5b6e94336d24c5c433088245e0c0351c704;p=thirdparty%2Fsquid.git

Do not leak Downloader-related objects.

The downloadFinished() method was responsible for the job clean up, but
that asynchronous method is not called when the Downloader job quits
before the call can be fired. This early termination happens when, for
example, the job finishes while still inside the start() method (e.g., a
memory hit with no async ACLs to check). It also happens if an exception
is thrown while the job is running under async call protections.

Ensure the cleanup happens regardless of the job path to finish.

This is a Measurement Factory project.
---

diff --git a/src/Downloader.cc b/src/Downloader.cc
index a9320b3775..6847094eab 100644
--- a/src/Downloader.cc
+++ b/src/Downloader.cc
@@ -73,6 +73,17 @@ Downloader::Downloader(SBuf &url, AsyncCall::Pointer &aCallback, unsigned int le
 
 Downloader::~Downloader()
 {
+    debugs(33, 6, this);
+}
+
+void
+Downloader::swanSong()
+{
+    debugs(33, 6, this);
+    if (context_) {
+        context_->finished();
+        context_ = nullptr;
+    }
 }
 
 bool
@@ -234,12 +245,6 @@ void
 Downloader::downloadFinished()
 {
     debugs(33, 7, this);
-    // We cannot delay http destruction until refcounting deletes
-    // DownloaderContext. The http object destruction will cause
-    // clientStream cleanup and will release the refcount to context_
-    // object hold by clientStream structures.
-    context_->finished();
-    context_ = nullptr;
     Must(done());
 }
 
@@ -256,12 +261,10 @@ Downloader::callBack(Http::StatusCode const statusCode)
     ScheduleCallHere(callback_);
     callback_ = nullptr;
 
-    // Calling deleteThis method here to finish Downloader
-    // may result to squid crash.
-    // This method called by handleReply method which maybe called
-    // by ClientHttpRequest::doCallouts. The doCallouts after this object
-    // deleted, may operate on non valid objects.
-    // Schedule an async call here just to force squid to delete this object.
+    // We cannot deleteThis() because we may be called synchronously from
+    // doCallouts() via handleReply() (XXX), and doCallouts() may crash if we
+    // disappear. Instead, schedule an async call now so that later, when the
+    // call firing code discovers a done() job, it deletes us.
     CallJobHere(33, 7, CbcPointer<Downloader>(this), Downloader, downloadFinished);
 }
 
diff --git a/src/Downloader.h b/src/Downloader.h
index 7c681ed3cb..249774620f 100644
--- a/src/Downloader.h
+++ b/src/Downloader.h
@@ -47,6 +47,7 @@ public:
 
     Downloader(SBuf &url, AsyncCall::Pointer &aCallback, unsigned int level = 0);
     virtual ~Downloader();
+    virtual void swanSong();
 
     /// delays destruction to protect doCallouts()
     void downloadFinished();