From: Amos Jeffries Date: Tue, 25 May 2010 09:35:01 +0000 (+1200) Subject: Audit corrections X-Git-Tag: take08~55^2~124^2~140 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=739b352a5c5b7b27af4a2a255ace606ee03439a5;p=thirdparty%2Fsquid.git Audit corrections --- diff --git a/src/adaptation/icap/Xaction.cc b/src/adaptation/icap/Xaction.cc index e08d181180..e9cf3c9f29 100644 --- a/src/adaptation/icap/Xaction.cc +++ b/src/adaptation/icap/Xaction.cc @@ -201,16 +201,6 @@ void Adaptation::Icap::Xaction::noteCommConnected(const CommConnectCbParams &io) if (io.flag != COMM_OK) dieOnConnectionFailure(); // throws - // TODO: do we still need the timeout handler set? - // there was no mention of un-setting it on success. - - // TODO: service bypass status may differ from that of a transaction - typedef CommCbMemFunT TimeoutDialer; - AsyncCall::Pointer timeoutCall = asyncCall(93, 5, "Adaptation::Icap::Xaction::noteCommTimedout", - TimeoutDialer(this,&Adaptation::Icap::Xaction::noteCommTimedout)); - - commSetTimeout(io.conn->fd, TheConfig.connect_timeout(service().cfg().bypass), timeoutCall); - typedef CommCbMemFunT CloseDialer; closer = asyncCall(93, 5, "Adaptation::Icap::Xaction::noteCommClosed", CloseDialer(this,&Adaptation::Icap::Xaction::noteCommClosed)); diff --git a/src/comm/ConnectStateData.cc b/src/comm/ConnectStateData.cc index 51c4248c0a..6ded18815d 100644 --- a/src/comm/ConnectStateData.cc +++ b/src/comm/ConnectStateData.cc @@ -22,7 +22,7 @@ ConnectStateData::ConnectStateData(Vector *paths, Asy ConnectStateData::ConnectStateData(Comm::Connection::Pointer c, AsyncCall::Pointer handler) : host(NULL), connect_timeout(Config.Timeout.connect), - paths(paths), + paths(NULL), solo(c), callback(handler), total_tries(0), @@ -30,26 +30,18 @@ ConnectStateData::ConnectStateData(Comm::Connection::Pointer c, AsyncCall::Point connstart(0) {} -void * -ConnectStateData::operator new(size_t size) -{ - CBDATA_INIT_TYPE(ConnectStateData); - return cbdataAlloc(ConnectStateData); -} - -void -ConnectStateData::operator delete(void *address) +ConnectStateData::~ConnectStateData() { - cbdataFree(address); + safe_free(host); + paths = NULL; // caller code owns them. + solo = NULL; } void ConnectStateData::callCallback(comm_err_t status, int xerrno) { - assert(paths != NULL); - int fd = -1; - if (paths->size() > 0) { + if (paths != NULL && paths->size() > 0) { fd = (*paths)[0]->fd; debugs(5, 3, HERE << "FD " << fd); comm_remove_close_handler(fd, ConnectStateData::EarlyAbort, this); @@ -60,10 +52,13 @@ ConnectStateData::callCallback(comm_err_t status, int xerrno) Params ¶ms = GetCommParams(callback); if (solo != NULL) { params.conn = solo; - } else { + } else if (paths != NULL) { params.paths = paths; if (paths->size() > 0) params.conn = (*paths)[0]; + } else { + /* catch the error case. */ + assert(paths != NULL && solo != NULL); } params.flag = status; params.xerrno = xerrno; @@ -138,8 +133,8 @@ ConnectStateData::connect() * based on the max-conn option. We need to increment here, * even if the connection may fail. */ - if (active->_peer) - active->_peer->stats.conn_open++; + if (active->getPeer()) + active->getPeer()->stats.conn_open++; /* TODO: remove this fd_table access. But old code still depends on fd_table flags to * indicate the state of a raw fd object being passed around. @@ -189,3 +184,17 @@ ConnectStateData::EarlyAbort(int fd, void *data) * squid shutting down or forcing abort on the connection attempt(s) are the only real fatal cases. */ } + +void +ConnectStateData::Connect(void *data) +{ + ConnectStateData *cs = static_cast(data); + cs->connect(); +} + +void +ConnectStateData::ConnectRetry(int fd, void *data) +{ + ConnectStateData *cs = static_cast(data); + cs->connect(); +} diff --git a/src/comm/ConnectStateData.h b/src/comm/ConnectStateData.h index 60585effb8..72aa1537ef 100644 --- a/src/comm/ConnectStateData.h +++ b/src/comm/ConnectStateData.h @@ -10,31 +10,36 @@ /** * State engine handling the opening of a remote outbound connection * to one of multiple destinations. - * - * Create with a list of possible links and a handler callback to call when connected. */ class ConnectStateData { public: /** open first working of a set of connections */ ConnectStateData(Vector *paths, AsyncCall::Pointer handler); + /** attempt to open one connection. */ ConnectStateData(Comm::Connection::Pointer, AsyncCall::Pointer handler); - void *operator new(size_t); - void operator delete(void *); + ~ConnectStateData(); + + /** + * Actual connect start function. + */ + void connect(); + +private: + /* These objects may NOT be created without connections to act on. Do not define this operator. */ + ConnectStateData(); + /* These objects may NOT be copied. Do not define this operator. */ + const ConnectStateData operator =(const ConnectStateData &c); /** * Wrapper to start the connection attempts happening. */ - static void Connect(void *data) { - ConnectStateData *cs = static_cast(data); - cs->connect(); - }; - static void ConnectRetry(int fd, void *data) { - ConnectStateData *cs = static_cast(data); - cs->connect(); - }; + static void Connect(void *data); + + /** retry */ + static void ConnectRetry(int fd, void *data); /** * Temporary close handler used during connect. @@ -42,21 +47,17 @@ public: */ static void EarlyAbort(int fd, void *data); - /** - * Actual connect start function. - */ - void connect(); - /** * Connection attempt are completed. One way or the other. * Pass the results back to the external handler. */ void callCallback(comm_err_t status, int xerrno); +public: char *host; ///< domain name we are trying to connect to. /** - * time at which to abandone the connection. + * time at which to abandon the connection. * the connection-done callback will be passed COMM_TIMEOUT */ time_t connect_timeout; @@ -70,7 +71,7 @@ private: int fail_retries; ///< number of retries current destination has been tried. time_t connstart; ///< time at which this series of connection attempts was started. - CBDATA_CLASS(ConnectStateData); + CBDATA_CLASS2(ConnectStateData); }; #endif /* _SQUID_SRC_COMM_CONNECTSTATEDATA_H */ diff --git a/src/comm/Connection.cc b/src/comm/Connection.cc index 3306c49517..29701ddc31 100644 --- a/src/comm/Connection.cc +++ b/src/comm/Connection.cc @@ -6,22 +6,32 @@ Comm::Connection::Connection() : local(), remote(), - _peer(NULL), peer_type(HIER_NONE), fd(-1), tos(0), - flags(COMM_NONBLOCKING) + flags(COMM_NONBLOCKING), + _peer(NULL) {} -Comm::Connection::Connection(Comm::Connection &c) : +Comm::Connection::Connection(const Comm::Connection &c) : local(c.local), remote(c.remote), - _peer(c._peer), peer_type(c.peer_type), fd(c.fd), tos(c.tos), flags(c.flags) -{} +{ + _peer = cbdataReference(c._peer); +} + +const Comm::Connection & +Comm::Connection::operator =(const Comm::Connection &c) +{ + memcpy(this, &c, sizeof(Comm::Connection)); + + /* ensure we have a cbdata reference to _peer not a straight ptr copy. */ + _peer = cbdataReference(c._peer); +} Comm::Connection::~Connection() { @@ -32,3 +42,22 @@ Comm::Connection::~Connection() cbdataReferenceDone(_peer); } } + +void +Comm::Connection::setPeer(peer *p) +{ + /* set to self. nothing to do. */ + if (_peer == p) + return; + + /* clear any previous ptr */ + if (_peer) { + cbdataReferenceDone(_peer); + _peer = NULL; + } + + /* set the new one (unless it is NULL */ + if (p) { + _peer = cbdataReference(p); + } +} diff --git a/src/comm/Connection.h b/src/comm/Connection.h index e35cbbe7db..df1717c09a 100644 --- a/src/comm/Connection.h +++ b/src/comm/Connection.h @@ -41,12 +41,12 @@ #include "ip/Address.h" #include "RefCount.h" -class peer; +struct peer; namespace Comm { /** COMM flags */ -/* TODO: make these a struct of boolean flags instead of a bitmap. */ +/* TODO: make these a struct of boolean flags in connection instead of a bitmap. */ #define COMM_UNSET 0x00 #define COMM_NONBLOCKING 0x01 #define COMM_NOCLOEXEC 0x02 @@ -54,38 +54,70 @@ namespace Comm { #define COMM_TRANSPARENT 0x08 #define COMM_DOBIND 0x10 +/** + * Store data about the physical and logical attributes of a connection. + * + * Some link state can be infered from the data, however this is not an + * object for state data. But a semantic equivalent for FD with easily + * accessible cached properties not requiring repeated complex lookups. + * + * While the properties may be changed, they should be considered read-only + * outside of the Comm layer code. + * + * These objects must not be passed around directly, + * but a Comm::Connection::Pointer must be passed instead. + */ class Connection : public RefCountable { public: typedef RefCount Pointer; + /** standard empty connection creation */ Connection(); - Connection(Connection &c); + + /** Clear the conection properties and close any open socket. */ ~Connection(); + /** Clone an existing connections properties. + * This includes the FD, if one is open its a good idea to set it to -1 (unopen) + * on one after copying to prevent both clones calling comm_close() when destructed. + */ + Connection(const Connection &c); + /** see Comm::Connection::Connection */ + const Connection & operator =(const Connection &c); + /** Address/Port for the Squid end of a TCP link. */ Ip::Address local; /** Address for the Remote end of a TCP link. */ Ip::Address remote; - /** cache_peer data object (if any) */ - peer *_peer; - /** Hierarchy code for this connection link */ hier_code peer_type; - /** - * Socket used by this connection. - * -1 if no socket has been opened. - */ + /** Socket used by this connection. -1 if no socket has been opened. */ int fd; - /** Quality of Service TOS values curtrently sent on this connection */ + /** Quality of Service TOS values currently sent on this connection */ int tos; /** COMM flags set on this connection */ int flags; + + /** retrieve the peer pointer for use. + * The caller is responsible for all CBDATA operations regarding the + * used of the pointer returned. + */ + peer * const getPeer() const { return _peer; } + + /** alter the stored peer pointer. + * Perform appropriate CBDATA operations for locking the peer pointer + */ + void setPeer(peer * p); + +private: + /** cache_peer data object (if any) */ + peer *_peer; }; }; // namespace Comm diff --git a/src/forward.cc b/src/forward.cc index 633a366a37..6acc1caec0 100644 --- a/src/forward.cc +++ b/src/forward.cc @@ -487,8 +487,8 @@ FwdState::serverClosed(int fd) debugs(17, 2, HERE << "FD " << fd << " " << entry->url()); assert(paths[0]->fd == fd); - if (paths[0]->_peer) { - paths[0]->_peer->stats.conn_open--; + if (paths[0]->getPeer()) { + paths[0]->getPeer()->stats.conn_open--; } retryOrBail(); @@ -520,7 +520,7 @@ FwdState::retryOrBail() cs->connect(); /* use eventAdd to break potential call sequence loops and to slow things down a little */ - eventAdd("fwdConnectStart", fwdConnectStartWrapper, this, (paths[0]->_peer == NULL) ? 0.05 : 0.005, 0); + eventAdd("fwdConnectStart", fwdConnectStartWrapper, this, (paths[0]->getPeer() == NULL) ? 0.05 : 0.005, 0); return; } // else bail. no more paths possible to try. @@ -577,9 +577,9 @@ FwdState::negotiateSSL(int fd) fail(anErr); - if (paths[0]->_peer) { - peerConnectFailed(paths[0]->_peer); - paths[0]->_peer->stats.conn_open--; + if (paths[0]->getPeer()) { + peerConnectFailed(paths[0]->getPeer()); + paths[0]->getPeer()->stats.conn_open--; } comm_close(paths[0]); @@ -587,11 +587,11 @@ FwdState::negotiateSSL(int fd) } } - if (paths[0]->_peer && !SSL_session_reused(ssl)) { - if (paths[0]->_peer->sslSession) - SSL_SESSION_free(paths[0]->_peer->sslSession); + if (paths[0]->getPeer() && !SSL_session_reused(ssl)) { + if (paths[0]->getPeer()->sslSession) + SSL_SESSION_free(paths[0]->getPeer()->sslSession); - paths[0]->_peer->sslSession = SSL_get1_session(ssl); + paths[0]->getPeer()->sslSession = SSL_get1_session(ssl); } dispatch(); @@ -602,7 +602,7 @@ FwdState::initiateSSL() { SSL *ssl; SSL_CTX *sslContext = NULL; - peer *peer = paths[0]->_peer; + const peer *peer = paths[0]->getPeer(); int fd = paths[0]->fd; if (peer) { @@ -675,8 +675,8 @@ FwdState::connectDone(Comm::Connection::Pointer conn, Vector 0) { - if (paths[0]->_peer) - peerConnectFailed(paths[0]->_peer); + if (paths[0]->getPeer()) + peerConnectFailed(paths[0]->getPeer()); comm_close(paths[0]); } @@ -691,12 +691,12 @@ FwdState::connectDone(Comm::Connection::Pointer conn, Vectorfd, fwdServerClosedWrapper, this); - if (paths[0]->_peer) - peerConnectSucceded(paths[0]->_peer); + if (paths[0]->getPeer()) + peerConnectSucceded(paths[0]->getPeer()); #if USE_SSL - if ((paths[0]->_peer && paths[0]->_peer->use_ssl) || - (!paths[0]->_peer && request->protocol == PROTO_HTTPS)) { + if ((paths[0]->getPeer() && paths[0]->getPeer()->use_ssl) || + (!paths[0]->getPeer() && request->protocol == PROTO_HTTPS)) { initiateSSL(); return; } @@ -721,8 +721,8 @@ FwdState::connectTimeout(int fd) /* This marks the peer DOWN ... */ if (paths.size() > 0) - if (paths[0]->_peer) - peerConnectFailed(paths[0]->_peer); + if (paths[0]->getPeer()) + peerConnectFailed(paths[0]->getPeer()); } comm_close(paths[0]); @@ -744,8 +744,8 @@ FwdState::connectStart() /* connection timeout */ int ctimeout; - if (conn->_peer) { - ctimeout = conn->_peer->connect_timeout > 0 ? conn->_peer->connect_timeout : Config.Timeout.peer_connect; + if (conn->getPeer()) { + ctimeout = conn->getPeer()->connect_timeout > 0 ? conn->getPeer()->connect_timeout : Config.Timeout.peer_connect; } else { ctimeout = Config.Timeout.connect; } @@ -762,11 +762,11 @@ FwdState::connectStart() if (conn->peer_type == PINNED) { ConnStateData *pinned_connection = request->pinnedConnection(); assert(pinned_connection); - conn->fd = pinned_connection->validatePinnedConnection(request, conn->_peer); + conn->fd = pinned_connection->validatePinnedConnection(request, conn->getPeer()); if (conn->fd >= 0) { pinned_connection->unpinConnection(); #if 0 - if (!conn->_peer) + if (!conn->getPeer()) conn->peer_type = HIER_DIRECT; #endif n_tries++; @@ -794,10 +794,10 @@ FwdState::connectStart() const char *host; int port; - if (conn->_peer) { - host = conn->_peer->host; - port = conn->_peer->http_port; - conn->fd = fwdPconnPool->pop(conn->_peer->name, conn->_peer->http_port, request->GetHost(), conn->local, checkRetriable()); + if (conn->getPeer()) { + host = conn->getPeer()->host; + port = conn->getPeer()->http_port; + conn->fd = fwdPconnPool->pop(conn->getPeer()->name, conn->getPeer()->http_port, request->GetHost(), conn->local, checkRetriable()); } else { host = request->GetHost(); port = request->port; @@ -809,7 +809,7 @@ FwdState::connectStart() debugs(17, 3, HERE << "reusing pconn FD " << conn->fd); n_tries++; - if (!conn->_peer) + if (!conn->getPeer()) origin_tries++; updateHierarchyInfo(); @@ -898,10 +898,10 @@ FwdState::dispatch() } #endif - if (paths.size() > 0 && paths[0]->_peer != NULL) { - paths[0]->_peer->stats.fetches++; - request->peer_login = paths[0]->_peer->login; - request->peer_domain = paths[0]->_peer->domain; + if (paths.size() > 0 && paths[0]->getPeer() != NULL) { + paths[0]->getPeer()->stats.fetches++; + request->peer_login = paths[0]->getPeer()->login; + request->peer_domain = paths[0]->getPeer()->domain; httpStart(this); } else { request->peer_login = NULL; @@ -1152,9 +1152,9 @@ FwdState::updateHierarchyInfo() char nextHop[256]; // - if (paths[0]->_peer) { + if (paths[0]->getPeer()) { // went to peer, log peer host name - snprintf(nextHop,256,"%s", paths[0]->_peer->name); + snprintf(nextHop,256,"%s", paths[0]->getPeer()->name); } else { // went DIRECT, must honor log_ip_on_direct if (!Config.onoff.log_ip_on_direct) @@ -1195,7 +1195,7 @@ getOutgoingAddress(HttpRequest * request, Comm::Connection::Pointer conn) // maybe use TPROXY client address if (request && request->flags.spoof_client_ip) { - if (!conn->_peer || !conn->_peer->options.no_tproxy) { + if (!conn->getPeer() || !conn->getPeer()->options.no_tproxy) { conn->local = request->client_addr; // some flags need setting on the socket to use this address conn->flags |= COMM_DOBIND; @@ -1210,7 +1210,7 @@ getOutgoingAddress(HttpRequest * request, Comm::Connection::Pointer conn) } ACLFilledChecklist ch(NULL, request, NULL); - ch.dst_peer = conn->_peer; + ch.dst_peer = conn->getPeer(); ch.dst_addr = conn->remote; // TODO use the connection details in ACL. diff --git a/src/http.cc b/src/http.cc index 8aa967b81e..8e291ef2d3 100644 --- a/src/http.cc +++ b/src/http.cc @@ -96,7 +96,7 @@ HttpStateData::HttpStateData(FwdState *theFwdState) : AsyncJob("HttpStateData"), orig_request->hier.peer_http_request_sent.tv_usec = 0; if (fwd->conn() != NULL) - _peer = fwd->conn()->_peer; /* might be NULL */ + _peer = cbdataReference(fwd->conn()->getPeer()); /* might be NULL */ if (_peer) { const char *url; diff --git a/src/tunnel.cc b/src/tunnel.cc index 3ac26e6ec9..41b965171d 100644 --- a/src/tunnel.cc +++ b/src/tunnel.cc @@ -475,9 +475,9 @@ tunnelConnectTimeout(int fd, void *data) ErrorState *err = NULL; if (tunnelState->paths != NULL && tunnelState->paths->size() > 0) { - if ((*(tunnelState->paths))[0]->_peer) + if ((*(tunnelState->paths))[0]->getPeer()) hierarchyNote(&tunnelState->request->hier, (*(tunnelState->paths))[0]->peer_type, - (*(tunnelState->paths))[0]->_peer->host); + (*(tunnelState->paths))[0]->getPeer()->host); else if (Config.onoff.log_ip_on_direct) hierarchyNote(&tunnelState->request->hier, (*(tunnelState->paths))[0]->peer_type, fd_table[tunnelState->server.fd()].ipaddr); @@ -569,12 +569,12 @@ tunnelConnectDone(Comm::Connection::Pointer unused, Vector_peer && conn->_peer->options.no_delay) + if (conn->getPeer() && conn->getPeer()->options.no_delay) tunnelState->server.setDelayId(DelayId()); #endif - if (conn != NULL && conn->_peer) - hierarchyNote(&tunnelState->request->hier, conn->peer_type, conn->_peer->host); + if (conn != NULL && conn->getPeer()) + hierarchyNote(&tunnelState->request->hier, conn->peer_type, conn->getPeer()->host); else if (Config.onoff.log_ip_on_direct) hierarchyNote(&tunnelState->request->hier, conn->peer_type, fd_table[conn->fd].ipaddr); else @@ -596,19 +596,19 @@ tunnelConnectDone(Comm::Connection::Pointer unused, Vectorserver.fd(), tunnelServerClosed, tunnelState); // TODO: hold the conn. drop these fields. - tunnelState->host = conn->_peer ? conn->_peer->host : xstrdup(request->GetHost()); - request->peer_host = conn->_peer ? conn->_peer->host : NULL; + tunnelState->host = conn->getPeer() ? conn->getPeer()->host : xstrdup(request->GetHost()); + request->peer_host = conn->getPeer() ? conn->getPeer()->host : NULL; tunnelState->port = conn->remote.GetPort(); - if (conn->_peer) { - tunnelState->request->peer_login = conn->_peer->login; + if (conn->getPeer()) { + tunnelState->request->peer_login = conn->getPeer()->login; tunnelState->request->flags.proxying = 1; } else { tunnelState->request->peer_login = NULL; tunnelState->request->flags.proxying = 0; } - if (conn->_peer) + if (conn->getPeer()) tunnelProxyConnected(tunnelState->server.fd(), tunnelState); else { tunnelConnected(tunnelState->server.fd(), tunnelState);