I am attaching a new patch for trunk which renames the noteClsure() to
noteClosureXXX().
If it is OK, I will post the squid-3.5 patch to.
Regards,
Christos
On 05/26/2015 10:59 AM, Amos Jeffries wrote:
On 26/05/2015 12:43 a.m., Tsantilas Christos wrote:
On 05/25/2015 02:37 PM, Amos Jeffries wrote:
On 25/05/2015 10:13 p.m., Tsantilas Christos wrote:
Hi all,
I am attaching new squid patches for bug3329.
+1 on the conversion of comm_close() to X-close()
However please name the noteClsure() as noteClosureXXX() to highlight
that this function is undesirable and we need to fix the underlying
problem still for the places which find themselves having to use it
The suggestion of this patch is to start use the noteClosure method to
all comm_close handlers to avoid such problems.
The problem is not appears only when the comm_close method on fds is
used. We may face the same problem when a timeout is expired but not a
timeout-handler is installed for the fd. In this case squid code will
just close the fd. Still some Comm::Connection objects may use the fd,
but not informed about the fd closure.
A better long term way would be the comm FD opening code to install the
first close callback that hold reference to the Comm::Connection and
sets the FD to -1 when its closed without close().
We cant do that until those abandoned and orphan connections are all
figured out though. So we dont end up with that close handler being a
lingering reference-count that holds all the state alive.
So I agree this method is needed right now, but only as a temporary fix.
Amos
___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev
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.
=== modified file 'src/client_side.cc'
--- src/client_side.cc 2015-05-19 07:51:31 +
+++ src/client_side.cc 2015-05-27 14:19:17 +
@@ -3607,41 +3607,41 @@
( ssl_error / ret ));
return -1;
}
/* NOTREACHED */
}
return 1;
}
/** negotiate an SSL connection */
static void
clientNegotiateSSL(int fd, void *data)
{
ConnStateData *conn = (ConnStateData *)data;
X509 *client_cert;
SSL *ssl
