------------------------------------------------------------ revno: 12520 revision-id: squid3@treenet.co.nz-20130329055646-f11tzb5neai6iqu9 parent: squid3@treenet.co.nz-20130329055545-rdedlnyw6wcjx9v5 fixes bug(s): http://bugs.squid-cache.org/show_bug.cgi?id=3565 committer: Amos Jeffries branch nick: 3.3 timestamp: Thu 2013-03-28 23:56:46 -0600 message: 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. ------------------------------------------------------------ # Bazaar merge directive format 2 (Bazaar 0.90) # revision_id: squid3@treenet.co.nz-20130329055646-f11tzb5neai6iqu9 # target_branch: http://bzr.squid-cache.org/bzr/squid3/3.3 # testament_sha1: 5f826de47294bce04fdd5e17d1065fa58b3bd304 # timestamp: 2013-03-29 06:00:05 +0000 # source_branch: http://bzr.squid-cache.org/bzr/squid3/3.3 # base_revision_id: squid3@treenet.co.nz-20130329055545-\ # rdedlnyw6wcjx9v5 # # Begin patch === modified file 'src/comm/AcceptLimiter.cc' --- src/comm/AcceptLimiter.cc 2012-08-14 11:53:07 +0000 +++ src/comm/AcceptLimiter.cc 2013-03-29 05:56:46 +0000 @@ -7,29 +7,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; - debugs(5, 5, HERE << afd->conn << " x" << afd->isLimited); - deferred.push_back(afd); + ++ (afd->isLimited); + debugs(5, 5, afd->conn << " x" << afd->isLimited); + 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. - debugs(5, 5, HERE << afd->conn << " x" << afd->isLimited); + 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, afd->conn << " x" << afd->isLimited); + ++abandonedClients; } } + debugs(5,4, "Abandoned " << abandonedClients << " client TCP SYN by closing socket: " << afd->conn); } void @@ -38,13 +42,14 @@ // 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, "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) { - debugs(5, 5, HERE << " doing one."); + TcpAcceptor::Pointer temp = deferred_.shift(); + if (temp.valid()) { + debugs(5, 5, "doing one."); -- temp->isLimited; temp->acceptNext(); break; === modified file 'src/comm/AcceptLimiter.h' --- src/comm/AcceptLimiter.h 2011-05-13 08:13:01 +0000 +++ src/comm/AcceptLimiter.h 2013-03-29 05:56:46 +0000 @@ -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 @@ * 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 @@ 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 @@ static AcceptLimiter Instance_; /** FIFO queue */ - Vector deferred; + Vector deferred_; }; }; // namepace Comm === modified file 'src/comm/TcpAcceptor.h' --- src/comm/TcpAcceptor.h 2011-01-31 11:50:28 +0000 +++ src/comm/TcpAcceptor.h 2013-03-29 05:56:46 +0000 @@ -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 TcpAcceptor : public AsyncJob { +public: + typedef CbcPointer Pointer; + private: virtual void start(); virtual bool doneAll() const; === modified file 'src/tests/stub_libcomm.cc' --- src/tests/stub_libcomm.cc 2012-09-23 09:04:21 +0000 +++ src/tests/stub_libcomm.cc 2013-03-29 05:56:46 +0000 @@ -7,8 +7,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"