From 167624c95deaf673bbc4380efda7522c8a28f70c Mon Sep 17 00:00:00 2001 From: Amos Jeffries Date: Fri, 29 Mar 2013 00:18:05 -0600 Subject: [PATCH] Bug 3565: Resuming postponed accept kills Squid This implements part 1 of Alex proposed fixes outlined in bug 3565. Crashes with deferred accept() pointer dereference. It replaces the raw-pointers stored in AcceptLimiter with a new TcpAcceptor::Pointer. It also adds a bit of TODO documentation about future optimizations which we might want to consider later. --- src/comm/AcceptLimiter.cc | 31 ++++++++++++++++++------------- src/comm/AcceptLimiter.h | 19 ++++++++++++++----- src/comm/TcpAcceptor.h | 13 +++++-------- src/tests/stub_libcomm.cc | 4 ++-- 4 files changed, 39 insertions(+), 28 deletions(-) diff --git a/src/comm/AcceptLimiter.cc b/src/comm/AcceptLimiter.cc index 1e5dee1cd5..dc16dbe934 100644 --- a/src/comm/AcceptLimiter.cc +++ b/src/comm/AcceptLimiter.cc @@ -6,29 +6,33 @@ Comm::AcceptLimiter Comm::AcceptLimiter::Instance_; -Comm::AcceptLimiter &Comm::AcceptLimiter::Instance() +Comm::AcceptLimiter & +Comm::AcceptLimiter::Instance() { return Instance_; } void -Comm::AcceptLimiter::defer(Comm::TcpAcceptor *afd) +Comm::AcceptLimiter::defer(const Comm::TcpAcceptor::Pointer &afd) { - ++ afd->isLimited; + ++ (afd->isLimited); debugs(5, 5, HERE << afd->conn << " x" << afd->isLimited); - deferred.push_back(afd); + deferred_.push_back(afd); } void -Comm::AcceptLimiter::removeDead(const Comm::TcpAcceptor *afd) +Comm::AcceptLimiter::removeDead(const Comm::TcpAcceptor::Pointer &afd) { - for (unsigned int i = 0; i < deferred.size() && afd->isLimited > 0; i++) { - if (deferred[i] == afd) { - -- deferred[i]->isLimited; - deferred[i] = NULL; // fast. kick() will skip empty entries later. + uint64_t abandonedClients = 0; + for (unsigned int i = 0; i < deferred_.size() && afd->isLimited > 0; ++i) { + if (deferred_[i] == afd) { + -- deferred_[i]->isLimited; + deferred_[i] = NULL; // fast. kick() will skip empty entries later. debugs(5, 5, HERE << afd->conn << " x" << afd->isLimited); + ++abandonedClients; } } + debugs(5,4, HERE << "Abandoned " << abandonedClients << " client TCP SYN by closing socket: " << afd->conn); } void @@ -37,12 +41,13 @@ Comm::AcceptLimiter::kick() // TODO: this could be optimized further with an iterator to search // looking for first non-NULL, followed by dumping the first N // with only one shift()/pop_front operation + // OR, by reimplementing as a list instead of Vector. - debugs(5, 5, HERE << " size=" << deferred.size()); - while (deferred.size() > 0 && fdNFree() >= RESERVED_FD) { + debugs(5, 5, HERE << " size=" << deferred_.size()); + while (deferred_.size() > 0 && fdNFree() >= RESERVED_FD) { /* NP: shift() is equivalent to pop_front(). Giving us a FIFO queue. */ - TcpAcceptor *temp = deferred.shift(); - if (temp != NULL) { + TcpAcceptor::Pointer temp = deferred_.shift(); + if (temp.valid()) { debugs(5, 5, HERE << " doing one."); -- temp->isLimited; temp->acceptNext(); diff --git a/src/comm/AcceptLimiter.h b/src/comm/AcceptLimiter.h index 5ee810688a..a31d96af25 100644 --- a/src/comm/AcceptLimiter.h +++ b/src/comm/AcceptLimiter.h @@ -2,12 +2,11 @@ #define _SQUID_SRC_COMM_ACCEPT_LIMITER_H #include "Array.h" +#include "comm/TcpAcceptor.h" namespace Comm { -class TcpAcceptor; - /** * FIFO Queue holding listener socket handlers which have been activated * ready to dupe their FD and accept() a new client connection. @@ -18,6 +17,16 @@ class TcpAcceptor; * removeDead - used only by Comm layer ConnAcceptor to remove themselves when dying. * kick - used by Comm layer when FD are closed. */ +/* TODO this algorithm can be optimized further: + * + * 1) reduce overheads by only pushing one entry per port to the list? + * use TcpAcceptor::isLimited as a flag whether to re-list when kick()'ing + * or to NULL an entry while scanning the list for empty spaces. + * Side effect: TcpAcceptor->kick() becomes allowed to pull off multiple accept()'s in bunches + * + * 2) re-implement as a list instead of vector? + * storing head/tail pointers for fast push/pop and avoiding the whole shift() overhead + */ class AcceptLimiter { @@ -26,10 +35,10 @@ public: static AcceptLimiter &Instance(); /** delay accepting a new client connection. */ - void defer(Comm::TcpAcceptor *afd); + void defer(const TcpAcceptor::Pointer &afd); /** remove all records of an acceptor. Only to be called by the ConnAcceptor::swanSong() */ - void removeDead(const Comm::TcpAcceptor *afd); + void removeDead(const TcpAcceptor::Pointer &afd); /** try to accept and begin processing any delayed client connections. */ void kick(); @@ -38,7 +47,7 @@ private: static AcceptLimiter Instance_; /** FIFO queue */ - Vector deferred; + Vector deferred_; }; }; // namepace Comm diff --git a/src/comm/TcpAcceptor.h b/src/comm/TcpAcceptor.h index ef3d2129de..48aee211ee 100644 --- a/src/comm/TcpAcceptor.h +++ b/src/comm/TcpAcceptor.h @@ -1,17 +1,11 @@ #ifndef SQUID_COMM_TCPACCEPTOR_H #define SQUID_COMM_TCPACCEPTOR_H -#include "base/AsyncCall.h" +#include "base/AsyncJob.h" +#include "base/CbcPointer.h" #include "base/Subscription.h" -#include "CommCalls.h" #include "comm_err_t.h" #include "comm/forward.h" -#include "comm/TcpAcceptor.h" -#include "ip/Address.h" - -#if HAVE_MAP -#include -#endif namespace Comm { @@ -32,6 +26,9 @@ class AcceptLimiter; */ class TcpAcceptor : public AsyncJob { +public: + typedef CbcPointer Pointer; + private: virtual void start(); virtual bool doneAll() const; diff --git a/src/tests/stub_libcomm.cc b/src/tests/stub_libcomm.cc index b2632fc0e9..f843460b8c 100644 --- a/src/tests/stub_libcomm.cc +++ b/src/tests/stub_libcomm.cc @@ -8,8 +8,8 @@ #include "comm/AcceptLimiter.h" Comm::AcceptLimiter dummy; Comm::AcceptLimiter & Comm::AcceptLimiter::Instance() STUB_RETVAL(dummy) -void Comm::AcceptLimiter::defer(Comm::TcpAcceptor *afd) STUB -void Comm::AcceptLimiter::removeDead(const Comm::TcpAcceptor *afd) STUB +void Comm::AcceptLimiter::defer(const Comm::TcpAcceptor::Pointer &afd) STUB +void Comm::AcceptLimiter::removeDead(const Comm::TcpAcceptor::Pointer &afd) STUB void Comm::AcceptLimiter::kick() STUB #include "comm/Connection.h" -- 2.47.2