]> git.ipfire.org Git - thirdparty/squid.git/commit
Bug 5090: Must(!request->pinnedConnection()) violation (#930)
authorAlex Rousskov <rousskov@measurement-factory.com>
Thu, 11 Nov 2021 09:17:54 +0000 (09:17 +0000)
committerSquid Anubis <squid-anubis@squid-cache.org>
Thu, 11 Nov 2021 20:10:18 +0000 (20:10 +0000)
commit15bde30c33e47a72650ef17766719a5fc7abee4c
treed9400f5ba94d43eb74143e29ee6a9d9bd5fea4fa
parent2081cefa236c16b5185a4e8009c218fef4e7e3af
Bug 5090: Must(!request->pinnedConnection()) violation (#930)

The bug may be asymptomatic. Visible bug symptoms, if any, may include:

    FATAL: check failed: !request->pinnedConnection()
    exception location: FwdState.cc(1124) connectStart

    FATAL: check failed: transportWait
    exception location: FwdState.cc(675) noteDestinationsEnd

FwdState::usingDestination() should cover 3 post-transportWait periods:

1. peerWait: opening a CONNECT tunnel through the cache_peer
2. encryptionWait: TLS negotiations to secure the connection
3. Comm::IsConnOpen(serverConn): a Squid-peer transaction

The condition for the last period did not account for the time between
FwdState::unregister() and FwdState::complete() (or their equivalents),
when the transport connection is either closed or moved to the pconn
pool, but FwdState is still waiting for complete() and must not attempt
to connect to another destination. The bug is usually hidden because
complete() is usually called immediately after unregister(). However,
RESPMOD adaptation (at least) may delay complete(). If peer selection
news comes during that delay, usingDestination() lies, and various
Must()s may fail, depending on Squid version and HTTP request details.

Now, FwdState does not rely on the to-peer connection state for the
third period condition. Instead, we explicitly track whether the last
dispatch()ed activity has ended. This tracking is tricky because
complete() may be called without dispatching an asynchronous activity,
and because there are at least three ways for an activity to end:
explicit complete(), explicit handleUnregisteredServerEnd(), and
implicit serverClosed() connection closure callback. This tracking will
become easier and more reliable when FwdState no longer co-owns/stores
the to-peer connection. This change simplifies the future removal of
that connection ownership.

Also reordered usingDestination() conditions to match the chronological
order of the corresponding three periods.

Also reordered transportWait-vs-usingDestination() checks to match the
chronological order of those two forwarding stages.
src/FwdState.cc
src/FwdState.h