Bug 3397: do not mark connection as opened until after SYN-ACK

assertion failed: comm.cc:1117: "isOpen(fd)" on FwdState destruct with
half-connected server. When the SYN was sent but ACK not yet received.

diff --git a/src/comm/ConnOpener.cc b/src/comm/ConnOpener.cc
index 2429c8c7ab1df99396f40f84253dcc1981c04b80..7883cea3270c64628037d324d2686ad19eb1811a 100644 (file)
--- a/src/comm/ConnOpener.cc
+++ b/src/comm/ConnOpener.cc
@@ -20,6 +20,7 @@ CBDATA_CLASS_INIT(ConnOpener);
 Comm::ConnOpener::ConnOpener(Comm::ConnectionPointer &c, AsyncCall::Pointer &handler, time_t ctimeout) :
         AsyncJob("Comm::ConnOpener"),
         host_(NULL),
+        temporaryFd_(-1),
         conn_(c),
         callback_(handler),
         totalTries_(0),
@@ -63,15 +64,6 @@ Comm::ConnOpener::swanSong()
         calls_.timeout_ = NULL;
     }
 
-    // rollback what we can from the job state
-    if (conn_ != NULL && conn_->isOpen()) {
-        // drop any handlers now to save a lot of cycles later
-        Comm::SetSelect(conn_->fd, COMM_SELECT_WRITE, NULL, NULL, 0);
-        commUnsetConnTimeout(conn_);
-        // it never reached fully open, so abort the FD
-        conn_->close();
-    }
-
     if (callback_ != NULL) {
         if (callback_->canceled())
             callback_ = NULL;
@@ -80,6 +72,12 @@ Comm::ConnOpener::swanSong()
             doneConnecting(COMM_ERR_CONNECT, 0);
     }
 
+    // rollback what we can from the job state
+    if (temporaryFd_ >= 0) {
+        // doneConnecting() handles partial FD connection cleanup
+        doneConnecting(COMM_ERR_CONNECT, 0);
+    }
+
     AsyncJob::swanSong();
 }
 
@@ -104,7 +102,7 @@ Comm::ConnOpener::getHost() const
 /**
  * Connection attempt are completed. One way or the other.
  * Pass the results back to the external handler.
- * NP: on connection errors the connection close() must be called first.
+ * NP: on errors the earlyAbort call should be cancelled first with a reason.
  */
 void
 Comm::ConnOpener::doneConnecting(comm_err_t status, int xerrno)
@@ -132,6 +130,21 @@ Comm::ConnOpener::doneConnecting(comm_err_t status, int xerrno)
         callback_ = NULL;
     }
 
+    if (temporaryFd_ >= 0) {
+        // it never reached fully open, so cleanup the FD handlers
+        // Note that comm_close() sequence does not happen for partially open FD
+        Comm::SetSelect(temporaryFd_, COMM_SELECT_WRITE, NULL, NULL, 0);
+        calls_.earlyAbort_ = NULL;
+        if (calls_.timeout_ != NULL) {
+            calls_.timeout_->cancel("Comm::ConnOpener::doneConnecting");
+            calls_.timeout_ = NULL;
+        }
+        fd_table[temporaryFd_].timeoutHandler = NULL;
+        fd_table[temporaryFd_].timeout = 0;
+        fd_close(temporaryFd_);
+        temporaryFd_ = -1;
+    }
+
     /* ensure cleared local state, we are done. */
     conn_ = NULL;
 }
@@ -142,15 +155,15 @@ Comm::ConnOpener::start()
     Must(conn_ != NULL);
 
     /* get a socket open ready for connecting with */
