From: Amos Jeffries Date: Tue, 5 Oct 2010 10:39:54 +0000 (+1300) Subject: Audit fixes X-Git-Tag: take08~55^2~124^2~46 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=80463bb4030d9455536fdbd89a356c052c2dd996;p=thirdparty%2Fsquid.git Audit fixes --- diff --git a/include/RefCount.h b/include/RefCount.h index a6281b2497..125dbaa408 100644 --- a/include/RefCount.h +++ b/include/RefCount.h @@ -69,13 +69,9 @@ public: bool operator !() const { return !p_; } - C const * operator-> () const {return p_; } + C * operator-> () const {return const_cast(p_); } - C * operator-> () {return const_cast(p_); } - - C const & operator * () const {return *p_; } - - C & operator * () {return *const_cast(p_); } + C & operator * () const { return *const_cast(p_); } C const * getRaw() const {return p_; } diff --git a/src/CommCalls.cc b/src/CommCalls.cc index fadad9b4b8..4191fed73e 100644 --- a/src/CommCalls.cc +++ b/src/CommCalls.cc @@ -118,6 +118,7 @@ CommTimeoutCbParams::CommTimeoutCbParams(void *aData): { } + /* CommAcceptCbPtrFun */ CommAcceptCbPtrFun::CommAcceptCbPtrFun(IOACB *aHandler, diff --git a/src/CommCalls.h b/src/CommCalls.h index 07303f1817..1933e0c11b 100644 --- a/src/CommCalls.h +++ b/src/CommCalls.h @@ -175,6 +175,7 @@ public: CommAcceptCbPtrFun(const CommAcceptCbPtrFun &o); void dial(); + virtual void print(std::ostream &os) const; public: diff --git a/src/base/AsyncCall.cc b/src/base/AsyncCall.cc index cb467bcc72..9bca17efe8 100644 --- a/src/base/AsyncCall.cc +++ b/src/base/AsyncCall.cc @@ -77,3 +77,4 @@ ScheduleCall(const char *fileName, int fileLine, AsyncCall::Pointer &call) AsyncCallQueue::Instance().schedule(call); return true; } + diff --git a/src/client_side.cc b/src/client_side.cc index 4719875bf9..d3218079c7 100644 --- a/src/client_side.cc +++ b/src/client_side.cc @@ -382,8 +382,7 @@ ClientSocketContext::wroteControlMsg(const Comm::ConnectionPointer &conn, char * // close on 1xx errors to be conservative and to simplify the code // (if we do not close, we must notify the source of a failure!) - Comm::ConnectionPointer nonConst = conn; - nonConst->close(); + conn->close(); } /// wroteControlMsg() wrapper: ClientSocketContext is not an AsyncJob @@ -3155,8 +3154,7 @@ httpsCreate(const Comm::ConnectionPointer &details, SSL_CTX *sslContext) if (!ssl) { const int ssl_error = ERR_get_error(); debugs(83, 1, "httpsAccept: Error allocating handle: " << ERR_error_string(ssl_error, NULL) ); - Comm::ConnectionPointer nonConst = details; - nonConst->close(); + details->close(); return NULL; } diff --git a/src/comm.cc b/src/comm.cc index 6aaf74a91c..014562b927 100644 --- a/src/comm.cc +++ b/src/comm.cc @@ -2031,8 +2031,7 @@ commHalfClosedReader(const Comm::ConnectionPointer &conn, char *, size_t size, c // if read failed, close the connection if (flag != COMM_OK) { debugs(5, 3, HERE << "closing " << conn); - Comm::ConnectionPointer nonConst = conn; - nonConst->close(); + conn->close(); return; } diff --git a/src/dns_internal.cc b/src/dns_internal.cc index 11fb152f8c..14b6a471f3 100644 --- a/src/dns_internal.cc +++ b/src/dns_internal.cc @@ -677,8 +677,7 @@ idnsSentQueryVC(const Comm::ConnectionPointer &conn, char *buf, size_t size, com return; if (flag != COMM_OK || size <= 0) { - Comm::ConnectionPointer nonConst = conn; - nonConst->close(); + conn->close(); return; } @@ -1263,10 +1262,8 @@ idnsReadVC(const Comm::ConnectionPointer &conn, char *buf, size_t len, comm_err_ return; if (flag != COMM_OK || len <= 0) { - if (Comm::IsConnOpen(conn)) { - Comm::ConnectionPointer nonConst = conn; - nonConst->close(); - } + if (Comm::IsConnOpen(conn)) + conn->close(); return; } @@ -1294,10 +1291,8 @@ idnsReadVCHeader(const Comm::ConnectionPointer &conn, char *buf, size_t len, com return; if (flag != COMM_OK || len <= 0) { - if (Comm::IsConnOpen(conn)) { - Comm::ConnectionPointer nonConst = conn; - nonConst->close(); - } + if (Comm::IsConnOpen(conn)) + conn->close(); return; } diff --git a/src/errorpage.cc b/src/errorpage.cc index 2b238d6eff..b111526d5e 100644 --- a/src/errorpage.cc +++ b/src/errorpage.cc @@ -484,8 +484,7 @@ errorSendComplete(const Comm::ConnectionPointer &conn, char *bufnotused, size_t err->callback(conn->fd, err->callback_data, size); } else { debugs(4, 3, "errorSendComplete: comm_close"); - Comm::ConnectionPointer nonConst = conn; - nonConst->close(); + conn->close(); } } diff --git a/src/forward.cc b/src/forward.cc index 9e8f956db9..65d5319a22 100644 --- a/src/forward.cc +++ b/src/forward.cc @@ -690,8 +690,7 @@ FwdState::connectDone(const Comm::ConnectionPointer &conn, comm_err_t status, in if (conn->getPeer()) peerConnectFailed(conn->getPeer()); - Comm::ConnectionPointer nonConst = conn; - nonConst->close(); + conn->close(); } retryOrBail(); return; @@ -820,11 +819,11 @@ FwdState::connectStart() port = request->port; } serverDestinations[0]->remote.SetPort(port); - fwdPconnPool->pop(serverDestinations[0], host, checkRetriable()); + Comm::ConnectionPointer temp = fwdPconnPool->pop(serverDestinations[0], host, checkRetriable()); // if we found an open persistent connection to use. use it. - if (Comm::IsConnOpen(serverDestinations[0])) { - serverConn = serverDestinations[0]; + if (temp != NULL && Comm::IsConnOpen(temp)) { + serverConn = temp; debugs(17, 3, HERE << "reusing pconn " << serverConnection()); n_tries++; diff --git a/src/ident/Ident.cc b/src/ident/Ident.cc index ecc069b6b0..3a68ada2cb 100644 --- a/src/ident/Ident.cc +++ b/src/ident/Ident.cc @@ -142,8 +142,7 @@ Ident::ConnectDone(const Comm::ConnectionPointer &conn, comm_err_t status, int x if (c == NULL) { /* no clients care */ - Comm::ConnectionPointer nonConst = conn; - nonConst->close(); + conn->close(); return; } diff --git a/src/pconn.cc b/src/pconn.cc index 94f79947ab..902540550d 100644 --- a/src/pconn.cc +++ b/src/pconn.cc @@ -72,32 +72,41 @@ IdleConnList::~IdleConnList() xfree(hash.key); } +/** Search the list. Matches by FD socket number. + * Performed from the end of list where newest entries are. + * + * \retval <0 The connection is not listed + * \retval >=0 The connection array index + */ int -IdleConnList::findIndex(const Comm::ConnectionPointer &conn) +IdleConnList::findIndexOf(const Comm::ConnectionPointer &conn) const { for (int index = nfds - 1; index >= 0; --index) { - if (conn->fd == theList[index]->fd) + if (conn->fd == theList[index]->fd) { + debugs(48, 3, HERE << "found " << conn << " at index " << index); return index; + } } + debugs(48, 2, HERE << conn << " NOT FOUND!"); return -1; } +/** Remove the entry at specified index. + * \retval false The index is not an in-use entry. + */ bool -IdleConnList::remove(const Comm::ConnectionPointer &conn) +IdleConnList::removeAt(int index) { - int index = findIndex(conn); - if (index < 0) { - debugs(48, 2, HERE << conn << " NOT FOUND!"); + if (index < 0 || index >= nfds) return false; - } - debugs(48, 3, HERE << "found " << conn << " at index " << index); + // shuffle the remaining entries to fill the new gap. for (; index < nfds - 1; index++) theList[index] = theList[index + 1]; if (--nfds == 0) { - debugs(48, 3, "IdleConnList::removeFD: deleting " << hashKeyStr(&hash)); + debugs(48, 3, HERE << "deleting " << hashKeyStr(&hash)); delete this; } return true; @@ -106,7 +115,7 @@ IdleConnList::remove(const Comm::ConnectionPointer &conn) void IdleConnList::clearHandlers(const Comm::ConnectionPointer &conn) { - comm_read_cancel(conn->fd, IdleConnList::read, this); + comm_read_cancel(conn->fd, IdleConnList::Read, this); commSetTimeout(conn->fd, -1, NULL, NULL); } @@ -130,8 +139,8 @@ IdleConnList::push(const Comm::ConnectionPointer &conn) } theList[nfds++] = conn; - comm_read(conn, fakeReadBuf, sizeof(fakeReadBuf), IdleConnList::read, this); - commSetTimeout(conn->fd, Config.Timeout.pconn, IdleConnList::timeout, this); + comm_read(conn, fakeReadBuf, sizeof(fakeReadBuf), IdleConnList::Read, this); + commSetTimeout(conn->fd, Config.Timeout.pconn, IdleConnList::Timeout, this); } /* @@ -161,15 +170,18 @@ IdleConnList::findUseable(const Comm::ConnectionPointer &key) if (!key->local.IsAnyAddr() && key->local.matchIPAddr(theList[i]->local) != 0) continue; - // finally, a match - return theList[i]; + // finally, a match. pop and return it. + Comm::ConnectionPointer result = theList[i]; + /* may delete this */ + removeAt(i); + return result; } - return key; + return Comm::ConnectionPointer(); } void -IdleConnList::read(const Comm::ConnectionPointer &conn, char *buf, size_t len, comm_err_t flag, int xerrno, void *data) +IdleConnList::Read(const Comm::ConnectionPointer &conn, char *buf, size_t len, comm_err_t flag, int xerrno, void *data) { debugs(48, 3, HERE << len << " bytes from " << conn); @@ -179,21 +191,25 @@ IdleConnList::read(const Comm::ConnectionPointer &conn, char *buf, size_t len, c } IdleConnList *list = (IdleConnList *) data; - /* might delete list */ - if (list && list->remove(conn)) { - Comm::ConnectionPointer nonConst = conn; - nonConst->close(); + int index = list->findIndexOf(conn); + if (index >= 0) { + /* might delete list */ + list->removeAt(index); + conn->close(); } } void -IdleConnList::timeout(int fd, void *data) +IdleConnList::Timeout(int fd, void *data) { - debugs(48, 3, "IdleConnList::timeout: FD " << fd); + debugs(48, 3, HERE << "FD " << fd); IdleConnList *list = (IdleConnList *) data; Comm::ConnectionPointer temp = new Comm::Connection; // XXX: transition. make timeouts pass conn in temp->fd = fd; - if (list->remove(temp)) { + int index = list->findIndexOf(temp); + if (index >= 0) { + /* might delete list */ + list->removeAt(index); temp->close(); } else temp->fd = -1; // XXX: transition. prevent temp erasure double-closing FD until timeout CB passess conn in. @@ -208,7 +224,7 @@ PconnPool::key(const Comm::ConnectionPointer &destLink, const char *domain) destLink->remote.ToURL(buf, SQUIDHOSTNAMELEN * 3 + 10); if (domain) { - int used = strlen(buf); + const int used = strlen(buf); snprintf(buf+used, SQUIDHOSTNAMELEN * 3 + 10-used, "/%s", domain); } @@ -269,14 +285,12 @@ void PconnPool::push(const Comm::ConnectionPointer &conn, const char *domain) { if (fdUsageHigh()) { - debugs(48, 3, "PconnPool::push: Not many unused FDs"); - Comm::ConnectionPointer nonConst = conn; - nonConst->close(); + debugs(48, 3, HERE << "Not many unused FDs"); + conn->close(); return; } else if (shutting_down) { - Comm::ConnectionPointer nonConst = conn; - nonConst->close(); - debugs(48, 3, "PconnPool::push: Squid is shutting down. Refusing to do anything"); + conn->close(); + debugs(48, 3, HERE << "Squid is shutting down. Refusing to do anything"); return; } @@ -285,10 +299,10 @@ PconnPool::push(const Comm::ConnectionPointer &conn, const char *domain) if (list == NULL) { list = new IdleConnList(aKey, this); - debugs(48, 3, "PconnPool::push: new IdleConnList for {" << hashKeyStr(&list->hash) << "}" ); + debugs(48, 3, HERE << "new IdleConnList for {" << hashKeyStr(&list->hash) << "}" ); hash_join(table, &list->hash); } else { - debugs(48, 3, "PconnPool::push: found IdleConnList for {" << hashKeyStr(&list->hash) << "}" ); + debugs(48, 3, HERE << "found IdleConnList for {" << hashKeyStr(&list->hash) << "}" ); } list->push(conn); @@ -300,32 +314,25 @@ PconnPool::push(const Comm::ConnectionPointer &conn, const char *domain) debugs(48, 3, HERE << "pushed " << conn << " for " << aKey); } -bool -PconnPool::pop(Comm::ConnectionPointer &destLink, const char *domain, bool isRetriable) +Comm::ConnectionPointer +PconnPool::pop(const Comm::ConnectionPointer &destLink, const char *domain, bool isRetriable) { const char * aKey = key(destLink, domain); IdleConnList *list = (IdleConnList *)hash_lookup(table, aKey); if (list == NULL) { - debugs(48, 3, "PconnPool::pop: lookup for key {" << aKey << "} failed."); - return false; + debugs(48, 3, HERE << "lookup for key {" << aKey << "} failed."); + return Comm::ConnectionPointer(); } else { - debugs(48, 3, "PconnPool::pop: found " << hashKeyStr(&list->hash) << (isRetriable?"(to use)":"(to kill)") ); + debugs(48, 3, HERE << "found " << hashKeyStr(&list->hash) << (isRetriable?"(to use)":"(to kill)") ); } + /* may delete list */ Comm::ConnectionPointer temp = list->findUseable(destLink); + if (Comm::IsConnOpen(temp) && !isRetriable) + temp->close(); - if (Comm::IsConnOpen(temp)) { - list->clearHandlers(temp); - - /* might delete list */ - if (list->remove(temp) && !isRetriable) - temp->close(); - else - destLink = temp; - } - - return true; + return temp; } void diff --git a/src/pconn.h b/src/pconn.h index fcca0dc66e..00db350aa0 100644 --- a/src/pconn.h +++ b/src/pconn.h @@ -28,49 +28,58 @@ class PconnPool; /** \ingroup PConnAPI * A list of connections currently open to a particular destination end-point. - * We currently define the end-point by the FQDN it is serving. */ class IdleConnList { public: IdleConnList(const char *key, PconnPool *parent); ~IdleConnList(); - int numIdle() { return nfds; } - - int findIndex(const Comm::ConnectionPointer &conn); ///< search from the end of array - - /** - * Search the list for an existing connection. Matches by FD. - * - * \retval false The connection does not currently exist in the list. - * We seem to have hit and lost a race condition. - * Nevermind, but MUST NOT do anything with the raw FD. - */ - bool remove(const Comm::ConnectionPointer &conn); + int numIdle() const { return nfds; } + /// Pass control of the connection to the idle list. void push(const Comm::ConnectionPointer &conn); - /** Search the list for a connection which matches the 'key' details. + /** Search the list for a connection which matches the 'key' details + * and pop it off the list. * The list is created based on remote IP:port hash. This further filters * the choices based on specific local-end details requested. - * If nothing usable is found the key is returned unchanged. + * If nothing usable is found the a nil pointer is returned. */ Comm::ConnectionPointer findUseable(const Comm::ConnectionPointer &key); + void clearHandlers(const Comm::ConnectionPointer &conn); private: - static IOCB read; - static PF timeout; + bool removeAt(int index); + int findIndexOf(const Comm::ConnectionPointer &conn) const; + static IOCB Read; + static PF Timeout; public: hash_link hash; /** must be first */ private: + /** List of connections we are holding. + * Sorted oldest to newest for most efficient speeds on pop() and findUsable() + * The worst-case pop() and scans occur on timeout and link closure events + * where timing is less critical. Occasional slow additions are okay. + */ Comm::ConnectionPointer *theList; + + /// Number of entries theList can currently hold without re-allocating (capacity). int nfds_alloc; + ///< Number of in-use entries in theList int nfds; + + /** The pool containing this sub-list. + * The parent performs all stats accounting, and + * will delete us when it dies. It persists for the + * full duration of our existence. + */ PconnPool *parent; + char fakeReadBuf[4096]; // TODO: kill magic number. + CBDATA_CLASS2(IdleConnList); }; @@ -105,7 +114,7 @@ public: * retriable to avoid having a growing number of open connections when many * transactions create persistent connections but are not retriable. */ - bool pop(Comm::ConnectionPointer &serverConn, const char *domain, bool retriable); + Comm::ConnectionPointer pop(const Comm::ConnectionPointer &destLink, const char *domain, bool retriable); void count(int uses); void dumpHist(StoreEntry *e) const; void dumpHash(StoreEntry *e) const; diff --git a/src/whois.cc b/src/whois.cc index c786686442..1d5284dc72 100644 --- a/src/whois.cc +++ b/src/whois.cc @@ -172,8 +172,7 @@ WhoisState::readReply(const Comm::ConnectionPointer &conn, char *aBuffer, size_t err = errorCon(ERR_READ_ERROR, HTTP_INTERNAL_SERVER_ERROR, fwd->request); err->xerrno = errno; fwd->fail(err); - Comm::ConnectionPointer nonConst = conn; - nonConst->close(); + conn->close(); do_next_read = 0; } } else { @@ -184,12 +183,8 @@ WhoisState::readReply(const Comm::ConnectionPointer &conn, char *aBuffer, size_t entry->setPublicKey(); fwd->complete(); - debugs(75, 3, "whoisReadReply: Done: " << entry->url() ); - - Comm::ConnectionPointer nonConst = conn; - nonConst->close(); - + conn->close(); do_next_read = 0; }