From a95ff429c936e07b671e00e5076e43a6cf083e50 Mon Sep 17 00:00:00 2001 From: Amos Jeffries Date: Wed, 7 Mar 2012 15:54:00 -0700 Subject: [PATCH] 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. --- src/comm/ConnOpener.cc | 64 +++++++++++++++++++++++++++--------------- src/comm/ConnOpener.h | 3 +- 2 files changed, 43 insertions(+), 24 deletions(-) diff --git a/src/comm/ConnOpener.cc b/src/comm/ConnOpener.cc index 2429c8c7ab..7883cea327 100644 --- 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 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 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 ¶ms = GetCommParams(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 0874605b5b..05fcd550f5 100644 --- 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. -- 2.47.2