* Fix build with OpenSSL v3.
* Refactor RSA key generation to avoid deprecated RSA_*() APIs.
* Refactor DH parameter and key config to avoid deprecated DH_*() APIs.
* Refactor ECDH key creation to avoid deprecated EC_*() APIs.
* Deprecate ssl_engine support in builds with OpenSSL v1-.
* Disable ssl_engine support in builds OpenSSL v3+.
We deprecated/removed ssl_engine support (as summarized in the last two
bullets above) without providing an OpenSSL Providers-based alternative
because of the following factors:
1. We do not have the resources to update ssl_engine code to build
(without deprecation warnings) with OpenSSL v3 when the feature is
unused.
2. We do not have the resources to create an OpenSSL v3 Provider-based
replacement for ssl_engine code that uses deprecated Engine APIs.
3. OpenSSL v3 deprecated Engine support (triggering deprecation warnings
in applications that use Engine APIs with OpenSSL v3). Since Squid
default builds use -Werror, doing nothing would break such builds.
4. Squid ssl_engine does not appear to be a popular feature.
Alex Rousskov [Fri, 12 Aug 2022 16:06:12 +0000 (16:06 +0000)]
Do not tell admins that we are replacing their .* with our .* (#1119)
acl hasHeaderFoo req_header Foo .*
2022/08/12 11:43:27| WARNING: regular expression '.*' has only
wildcards and matches all strings. Using '.*' instead.
Broken since RE optimization inception (commit 6564dae), but the
excessive WARNING was "invisible" to many admins until 61be1d8 started
saving early level-0/1 messages to cache.log.
Alex Rousskov [Tue, 19 Jul 2022 21:25:32 +0000 (21:25 +0000)]
Discarded connections do not contribute to forward_max_tries (#1093)
The forward_max_tries directive is said to limit the number of
"high-level request forwarding attempts", but its documentation does not
specify whether each Happy Eyeballs connection opening attempt
constitutes such a forwarding attempt (because commit 3eebd26 that
clarified forward_max_tries meaning came before Happy Eyeballs code
started opening multiple concurrent connections in commit 5562295).
Official Squid counts each Happy Eyeballs connection attempt as a
request forwarding attempt. For example, if forward_max_tries is 2, then
Squid, to a likely admin surprise, may refuse to re-forward the request
after an HTTP failure because the second request forwarding attempt has
been used up by the (canceled!) spare TCP connection opening attempt.
This change stops counting discarded connections as request forwarding
attempts. For example, if forward_max_tries is 2, Squid will re-forward
the request (if needed and possible) even if Squid must open up to 3
connections to do that (discarding the second one in the process). In
this context, discarding the race-losing connection and going with the
winning one may be viewed as a low-level retry activity that
forward_max_tries is already documented to ignore.
Eventually, Squid may stop discarding connections that lost the Happy
Eyeballs race. When/if that complicated improvement is implemented,
those canceled connection opening attempts should be counted, but the
then-saved connections should be used for request re-forwarding,
preserving the same overall outcome: With forward_max_tries set to 2,
Squid will still re-forward the request (if needed and possible).
Before this change, setting forward_max_tries to 1 prevented all spare
connection attempts: The first DNS response received (A or AAAA) would
be used for the primary connection attempt, increasing n_tries, making
ranOutOfTimeOrAttempts() true, and blocking any spare attempt (both
concurrent and sequential). That low-level side effect is as wrong as
the blocked retries described in the "likely admin surprise" example
above. Now, admins that really want to disable spare connection attempts
may set forward_max_tries to 1 _and_ happy_eyeballs_connect_limit to 0.
Alex Rousskov [Sun, 3 Jul 2022 14:48:50 +0000 (14:48 +0000)]
Maintenance: Fix cf_gen build warnings with clang v12 (#1076)
cf_gen.cc:... warning: comparison of array 'buff' not equal to a
null pointer is always true [-Wtautological-pointer-compare]
These conditions are not just always true but a bit odd or misleading
because they check the pointer after it was dereferenced. They were
added in c1f8bbd which misinterpreted the "else" conditions. The two
"regular DOC/CONFIG line" conditions are self-documented well enough.
When built with Samba TrivialDB, ext_session_acl would never
successfully look a session up due to an uninitialized key argument. As
a result, the helper would be unable to validate that an user had logged
in. Broken since TrivialDB support was added in acd207a.
Detected by Coverity. CID 1441979: Uninitialized scalar variable
(UNINIT).
Use SBuf for Fs::Ufs::RebuildState file paths (#1063)
In theory, a combination of a non-terminating-on-overflows snprintf()
and an incorrectly guessed MAXPATHLEN (or a cache_dir root path being at
the edge of a correctly guessed MAXPATHLEN) may result in non-terminated
fullpath. Use SBuf to avoid these particular headaches.
Detected by Coverity. CID 1461166: Copy of overlapping memory
(OVERLAPPING_COPY).
When the "no more destinations to try" notification comes after the last
forwarding/tunneling attempt has failed, the !destinationsFound block
does not run (because destinations were found), the usingDestination()
block does not run (because we are done with that last/failed
destination), but transportWait is false (for the same reason).
Also applied Bug 5090 (master/v6 commit 15bde30) FwdState protections to
tunnel.cc code. Tunnels may continue writing to the client while the
to-server connection is closing or closed, so TunnelStateData can be
potentially exposed to bug 5090 "no server connection but still
transporting" concerns. TunnelStateData never _retries_ successfully
established tunnels and, hence, can (and now does) stop receiving spare
destinations after committedToServer becomes true, but future tunnels
may start reforwarding in many cases, and most of the code does not need
to know about this (temporary?) simplification.
Also re-unified and polished related FwdState and TunnelStateData code,
including fixing lying source code comments and debug messages.
Segmentation fault in tests/testStore is another symptom.
testStoreHashIndex calls Store::Root().init() which eventually gets to
MemStore::Requested() which correctly requires Config.memShared to be
configured. Hack the missing configuration replacement, following the
inconsistent pattern in three other test suites.
TODO: Reduce configuration code duplication and such bugs by providing
initialization method(s) for all Store::Root()-using tests to (re)use.
What does LTO have to do with any of it? CPPUnit [allegedly] randomizes
execution of test suites using raw C++ pointers, and a test suite in the
testStore collection probably contains the right initialization code.
Unfortunately, Config initialization is evidently shared across test
suites (XXX). LTO evidently changes those raw pointer values enough to
change the order in an unfortunate way.
Initialise default_keytab in negotiate_kerberos_auth (#1032)
Address a Coverity-identified issue, where default_keytab might be read
when uninitialised in negotiate_kerberos_auth.
Ensure it is initialised at declaration.
Detected by Coverity, CID 1503291 (Uninitialized scalar variable)
Alex Rousskov [Thu, 28 Apr 2022 10:37:56 +0000 (10:37 +0000)]
Bug 5208: Part 1: Restart kids killed by SIGKILL (#1035)
OOM killer uses SIGKILL. Squid did not restart kids killed by SIGKILL.
Kids are essential Squid components. Essential components should be
revived (by default) because providing a service without an essential
component would violate Squid functionality requirements.
Squid did not revive a kid killed by SIGKILL because we thought that
doing so will interfere with the "squid -k kill" feature that uses that
signal to kill the whole Squid instance. However, that feature does not
actually work[^1] -- the signal is sent to (and kills) the master
process only, the process which PID is in squid.pid file. This change is
orthogonal to fixing "squid -k kill" (a difficult out-of-scope project).
[^1]: Except in the special case of the no-daemon (squid -N) mode.
Rob Cowart [Tue, 19 Apr 2022 22:39:11 +0000 (22:39 +0000)]
Fix SQUID-MIB smilint errors (#1020)
- Reorganized cachePeerTable and related objects to fix
smilint errors which prevent importing the MIB in compilers
with more strict validation (e.g. MGsoft).
- Import of Unsigned32 from SNMPv2-SMI was removed as it was
not being used.
Kid restart leads to persistent queue overflows, delays/timeouts (#706) (#1012)
WARNING: communication with ... may be too slow or disrupted...
WARNING: abandoning ... I/Os
ERROR: worker I/O push queue for ... overflow...
ERROR: Collapsed forwarding queue overflow...
SMP queues rely on the shared memory QueueReader::popSignal flag to
reduce the number of UDS messages that queue readers and writers need to
send each other. If the flag is true but there is no corresponding "wake
up, you have new queued items to read" UDS message for the reader, the
reader may stall. This happens when the reader restarts (e.g., after
hitting an assertion) while the flag is true. A stalled queue reader
leads to delays and queue overflows:
* When the problem affects worker-disker queues, disk I/O delays under
the hard-coded 7-second timeout are not reported to the admin but may
affect user experience. Larger delays trigger level-1 WARNINGs. Push
queue overflows trigger level-1 ERRORs.
* Transient worker-worker queue problems may stall concurrent
transactions that are reading from the cache entry being written by
another process. Overflows trigger level-1 ERRORs.
The restarted worker usually starts working just fine because it does
not expect any messages. A busy restarted worker may also appear to
continue working fine because workers always pop queued items before
pushing new ones -- as long as the worker queues new items, it will see
and pop responses to earlier requests, masking the problem. However, the
"stuck popSignal" problem never goes away: Squid only clears the flag
when receiving a notification, but sending new notifications is blocked
by that stuck flag.
Upon kid start, we now clear popSignal (to reflect the actual
communication state) and empty the queue (to reduce overflows). Since
commit 4c21861, any corresponding in-flight UDS queue notification is
ignored because it was sent to the previous process playing the same kid
role. The queue writer will see the false popSignal flag and send a new
notification when queuing a new item, preventing queue stalls.
Also properly ignore stale disker responses about cache_dirs we have not
opened yet, especially since we are now trying to empty the queues ASAP
after a restart, before Coordinator has a chance to inform us about
available diskers, populating the IpcIoFiles container. We already have
similar protection from stale UDS messages and from stale disker queue
messages about _opened_ cache_dirs ("LATE disker response to...").
Nick Wellnhofer [Sun, 20 Feb 2022 19:42:40 +0000 (19:42 +0000)]
ESI: Drop incorrect and unnecessary xmlSetFeature() call (#988)
xmlSetFeature() has been deprecated for 10+ years and will eventually be
removed from libxml2. Squid calls xmlSetFeature() with the wrong
argument: a nil `value` pointer instead of a pointer to a zero value.
When called with a nil `value`, the function does nothing but returning
an error. Squid does not check whether xmlSetFeature() call is
successful, and the bug went unnoticed since libxml2 support was added
in commit 964b44c.
Since libxml2 does not substitute entities by default, the call can be
removed to achieve the intended effect.
Amos Jeffries [Sat, 29 Jan 2022 05:02:38 +0000 (05:02 +0000)]
Bug 5192: esi_parser default is incorrect (#968)
Since commit f5f7786 reversed the internal list order of ESI parser
registry, the esi_parser documentation and code comment in esi/Module.cc
have been incorrect.
Return ESI parsers to the documented default behaviour and make that
default explicit in the code selecting which Parser to initialize.
Also fixed an inverted test that resulted in the esi_parser configured
library _not_ to be the one used.
Alex Rousskov [Wed, 5 Jan 2022 18:38:07 +0000 (18:38 +0000)]
Bug 5177: clientca certificates sent to https_port clients (#955)
When sending an https_port _server_ certificate chain to the client,
Squid may send intermediate CA certificates found in clientca=... or
tls-cafile=... client certificate bundles. This "leak" of client CAs
surprises admins, may trigger traffic monitoring alarms, and might even
break https_port certificate validation in some TLS clients.
This surprising "leak" of client CAs is triggered by OpenSSL default
behavior of auto-completing server certificate chains using whatever CA
certificates happened to be in the TLS context certificate store. When
client certificate authentication is enabled, that store may contain
clientca CAs (or equivalent). OpenSSL CHANGES file acknowledges that
this aggressive default behavior can be a problem and introduces
SSL_MODE_NO_AUTO_CHAIN as a way to disable it.
This fix breaks misconfigured Squid deployments that (usually
unknowingly) rely on the OpenSSL clientca "leak" to build a complete
https_port server certificate chain sent to TLS clients. Such
deployments should add the right intermediate CA certificate(s) to their
https_port tls-cert=... bundle (or equivalent).
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.
That (void) was added in commit 7ec6d51 to "remove GNU-specific syntax",
but we cannot tell what specific problem that 10-year old change solved.
Known 3-parameter CPU_AND(3) documentation says the call returns void.
OpenBSD doesn't offer CPU_SET and CPU_ISSET. Implement their stubs as
inline functions to give the compiler proper hints about arguments (non)
use.
Update buildtest.sh to try and use relative paths first. This prevents
autoconf complaining and failing if the directory path includes
characters from an unsafe set.
This change has several overlapping parts. Unfortunately, merging
individual parts is both difficult and likely to cause crashes.
## Part 1: Bug 5055.
FwdState used to check serverConn to decide whether to open a connection
to forward the request. Since commit 25b0ce4, a nil serverConn pointer
no longer implies that a new connection should be opened: FwdState
helper jobs may already be working on preparing an existing open
connection (e.g., sending a CONNECT request or negotiating encryption).
Bad serverConn checks in both FwdState::noteDestination() and
FwdState::noteDestinationsEnd() methods led to extra connectStart()
calls creating two conflicting concurrent helper jobs.
To fix this, we replaced direct serverConn inspection with a
usingDestination() call which also checks whether we are waiting for a
helper job. Testing that fix exposed another set of bugs: The helper job
pointers or in-job connections left stale/set after forwarding failures.
The changes described below addressed (most of) those problems.
## Part 2: Connection establishing helper jobs and their callbacks
A proper fix for Bug 5055 required answering a difficult question: When
should a dying job call its callbacks? We only found one answer which
required cooperation from the job creator and led to the following
rules:
* AsyncJob destructors must not call any callbacks.
* AsyncJob::swanSong() is responsible for async-calling any remaining
(i.e. set, uncalled, and uncancelled) callbacks.
* AsyncJob::swanSong() is called (only) for started jobs.
* AsyncJob destructing sequence should validate that no callbacks remain
uncalled for started jobs.
... where an AsyncJob x is considered "started" if AsyncJob::Start(x)
has returned without throwing.
A new JobWait class helps job creators follow these rules while keeping
track on in-progress helper jobs and killing no-longer-needed helpers.
Also fixed very similar bugs in tunnel.cc code.
## Part 3: ConnOpener fixes
1. Many ConnOpener users are written to keep a ConnectionPointer to the
destination given to ConnOpener. This means that their connection
magically opens when ConnOpener successfully connects, before
ConnOpener has a chance to notify the user about the changes. Having
multiple concurrent connection owners is always dangerous, and the
user cannot even have a close handler registered for its now-open
connection. When something happens to ConnOpener or its answer, the
user job may be in trouble. Now, ConnOpener callers no longer pass
Connection objects they own, cloning them as needed. That adjustment
required adjustment 2:
2. Refactored ConnOpener users to stop assuming that the answer contains
a pointer to their connection object. After adjustment 1 above, it
does not. HappyConnOpener relied on that assumption quite a bit so we
had to refactor to use two custom callback methods instead of one
with a complicated if-statement distinguishing prime from spare
attempts. This refactoring is an overall improvement because it
simplifies the code. Other ConnOpener users just needed to remove a
few no longer valid paranoid assertions/Musts.
3. ConnOpener users were forced to remember to close params.conn when
processing negative answers. Some, naturally, forgot, triggering
warnings about orphaned connections (e.g., Ident and TcpLogger).
ConnOpener now closes its open connection before sending a negative
answer.
4. ConnOpener would trigger orphan connection warnings if the job ended
after opening the connection but without supplying the connection to
the requestor (e.g., because the requestor has gone away). Now,
ConnOpener explicitly closes its open connection if it has not been
sent to the requestor.
Also fixed Comm::ConnOpener::cleanFd() debugging that was incorrectly
saying that the method closes the temporary descriptor.
Also fixed ConnOpener callback's syncWithComm(): The stale
CommConnectCbParams override was testing unused (i.e. always negative)
CommConnectCbParams::fd and was trying to cancel the callback that most
(possibly all) recipients rely on: ConnOpener users expect a negative
answer rather than no answer at all.
Also, after comparing the needs of two old/existing and a temporary
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).
Also added Comm::Connection::cloneDestinationDetails() debugging to
simplify tracking dependencies between half-baked Connection objects
carrying destination/flags/other metadata and open Connection objects
created by ConnOpener using that metadata (which are later delivered to
ConnOpener users and, in some cases, replace those half-baked
connections mentioned earlier. Long-term, we need to find a better way
to express these and other Connection states/stages than comments and
debugging messages.
## Part 4: Comm::Connection closure callbacks
We improved many closure callbacks to make sure (to the extent possible)
that Connection and other objects are in sync with Comm. 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 they get us closer to 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).
## Part 5: Other notable improvements in the adjusted code
Improved Security::PeerConnector::serverConn and
Http::Tunneler::connection management, especially when sending negative
answers. When sending a negative answer, we would set answer().conn to
an open connection, async-send that answer, and then hurry to close the
connection using our pointer to the shared Connection object. If
everything went according to the plan, the recipient would get a non-nil
but closed Connection object. Now, a negative answer simply means no
connection at all. Same for a tunneled answer.
Refactored ICAP connection-establishing code to to delay Connection
ownership until the ICAP connection is fully ready. This change
addresses primary Connection ownership concerns (as they apply to this
ICAP code) except orphaning of the temporary Connection object by helper
job start exceptions (now an explicit XXX). For example, the transaction
no longer shares a Connection object with ConnOpener and
IcapPeerConnector jobs.
Probably fixed a bug where PeerConnector::negotiate() assumed that a
sslFinalized() does not return true after callBack(). It may (e.g., when
CertValidationHelper::Submit() throws). Same for
PeekingPeerConnector::checkForPeekAndSpliceMatched().
Fixed FwdState::advanceDestination() bug that did not save
ERR_GATEWAY_FAILURE details and "lost" the address of that failed
destination, making it unavailable to future retries (if any).
Polished PeerPoolMgr, Ident, and Gopher code to be able to fix similar
job callback and connection management issues.
Polished AsyncJob::Start() API. Start() used to return a job pointer,
but that was a bad idea:
* It implies that a failed Start() will return a nil pointer, and that
the caller should check the result. Neither is true.
* It encourages callers to dereference the returned pointer to further
adjust the job. That technically works (today) but violates the rules
of communicating with an async job. The Start() method is the boundary
after which the job is deemed asynchronous.
Also removed old "and wait for..." post-Start() comments because the
code itself became clear enough, and those comments were becoming
increasingly stale (because they duplicated the callback names above
them).
Fix Tunneler and PeerConnector handling of last-resort callback
requirements. Events like handleStopRequest() and callException() stop
the job but should not be reported as a BUG (e.g., it would be up to the
callException() to decide how to report the caught exception). There
might (or there will) be other, similar cases where the job is stopped
prematurely for some non-BUG reason beyond swanSong() knowledge. The
existence of non-bug cases does not mean there could be no bugs worth
reporting here, but until they can be identified more reliably than all
these benign/irrelevant cases, reporting no BUGs is a (much) lesser
evil.
TODO: Revise AsyncJob::doneAll(). Many of its overrides are written to
check for both positive (i.e. mission accomplished) and negative (i.e.
mission cancelled or cannot be accomplished) conditions, but the latter
is usually unnecessary, especially after we added handleStopRequest()
API to properly support external job cancellation events. Many doneAll()
overrides can probably be greatly simplified.
In git log order:
* e64a6c1: Undone an out-of-scope change and added a missing 'auto'
* aeaf83d: Fixed an 'unused parameter' error
* f49d009: fixup: No explicit destructors with implicit copying methods
* c30c37f: Removed an unnecessary explicit copy constructor
* 012f5ec: Excluded Connection::rfc931 from cloning
* 366c78a: ICAP: do not set connect_timeout on the established conn...
This branch is now in sync with SQUID-568-premature-serverconn-use (S)
commit e64a6c1 (except for official changes merged from master/v6 into S
closer to the end of PR 877 work (i.e. S' merge commit 0a7432a).
Since master/v6 commit 2b6b1bc, a timeout on a ready-to-shovel
Squid-service ICAP connection was decrementing theBusyConns level one
extra time because Adaptation::Icap::Xaction::noteCommTimedout() started
calling both noteConnectionFailed() and closeConnection(). Depending on
the actual theBusyConns level, the extra decrement could result in FATAL
errors later, when putConnection() was called (for a different ICAP
transaction) with zero theBusyConns in an exception-unprotected context.
Throughout these changes, Xaction still counts the above timeouts as a
service failure. That is done by calling ServiceRep::noteFailure() from
Xaction::callException(), including in timeout cases described above.
The order was reversed in commit cf48712, probably by accident. Wrong
order violates TLS protocol and breaks TLS clients that are incapable of
reordering received intermediate CAs. Squid deployments that use
wrong-order bundles (to compensate for this bug) should reorder their
bundles when deploying this fix (or wait for Squid to order certificates
correctly, regardless of the bundle order -- a work in progress).
Bug 5187: Properly track (and mark) truncated store entries (#909)
Squid used an error-prone approach to identifying truncated responses:
The response is treated as whole[^1] unless somebody remembers to mark
it as truncated. This dangerous default naturally resulted in bugs where
truncated responses are treated as complete under various conditions.
This change reverses that approach: Responses not explicitly marked as
whole are treated as truncated. This change affects all Squid-server
FwsState-dispatched communications: HTTP, FTP, Gopher, and WHOIS. It
also affects responses received from the adaptation services.
Squid still tries to deliver responses with truncated bodies to clients
in most cases (no changes are expected/intended in that area).
[^1]: A better word to describe a "whole" response would be "complete",
but several key Squid APIs use "complete" to mean "no more content
is coming", misleading developers into thinking that a "completed"
response has all the expected bytes and may be cached/shared/etc.
Transactions that failed due to origin server or peer timeout (a common
source of truncation) are now logged with a _TIMEOUT %Ss suffix and
ERR_READ_TIMEOUT/WITH_SRV %err_code/%err_detail.
Transactions prematurely canceled by Squid during client-Squid
communication (usually due to various timeouts) now have WITH_CLT
default %err_detail. This detail helps distinguish otherwise
similarly-logged problems that may happen when talking to the client or
to the origin server/peer.
FwdState now (indirectly) complete()s truncated entries _after_
releasing/adjusting them so that invokeHandlers() and others do not get
a dangerous glimpse at a seemingly OK entry before its release().
Alex Rousskov [Sun, 9 Jan 2022 10:41:24 +0000 (10:41 +0000)]
Bug 5132: Close the tunnel if to-server conn closes after client (#957)
Since commit 25d2603, blind CONNECT tunnel "jobs" (and equivalent) were
not destroyed upon a "lonely" to-server connection closure, leading to
memory leaks. And when a from-client connection was still present at the
time of the to-server connection closure, we did not try to reforward,
violating the spirit of commit 25d2603 changes. Calling retryOrBail() is
sufficient to handle both cases.
Make sure the StoreMap anchor we open for reading has our key (and not
just happens to be at our hash position). Prior to this change, two
openForReadingAt() calls were missing a sameKey() post-call check due to
a buggy backport (v5 commit ec50061; mis-attributed to me).
Now the sameKey() check is integrated into the openForReadingAt() method
itself, as was already done in master/v6 since commit b2aca62.
Alex Rousskov [Sat, 30 Oct 2021 11:41:10 +0000 (11:41 +0000)]
Docs: %adapt::sum_trs entries may well exceed %icap::tt (#914)
%icap::tt documentation incorrectly implied that the measurement
includes the entire ICAP transaction(s) lifetime. In reality, individual
ICAP transaction contribution stops with
Adaptation::Icap::ModXactLauncher::swanSong(), which is normally
triggered by Adaptation::Icap::Launcher::noteAdaptationAnswer(). Here,
the "answer" does not include the entire ICAP response, but just enough
information to form adapted HTTP message headers (echoed or received).
Thus, a large or slow ICAP response body may result in %adapt::sum_trs
values that far exceed the corresponding %icap::tt "total".
This change does not imply that %icap::tt should (not) work differently.
Also fixed a typo in %adapt::all_trs and polished %adapt::sum_trs docs.
When an address is expected to be a string in the form "Host:Port", the
Host MUST be wrapped in square brackets iff it is a numeric IPv6
address. The ":Port" part is usually optional. i.e. "[2001:db8::1]:443"
or "[2001:db8::1]".
There are 2 bugs relating to the handling of numeric IPv6 addresses:
* Bug 1: AnyP::Uri::host(const char *) is supposed to accept the host
part of a URI, but just copied it into hostAddr_ instead of using the
fromHost() method to set hostAddr_. Since the argument is the host
part of a URI, numeric IPv6 addresses are wrapped in square brackets,
which would never be stripped and hostIsNumeric was therefore not set.
We now use the fromHost() method to process the host name correctly.
* Bug 2: Conversely, AnyP::Uri::hostOrIp() is supposed to return the
host name, suitable for use in a URI or Host header, which means that
numeric IPv6 addresses need to be wrapped in square brackets.
However, that wrapping was never done. We now use the toHostStr()
method to correctly format the string.
As far as I know, neither of these bugs actually break anything at the
moment, but they have the potential to break future developments, so
applying this fix should have no operational impact.
Also removed wrong IP::Address::toStr() description from .cc file that
(poorly) duplicated correct description of that method in the header.
unlockSharedAndSwitchToExclusive() was not properly failing when future
readers violated preconditions for obtaining an exclusive lock:
- `if (!readers)` means no old readers
+ `if (!readLevel)` means no old readers and nobody is becoming a reader
That missing "becoming a reader" condition covers a lockShared() caller
that had passed their writeLevel test before we incremented writeLevel
to lock other lockShared() callers out. That caller increments `readers`
while unlockSharedAndSwitchToExclusive() makes `writing` true.
Introduced in commit 1af789e due to poor code duplication. This change
also removes that code duplication by adding finalizeExclusive().
Bug 5164: a copy-paste typo in HttpHdrCc::hasMinFresh() (#901)
This bug could result in unexpected hits/misses because Squid used the
cached CC:max-stale value (or -1) instead of CC:min-fresh when
calculating entry freshness.
Leaking since commit 4eac340 that migrated from statically-allocated
ACLStrategy<...>::Instance_ objects to dynamically allocated ones.
Most ACLStrategy objects have no data members, probably leaking just one
virtual table pointer (per named ACL), but strategies that support ACL
(data) --options, like ACLDestinationDomainStrategy and
ACLServerNameStrategy, leak memory used for options storage.
Busy "class helper" objects often outlive configuration that was used to
create them. Ideally, the supplied helper category name should be copied
and maintained by the helper object itself, but that long-term solution
requires a lot more work (TODO) due to out-of-scope bugs.
The helper.cc code (ab)uses two different Connection objects with the
same FD. Properly fixing that problem requires significant work, but the
official code almost gets away with it, in part, because helper.cc, with
one exception, uses heavily customized code to "safely" close its
connections (instead of just calling conn->close() as many Squid jobs
still do). This fix replaces that single exceptional case with a
closeWritePipeSafely() call which prevents orphaning readPipe and
triggering BUG 3329 warnings. It also restores the symmetry between the
corresponding (previously buggy) stateless and (OK) statefull code.
A code merge mistake in commit b038892 created this bug AFAICT.
Amos Jeffries [Fri, 11 Jun 2021 09:28:25 +0000 (09:28 +0000)]
Maintenance: Update astyle version to 3.1 (#841)
Version 2.04 is quite outdated now, and there are only minor
formatting differences with v3.1. All changes look to be good syntax.
Also, pass the astyle executable with command line option to
formater.pl. Avoiding environment variables which do not work on servers
with sanitized sub-shell environments such as our main server.
These custom operators did not cover all new/delete cases (e.g., array
allocations), were not declared according to C++ standards (triggering
compiler warnings), and were not enabled in clang builds.
These customizations enabled custom OOM handling (for covered cases),
but it is not clear whether that feature is desirable overall, and C++
has better ways to implement such handling (i.e. set_new_handler()).
These customizations participated in collection of optional statistics
(--enable-xmalloc-statistics), but it is not clear whether that feature
implementation is good enough, and, even if it is, providing these
partial stats does not outweigh recurring customization problems.
The removed code has not been actively used almost since it was added.
It is now widely accepted that NAT and TPROXY can only be done on the
machine running Squid. The corresponding address lookup errors are an
indication of either a system misconfiguration or an adverse external
event such as flushing of conntrack tables. Since these errors should be
fatal to the affected transactions and the admin usually has the power
to address them, Squid should report them at level 1.
Alex Rousskov [Fri, 21 May 2021 18:47:36 +0000 (18:47 +0000)]
Bug 4528: ICAP transactions quit on async DNS lookups (#795)
The bug directly affected some ICAP OPTIONS transactions and indirectly
affected some ICAP REQMOD/RESPMOD transactions:
* OPTIONS: When a transaction needed to look up an IP address of the
ICAP service, and that address was not cached by Squid, it ended
prematurely because Adaptation::Icap::Xaction::doneAll() was unaware
of ipcache_nbgethostbyname()'s async nature. This bug is fixed now.
* REQMOD/RESPMOD: Adaptation::Icap::ModXact masked the _direct_ effects
of the bug: ModXact::startWriting() sets state.writing before calling
openConnection() which schedules the DNS lookup. That "I am still
writing" state makes ModXact::doneAll() false while a REQMOD or
RESPMOD transaction waits for the DNS lookup.
However, REQMOD and RESPMOD transactions that require an OPTIONS
transaction (because the service options have never been fetched
before or have expired) could still fail because the OPTIONS
transaction they trigger could fail as described in the first bullet.
For example, the first few REQMOD and RESPMOD transactions for a given
service -- all those started before the DNS lookup completes and Squid
caches its result -- could fail this way. With the OPTIONS now fixed,
these REQMOD and RESPMOD transactions should work correctly.
Amos Jeffries [Wed, 19 May 2021 11:37:34 +0000 (11:37 +0000)]
Bug 4832: '!schemeAccess' assertion on exit (#819)
* Add test to detect the bug and prevent regressions
* delete the access list if not cleared by the time
Auth::Config instance is destructed. This is what the
free_AuthSchemes() code does when the function call
nesting and debugs are pruned away.
Amos Jeffries [Thu, 6 May 2021 08:38:29 +0000 (08:38 +0000)]
Fix GCC -Werror=range-loop-construct (#808)
This warning detects unnecessary object copying in C++ range loops, with
a focus on large objects and copies triggered by implicit type
conversions. Included in -Wall.
error: loop variable ... creates a copy from type ...
Alex Rousskov [Mon, 3 May 2021 21:40:14 +0000 (21:40 +0000)]
Stop processing a response if the Store entry is gone (#806)
HttpStateData::processReply() is usually called synchronously, after
checking the Store entry status, but there are other call chains.
StoreEntry::isAccepting() adds STORE_PENDING check to the ENTRY_ABORTED
check. An accepting entry is required for writing into Store. In theory,
an entry may stop accepting new writes (without being aborted) if
FwdState or another entry co-owner writes an error response due to a
timeout or some other problem that happens while we are waiting for an
I/O callback or some such.
N.B. HTTP and FTP code cannot use StoreEntry::isAccepting() directly
because their network readers may not be the ones writing into Store --
the content may go through the adaptation layer first and that layer
might complete the store entry before the entire peer response is
received. For example, imagine an adaptation service that wants to log
the whole response containing a virus but also replaces that (large)
response with a small error reply.
Use already parsed request-target URL in cache manager and
update CacheManager to Tokanizer based URL parse
Removing use of sscan() and regex string processing which have
proven to be problematic on many levels. Most particularly with
regards to tolerance of normally harmless garbage syntax in URLs
received.
Support for generic URI schemes is added possibly resolving some
issues reported with ftp:// URL and manager access via ftp_port
sockets.
Truly generic support for /squid-internal-mgr/ path prefix is
added, fixing some user confusion about its use on cache_object:
scheme URLs.
TODO: support for single-name parameters and URL #fragments
are left to future updates. As is refactoring the QueryParams
data storage to avoid SBuf data copying.
Protect SMP kids from unsolicited IPC answers (#771)
When an SMP kid restarts, it recreates its IPC socket to flush any old
messages, but it might still receive IPC messages intended for its
previous OS process because the sender may write the message _after_
kid's IPC socket is flushed. Squid IPC communication is connectionless,
so the sender cannot easily detect the reopening of the recipient socket
to prevent this race condition. Some notifications are desirable across
kid restarts, so properly switching to connection-oriented IPC
communication would only complicate things further.
This change protects kids from, for the lack of a better term, an
"unsolicited" answer: An answer to a question the recipient did not ask.
When allowed to reach regular message-handling code, unsolicited answers
result in misleading diagnostics, may trigger assertions, and might even
result in a bad recipient state. For example, a kid might think it has
been successfully registered with Coordinator while its registration
attempt was actually dropped due to a Coordinator restart.
Our protection targets one specific use case: Responses that were born
(or became) "unsolicited" due to a recipient restart. Other problematic
cases may not require any protection, may require a very different
protection mechanism (e.g. cryptography), may deal with requests rather
than responses, or even cannot be reliably detected. For example:
* messages sent by a malicious attacker
* requests sent by a misconfigured Squid instance
* requests sent by a previous Squid instance
* messages sent by a no-longer-running kid process
* messages sent by buggy Squid code
----
Also marked a few out-of-scope problems/improvements, including a bug:
Improved handling of Coordinator and Strand exceptions exposed and
partially addressed an old problem: When configured to listen on an
inaccessible port, Squid used to kill the Coordinator job, resulting in
subsequent kid registration timeouts. Squid now correctly keeps the
Coordinator job running, logging a detailed report:
Still, the affected kids do not report the port opening problem and
continue running, listening on the problem-free ports (if any).
Depending on the exception timing, similar behavior was possible before
these changes. The correct action here is to send the port opening error
to the kid process without throwing the putFd() exception, but we should
decide how Squid should handle such inaccessible configured ports first.
Alex Rousskov [Tue, 16 Mar 2021 06:23:02 +0000 (06:23 +0000)]
Replace defective Must2(c, "not c") API (#785)
... with Must3(c, "c", Here())
* We should not make a Must*() API different from the standard C++
static_assert() API. It is just too confusing, especially since the
two calls are very closely related.
* We should not tell Must*() writers to specify one condition (i.e. what
must happen) but then describe the opposite condition (i.e. what went
wrong). When the text describes the opposite condition, it is easy for
a human writing or thinking about the text to type the wrong
condition: Must2(got < need, "incomplete message") looks correct!
We should not keep the same macro name when changing the meaning of the
second parameter. Fortunately, adding a third argument to the macro fits
nicely into how modern Squid code should pass the source code location.
Must3() does not support SBuf descriptions. I tried to support them, but
doing so without duplicating non-trivial code is too difficult, and
since the current code does not actually use them, wasteful.
If future (complex) code needs SBuf conditions, then it should probably
use an explicit throw statement instead, especially since Must*() is,
like assert(), supposed to quickly check source code sanity rather than
validate unsafe input. A compile-time condition description and the
source code location is usually enough for sanity checks, while proper
reporting of invalid input usually also requires parsing context info.
Traffic parsing errors should be reported at level 2 (or below) because
Squid admins can usually do nothing about them and a noisy cache.log
hides important problems that they can and should do something about.
TODO: Detail this and similar parsing errors for %err_detail logging.
Alex Rousskov [Mon, 15 Mar 2021 14:05:05 +0000 (14:05 +0000)]
Fix HttpHeaderStats definition to include hoErrorDetail (#787)
... when Squid is built --with-openssl.
We were "lucky" that the memory area after HttpHeaderStats was not,
apparently, used for anything important enough when HttpHeader::parse(),
indirectly called from errorInitialize() during initial Squid
configuration, was writing to it.
Detected by using AddressSanitizer.
The bug was created in commit 02259ff and cemented by commit 2673511.
Handling missing issuer certificates for TLSv1.3 (#766)
Prior to TLS v1.3 Squid could detect and fetch missing intermediate
server certificates by parsing TLS ServerHello. TLS v1.3 encrypts the
relevant part of the handshake, making such "prefetch" impossible.
Instead of looking for certificates in TLS ServerHello, Squid now waits
for the OpenSSL built-in certificate validation to fail with an
X509_V_ERR_UNABLE_TO_GET_ISSUER_CERT_LOCALLY error. Squid-supplied
verify_callback function tells OpenSSL to ignore that error. Squid
SSL_connect()-calling code detects that the error was ignored and, if
possible, fetches the missing certificates and orchestrates certificate
chain validation outside the SSL_connect() sequence. If that validation
is successful, Squid continues with SSL_connect(). See comments inside
Security::PeerConnector::negotiate() for low-level details.
In some cases, OpenSSL is able to complete SSL_connect() with an ignored
X509_V_ERR_UNABLE_TO_GET_ISSUER_CERT_LOCALLY error. If Squid validation
fails afterwards, the TLS connection is closed (before any payload is
exchanged). Ideally, the negotiation with an untrusted server should not
complete, but complexity BIO changes required to prevent such premature
completion is probably not worth reaching that ideal, especially since
all of this is just a workaround.
The long-term solution is adding SSL_ERROR_WANT_RETRY_VERIFY to OpenSSL,
giving an application a chance to download the missing certificates
during SSL_connect() negotiations. We assist OpenSSL team with that
change, but it will not be available at least until OpenSSL v3.0.
This description and changes are not specific to SslBump code paths.
Alex Rousskov [Fri, 19 Feb 2021 16:14:37 +0000 (16:14 +0000)]
Bug 3556: "FD ... is not an open socket" for accept() problems (#777)
Many things could go wrong after Squid successfully accept(2)ed a socket
and before that socket was registered with Comm. During that window, the
socket is stored in a refcounted Connection object. When that object was
auto-destroyed on the error handling path, its attempt to auto-close the
socket would trigger level-1 BUG 3556 errors because the socket was not
yet opened from Comm point of view. This change eliminates that "already
in Connection but not yet in Comm" window.
The fixed BUG 3556 errors stalled affected clients and leaked their FDs.
TODO: Keeping that window closed should not require a human effort, but
achieving that goal probably requires significant changes. We are
investigating.
projects / thirdparty / squid.git / log
