From e05a15516e0045f20d22dfdc0936cbb36a4fe9f7 Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Sun, 8 Aug 2021 22:46:31 -0400 Subject: [PATCH] Keep Connection and other objects in sync with Comm in closure callbacks There are lots of small bugs, inconsistencies, and other problems in Connection closure handlers. It is not clear whether any of those problems could result in serious runtime errors or leaks. In theory, the rest of the code could neutralize their negative side effects. However, even in that case, it was just a matter of time before the next bug will bite us due to stale Connection::fd and such. These changes themselves carry elevated risk, but I think we have to do them to get closer to a reliable code as far as Connection maintenance is concerned; otherwise, we will keep chasing their deadly side effects. Long-term, all these manual efforts to keep things in sync should become unnecessary with the introduction of appropriate Connection ownership APIs that automatically maintain the corresponding environments (TODO). Also marked a few newly uncovered bugs in the official code. --- src/FwdState.cc | 22 ++++++++----- src/FwdState.h | 2 +- src/adaptation/icap/Xaction.cc | 4 +++ src/client_side.cc | 4 +++ src/clients/FtpClient.cc | 5 ++- src/clients/HttpTunneler.cc | 27 +++++++++++----- src/comm.cc | 13 ++++++++ src/comm/TcpAcceptor.cc | 5 ++- src/dns_internal.cc | 4 +++ src/gopher.cc | 3 ++ src/ident/Ident.cc | 5 ++- src/log/TcpLogger.cc | 5 ++- src/mgr/Forwarder.cc | 6 +++- src/mgr/Inquirer.cc | 7 +++-- src/mgr/StoreToCommWriter.cc | 6 +++- src/security/PeerConnector.cc | 56 ++++++++++++++++++++++++++++------ src/snmp/Forwarder.cc | 8 +++-- src/snmp/Inquirer.cc | 10 +++++- src/whois.cc | 1 + 19 files changed, 156 insertions(+), 37 deletions(-) diff --git a/src/FwdState.cc b/src/FwdState.cc index 14718c5768..3de8e563f3 100644 --- a/src/FwdState.cc +++ b/src/FwdState.cc @@ -662,7 +662,7 @@ static void fwdServerClosedWrapper(const CommCloseCbParams ¶ms) { FwdState *fwd = (FwdState *)params.data; - fwd->serverClosed(params.fd); + fwd->serverClosed(); } /**** PRIVATE *****************************************************************/ @@ -732,14 +732,22 @@ FwdState::checkRetriable() } void -FwdState::serverClosed(int fd) +FwdState::serverClosed() { - // XXX: fd is often -1 here - debugs(17, 2, "FD " << fd << " " << entry->url() << " after " << - (fd >= 0 ? fd_table[fd].pconn.uses : -1) << " requests"); - if (fd >= 0 && serverConnection()->fd == fd) - fwdPconnPool->noteUses(fd_table[fd].pconn.uses); + // XXX: This method logic attempts to tolerate Connection::close() called + // for serverConn earlier, by one of our dispatch()ed jobs. If that happens, + // serverConn will already be closed here or, worse, it will already be open + // for the next forwarding attempt. The current code prevents us getting + // stuck, but the long term solution is to stop sharing serverConn. + debugs(17, 2, serverConn); + if (Comm::IsConnOpen(serverConn)) { + const auto uses = fd_table[serverConn->fd].pconn.uses; + debugs(17, 3, "prior uses: " << uses); + fwdPconnPool->noteUses(uses); // XXX: May not have come from fwdPconnPool + serverConn->noteClosure(); + } serverConn = nullptr; + closeHandler = nullptr; destinationReceipt = nullptr; retryOrBail(); } diff --git a/src/FwdState.h b/src/FwdState.h index 916db122e0..4c9a66d9de 100644 --- a/src/FwdState.h +++ b/src/FwdState.h @@ -88,7 +88,7 @@ public: void handleUnregisteredServerEnd(); int reforward(); bool reforwardableStatus(const Http::StatusCode s) const; - void serverClosed(int fd); + void serverClosed(); void connectStart(); void connectDone(const Comm::ConnectionPointer & conn, Comm::Flag status, int xerrno); bool checkRetry(); diff --git a/src/adaptation/icap/Xaction.cc b/src/adaptation/icap/Xaction.cc index fb4bba7282..77360fb88d 100644 --- a/src/adaptation/icap/Xaction.cc +++ b/src/adaptation/icap/Xaction.cc @@ -367,6 +367,10 @@ void Adaptation::Icap::Xaction::noteCommTimedout(const CommTimeoutCbParams &) // unexpected connection close while talking to the ICAP service void Adaptation::Icap::Xaction::noteCommClosed(const CommCloseCbParams &) { + if (connection) { + connection->noteClosure(); + connection = nullptr; + } closer = NULL; detailError(ERR_DETAIL_ICAP_XACT_CLOSE); mustStop("ICAP service connection externally closed"); diff --git a/src/client_side.cc b/src/client_side.cc index 4aed1a53e0..bf2a0bfb28 100644 --- a/src/client_side.cc +++ b/src/client_side.cc @@ -502,6 +502,10 @@ httpRequestFree(void *data) /* This is a handler normally called by comm_close() */ void ConnStateData::connStateClosed(const CommCloseCbParams &) { + if (clientConnection) { + clientConnection->noteClosure(); + // keep closed clientConnection for logging, clientdb cleanup, etc. + } deleteThis("ConnStateData::connStateClosed"); } diff --git a/src/clients/FtpClient.cc b/src/clients/FtpClient.cc index 9830af8cb9..c277e73fe5 100644 --- a/src/clients/FtpClient.cc +++ b/src/clients/FtpClient.cc @@ -790,10 +790,11 @@ void Ftp::Client::dataClosed(const CommCloseCbParams &) { debugs(9, 4, status()); + if (data.conn) + data.conn->noteClosure(); if (data.listenConn != NULL) { data.listenConn->close(); data.listenConn = NULL; - // NP clear() does the: data.fd = -1; } data.clear(); } @@ -858,6 +859,8 @@ void Ftp::Client::ctrlClosed(const CommCloseCbParams &) { debugs(9, 4, status()); + if (ctrl.conn) + ctrl.conn->noteClosure(); ctrl.clear(); doneWithFwd = "ctrlClosed()"; // assume FwdState is monitoring too mustStop("Ftp::Client::ctrlClosed"); diff --git a/src/clients/HttpTunneler.cc b/src/clients/HttpTunneler.cc index 3c99987000..dd8a4ba947 100644 --- a/src/clients/HttpTunneler.cc +++ b/src/clients/HttpTunneler.cc @@ -100,6 +100,11 @@ Http::Tunneler::start() void Http::Tunneler::handleConnectionClosure(const CommCloseCbParams ¶ms) { + if (connection) { + connection->noteClosure(); + // TODO: Properly get rid of connection here instead of keeping a closed + // connection object for peerConnectFailed(),noteUses() in bailWith(). + } closer = nullptr; bailWith(new ErrorState(ERR_CONNECT_FAIL, Http::scBadGateway, request.getRaw(), al)); } @@ -366,10 +371,15 @@ Http::Tunneler::bailWith(ErrorState *error) callBack(); disconnect(); - if (noteFwdPconnUse) - fwdPconnPool->noteUses(fd_table[connection->fd].pconn.uses); - // TODO: Reuse to-peer connections after a CONNECT error response. - connection->close(); + // TODO: Close before callBack(); do not pretend to send an open connection. + if (Comm::IsConnOpen(connection)) { + if (noteFwdPconnUse) + fwdPconnPool->noteUses(fd_table[connection->fd].pconn.uses); + // TODO: Reuse to-peer connections after a CONNECT error response. + + assert(!closer); // the above disconnect() removes it + connection->close(); + } connection = nullptr; } @@ -390,12 +400,13 @@ Http::Tunneler::disconnect() } if (reader) { - Comm::ReadCancel(connection->fd, reader); + if (Comm::IsConnOpen(connection)) + Comm::ReadCancel(connection->fd, reader); reader = nullptr; } - // remove connection timeout handler - commUnsetConnTimeout(connection); + if (Comm::IsConnOpen(connection)) + commUnsetConnTimeout(connection); } void @@ -415,7 +426,7 @@ Http::Tunneler::swanSong() AsyncJob::swanSong(); if (callback) { - if (requestWritten && tunnelEstablished) { + if (requestWritten && tunnelEstablished && Comm::IsConnOpen(connection)) { sendSuccess(); } else { // job-ending emergencies like handleStopRequest() or callException() diff --git a/src/comm.cc b/src/comm.cc index e49268ddae..89aa25ab43 100644 --- a/src/comm.cc +++ b/src/comm.cc @@ -743,6 +743,10 @@ commCallCloseHandlers(int fd) // If call is not canceled schedule it for execution else ignore it if (!call->canceled()) { debugs(5, 5, "commCallCloseHandlers: ch->handler=" << call); + // XXX: Without the following code, callback fd may be -1. + // typedef CommCloseCbParams Params; + // auto ¶ms = GetCommParams(call); + // params.fd = fd; ScheduleCallHere(call); } } @@ -1787,6 +1791,10 @@ DeferredReadManager::CloseHandler(const CommCloseCbParams ¶ms) CbDataList *temp = (CbDataList *)params.data; temp->element.closer = NULL; + if (temp->element.theRead.conn) { + temp->element.theRead.conn->noteClosure(); + temp->element.theRead.conn = nullptr; + } temp->element.markCancelled(); } @@ -1860,6 +1868,11 @@ DeferredReadManager::kickARead(DeferredRead const &aRead) if (aRead.cancelled) return; + // TODO: This check still allows theReader call with a closed theRead.conn. + // If a delayRead() caller has a close connection handler, then such a call + // would be useless and dangerous. If a delayRead() caller does not have it, + // then the caller will get stuck when an external connection closure makes + // aRead.cancelled (checked above) true. if (Comm::IsConnOpen(aRead.theRead.conn) && fd_table[aRead.theRead.conn->fd].closing()) return; diff --git a/src/comm/TcpAcceptor.cc b/src/comm/TcpAcceptor.cc index 371d6cb06c..eb35c97fd4 100644 --- a/src/comm/TcpAcceptor.cc +++ b/src/comm/TcpAcceptor.cc @@ -206,7 +206,10 @@ void Comm::TcpAcceptor::handleClosure(const CommCloseCbParams &) { closer_ = NULL; - conn = NULL; + if (conn) { + conn->noteClosure(); + conn = nullptr; + } Must(done()); } diff --git a/src/dns_internal.cc b/src/dns_internal.cc index d18c4bc771..ef3355d91a 100644 --- a/src/dns_internal.cc +++ b/src/dns_internal.cc @@ -872,6 +872,10 @@ static void idnsVCClosed(const CommCloseCbParams ¶ms) { nsvc * vc = (nsvc *)params.data; + if (vc->conn) { + vc->conn->noteClosure(); + vc->conn = nullptr; + } delete vc; } diff --git a/src/gopher.cc b/src/gopher.cc index 6f05039449..041fff6d18 100644 --- a/src/gopher.cc +++ b/src/gopher.cc @@ -145,6 +145,8 @@ static void gopherStateFree(const CommCloseCbParams ¶ms) { GopherStateData *gopherState = (GopherStateData *)params.data; + // Assume that FwdState is monitoring and calls noteClosure(). See XXX about + // Connection sharing with FwdState in gopherStart(). delete gopherState; } @@ -945,6 +947,7 @@ gopherStart(FwdState * fwd) return; } + // XXX: Sharing open Connection with FwdState that has its own handlers/etc. gopherState->serverConn = fwd->serverConnection(); gopherSendRequest(fwd->serverConnection()->fd, gopherState); AsyncCall::Pointer timeoutCall = commCbCall(5, 4, "gopherTimeout", diff --git a/src/ident/Ident.cc b/src/ident/Ident.cc index ffc2f8526b..3b52f76530 100644 --- a/src/ident/Ident.cc +++ b/src/ident/Ident.cc @@ -125,7 +125,10 @@ void Ident::Close(const CommCloseCbParams ¶ms) { IdentStateData *state = (IdentStateData *)params.data; - // XXX: A Connection closure handler must update its Connection object. + if (state->conn) { + state->conn->noteClosure(); + state->conn = nullptr; + } state->deleteThis("connection closed"); } diff --git a/src/log/TcpLogger.cc b/src/log/TcpLogger.cc index c16f6d42a6..c046689a15 100644 --- a/src/log/TcpLogger.cc +++ b/src/log/TcpLogger.cc @@ -370,7 +370,10 @@ Log::TcpLogger::handleClosure(const CommCloseCbParams &) { assert(inCall != NULL); closer = NULL; - conn = NULL; + if (conn) { + conn->noteClosure(); + conn = nullptr; + } // in all current use cases, we should not try to reconnect mustStop("Log::TcpLogger::handleClosure"); } diff --git a/src/mgr/Forwarder.cc b/src/mgr/Forwarder.cc index 0486d01ce6..fa17cd6e0e 100644 --- a/src/mgr/Forwarder.cc +++ b/src/mgr/Forwarder.cc @@ -97,7 +97,11 @@ void Mgr::Forwarder::noteCommClosed(const CommCloseCbParams &) { debugs(16, 5, HERE); - conn = NULL; // needed? + closer = nullptr; + if (conn) { + conn->noteClosure(); + conn = nullptr; + } mustStop("commClosed"); } diff --git a/src/mgr/Inquirer.cc b/src/mgr/Inquirer.cc index d38bf3a30b..b6294bcd68 100644 --- a/src/mgr/Inquirer.cc +++ b/src/mgr/Inquirer.cc @@ -110,8 +110,11 @@ void Mgr::Inquirer::noteCommClosed(const CommCloseCbParams& params) { debugs(16, 5, HERE); - Must(!Comm::IsConnOpen(conn) && params.conn.getRaw() == conn.getRaw()); - conn = NULL; + closer = nullptr; + if (conn) { + conn->noteClosure(); + conn = nullptr; + } mustStop("commClosed"); } diff --git a/src/mgr/StoreToCommWriter.cc b/src/mgr/StoreToCommWriter.cc index 251440eb92..7d5bd6e9aa 100644 --- a/src/mgr/StoreToCommWriter.cc +++ b/src/mgr/StoreToCommWriter.cc @@ -131,7 +131,11 @@ void Mgr::StoreToCommWriter::noteCommClosed(const CommCloseCbParams &) { debugs(16, 6, HERE); - Must(!Comm::IsConnOpen(clientConnection)); + if (clientConnection) { + clientConnection->noteClosure(); + clientConnection = nullptr; + } + closer = nullptr; mustStop("commClosed"); } diff --git a/src/security/PeerConnector.cc b/src/security/PeerConnector.cc index 0f572f9bf0..d28169f4df 100644 --- a/src/security/PeerConnector.cc +++ b/src/security/PeerConnector.cc @@ -88,9 +88,17 @@ Security::PeerConnector::start() void Security::PeerConnector::commCloseHandler(const CommCloseCbParams ¶ms) { + debugs(83, 5, "FD " << params.fd << ", Security::PeerConnector=" << params.data); + closeHandler = nullptr; + if (serverConn) { + // TODO: Calling PconnPool::noteUses() should not be our responsibility. + if (noteFwdPconnUse && serverConn->isOpen()) + fwdPconnPool->noteUses(fd_table[serverConn->fd].pconn.uses); + serverConn->noteClosure(); + serverConn = nullptr; + } - debugs(83, 5, "FD " << params.fd << ", Security::PeerConnector=" << params.data); const auto err = new ErrorState(ERR_SECURE_CONNECT_FAIL, Http::scServiceUnavailable, request.getRaw(), al); #if USE_OPENSSL err->detail = new Ssl::ErrorDetail(SQUID_ERR_SSL_HANDSHAKE, nullptr, nullptr); @@ -112,6 +120,8 @@ Security::PeerConnector::commTimeoutHandler(const CommTimeoutCbParams &) bool Security::PeerConnector::initialize(Security::SessionPointer &serverSession) { + Must(Comm::IsConnOpen(serverConnection())); + Security::ContextPointer ctx(getTlsContext()); debugs(83, 5, serverConnection() << ", ctx=" << (void*)ctx.get()); @@ -153,6 +163,8 @@ Security::PeerConnector::initialize(Security::SessionPointer &serverSession) void Security::PeerConnector::recordNegotiationDetails() { + Must(Comm::IsConnOpen(serverConnection())); + const int fd = serverConnection()->fd; Security::SessionPointer session(fd_table[fd].ssl); @@ -171,6 +183,8 @@ Security::PeerConnector::recordNegotiationDetails() void Security::PeerConnector::negotiate() { + Must(Comm::IsConnOpen(serverConnection())); + const int fd = serverConnection()->fd; if (fd_table[fd].closing()) return; @@ -217,6 +231,7 @@ Security::PeerConnector::sslFinalized() { #if USE_OPENSSL if (Ssl::TheConfig.ssl_crt_validator && useCertValidator_) { + Must(Comm::IsConnOpen(serverConnection())); const int fd = serverConnection()->fd; Security::SessionPointer session(fd_table[fd].ssl); @@ -260,6 +275,7 @@ void Security::PeerConnector::sslCrtvdHandleReply(Ssl::CertValidationResponse::Pointer validationResponse) { Must(validationResponse != NULL); + Must(Comm::IsConnOpen(serverConnection())); Ssl::ErrorDetail *errDetails = NULL; bool validatorFailed = false; @@ -308,6 +324,8 @@ Security::PeerConnector::sslCrtvdHandleReply(Ssl::CertValidationResponse::Pointe Security::CertErrors * Security::PeerConnector::sslCrtvdCheckForErrors(Ssl::CertValidationResponse const &resp, Ssl::ErrorDetail *& errDetails) { + Must(Comm::IsConnOpen(serverConnection())); + ACLFilledChecklist *check = NULL; Security::SessionPointer session(fd_table[serverConnection()->fd].ssl); @@ -383,6 +401,8 @@ Security::PeerConnector::negotiateSsl() void Security::PeerConnector::handleNegotiateError(const int ret) { + Must(Comm::IsConnOpen(serverConnection())); + const int fd = serverConnection()->fd; const Security::SessionPointer session(fd_table[fd].ssl); unsigned long ssl_lib_error = ret; @@ -447,8 +467,10 @@ Security::PeerConnector::handleNegotiateError(const int ret) void Security::PeerConnector::noteWantRead() { - const int fd = serverConnection()->fd; debugs(83, 5, serverConnection()); + + Must(Comm::IsConnOpen(serverConnection())); + const int fd = serverConnection()->fd; #if USE_OPENSSL Security::SessionPointer session(fd_table[fd].ssl); BIO *b = SSL_get_rbio(session.get()); @@ -485,8 +507,10 @@ Security::PeerConnector::noteWantRead() void Security::PeerConnector::noteWantWrite() { - const int fd = serverConnection()->fd; debugs(83, 5, serverConnection()); + Must(Comm::IsConnOpen(serverConnection())); + + const int fd = serverConnection()->fd; Comm::SetSelect(fd, COMM_SELECT_WRITE, &NegotiateSsl, new Pointer(this), 0); return; } @@ -507,6 +531,7 @@ Security::PeerConnector::noteNegotiationError(const int ret, const int ssl_error #endif int xerr = errno; + Must(Comm::IsConnOpen(serverConnection())); const int fd = serverConnection()->fd; debugs(83, DBG_IMPORTANT, "ERROR: negotiating TLS on FD " << fd << ": " << Security::ErrorString(ssl_lib_error) << " (" << @@ -547,15 +572,21 @@ Security::PeerConnector::bail(ErrorState *error) Must(dialer); dialer->answer().error = error; - if (const auto p = serverConnection()->getPeer()) - peerConnectFailed(p); + if (serverConnection()) { + if (const auto p = serverConnection()->getPeer()) + peerConnectFailed(p); + } callBack(); disconnect(); - if (noteFwdPconnUse) - fwdPconnPool->noteUses(fd_table[serverConn->fd].pconn.uses); - serverConn->close(); + // TODO: Close before callBack(); do not pretend to send an open connection. + if (Comm::IsConnOpen(serverConn)) { + if (noteFwdPconnUse) + fwdPconnPool->noteUses(fd_table[serverConn->fd].pconn.uses); + assert(!closeHandler); // the above disconnect() removes it + serverConn->close(); + } serverConn = nullptr; } @@ -569,6 +600,9 @@ Security::PeerConnector::sendSuccess() void Security::PeerConnector::disconnect() { + if (!Comm::IsConnOpen(serverConnection())) + return; + if (closeHandler) { comm_remove_close_handler(serverConnection()->fd, closeHandler); closeHandler = nullptr; @@ -588,7 +622,7 @@ Security::PeerConnector::callBack() callback = NULL; // this should make done() true CbDialer *dialer = dynamic_cast(cb->getDialer()); Must(dialer); - dialer->answer().conn = serverConnection(); + dialer->answer().conn = serverConnection(); // may be nil ScheduleCallHere(cb); } @@ -620,7 +654,7 @@ Security::PeerConnector::status() const buf.append("Stopped, reason:", 16); buf.appendf("%s",stopReason); } - if (serverConn != NULL) + if (Comm::IsConnOpen(serverConn)) buf.appendf(" FD %d", serverConn->fd); buf.appendf(" %s%u]", id.prefix(), id.value); buf.terminate(); @@ -667,6 +701,7 @@ Security::PeerConnector::certDownloadingDone(SBuf &obj, int downloadStatus) debugs(81, 5, "Certificate downloading status: " << downloadStatus << " certificate size: " << obj.length()); // get ServerBio from SSL object + Must(Comm::IsConnOpen(serverConnection())); const int fd = serverConnection()->fd; Security::SessionPointer session(fd_table[fd].ssl); BIO *b = SSL_get_rbio(session.get()); @@ -714,6 +749,7 @@ Security::PeerConnector::checkForMissingCertificates() if (csd && csd->nestedLevel() >= MaxNestedDownloads) return false; + Must(Comm::IsConnOpen(serverConnection())); const int fd = serverConnection()->fd; Security::SessionPointer session(fd_table[fd].ssl); BIO *b = SSL_get_rbio(session.get()); diff --git a/src/snmp/Forwarder.cc b/src/snmp/Forwarder.cc index 51b2932272..961818f0ef 100644 --- a/src/snmp/Forwarder.cc +++ b/src/snmp/Forwarder.cc @@ -53,6 +53,7 @@ Snmp::Forwarder::noteCommClosed(const CommCloseCbParams& params) { debugs(49, 5, HERE); Must(fd == params.fd); + closer = nullptr; fd = -1; mustStop("commClosed"); } @@ -68,8 +69,7 @@ void Snmp::Forwarder::handleException(const std::exception& e) { debugs(49, 3, HERE << e.what()); - if (fd >= 0) - sendError(SNMP_ERR_GENERR); + sendError(SNMP_ERR_GENERR); Ipc::Forwarder::handleException(e); } @@ -78,6 +78,10 @@ void Snmp::Forwarder::sendError(int error) { debugs(49, 3, HERE); + + if (fd < 0) + return; // client gone + Snmp::Request& req = static_cast(*request); req.pdu.command = SNMP_PDU_RESPONSE; req.pdu.errstat = error; diff --git a/src/snmp/Inquirer.cc b/src/snmp/Inquirer.cc index f69388ed39..589e527c0a 100644 --- a/src/snmp/Inquirer.cc +++ b/src/snmp/Inquirer.cc @@ -88,7 +88,11 @@ Snmp::Inquirer::noteCommClosed(const CommCloseCbParams& params) { debugs(49, 5, HERE); Must(!Comm::IsConnOpen(conn) || conn->fd == params.conn->fd); - conn = NULL; + closer = nullptr; + if (conn) { + conn->noteClosure(); + conn = nullptr; + } mustStop("commClosed"); } @@ -102,6 +106,10 @@ void Snmp::Inquirer::sendResponse() { debugs(49, 5, HERE); + + if (!Comm::IsConnOpen(conn)) + return; // client gone + aggrPdu.fixAggregate(); aggrPdu.command = SNMP_PDU_RESPONSE; u_char buffer[SNMP_REQUEST_SIZE]; diff --git a/src/whois.cc b/src/whois.cc index a1a3beb7b1..fddb338945 100644 --- a/src/whois.cc +++ b/src/whois.cc @@ -169,6 +169,7 @@ whoisClose(const CommCloseCbParams ¶ms) { WhoisState *p = (WhoisState *)params.data; debugs(75, 3, "whoisClose: FD " << params.fd); + // We do not own a Connection. Assume that FwdState is also monitoring. p->entry->unlock("whoisClose"); delete p; } -- 2.47.2