Keep ::helper objects alive while in use by helper_servers (#1389)

Squid creates ::helper objects to manage a given configured helper. For
each ::helper object, Squid may create one or more helper_server objects
to manage communication with individual helper processes. A
helper_server object uses its so called "parent" ::helper object to
access configuration and for helper process death notification purposes.
There are no checks that the "parent" object is still alive when used.

The same problem applies to statefulhelper and helper_stateful_server.

helper_server code evidently attempted to extend their "parent" lifetime
using cbdata, but that does not work, especially in C++ world where the
object is destructed (and, hence, enters an undefined state) even though
its top-level memory is preserved by cbdata. Non-trivial members like
helper::queue (std::queue) and statefulhelper::reservations
(std::unordered_map) release their internal memory when destructed.

We now refcount ::helper objects to keep them alive until the last
object using them is gone. This does not result in reference loops
because the ::helper object uses raw dlink_node pointers to store its
helper_servers.

The following helpers (listed here by their container names) were
destructed while possibly still in use by their helper_server objects:
external_acl::theHelper, Ssl::CertValidationHelper::ssl_crt_validator,
Ssl::Helper::ssl_crtd. The following helpers are not destructed during
reconfiguration: redirectors, storeIds, basicauthenticators,
ntlmauthenticators, negotiateauthenticators, and digestauthenticators
(even though casual reading of relevant code may suggest otherwise).

This bug fix does not address or mark many remaining helper bugs.

diff --git a/src/auth/basic/Config.cc b/src/auth/basic/Config.cc
index ddd214177a7dac83dc4b0e43190df80718628ee9..901fdbe5f8ee342883c36beb71dd2950a34b7fff 100644 (file)
--- a/src/auth/basic/Config.cc
+++ b/src/auth/basic/Config.cc
@@ -36,7 +36,7 @@
 /* Basic Scheme */
 static AUTHSSTATS authenticateBasicStats;
 
-helper *basicauthenticators = nullptr;
+Helper::ClientPointer basicauthenticators;
 
 static int authbasic_initialised = 0;
 
@@ -109,7 +109,6 @@ Auth::Basic::Config::done()
         helperShutdown(basicauthenticators);
     }
 
-    delete basicauthenticators;
     basicauthenticators = nullptr;
 
     if (authenticateProgram)
@@ -305,7 +304,7 @@ Auth::Basic::Config::init(Auth::SchemeConfig *)
         authbasic_initialised = 1;
 
         if (basicauthenticators == nullptr)
-            basicauthenticators = new helper("basicauthenticator");
+            basicauthenticators = helper::Make("basicauthenticator");
 
         basicauthenticators->cmdline = authenticateProgram;
 

diff --git a/src/auth/basic/Config.h b/src/auth/basic/Config.h
index a6555d28c656e882f0050fa8f232927db7d56ca9..8d1d95f6e975ab5fceb1a783240f9095fa4640db 100644 (file)
--- a/src/auth/basic/Config.h
+++ b/src/auth/basic/Config.h
@@ -50,7 +50,7 @@ private:
 } // namespace Basic
 } // namespace Auth
 
-extern helper *basicauthenticators;
+extern Helper::ClientPointer basicauthenticators;
 
 #endif /* HAVE_AUTH_MODULE_BASIC */
 #endif /* __AUTH_BASIC_H__ */

diff --git a/src/auth/digest/Config.cc b/src/auth/digest/Config.cc
index f00e2ba6828f29a04c4e845790ad9ae700ef7ceb..b3767c963da1310a1bd019cac3155cc61c4d121d 100644 (file)
--- a/src/auth/digest/Config.cc
+++ b/src/auth/digest/Config.cc
@@ -46,7 +46,7 @@
 
 static AUTHSSTATS authenticateDigestStats;
 
-helper *digestauthenticators = nullptr;
+Helper::ClientPointer digestauthenticators;
 
 static hash_table *digest_nonce_cache;
 
@@ -525,7 +525,7 @@ Auth::Digest::Config::init(Auth::SchemeConfig *)
         authdigest_initialised = 1;
 
         if (digestauthenticators == nullptr)
-            digestauthenticators = new helper("digestauthenticator");
+            digestauthenticators = helper::Make("digestauthenticator");
 
         digestauthenticators->cmdline = authenticateProgram;
 
@@ -559,7 +559,6 @@ Auth::Digest::Config::done()
     if (!shutting_down)
         return;
 
-    delete digestauthenticators;
     digestauthenticators = nullptr;
 
     if (authenticateProgram)

diff --git a/src/auth/digest/Config.h b/src/auth/digest/Config.h
index 174a1a2cfdf2ab5717df97ce404570cd691596e7..f6bbbce2f0f44f870aa311bcfd99d636e5690d3d 100644 (file)
--- a/src/auth/digest/Config.h
+++ b/src/auth/digest/Config.h
@@ -100,7 +100,7 @@ public:
 /* strings */
 #define QOP_AUTH "auth"
 