-    if (!conn_->isOpen()) {
+    if (temporaryFd_ < 0) {
 #if USE_IPV6
         /* outbound sockets have no need to be protocol agnostic. */
         if (conn_->remote.IsIPv4()) {
             conn_->local.SetIPv4();
         }
 #endif
-        conn_->fd = comm_openex(SOCK_STREAM, IPPROTO_TCP, conn_->local, conn_->flags, conn_->tos, conn_->nfmark, host_);
-        if (!conn_->isOpen()) {
+        temporaryFd_ = comm_openex(SOCK_STREAM, IPPROTO_TCP, conn_->local, conn_->flags, conn_->tos, conn_->nfmark, host_);
+        if (temporaryFd_ < 0) {
             doneConnecting(COMM_ERR_CONNECT, 0);
             return;
         }
@@ -158,12 +171,20 @@ Comm::ConnOpener::start()
 
     typedef CommCbMemFunT<Comm::ConnOpener, CommCloseCbParams> abortDialer;
     calls_.earlyAbort_ = JobCallback(5, 4, abortDialer, this, Comm::ConnOpener::earlyAbort);
-    comm_add_close_handler(conn_->fd, calls_.earlyAbort_);
+    comm_add_close_handler(temporaryFd_, calls_.earlyAbort_);
 
     typedef CommCbMemFunT<Comm::ConnOpener, CommTimeoutCbParams> timeoutDialer;
     calls_.timeout_ = JobCallback(5, 4, timeoutDialer, this, Comm::ConnOpener::timeout);
     debugs(5, 3, HERE << conn_ << " timeout " << connectTimeout_);
-    commSetConnTimeout(conn_, connectTimeout_, calls_.timeout_);
+
+    // Update the fd_table directly because conn_ is not yet storing the FD
+    assert(temporaryFd_ < Squid_MaxFD);
+    assert(fd_table[temporaryFd_].flags.open);
+    typedef CommTimeoutCbParams Params;
+    Params &params = GetCommParams<Params>(calls_.timeout_);
+    params.conn = conn_;
+    fd_table[temporaryFd_].timeoutHandler = calls_.timeout_;
+    fd_table[temporaryFd_].timeout = squid_curtime + (time_t) connectTimeout_;
 
     connectStart_ = squid_curtime;
     connect();
@@ -172,6 +193,9 @@ Comm::ConnOpener::start()
 void
 Comm::ConnOpener::connected()
 {
+    conn_->fd = temporaryFd_;
+    temporaryFd_ = -1;
+
     /*
      * stats.conn_open is used to account for the number of
      * connections that we have open to the peer, so we can limit
@@ -206,20 +230,18 @@ Comm::ConnOpener::connect()
 
     totalTries_++;
 
-    switch (comm_connect_addr(conn_->fd, conn_->remote) ) {
+    switch (comm_connect_addr(temporaryFd_, conn_->remote) ) {
 
     case COMM_INPROGRESS:
         // check for timeout FIRST.
         if (squid_curtime - connectStart_ > connectTimeout_) {
             debugs(5, 5, HERE << conn_ << ": * - ERR took too long already.");
             calls_.earlyAbort_->cancel("Comm::ConnOpener::connect timed out");
-            calls_.earlyAbort_ = NULL;
-            conn_->close();
             doneConnecting(COMM_TIMEOUT, errno);
             return;
         } else {
             debugs(5, 5, HERE << conn_ << ": COMM_INPROGRESS");
-            Comm::SetSelect(conn_->fd, COMM_SELECT_WRITE, Comm::ConnOpener::InProgressConnectRetry, this, 0);
+            Comm::SetSelect(temporaryFd_, COMM_SELECT_WRITE, Comm::ConnOpener::InProgressConnectRetry, this, 0);
         }
         break;
 
@@ -236,8 +258,6 @@ Comm::ConnOpener::connect()
         if (squid_curtime - connectStart_ > connectTimeout_) {
             debugs(5, 5, HERE << conn_ << ": * - ERR took too long to receive response.");
             calls_.earlyAbort_->cancel("Comm::ConnOpener::connect timed out");
-            calls_.earlyAbort_ = NULL;
-            conn_->close();
             doneConnecting(COMM_TIMEOUT, errno);
         } else if (failRetries_ < Config.connect_retries) {
             debugs(5, 5, HERE << conn_ << ": * - try again");
@@ -247,8 +267,6 @@ Comm::ConnOpener::connect()
             // send ERROR back to the upper layer.
             debugs(5, 5, HERE << conn_ << ": * - ERR tried too many times already.");
             calls_.earlyAbort_->cancel("Comm::ConnOpener::connect failed");
-            calls_.earlyAbort_ = NULL;
-            conn_->close();
             doneConnecting(COMM_ERR_CONNECT, errno);
         }
     }

diff --git a/src/comm/ConnOpener.h b/src/comm/ConnOpener.h
index 0874605b5b1f0a0ce11a8e20f700371f7e17d590..05fcd550f55ca7cc1b4911d28dc4ae454035498f 100644 (file)
--- a/src/comm/ConnOpener.h
+++ b/src/comm/ConnOpener.h
@@ -45,7 +45,8 @@ private:
 
 private:
     char *host_;                         ///< domain name we are trying to connect to.
-    Comm::ConnectionPointer conn_;       ///< single connection currently being opened.
+    int temporaryFd_;                    ///< the FD being opened. Do NOT set conn_->fd until it is fully open.
+    Comm::ConnectionPointer conn_;       ///< single connection currently to be opened.
     AsyncCall::Pointer callback_;        ///< handler to be called on connection completion.
 
     int totalTries_;   ///< total number of connection attempts over all destinations so far.