From: Alex Rousskov Date: Thu, 5 Aug 2021 21:56:12 +0000 (-0400) Subject: Revised Connection cloning during ConnOpener creation X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=5b1d7d03a39e65468d415a4875489109d7944995;p=thirdparty%2Fsquid.git Revised Connection cloning during ConnOpener creation Item 1 in recent branch commit 21f1082 correctly identified the Connection sharing problem, but its solution (i.e. let ConnOpener clone the passed Connection) was not only somewhat expensive but buggy: ConnOpener assumed that cloneDestinationDetails() is the right cloning method, but that method missed Connection properties like tos and nfmark (at least). Another method -- cloneIdentDetails() would copy those, but calling that method in non-IDENT contexts would look weird, and even that method could have missed something (now or in the future). The core problem here was not in the correct cloning method selection, but that ConnOpener is not in a position to select at all! ConnOpener should be using/forwarding all the provided Connection details, not guessing which ones are relevant to the _caller_. After that became clear, we had two choices: A. ConnOpener clones everything. This is a simple, safe choice. However, it means double-cloning: First, the caller would need to clone to copy just the right set of Connection members (or effectively clear the irrelevant ones). And then ConnOpener would clone again to break the Connection ownership link with poorly implemented callers. In some cases, that cloning was already following ResolvedPeers cloning, resulting in triple cloning of each used destination on the common forwarding path! Such cloning is both expensive and complicates triage by creating long chains of Connection IDs. B. ConnOpener clones nothing; ConnOpener creator is responsible for supplying ConnOpener with a non-owned Connection object (that does not need to be cloned by ConnOpener). This is a clean, efficient, forward-looking solution but it is a bit less safe because the compiler will not identify buggy creators that share a Connection object they own with ConnOpener. After weighing all these impossible-to-compare factors, we went with B. Implementing B required adjusting several callers that were still passing the Connections they owned to ConnOpener. Also, after comparing the needs of existing and newly added ("clone everything") Connection cloning method callers, we decided there is no need to maintain three different methods. All existing callers should be fine with a single method because none of them suffers from "extra" copying of members that others need. Right now, the new cloneProfile() method copies everything except FD and a few special-purpose members (with documented reasons for not copying). --- diff --git a/src/HappyConnOpener.cc b/src/HappyConnOpener.cc index 2ddf3f0786..94eef88104 100644 --- a/src/HappyConnOpener.cc +++ b/src/HappyConnOpener.cc @@ -565,21 +565,20 @@ HappyConnOpener::openFreshConnection(Attempt &attempt, PeerConnectionPointer &de entry->mem_obj->checkUrlChecksum(); #endif - GetMarkingsToServer(cause.getRaw(), *dest); + const auto conn = dest->cloneProfile(); + GetMarkingsToServer(cause.getRaw(), *conn); - // ConnOpener modifies its destination argument so we reset the source port - // in case we are reusing the destination already used by our predecessor. - dest->local.port(0); ++n_tries; typedef CommCbMemFunT Dialer; AsyncCall::Pointer callConnect = asyncCall(48, 5, attempt.callbackMethodName, Dialer(this, attempt.callbackMethod)); const time_t connTimeout = dest->connectTimeout(fwdStart); - Comm::ConnOpener *cs = new Comm::ConnOpener(dest, callConnect, connTimeout); - if (!dest->getPeer()) + Comm::ConnOpener *cs = new Comm::ConnOpener(conn, callConnect, connTimeout); + if (!conn->getPeer()) cs->setHost(host_); + attempt.path = dest; // but not the being-opened conn! attempt.connWait.start(cs, callConnect); AsyncJob::Start(cs); diff --git a/src/ResolvedPeers.cc b/src/ResolvedPeers.cc index a7accc36e5..8079ff3b75 100644 --- a/src/ResolvedPeers.cc +++ b/src/ResolvedPeers.cc @@ -151,7 +151,7 @@ ResolvedPeers::extractFound(const char *description, const Paths::iterator &foun while (++pathsToSkip < paths_.size() && !paths_[pathsToSkip].available) {} } - const auto cleanPath = path.connection->cloneDestinationDetails(); + const auto cleanPath = path.connection->cloneProfile(); return PeerConnectionPointer(cleanPath, found - paths_.begin()); } diff --git a/src/comm/ConnOpener.cc b/src/comm/ConnOpener.cc index 6c7032b6eb..58c4c9db6b 100644 --- a/src/comm/ConnOpener.cc +++ b/src/comm/ConnOpener.cc @@ -34,13 +34,21 @@ Comm::ConnOpener::ConnOpener(const Comm::ConnectionPointer &c, const AsyncCall:: AsyncJob("Comm::ConnOpener"), host_(NULL), temporaryFd_(-1), - conn_(c->cloneDestinationDetails()), + conn_(c), callback_(handler), totalTries_(0), failRetries_(0), deadline_(squid_curtime + static_cast(ctimeout)) { debugs(5, 3, "will connect to " << c << " with " << ctimeout << " timeout"); + assert(conn_); // we know where to go + + // Sharing a being-modified Connection object with the caller is dangerous, + // but we cannot ban (or even check for) that using existing APIs. We do not + // want to clone "just in case" because cloning is a bit expensive, and most + // callers already have a non-owned Connection object to give us. Until the + // APIs improve, we can only check that the connection is not open. + assert(!conn_->isOpen()); } Comm::ConnOpener::~ConnOpener() diff --git a/src/comm/Connection.cc b/src/comm/Connection.cc index 72e51b84d7..7e2cad2b30 100644 --- a/src/comm/Connection.cc +++ b/src/comm/Connection.cc @@ -61,27 +61,45 @@ Comm::Connection::~Connection() } Comm::ConnectionPointer -Comm::Connection::cloneDestinationDetails() const +Comm::Connection::cloneProfile() const { - const ConnectionPointer c = new Comm::Connection; - c->setAddrs(local, remote); - c->peerType = peerType; - c->flags = flags; - c->peer_ = cbdataReference(getPeer()); - debugs(5, 5, this << " made " << c); - assert(!c->isOpen()); - return c; -} + const ConnectionPointer clone = new Comm::Connection; + auto &c = *clone; // optimization + + /* + * Copy or excuse each data member. Excused members do not belong to a + * Connection configuration profile because their values cannot be reused + * across (co-existing) Connection objects and/or are tied to their own + * object lifetime. + */ + + c.setAddrs(local, remote); + c.peerType = peerType; + // fd excused + c.tos = tos; + c.nfmark = nfmark; + c.nfConnmark = nfConnmark; + // COMM_ORPHANED is not a part of connection opening instructions + c.flags = flags & ~COMM_ORPHANED; + if (*rfc931) // optimization + memcpy(c.rfc931, rfc931, sizeof(rfc931)); + +#if USE_SQUID_EUI + // These are currently only set when accepting connections and never used + // for establishing new ones, so this copying is currently in vain, but, + // technically, they can be a part of connection opening instructions. + c.remoteEui48 = remoteEui48; + c.remoteEui64 = remoteEui64; +#endif -Comm::ConnectionPointer -Comm::Connection::cloneIdentDetails() const -{ - auto c = cloneDestinationDetails(); - c->tos = tos; - c->nfmark = nfmark; - c->nfConnmark = nfConnmark; - c->startTime_ = startTime_; - return c; + // id excused + c.peer_ = cbdataReference(getPeer()); + // startTime_ excused + // tlsHistory excused + + debugs(5, 5, this << " made " << c); + assert(!c.isOpen()); + return clone; } void diff --git a/src/comm/Connection.h b/src/comm/Connection.h index 202358b2c7..02ecb04698 100644 --- a/src/comm/Connection.h +++ b/src/comm/Connection.h @@ -77,13 +77,12 @@ public: /** Clear the connection properties and close any open socket. */ virtual ~Connection(); - /// Create a new (closed) IDENT Connection object based on our from-Squid - /// connection properties. - ConnectionPointer cloneIdentDetails() const; + /// To prevent accidental copying of Connection objects that we started to + /// open or that are open, use cloneProfile() instead. + Connection(const Connection &&) = delete; - /// Create a new (closed) Connection object pointing to the same destination - /// as this from-Squid connection. - ConnectionPointer cloneDestinationDetails() const; + /// Create a new closed Connection with the same configuration as this one. + ConnectionPointer cloneProfile() const; /// close the still-open connection when its last reference is gone void enterOrphanage() { flags |= COMM_ORPHANED; } @@ -140,17 +139,6 @@ public: virtual ScopedId codeContextGist() const override; virtual std::ostream &detailCodeContext(std::ostream &os) const override; -private: - /** These objects may not be exactly duplicated. Use cloneIdentDetails() or - * cloneDestinationDetails() instead. - */ - Connection(const Connection &c); - - /** These objects may not be exactly duplicated. Use cloneIdentDetails() or - * cloneDestinationDetails() instead. - */ - Connection & operator =(const Connection &c); - public: /** Address/Port for the Squid end of a TCP link. */ Ip::Address local; diff --git a/src/ident/Ident.cc b/src/ident/Ident.cc index df886dd705..5f9e574ef5 100644 --- a/src/ident/Ident.cc +++ b/src/ident/Ident.cc @@ -124,6 +124,7 @@ void Ident::Close(const CommCloseCbParams ¶ms) { IdentStateData *state = (IdentStateData *)params.data; + // XXX: A Connection closure handler must update its Connection object. state->deleteThis("connection closed"); } @@ -141,14 +142,13 @@ Ident::ConnectDone(const Comm::ConnectionPointer &conn, Comm::Flag status, int, IdentStateData *state = (IdentStateData *)data; state->connWait.finish(); - // Finalize the details and start owning the supplied connection (so that it - // is not orphaned if this function bails early). As a (tiny) optimization - // or perhaps just diff minimization, the close handler is added later, when - // we know we are not bailing. This delay is safe because - // comm_remove_close_handler() forgives missing handlers. + // Start owning the supplied connection (so that it is not orphaned if this + // function bails early). As a (tiny) optimization or perhaps just diff + // minimization, the close handler is added later, when we know we are not + // bailing. This delay is safe because comm_remove_close_handler() forgives + // missing handlers. assert(conn); // but may be closed - assert(state->conn); - assert(!state->conn->isOpen()); + assert(!state->conn); state->conn = conn; if (status != Comm::OK) { @@ -282,10 +282,10 @@ Ident::Start(const Comm::ConnectionPointer &conn, IDCB * callback, void *data) state->hash.key = xstrdup(key); // copy the conn details. We do not want the original FD to be re-used by IDENT. - state->conn = conn->cloneIdentDetails(); + const auto identConn = conn->cloneProfile(); // NP: use random port for secure outbound to IDENT_PORT - state->conn->local.port(0); - state->conn->remote.port(IDENT_PORT); + identConn->local.port(0); + identConn->remote.port(IDENT_PORT); // build our query from the original connection details state->queryMsg.init(); @@ -295,7 +295,7 @@ Ident::Start(const Comm::ConnectionPointer &conn, IDCB * callback, void *data) hash_join(ident_hash, &state->hash); AsyncCall::Pointer call = commCbCall(30,3, "Ident::ConnectDone", CommConnectCbPtrFun(Ident::ConnectDone, state)); - const auto connOpener = new Comm::ConnOpener(state->conn, call, Ident::TheConfig.timeout); + const auto connOpener = new Comm::ConnOpener(identConn, call, Ident::TheConfig.timeout); state->connWait.start(connOpener, call); AsyncJob::Start(connOpener); } diff --git a/src/servers/FtpServer.cc b/src/servers/FtpServer.cc index 1ed6f9623b..acafb3fbbb 100644 --- a/src/servers/FtpServer.cc +++ b/src/servers/FtpServer.cc @@ -1679,7 +1679,7 @@ Ftp::Server::checkDataConnPre() // active transfer: open a data connection from Squid to client typedef CommCbMemFunT Dialer; AsyncCall::Pointer callback = JobCallback(17, 3, Dialer, this, Ftp::Server::connectedForData); - const auto cs = new Comm::ConnOpener(dataConn, callback, + const auto cs = new Comm::ConnOpener(dataConn->cloneProfile(), callback, Config.Timeout.connect); dataConnWait.start(cs, callback); AsyncJob::Start(cs); @@ -1708,6 +1708,7 @@ Ftp::Server::connectedForData(const CommConnectCbParams ¶ms) Http::StreamPointer context = pipeline.front(); Must(context->http); Must(context->http->storeEntry() != NULL); + // TODO: call closeDataConnection() to reset data conn processing? } else { // Finalize the details and start owning the supplied connection. assert(params.conn); diff --git a/src/tests/stub_libcomm.cc b/src/tests/stub_libcomm.cc index 3d4d32743e..5cfeb98978 100644 --- a/src/tests/stub_libcomm.cc +++ b/src/tests/stub_libcomm.cc @@ -22,8 +22,7 @@ void Comm::AcceptLimiter::kick() STUB #include "comm/Connection.h" Comm::Connection::Connection() STUB Comm::Connection::~Connection() STUB -Comm::ConnectionPointer Comm::Connection::cloneIdentDetails() const STUB_RETVAL(nullptr) -Comm::ConnectionPointer Comm::Connection::cloneDestinationDetails() const STUB_RETVAL(nullptr) +Comm::ConnectionPointer Comm::Connection::cloneProfile() const STUB_RETVAL(nullptr) void Comm::Connection::close() STUB void Comm::Connection::noteClosure() STUB CachePeer * Comm::Connection::getPeer() const STUB_RETVAL(NULL)