-extern helper *digestauthenticators;
+extern Helper::ClientPointer digestauthenticators;
 
 #endif /* HAVE_AUTH_MODULE_DIGEST */
 #endif

diff --git a/src/auth/negotiate/Config.cc b/src/auth/negotiate/Config.cc
index cf0033526c76b3bc6d5f483f002b3aa35b786f82..e1f5e2669bcdae336cf531b4c6322e2143cb732a 100644 (file)
--- a/src/auth/negotiate/Config.cc
+++ b/src/auth/negotiate/Config.cc
@@ -32,7 +32,7 @@
 
 static AUTHSSTATS authenticateNegotiateStats;
 
-statefulhelper *negotiateauthenticators = nullptr;
+Helper::StatefulClientPointer negotiateauthenticators;
 
 static int authnegotiate_initialised = 0;
 
@@ -63,7 +63,6 @@ Auth::Negotiate::Config::done()
     if (!shutting_down)
         return;
 
-    delete negotiateauthenticators;
     negotiateauthenticators = nullptr;
 
     if (authenticateProgram)
@@ -90,7 +89,7 @@ Auth::Negotiate::Config::init(Auth::SchemeConfig *)
         authnegotiate_initialised = 1;
 
         if (negotiateauthenticators == nullptr)
-            negotiateauthenticators = new statefulhelper("negotiateauthenticator");
+            negotiateauthenticators = statefulhelper::Make("negotiateauthenticator");
 
         if (!proxy_auth_cache)
             proxy_auth_cache = hash_create((HASHCMP *) strcmp, 7921, hash_string);

diff --git a/src/auth/negotiate/Config.h b/src/auth/negotiate/Config.h
index b4dc2fe4166c343aca41f738592ba088efdbb67e..b037f8699ec575a652c678ac4e4bd9f74af77b4b 100644 (file)
--- a/src/auth/negotiate/Config.h
+++ b/src/auth/negotiate/Config.h
@@ -39,7 +39,7 @@ public:
 } // namespace Negotiate
 } // namespace Auth
 
-extern statefulhelper *negotiateauthenticators;
+extern Helper::StatefulClientPointer negotiateauthenticators;
 
 #endif /* HAVE_AUTH_MODULE_NEGOTIATE */
 #endif

diff --git a/src/auth/ntlm/Config.cc b/src/auth/ntlm/Config.cc
index 0119ca1289a9b8562352ab352ea13fd0e304e136..0928a970efce4104e3a8dfda15b5b25771ea3754 100644 (file)
--- a/src/auth/ntlm/Config.cc
+++ b/src/auth/ntlm/Config.cc
@@ -33,7 +33,7 @@
 /* NTLM Scheme */
 static AUTHSSTATS authenticateNTLMStats;
 
-statefulhelper *ntlmauthenticators = nullptr;
+Helper::StatefulClientPointer ntlmauthenticators;
 static int authntlm_initialised = 0;
 
 static hash_table *proxy_auth_cache = nullptr;
@@ -64,7 +64,6 @@ Auth::Ntlm::Config::done()
     if (!shutting_down)
         return;
 
-    delete ntlmauthenticators;
     ntlmauthenticators = nullptr;
 
     if (authenticateProgram)
@@ -89,7 +88,7 @@ Auth::Ntlm::Config::init(Auth::SchemeConfig *)
         authntlm_initialised = 1;
 
         if (ntlmauthenticators == nullptr)
-            ntlmauthenticators = new statefulhelper("ntlmauthenticator");
+            ntlmauthenticators = statefulhelper::Make("ntlmauthenticator");
 
         if (!proxy_auth_cache)
             proxy_auth_cache = hash_create((HASHCMP *) strcmp, 7921, hash_string);

diff --git a/src/auth/ntlm/Config.h b/src/auth/ntlm/Config.h
index 86570d226589baad4a2ef53fd8750629ce1b95f0..3c3c1979823c95c539b800e5c666e2181c65fd75 100644 (file)
--- a/src/auth/ntlm/Config.h
+++ b/src/auth/ntlm/Config.h
@@ -42,7 +42,7 @@ public:
 } // namespace Ntlm
 } // namespace Auth
 
-extern statefulhelper *ntlmauthenticators;
+extern Helper::StatefulClientPointer ntlmauthenticators;
 
 #endif /* HAVE_AUTH_MODULE_NTLM */
 #endif

diff --git a/src/external_acl.cc b/src/external_acl.cc
index e411c9a893e020f83a3a91593c80b39b49ed0acb..62817ec76d036798e42be63af198020e9724f3a6 100644 (file)
--- a/src/external_acl.cc
+++ b/src/external_acl.cc
@@ -100,7 +100,7 @@ public:
 
     Helper::ChildConfig children;
 
