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.

diff --git a/src/comm/AcceptLimiter.cc b/src/comm/AcceptLimiter.cc
index a61b19e2c832d8b543658877d8445cda48c0dade..7af4beaaf82eb058757adf917fbea30184ded9c5 100644 (file)
--- a/src/comm/AcceptLimiter.cc
+++ b/src/comm/AcceptLimiter.cc
@@ -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 @@ 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, "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;

diff --git a/src/comm/AcceptLimiter.h b/src/comm/AcceptLimiter.h
index 5ee810688af8fc40fbbf916077225f2068b8cc83..8cda97f723b2a411223551d675da905724cd2dd9 100644 (file)
--- 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<Comm::TcpAcceptor*> deferred;
+    Vector<TcpAcceptor::Pointer> deferred_;
 };
 
 }; // namepace Comm

diff --git a/src/comm/TcpAcceptor.h b/src/comm/TcpAcceptor.h
index ef3d2129deab685d59a1e81f3715362f44776473..48aee211eeb0d92fed8c4f2400dd7765e5f9b4d5 100644 (file)
--- 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 <map>
-#endif
 
 namespace Comm
 {
@@ -32,6 +26,9 @@ class AcceptLimiter;
  */
 class TcpAcceptor : public AsyncJob
 {
+public:
+    typedef CbcPointer<Comm::TcpAcceptor> Pointer;
+
 private:
     virtual void start();
     virtual bool doneAll() const;

diff --git a/src/tests/stub_libcomm.cc b/src/tests/stub_libcomm.cc
index 4fa35172f7e6cc7c12bbac36d3779c731f0423ac..16f19d1ba3bb7bd9ad1f473bc8d6c3063cd9095a 100644 (file)
--- a/src/tests/stub_libcomm.cc
+++ b/src/tests/stub_libcomm.cc
@@ -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"