From: Amos Jeffries Date: Wed, 22 Sep 2010 14:24:38 +0000 (+1200) Subject: Audit fixes X-Git-Tag: take08~55^2~124^2~58 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=418b3087cf44c6b8b4c3531f62dbdb9087c047c4;p=thirdparty%2Fsquid.git Audit fixes --- diff --git a/src/comm/AcceptLimiter.cc b/src/comm/AcceptLimiter.cc index a3ce69935a..cad3c324a0 100644 --- a/src/comm/AcceptLimiter.cc +++ b/src/comm/AcceptLimiter.cc @@ -41,8 +41,7 @@ Comm::AcceptLimiter::kick() 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. */ - ConnAcceptor *temp = deferred.shift(); - if (temp != NULL) { + if ((ConnAcceptor *temp = deferred.shift()) != NULL) { debugs(5, 5, HERE << " doing one."); temp->isLimited--; temp->acceptNext(); diff --git a/src/comm/ConnAcceptor.cc b/src/comm/ConnAcceptor.cc index afabcdc102..f10f14e1d0 100644 --- a/src/comm/ConnAcceptor.cc +++ b/src/comm/ConnAcceptor.cc @@ -189,7 +189,7 @@ Comm::ConnAcceptor::doAccept(int fd, void *data) } commSetSelect(fd, COMM_SELECT_READ, Comm::ConnAcceptor::doAccept, afd, 0); - } catch(TextException &e) { + } catch(const TextException &e) { fatalf("FATAL: error while accepting new client connection: %s\n", e.message); } catch(...) { fatal("FATAL: error while accepting new client connection: [unkown]\n"); @@ -256,9 +256,10 @@ Comm::ConnAcceptor::acceptNext() acceptOne(); } +// XXX: obsolete comment? // NP: can't be a const function because syncWithComm() side effects hit theCallSub->callback(). void -Comm::ConnAcceptor::notify(comm_err_t flag, const Comm::ConnectionPointer &newConnDetails) +Comm::ConnAcceptor::notify(comm_err_t flag, const Comm::ConnectionPointer &newConnDetails) const { // listener socket handlers just abandon the port with COMM_ERR_CLOSING // it should only happen when this object is deleted... diff --git a/src/comm/ConnOpener.cc b/src/comm/ConnOpener.cc index 9b4db2006b..b94ef154f4 100644 --- a/src/comm/ConnOpener.cc +++ b/src/comm/ConnOpener.cc @@ -3,7 +3,7 @@ */ #include "config.h" -#include "base/TextException.h" +//#include "base/TextException.h" #include "comm/ConnOpener.h" #include "comm/Connection.h" #include "comm.h" @@ -23,7 +23,7 @@ Comm::ConnOpener::ConnOpener(Comm::ConnectionPointer &c, AsyncCall::Pointer &han totalTries_(0), failRetries_(0), connectTimeout_(ctimeout), - connStart_(0) + connectStart_(0) {} Comm::ConnOpener::~ConnOpener() @@ -64,9 +64,13 @@ Comm::ConnOpener::swanSong() // recover what we can from the job if (conn_ != NULL && conn_->isOpen()) { // it never reached fully open, so abort the FD + commSetSelect(conn_->fd, COMM_SELECT_WRITE, NULL, NULL, 0); + commSetTimeout(conn_->fd, -1, NULL, NULL); conn_->close(); - fd_table[conn_->fd].flags.open = 0; - // inform the caller + } + + if (callback_ != NULL) { + // inform the still-waiting caller we are dying doneConnecting(COMM_ERR_CONNECT, 0); } @@ -130,38 +134,51 @@ Comm::ConnOpener::start() doneConnecting(COMM_ERR_CONNECT, 0); return; } - - if (calls_.earlyAbort_ == NULL) { - typedef CommCbMemFunT Dialer; - calls_.earlyAbort_ = asyncCall(5, 4, "Comm::ConnOpener::earlyAbort", - Dialer(this, &Comm::ConnOpener::earlyAbort)); - comm_add_close_handler(conn_->fd, calls_.earlyAbort_); - } - - if (calls_.timeout_ == NULL) { - typedef CommCbMemFunT Dialer; - calls_.timeout_ = asyncCall(5, 4, "Comm::ConnOpener::timeout", - Dialer(this, &Comm::ConnOpener::timeout)); - debugs(5, 3, HERE << conn_ << " timeout " << connectTimeout_); - commSetTimeout(conn_->fd, connectTimeout_, calls_.timeout_); - } - - if (connStart_ == 0) { - connStart_ = squid_curtime; - } } typedef CommCbMemFunT Dialer; - calls_.connect_ = asyncCall(5, 4, "Comm::ConnOpener::connect", - Dialer(this, &Comm::ConnOpener::connect)); - ScheduleCallHere(calls_.connect_); + calls_.earlyAbort_ = asyncCall(5, 4, "Comm::ConnOpener::earlyAbort", + Dialer(this, &Comm::ConnOpener::earlyAbort)); + comm_add_close_handler(conn_->fd, calls_.earlyAbort_); + + typedef CommCbMemFunT Dialer; + calls_.timeout_ = asyncCall(5, 4, "Comm::ConnOpener::timeout", + Dialer(this, &Comm::ConnOpener::timeout)); + debugs(5, 3, HERE << conn_ << " timeout " << connectTimeout_); + commSetTimeout(conn_->fd, connectTimeout_, calls_.timeout_); + + connectStart_ = squid_curtime; + connect(); +} + +void +Comm::ConnOpener::connected() +{ + /* + * stats.conn_open is used to account for the number of + * connections that we have open to the peer, so we can limit + * based on the max-conn option. We need to increment here, + * even if the connection may fail. + */ + if (conn_->getPeer()) + conn_->getPeer()->stats.conn_open++; + + lookupLocalAddress(); + + /* TODO: remove these fd_table accesses. But old code still depends on fd_table flags to + * indicate the state of a raw fd object being passed around. + * Also, legacy code still depends on comm_local_port() with no access to Comm::Connection + * when those are done comm_local_port can become one of our member functions to do the below. + */ + fd_table[conn_->fd].flags.open = 1; + fd_table[conn_->fd].local_addr = conn_->local; } /** Make an FD connection attempt. * Handles the case(s) when a partially setup connection gets closed early. */ void -Comm::ConnOpener::connect(const CommConnectCbParams &unused) +Comm::ConnOpener::connect() { Must(conn_ != NULL); @@ -171,37 +188,21 @@ Comm::ConnOpener::connect(const CommConnectCbParams &unused) case COMM_INPROGRESS: // check for timeout FIRST. - if(squid_curtime - connStart_ > connectTimeout_) { + if (squid_curtime - connectStart_ > connectTimeout_) { debugs(5, 5, HERE << conn_ << ": * - ERR took too long already."); + conn_->close(); doneConnecting(COMM_TIMEOUT, errno); return; } else { debugs(5, 5, HERE << conn_ << ": COMM_INPROGRESS"); - commSetSelect(conn_->fd, COMM_SELECT_WRITE, Comm::ConnOpener::ConnectRetry, this, 0); + commSetSelect(conn_->fd, COMM_SELECT_WRITE, Comm::ConnOpener::InProgressConnectRetry, this, 0); } break; case COMM_OK: debugs(5, 5, HERE << conn_ << ": COMM_OK - connected"); - /* - * stats.conn_open is used to account for the number of - * connections that we have open to the peer, so we can limit - * based on the max-conn option. We need to increment here, - * even if the connection may fail. - */ - if (conn_->getPeer()) - conn_->getPeer()->stats.conn_open++; - - lookupLocalAddress(); - - /* TODO: remove these fd_table accesses. But old code still depends on fd_table flags to - * indicate the state of a raw fd object being passed around. - * Also, legacy code still depends on comm_local_port() with no access to Comm::Connection - * when those are done comm_local_port can become one of our member functions to do the below. - */ - fd_table[conn_->fd].flags.open = 1; - fd_table[conn_->fd].local_addr = conn_->local; + connected(); if (host_ != NULL) ipcacheMarkGoodAddr(host_, conn_->remote); @@ -219,14 +220,16 @@ Comm::ConnOpener::connect(const CommConnectCbParams &unused) #endif // check for timeout FIRST. - if(squid_curtime - connStart_ > connectTimeout_) { + if(squid_curtime - connectStart_ > connectTimeout_) { debugs(5, 5, HERE << conn_ << ": * - ERR took too long already."); + conn_->close(); doneConnecting(COMM_TIMEOUT, errno); } else if (failRetries_ < Config.connect_retries) { - ScheduleCallHere(calls_.connect_); + eventAdd("Comm::ConnOpener::DelayedConnectRetry", Comm::ConnOpener::DelayedConnectRetry, this, 0.05, 0); } else { // send ERROR back to the upper layer. debugs(5, 5, HERE << conn_ << ": * - ERR tried too many times already."); + conn_->close(); doneConnecting(COMM_ERR_CONNECT, errno); } } @@ -268,23 +271,39 @@ Comm::ConnOpener::earlyAbort(const CommConnectCbParams &io) * NP: When commSetTimeout accepts generic CommCommonCbParams this can die. */ void -Comm::ConnOpener::timeout(const CommTimeoutCbParams &unused) +Comm::ConnOpener::timeout(const CommTimeoutCbParams &) { - ScheduleCallHere(calls_.connect_); + connect(); } /* Legacy Wrapper for the retry event after COMM_INPROGRESS - * TODO: As soon as comm IO accepts Async calls we can use a ConnOpener::connect call + * XXX: As soon as comm commSetSelect() accepts Async calls we can use a ConnOpener::connect call */ void -Comm::ConnOpener::ConnectRetry(int fd, void *data) +Comm::ConnOpener::InProgressConnectRetry(int fd, void *data) { ConnOpener *cs = static_cast(data); + assert(cs); // Ew. we are now outside the all AsyncJob protections. // get back inside by scheduling another call... - typedef CommCbMemFunT Dialer; - AsyncCall::Pointer call = asyncCall(5, 4, "Comm::ConnOpener::connect", - Dialer(cs, &Comm::ConnOpener::connect)); + typedef NullaryMemFunT Dialer; + AsyncCall::Pointer call = JobCallback(5, 4, Dialer, cs, Comm::ConnOpener::connect); + ScheduleCallHere(call); +} + +/* Legacy Wrapper for the retry event with small delay after errors. + * XXX: As soon as eventAdd() accepts Async calls we can use a ConnOpener::connect call + */ +void +Comm::ConnOpener::DelayedConnectRetry(void *data) +{ + ConnOpener *cs = static_cast(data); + assert(cs); + + // Ew. we are now outside the all AsyncJob protections. + // get back inside by scheduling another call... + typedef NullaryMemFunT Dialer; + AsyncCall::Pointer call = JobCallback(5, 4, Dialer, cs, Comm::ConnOpener::connect); ScheduleCallHere(call); } diff --git a/src/comm/ConnOpener.h b/src/comm/ConnOpener.h index 6fbbe5e6bc..2a5c0c7c0b 100644 --- a/src/comm/ConnOpener.h +++ b/src/comm/ConnOpener.h @@ -33,11 +33,13 @@ private: ConnOpener(const ConnOpener &); ConnOpener & operator =(const ConnOpener &c); - void connect(const CommConnectCbParams &unused); void earlyAbort(const CommConnectCbParams &); - void timeout(const CommTimeoutCbParams &unused); + void timeout(const CommTimeoutCbParams &); void doneConnecting(comm_err_t status, int xerrno); - static void ConnectRetry(int fd, void *data); + static void InProgressConnectRetry(int fd, void *data); + static void DelayedConnectRetry(void *data); + void connect(); + void connected(); void lookupLocalAddress(); private: @@ -59,7 +61,6 @@ private: /// handles to calls which we may need to cancel. struct Calls { - AsyncCall::Pointer connect_; AsyncCall::Pointer earlyAbort_; AsyncCall::Pointer timeout_; } calls_; diff --git a/src/comm/Connection.cc b/src/comm/Connection.cc index 34ef645189..d1bdc16d95 100644 --- a/src/comm/Connection.cc +++ b/src/comm/Connection.cc @@ -23,7 +23,7 @@ Comm::Connection::Connection() : Comm::Connection::~Connection() { close(); - cbdataReferenceDone(_peer); + cbdataReferenceDone(getPeer()); } Comm::ConnectionPointer @@ -41,7 +41,7 @@ Comm::Connection::copyDetails() const c->fd = -1; // ensure we have a cbdata reference to _peer not a straight ptr copy. - c->_peer = cbdataReference(_peer); + c->_peer = cbdataReference(getPeer()); return c; } @@ -52,23 +52,30 @@ Comm::Connection::close() if (isOpen()) { comm_close(fd); fd = -1; - if (_peer) - _peer->stats.conn_open--; + if (getPeer()) + getPeer()->stats.conn_open--; } } +peer * +Comm::Connection::getPeer() const +{ + if (cbdataReferenceValid(_peer)) + return _peer; + + return NULL; +} + void Comm::Connection::setPeer(peer *p) { /* set to self. nothing to do. */ - if (_peer == p) + if (getPeer() == p) return; /* clear any previous ptr */ - if (_peer) { + if (getPeer()) cbdataReferenceDone(_peer); - _peer = NULL; - } /* set the new one (unless it is NULL */ if (p) { diff --git a/src/comm/Connection.h b/src/comm/Connection.h index 9838a099ab..b529ca49c1 100644 --- a/src/comm/Connection.h +++ b/src/comm/Connection.h @@ -104,7 +104,7 @@ public: * The caller is responsible for all CBDATA operations regarding the * used of the pointer returned. */ - peer * const getPeer() const { return _peer; } + peer * const getPeer() const; /** alter the stored peer pointer. * Perform appropriate CBDATA operations for locking the peer pointer @@ -138,6 +138,10 @@ public: int flags; private: + // XXX: we need to call this member peer_ but the struct peer_ global type + // behind peer* clashes despite our private Comm:: namespace + // (it being global gets inherited here too). + /** cache_peer data object (if any) */ peer *_peer; };