-    helper *theHelper;
+    helper::Pointer theHelper;
 
     hash_table *cache;
 
@@ -157,7 +157,6 @@ external_acl::~external_acl()
 
     if (theHelper) {
         helperShutdown(theHelper);
-        delete theHelper;
         theHelper = nullptr;
     }
 
@@ -1114,7 +1113,7 @@ externalAclInit(void)
             p->cache = hash_create((HASHCMP *) strcmp, hashPrime(1024), hash4);
 
         if (!p->theHelper)
-            p->theHelper = new helper("external_acl_type");
+            p->theHelper = helper::Make("external_acl_type");
 
         p->theHelper->cmdline = p->cmdline;
 

diff --git a/src/helper.cc b/src/helper.cc
index c83cb53dd411bf107a9786b5b13cd2d7c8831559..e09841fdc0172434eddbf6d5a446079cb1efe30b 100644 (file)
--- a/src/helper.cc
+++ b/src/helper.cc
@@ -44,18 +44,16 @@ const size_t ReadBufSize(32*1024);
 static IOCB helperHandleRead;
 static IOCB helperStatefulHandleRead;
 static void Enqueue(helper * hlp, Helper::Xaction *);
-static helper_server *GetFirstAvailable(const helper * hlp);
-static helper_stateful_server *StatefulGetFirstAvailable(statefulhelper * hlp);
+static helper_server *GetFirstAvailable(const helper::Pointer &);
+static helper_stateful_server *StatefulGetFirstAvailable(const statefulhelper::Pointer &);
 static void helperDispatch(helper_server * srv, Helper::Xaction * r);
 static void helperStatefulDispatch(helper_stateful_server * srv, Helper::Xaction * r);
-static void helperKickQueue(helper * hlp);
-static void helperStatefulKickQueue(statefulhelper * hlp);
+static void helperKickQueue(const helper::Pointer &);
+static void helperStatefulKickQueue(const statefulhelper::Pointer &);
 static void helperStatefulServerDone(helper_stateful_server * srv);
 static void StatefulEnqueue(statefulhelper * hlp, Helper::Xaction * r);
 
-CBDATA_CLASS_INIT(helper);
 CBDATA_CLASS_INIT(helper_server);
-CBDATA_CLASS_INIT(statefulhelper);
 CBDATA_CLASS_INIT(helper_stateful_server);
 
 InstanceIdDefinitions(HelperServerBase, "Hlpr");
@@ -169,7 +167,6 @@ helper_server::~helper_server()
     -- parent->childs.n_running;
 
     assert(requests.empty());
-    cbdataReferenceDone(parent);
 }
 
 void
@@ -193,12 +190,10 @@ helper_stateful_server::~helper_stateful_server()
     -- parent->childs.n_running;
 
     assert(requests.empty());
-
-    cbdataReferenceDone(parent);
 }
 
 void
