From 3bd118d6c7951143612f304b8d2ada2b01633201 Mon Sep 17 00:00:00 2001 From: Eduard Bagdasaryan Date: Mon, 14 Aug 2023 01:39:29 +0000 Subject: [PATCH] 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. --- src/auth/basic/Config.cc | 5 +- src/auth/basic/Config.h | 2 +- src/auth/digest/Config.cc | 5 +- src/auth/digest/Config.h | 2 +- src/auth/negotiate/Config.cc | 5 +- src/auth/negotiate/Config.h | 2 +- src/auth/ntlm/Config.cc | 5 +- src/auth/ntlm/Config.h | 2 +- src/external_acl.cc | 5 +- src/helper.cc | 69 ++++++++++++++------------ src/helper.h | 94 ++++++++++++++++-------------------- src/helper/forward.h | 5 ++ src/redirect.cc | 12 ++--- src/ssl/helper.cc | 10 ++-- src/ssl/helper.h | 4 +- src/tests/stub_helper.cc | 14 +++--- 16 files changed, 116 insertions(+), 125 deletions(-) diff --git a/src/auth/basic/Config.cc b/src/auth/basic/Config.cc index ddd214177a..901fdbe5f8 100644 --- 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 a6555d28c6..8d1d95f6e9 100644 --- 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 f00e2ba682..b3767c963d 100644 --- 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 174a1a2cfd..f6bbbce2f0 100644 --- 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 cf0033526c..e1f5e2669b 100644 --- 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 b4dc2fe416..b037f8699e 100644 --- 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 0119ca1289..0928a970ef 100644 --- 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 86570d2265..3c3c197982 100644 --- 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 e411c9a893..62817ec76d 100644 --- 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 c83cb53dd4..e09841fdc0 100644 --- 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(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 7156488775..3b02430529 100644 --- 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; + + /// \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 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; typedef std::unordered_map 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 c833cce17b..c86021326f 100644 --- 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; +using StatefulClientPointer = RefCount; + } // namespace Helper typedef void HLPCB(void *, const Helper::Reply &); diff --git a/src/redirect.cc b/src/redirect.cc index d1d284be2a..2dbfd012b0 100644 --- 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 bd0c158685..6ea027ae8f 100644 --- 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 220088258b..0810049e13 100644 --- 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 CacheType; static CacheType *HelperCache; ///< cache for cert validation helper diff --git a/src/tests/stub_helper.cc b/src/tests/stub_helper.cc index 412c524e95..d797ad26f9 100644 --- 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 -- 2.47.2