]> git.ipfire.org Git - thirdparty/squid.git/commit
Bug3329: The server side pinned connection is not closed properly
authorChristos Tsantilas <chtsanti@users.sourceforge.net>
Tue, 2 Jun 2015 10:15:06 +0000 (13:15 +0300)
committerChristos Tsantilas <chtsanti@users.sourceforge.net>
Tue, 2 Jun 2015 10:15:06 +0000 (13:15 +0300)
commitb54a7c5a37afd21e43519644de32525656962125
tree7917d4ff66e24539d1a73d4f25ef58991b77854d
parent721dcff0234670725c0d081aae71fa333d463006
Bug3329: The server side pinned connection is not closed properly
in ConnStateData::clientPinnedConnectionClosed CommClose handler.

Squid enters a buggy state when an idle connection pinned to a peer closes:

 - The ConnStateData::clientPinnedConnectionRead, the pinned peer
   connection read handler, is called with the io.flag set to
   Comm::ERR_CLOSING. The read handler does not close the peer
   Comm::Connection object. This is correct and expected -- the I/O
   handler must exit on ERR_CLOSING without doing anything.

 - The ConnStateData::clientPinnedConnectionClosed close handler is called,
   but it does not close the peer Comm::Connection object either. Again,
   this is correct and expected -- the close handler is not the place to
   close a being-closed connection.

 - The corresponding fde object is marked as closed (fde::flags.open
   is false), but the peer Comm::Connection object is still open
   (Comm::Connection.fd >= 0)! From this point on, we have an inconsistency
   between the peer Comm::Connection object state and the real world.

 - When the ConnStateData::pinning::serverConnection object is later
   destroyed (by refcounting), it will try to close its fd. If that fd
   is already in use (e.g., by another Comm::Connection), bad things
   happen (crashes, segfaults, etc). Otherwise (i.e., if that fd is
   not open), comm_close may cry about BUG 3556 (or worse).

To fix this problem, we must not allow Comm::Connections to get out
of sync with fd_table, even when a descriptor is closed without going
through Connection::close(). There are two ways to accomplished that:

 * Change Comm to always store Comm::Connections and similar high-level
   objects instead of fdes. This is a huge change that has been long on
   the TODO list (those "other high-level objects" is on of the primary
   obstacles there because not everything with a FD is a Connection).

 * Notify Comm::Connections about closure in their closing handlers
   (this change). This design relies on every Comm::Connection having
   a close handler that notifies it. It may take us some time to reach
   that goal, but this change is the first step providing the necessary
   API, a known bug fix, and a few preventive changes.

This change:

 - Adds a new Comm::Connection::noteClosure() method to inform the
   Comm::Connection object that somebody is closing its FD.

 - Uses the new method inside ConnStateData::clientPinnedConnectionClosed
   handler to inform the ConnStateData::pinning::serverConnection object
   that its FD is being closed.

 - Replaces comm_close calls which may cause bug #3329 in other places with
   Comm::Connection->close() calls.

Initially based on Nathan Hoad research for bug 3329.

This is a Measurement Factory project.
src/client_side.cc
src/comm/Connection.cc
src/comm/Connection.h
src/ssl/PeerConnector.cc