-helperOpenServers(helper * hlp)
+helperOpenServers(const helper::Pointer &hlp)
 {
     char *s;
     char *progname;
@@ -288,7 +283,7 @@ helperOpenServers(helper * hlp)
         srv->nextRequestId = 0;
         srv->replyXaction = nullptr;
         srv->ignoreToEom = false;
-        srv->parent = cbdataReference(hlp);
+        srv->parent = hlp;
         dlinkAddTail(srv, &srv->link, &hlp->servers);
 
         if (rfd == wfd) {
@@ -339,7 +334,7 @@ helperOpenServers(helper * hlp)
  * helperStatefulOpenServers: create the stateful child helper processes
  */
 void
-helperStatefulOpenServers(statefulhelper * hlp)
+helperStatefulOpenServers(const statefulhelper::Pointer &hlp)
 {
     char *shortname;
     const char *args[HELPER_MAX_ARGS+1]; // save space for a NULL terminator
@@ -421,7 +416,7 @@ helperStatefulOpenServers(statefulhelper * hlp)
         srv->writePipe->fd = wfd;
         srv->rbuf = (char *)memAllocBuf(ReadBufSize, &srv->rbuf_sz);
         srv->roffset = 0;
-        srv->parent = cbdataReference(hlp);
+        srv->parent = hlp;
         srv->reservationStart = 0;
 
         dlinkAddTail(srv, &srv->link, &hlp->servers);
@@ -477,7 +472,7 @@ helper::submitRequest(Helper::Xaction *r)
 
 /// handles helperSubmit() and helperStatefulSubmit() failures
 static void
-SubmissionFailure(helper *hlp, HLPCB *callback, void *data)
+SubmissionFailure(const helper::Pointer &hlp, HLPCB *callback, void *data)
 {
     auto result = Helper::Error;
     if (!hlp) {
@@ -490,7 +485,7 @@ SubmissionFailure(helper *hlp, HLPCB *callback, void *data)
 }
 
 void
-helperSubmit(helper * hlp, const char *buf, HLPCB * callback, void *data)
+helperSubmit(const helper::Pointer &hlp, const char *buf, HLPCB * callback, void *data)
 {
     if (!hlp || !hlp->trySubmit(buf, callback, data))
         SubmissionFailure(hlp, callback, data);
@@ -584,7 +579,7 @@ helper::submit(const char *buf, HLPCB * callback, void *data)
 /// Submit request or callback the caller with a Helper::Error error.
 /// If the reservation is not set then reserves a new helper.
 void
-helperStatefulSubmit(statefulhelper * hlp, const char *buf, HLPCB * callback, void *data, const Helper::ReservationId & reservation)
+helperStatefulSubmit(const statefulhelper::Pointer &hlp, const char *buf, HLPCB * callback, void *data, const Helper::ReservationId & reservation)
 {
     if (!hlp || !hlp->trySubmit(buf, callback, data, reservation))
         SubmissionFailure(hlp, callback, data);
@@ -756,8 +751,20 @@ helper::willOverload() const {
     return queueFull() && !(childs.needNew() || GetFirstAvailable(this));
 }
 
+helper::Pointer
+helper::Make(const char *name)
+{
+    return new helper(name);
+}
+
+statefulhelper::Pointer
+statefulhelper::Make(const char *name)
+{
+    return new statefulhelper(name);
+}
+
 void
-helperShutdown(helper * hlp)
+helperShutdown(const helper::Pointer &hlp)
 {
     dlink_node *link = hlp->servers.head;
 
@@ -794,7 +801,7 @@ helperShutdown(helper * hlp)
 }
 
 void
-helperStatefulShutdown(statefulhelper * hlp)
+helperStatefulShutdown(const statefulhelper::Pointer &hlp)
 {
     dlink_node *link = hlp->servers.head;
     helper_stateful_server *srv;
@@ -890,7 +897,7 @@ helper::handleFewerServers(const bool madeProgress)
 void
 helper_server::HelperServerClosed(helper_server *srv)
 {
-    helper *hlp = srv->getParent();
+    const auto hlp = srv->parent;
 
     bool needsNewServers = false;
     hlp->handleKilledServer(srv, needsNewServers);
@@ -905,11 +912,11 @@ helper_server::HelperServerClosed(helper_server *srv)
 }
 
 // XXX: Almost duplicates helper_server::HelperServerClosed() because helperOpenServers() is not a virtual method of the `helper` class
-// TODO: Fix the `helper` class hierarchy to use CbdataParent and virtual functions.
+// TODO: Fix the `helper` class hierarchy to use virtual functions.
 void
 helper_stateful_server::HelperServerClosed(helper_stateful_server *srv)
 {
-    statefulhelper *hlp = static_cast<statefulhelper *>(srv->getParent());
+    const auto hlp = srv->parent;
 
     bool needsNewServers = false;
     hlp->handleKilledServer(srv, needsNewServers);
@@ -947,7 +954,7 @@ helper_server::popRequest(int request_number)
 
 /// Calls back with a pointer to the buffer with the helper output
 static void
-helperReturnBuffer(helper_server * srv, helper * hlp, char * msg, size_t msgSize, char * msgEnd)
+helperReturnBuffer(helper_server * srv, const helper::Pointer &hlp, char * msg, size_t msgSize, char * msgEnd)
 {
     if (Helper::Xaction *r = srv->replyXaction) {
         const bool hasSpace = r->reply.accumulate(msg, msgSize);
@@ -1014,7 +1021,7 @@ static void
 helperHandleRead(const Comm::ConnectionPointer &conn, char *, size_t len, Comm::Flag flag, int, void *data)
 {
     helper_server *srv = (helper_server *)data;
-    helper *hlp = srv->parent;
+    const auto hlp = srv->parent;
     assert(cbdataReferenceValid(data));
 
     /* Bail out early on Comm::ERR_CLOSING - close handlers will tidy up for us */
@@ -1131,7 +1138,7 @@ helperStatefulHandleRead(const Comm::ConnectionPointer &conn, char *, size_t len
 {
     char *t = nullptr;
     helper_stateful_server *srv = (helper_stateful_server *)data;
-    statefulhelper *hlp = srv->parent;
+    const auto hlp = srv->parent;
     assert(cbdataReferenceValid(data));
 
     /* Bail out early on Comm::ERR_CLOSING - close handlers will tidy up for us */
@@ -1304,7 +1311,7 @@ helper::nextRequest()
 }
 
 static helper_server *
-GetFirstAvailable(const helper * hlp)
+GetFirstAvailable(const helper::Pointer &hlp)
 {
     dlink_node *n;
     helper_server *srv;
@@ -1350,7 +1357,7 @@ GetFirstAvailable(const helper * hlp)
 }
 
 static helper_stateful_server *
-StatefulGetFirstAvailable(statefulhelper * hlp)
+StatefulGetFirstAvailable(const statefulhelper::Pointer &hlp)
 {
     dlink_node *n;
     helper_stateful_server *srv = nullptr;
@@ -1422,7 +1429,7 @@ helperDispatchWriteDone(const Comm::ConnectionPointer &, char *, size_t, Comm::F
 static void
 helperDispatch(helper_server * srv, Helper::Xaction * r)
 {
-    helper *hlp = srv->parent;
+    const auto hlp = srv->parent;
     const uint64_t reqId = ++srv->nextRequestId;
 
     if (!cbdataReferenceValid(r->request.data)) {
@@ -1469,7 +1476,7 @@ helperStatefulDispatchWriteDone(const Comm::ConnectionPointer &, char *, size_t,
 static void
 helperStatefulDispatch(helper_stateful_server * srv, Helper::Xaction * r)
 {
-    statefulhelper *hlp = srv->parent;
+    const auto hlp = srv->parent;
 
     if (!cbdataReferenceValid(r->request.data)) {
         debugs(84, DBG_IMPORTANT, "ERROR: helperStatefulDispatch: invalid callback data");
@@ -1503,7 +1510,7 @@ helperStatefulDispatch(helper_stateful_server * srv, Helper::Xaction * r)
     srv->requests.push_back(r);
     srv->dispatch_time = current_time;
     AsyncCall::Pointer call = commCbCall(5,5, "helperStatefulDispatchWriteDone",
-                                         CommIoCbPtrFun(helperStatefulDispatchWriteDone, hlp));
+                                         CommIoCbPtrFun(helperStatefulDispatchWriteDone, srv));
     Comm::Write(srv->writePipe, r->request.buf, strlen(r->request.buf), call, nullptr);
     debugs(84, 5, "helperStatefulDispatch: Request sent to " <<
            hlp->id_name << " #" << srv->index << ", " <<
@@ -1515,7 +1522,7 @@ helperStatefulDispatch(helper_stateful_server * srv, Helper::Xaction * r)
 }
 
 static void
-helperKickQueue(helper * hlp)
+helperKickQueue(const helper::Pointer &hlp)
 {
     Helper::Xaction *r;
     helper_server *srv;
@@ -1525,7 +1532,7 @@ helperKickQueue(helper * hlp)
 }
 
 static void
-helperStatefulKickQueue(statefulhelper * hlp)
+helperStatefulKickQueue(const statefulhelper::Pointer &hlp)
 {
     Helper::Xaction *r;
     helper_stateful_server *srv;

diff --git a/src/helper.h b/src/helper.h
index 71564887752f173997edaa9b28126a12f48d0ec1..3b02430529732279d0a6ec82673b00aab0e5736d 100644 (file)
--- a/src/helper.h
+++ b/src/helper.h
@@ -13,6 +13,7 @@
 
 #include "base/AsyncCall.h"
 #include "base/InstanceId.h"
+#include "base/RefCount.h"
 #include "cbdata.h"
 #include "comm/forward.h"
 #include "dlink.h"
@@ -60,26 +61,15 @@ class HelperServerBase;
  * If an overloaded helper has been overloaded for 3+ minutes, an attempt to use
  *   it results in on-persistent-overload action, which may kill worker.
  */
-class helper
+class helper: public RefCountable
 {
-    CBDATA_CLASS(helper);
-
 public:
+    using Pointer = RefCount<helper>;
+
+    /// \returns a newly created instance of the named helper
     /// \param name admin-visible helper category (with this process lifetime)
-    inline helper(const char *name) :
-        cmdline(nullptr),
-        id_name(name),
-        ipc_type(0),
-        droppedRequests(0),
-        overloadStart(0),
-        last_queue_warn(0),
-        last_restart(0),
-        timeout(0),
-        retryTimedOut(false),
-        retryBrokenHelper(false),
-        eom('\n') {
-        memset(&stats, 0, sizeof(stats));
-    }
+    static Pointer Make(const char *name);
+
     ~helper();
 
     /// \returns next request in the queue, or nil.
@@ -109,33 +99,37 @@ public:
     void handleFewerServers(bool madeProgress);
 
 public:
-    wordlist *cmdline;
+    wordlist *cmdline = nullptr;
     dlink_list servers;
     std::queue<Helper::Xaction *> queue;
-    const char *id_name;
+    const char *id_name = nullptr;
     Helper::ChildConfig childs;    ///< Configuration settings for number running.
-    int ipc_type;
+    int ipc_type = 0;
     Ip::Address addr;
-    unsigned int droppedRequests; ///< requests not sent during helper overload
-    time_t overloadStart; ///< when the helper became overloaded (zero if it is not)
-    time_t last_queue_warn;
-    time_t last_restart;
-    time_t timeout; ///< Requests timeout
-    bool retryTimedOut; ///< Whether the timed-out requests must retried
-    bool retryBrokenHelper; ///< Whether the requests must retried on BH replies
+    unsigned int droppedRequests = 0; ///< requests not sent during helper overload
+    time_t overloadStart = 0; ///< when the helper became overloaded (zero if it is not)
+    time_t last_queue_warn = 0;
+    time_t last_restart = 0;
+    time_t timeout = 0; ///< Requests timeout
+    bool retryTimedOut = false; ///< Whether the timed-out requests must retried
+    bool retryBrokenHelper = false; ///< Whether the requests must retried on BH replies
     SBuf onTimedOutResponse; ///< The response to use when helper response timedout
-    char eom;   ///< The char which marks the end of (response) message, normally '\n'
+    char eom = '\n';   ///< The char which marks the end of (response) message, normally '\n'
 
     struct _stats {
-        int requests;
-        int replies;
-        int timedout;
-        int queue_size;
-        int avg_svc_time;
+        int requests = 0;
+        int replies = 0;
+        int timedout = 0;
+        int queue_size = 0;
+        int avg_svc_time = 0;
     } stats;
 
 protected:
-    friend void helperSubmit(helper * hlp, const char *buf, HLPCB * callback, void *data);
+    friend void helperSubmit(const helper::Pointer &, const char *buf, HLPCB * callback, void *data);
+
+    /// \param name admin-visible helper category (with this process lifetime)
+    explicit helper(const char *name): id_name(name) {}
+
     bool queueFull() const;
     bool overloaded() const;
     void syncQueueStats();
@@ -145,15 +139,15 @@ protected:
 
 class statefulhelper : public helper
 {
-    CBDATA_CLASS(statefulhelper);
-
 public:
+    using Pointer = RefCount<statefulhelper>;
     typedef std::unordered_map<Helper::ReservationId, helper_stateful_server *> Reservations;
 
-    inline statefulhelper(const char *name) : helper(name) {}
     inline ~statefulhelper() {}
 
 public:
+    static Pointer Make(const char *name);
+
     /// reserve the given server
     void reserveServer(helper_stateful_server * srv);
 
@@ -161,7 +155,9 @@ public:
     void cancelReservation(const Helper::ReservationId reservation);
 
 private:
-    friend void helperStatefulSubmit(statefulhelper * hlp, const char *buf, HLPCB * callback, void *data, const Helper::ReservationId & reservation);
+    friend void helperStatefulSubmit(const statefulhelper::Pointer &, const char *buf, HLPCB *, void *cbData, const Helper::ReservationId &);
+
+    explicit statefulhelper(const char *name): helper(name) {}
 
     /// \return the previously reserved server (if the reservation is still valid) or nil
     helper_stateful_server *findServer(const Helper::ReservationId & reservation);
@@ -200,9 +196,6 @@ public:
     /// dequeues and sends a Helper::Unknown answer to all queued requests
     virtual void dropQueued();
 
-    /// the helper object that created this server
-    virtual helper *getParent() const = 0;
-
 public:
     /// Helper program identifier; does not change when contents do,
     ///   including during assignment
@@ -256,7 +249,7 @@ public:
     MemBuf *wqueue;
     MemBuf *writebuf;
 
-    helper *parent;
+    helper::Pointer parent;
 
     /// The helper request Xaction object for the current reply .
     /// A helper reply may be distributed to more than one of the retrieved
@@ -285,7 +278,6 @@ public:
     /*HelperServerBase API*/
     bool reserved() override {return false;}
     void dropQueued() override;
-    helper *getParent() const override {return parent;}
 
     /// Read timeout handler
     static void requestTimeout(const CommTimeoutCbParams &io);
@@ -307,12 +299,11 @@ public:
 
     /* HelperServerBase API */
     bool reserved() override {return reservationId.reserved();}
-    helper *getParent() const override {return parent;}
 
     /// close handler to handle exited server processes
     static void HelperServerClosed(helper_stateful_server *srv);
 
-    statefulhelper *parent;
+    statefulhelper::Pointer parent;
 
     // Reservations temporary lock the server for an exclusive "client" use. The
     // client keeps the reservation ID as a proof of her reservation. If a
@@ -322,13 +313,12 @@ public:
     time_t reservationStart; ///< when the last `reservation` was made
 };
 
-/* helper.c */
-void helperOpenServers(helper * hlp);
-void helperStatefulOpenServers(statefulhelper * hlp);
-void helperSubmit(helper * hlp, const char *buf, HLPCB * callback, void *data);
-void helperStatefulSubmit(statefulhelper * hlp, const char *buf, HLPCB * callback, void *data, uint64_t reservation);
-void helperShutdown(helper * hlp);
-void helperStatefulShutdown(statefulhelper * hlp);
+void helperOpenServers(const helper::Pointer &);
+void helperStatefulOpenServers(const statefulhelper::Pointer &);
+void helperSubmit(const helper::Pointer &, const char *buf, HLPCB *, void *cbData);
+void helperStatefulSubmit(const statefulhelper::Pointer &, const char *buf, HLPCB *, void *cbData, uint64_t reservation);
+void helperShutdown(const helper::Pointer &);
+void helperStatefulShutdown(const statefulhelper::Pointer &);
 
 #endif /* SQUID_HELPER_H */
 

diff --git a/src/helper/forward.h b/src/helper/forward.h
index c833cce17b6bc24f2e33ffe8c42c62ad07292fad..c86021326f370b50d2258df67d58760436329c18 100644 (file)
--- a/src/helper/forward.h
+++ b/src/helper/forward.h
@@ -9,6 +9,8 @@
 #ifndef SQUID_SRC_HELPER_FORWARD_H
 #define SQUID_SRC_HELPER_FORWARD_H
 
+#include "base/forward.h"
+
 class helper;
 class statefulhelper;
 
@@ -22,6 +24,9 @@ namespace Helper
 class Reply;
 class Request;
 
+using ClientPointer = RefCount<helper>;
+using StatefulClientPointer = RefCount<statefulhelper>;
+
 } // namespace Helper
 
 typedef void HLPCB(void *, const Helper::Reply &);

diff --git a/src/redirect.cc b/src/redirect.cc
index d1d284be2a1cc3ef3397b1f9434f209b4cd9db72..2dbfd012b016511551c5cf412b55cc0ae55043ed 100644 (file)
--- a/src/redirect.cc
+++ b/src/redirect.cc
@@ -54,8 +54,8 @@ public:
 
 static HLPCB redirectHandleReply;
 static HLPCB storeIdHandleReply;
-static helper *redirectors = nullptr;
-static helper *storeIds = nullptr;
+static helper::Pointer redirectors;
+static helper::Pointer storeIds;
 static OBJH redirectStats;
 static OBJH storeIdStats;
 static int redirectorBypassed = 0;
@@ -223,7 +223,7 @@ storeIdStats(StoreEntry * sentry)
 }
 
 static void
-constructHelperQuery(const char *name, helper *hlp, HLPCB *replyHandler, ClientHttpRequest * http, HLPCB *handler, void *data, Format::Format *requestExtrasFmt)
+constructHelperQuery(const char *name, const helper::Pointer &hlp, HLPCB *replyHandler, ClientHttpRequest * http, HLPCB *handler, void *data, Format::Format *requestExtrasFmt)
 {
     char buf[MAX_REDIRECTOR_REQUEST_STRLEN];
     int sz;
@@ -342,7 +342,7 @@ redirectInit(void)
     if (Config.Program.redirect) {
 
         if (redirectors == nullptr)
-            redirectors = new helper("redirector");
+            redirectors = helper::Make("redirector");
 
         redirectors->cmdline = Config.Program.redirect;
 
@@ -369,7 +369,7 @@ redirectInit(void)
     if (Config.Program.store_id) {
 
         if (storeIds == nullptr)
-            storeIds = new helper("store_id");
+            storeIds = helper::Make("store_id");
 
         storeIds->cmdline = Config.Program.store_id;
 
@@ -418,10 +418,8 @@ redirectShutdown(void)
     if (!shutting_down)
         return;
 
-    delete redirectors;
     redirectors = nullptr;
 
-    delete storeIds;
     storeIds = nullptr;
 
     delete redirectorExtrasFmt;

diff --git a/src/ssl/helper.cc b/src/ssl/helper.cc
index bd0c158685c761d0194842a10a21c16ea02b3cfe..6ea027ae8fc73f4b811e1b463180363cae987988 100644 (file)
--- a/src/ssl/helper.cc
+++ b/src/ssl/helper.cc
@@ -72,7 +72,7 @@ operator <<(std::ostream &os, const Ssl::GeneratorRequest &gr)
 /// pending Ssl::Helper requests (to all certificate generator helpers combined)
 static Ssl::GeneratorRequests TheGeneratorRequests;
 
-helper *Ssl::Helper::ssl_crtd = nullptr;
+helper::Pointer Ssl::Helper::ssl_crtd = nullptr;
 
 void Ssl::Helper::Init()
 {
@@ -86,7 +86,7 @@ void Ssl::Helper::Init()
     if (!found)
         return;
 
-    ssl_crtd = new helper("sslcrtd_program");
+    ssl_crtd = helper::Make("sslcrtd_program");
     ssl_crtd->childs.updateLimits(Ssl::TheConfig.ssl_crtdChildren);
     ssl_crtd->ipc_type = IPC_STREAM;
     // The crtd messages may contain the eol ('\n') character. We are
@@ -111,7 +111,6 @@ void Ssl::Helper::Shutdown()
         return;
     helperShutdown(ssl_crtd);
     wordlistDestroy(&ssl_crtd->cmdline);
-    delete ssl_crtd;
     ssl_crtd = nullptr;
 }
 
@@ -167,7 +166,7 @@ Ssl::HandleGeneratorReply(void *data, const ::Helper::Reply &reply)
 }
 #endif //USE_SSL_CRTD
 
-helper *Ssl::CertValidationHelper::ssl_crt_validator = nullptr;
+helper::Pointer Ssl::CertValidationHelper::ssl_crt_validator = nullptr;
 
 void Ssl::CertValidationHelper::Init()
 {
@@ -183,7 +182,7 @@ void Ssl::CertValidationHelper::Init()
     if (!found)
         return;
 
-    ssl_crt_validator = new helper("ssl_crt_validator");
+    ssl_crt_validator = helper::Make("ssl_crt_validator");
     ssl_crt_validator->childs.updateLimits(Ssl::TheConfig.ssl_crt_validator_Children);
     ssl_crt_validator->ipc_type = IPC_STREAM;
     // The crtd messages may contain the eol ('\n') character. We are
@@ -236,7 +235,6 @@ void Ssl::CertValidationHelper::Shutdown()
         return;
     helperShutdown(ssl_crt_validator);
     wordlistDestroy(&ssl_crt_validator->cmdline);
-    delete ssl_crt_validator;
     ssl_crt_validator = nullptr;
 
     // CertValidationHelper::HelperCache is a static member, it is not good policy to

diff --git a/src/ssl/helper.h b/src/ssl/helper.h
index 220088258bce948a69358f8ec14353ea0a9cb41b..0810049e1347af1f94ee23559c66b48bd1435127 100644 (file)
--- a/src/ssl/helper.h
+++ b/src/ssl/helper.h
@@ -34,7 +34,7 @@ public:
     /// Submit crtd message to external crtd server.
     static void Submit(CrtdMessage const & message, HLPCB * callback, void *data);
 private:
-    static helper * ssl_crtd; ///< helper for management of ssl_crtd.
+    static ::Helper::ClientPointer ssl_crtd; ///< helper for management of ssl_crtd.
 };
 #endif
 
@@ -53,7 +53,7 @@ public:
     /// Submit crtd request message to external crtd server.
     static void Submit(const Ssl::CertValidationRequest &, const Callback &);
 private:
-    static helper * ssl_crt_validator; ///< helper for management of ssl_crtd.
+    static ::Helper::ClientPointer ssl_crt_validator; ///< helper for management of ssl_crtd.
 public:
     typedef ClpMap<SBuf, CertValidationResponse::Pointer, CertValidationResponse::MemoryUsedByResponse> CacheType;
     static CacheType *HelperCache; ///< cache for cert validation helper

diff --git a/src/tests/stub_helper.cc b/src/tests/stub_helper.cc
index 412c524e95207589ac2ecfcf5ee335cf809da756..d797ad26f9974b94feb7e97c62d040a8b85c7687 100644 (file)
--- a/src/tests/stub_helper.cc
+++ b/src/tests/stub_helper.cc
@@ -12,15 +12,13 @@
 #define STUB_API "helper.cc"
 #include "tests/STUB.h"
 
-void helperSubmit(helper *, const char *, HLPCB *, void *) STUB
-void helperStatefulSubmit(statefulhelper *, const char *, HLPCB *, void *, uint64_t) STUB
+void helperSubmit(const helper::Pointer &, const char *, HLPCB *, void *) STUB
+void helperStatefulSubmit(const statefulhelper::Pointer &, const char *, HLPCB *, void *, const Helper::ReservationId &) STUB
 helper::~helper() STUB
-CBDATA_CLASS_INIT(helper);
 void helper::packStatsInto(Packable *, const char *) const STUB
 
-void helperShutdown(helper *) STUB
-void helperStatefulShutdown(statefulhelper *) STUB
-void helperOpenServers(helper *) STUB
-void helperStatefulOpenServers(statefulhelper *) STUB
-CBDATA_CLASS_INIT(statefulhelper);
+void helperShutdown(const helper::Pointer &) STUB
+void helperStatefulShutdown(const statefulhelper::Pointer &) STUB
+void helperOpenServers(const helper::Pointer &) STUB
+void helperStatefulOpenServers(const statefulhelper::Pointer &) STUB