Alex Rousskov [Fri, 4 Feb 2022 14:25:58 +0000 (14:25 +0000)]
Moved regcomp(3)-specific RegexPattern code inside RegexPattern (#972)
Also moved non-ACL regex configuration parsing code inside ConfigParser.
It is possible to move ACL (data) regex configuration parsing code as
well, but that move is a lot more complex due to regex-pattern-joining
optimizations, and there are no immediate plans to support
non-regcomp(3) regexes in ACL data. We may do that move later as we get
more experience with non-regcomp(3) regexes and decide to join them too.
These moves clean up existing regex-using code and allow adding support
for non-regcomp(3) regexes (e.g., regexes based on C++11 <regex>)
without adjusting ConfigParser::regex() callers. Such support would also
require "virtualizing" RegexPattern. To avoid increasing complexity and
hurting performance, that (simpler) step should be done only if we
decide to actually add support for non-regcomp(3) regexes.
The above changes allowed us to improve RegexData error reporting: Squid
no longer reports REs _optimization_ failure when it is an individual RE
that is broken (and reported as such). Squid still ignores the fact that
broken REs can be "optimized" into a completely "different" valid
combined RE: We do not compile individual REs unless optimization fails.
Alex Rousskov [Thu, 3 Feb 2022 22:04:52 +0000 (22:04 +0000)]
Log early level-0/1 debugs() messages to cache_log (#950)
Commit d7ca82e dropped cache.log-recording of debugs() messages produced
by `finalizeConfig` runners (e.g., `WARNING: mem-cache size is...`).
This change restores that functionality (by buffering early messages
until the cache.log file is opened) and improves early debugs() handling
as detailed below.
## Squid has three channels for debugs() messages:
* cache.log (`cache_log`): Settles as squid.conf settings take effect.
* stderr (mostly `-d`): Settles when command line options take effect.
* syslog (mostly `-s`): Settles when command line options take effect.
Squid always ignores debugging messages with section/level mismatching
Debug::Levels configuration (driven by a combination of the `-k debug`,
`-X`, and `debug_options` directives). _Beyond_ that ever-present
top-level filter, each debugging channel has its own set of rules that
determine which filtered debugs() messages the channel accepts; the
following approximate summary is based on the changes in this commit:
* cache.log: all messages;
* stderr: either messages satisfying explicit `-d` level restrictions or
messages that Squid failed to write to cache.log (if no `-d`);
* syslog: level-0/1 messages and `ForceAllert` messages.
This change encapsulates channel-specific logic in dedicated classes.
## Guiding design principles
* **no repetition**: A channel must record each message at most once.
* **no reordering**: Each channel must preserve same-process debugs()
call order across all recorded messages.
* **no loss**: Each channel must log all messages matching the channel
configuration/filters.
* **no delay**: Each channel must record each message ASAP.
* **cache.log primacy**: Admins want messages logged to cache.log if
possible and to stderr/syslog only if necessary or explicitly
requested.
* **cache.log locking**: No cache.log updates without a PID lock.
## The fix
Commit d7ca82e dropped level-0/1 messages produced by `finalizeConfig`
runners because, since that commit, the cache.log channel was opened
_after_ the `finalizeConfig` event. Official code also dropped other,
even earlier level-0/1 messages, violating the "cache.log primacy"
principle. We now save early level-0/1 messages into a buffer. This
buffering can be misinterpreted as violating the "no [artificial] delay"
principle, but the messages are actually written to cache.log ASAP;
without the buffer the messages would be missing from the "primary"
cache.log and, in many use cases without stderr capturing, completely.
If Squid fails to open cache.log, early messages saved for the cache.log
channel are given to the stderr channel (following the "no loss"
principle). The stderr channel logs those messages that obey explicit
`-d` restrictions and do not violate the "no reordering" and "no
repetition" principles. Violations are tracked by assigning each message
its debugs() call sequence number.
The early messages buffer is currently dedicated to level-0/1 messages
because we were worried that level-2+ messages (if enabled via `-X`)
would overflow any reasonably-sized buffer[^1]. Correctly handling such
overflows is very difficult (we tried), so we avoid them instead.
## Side effects and surprises
To allow admins to see early level-1 cache.log messages (without adding
an `if early` check to the debugs() macro[^2]), we changed the _initial_
Debug::Levels value from `ALL,0` to `ALL,1`, matching the debug_options
default set later. This fix uncovered a few early level-1 messages that
were previously hidden[^3]:
The fact that many debugs() messages happen before cache_log can be
opened is fairly obvious, but it is also true that stderr and syslog
channels cannot write messages immediately. Both channels need to wait
for the command line options to be parsed. Even the global `stderr`
variable may not be available during very early debugs() calls! Each
channel now buffers level-0/1 messages until it settles.
Since the early message buffers are limited to level-0/1 messages,
initial cache.log records logged by `squid -X` are level-0/1 messages,
followed by true ALL,9 debugging. The admins can get early ALL,9
messages via stderr, of course.
The `-z` command-line option no longer overrides `-d` settings.
Squid no longer writes to cache_log after removing the PID file.
fatal() text and the `Squid Cache...: Terminated abnormally.` message
are no longer dropped during certain early process terminations.
`squid -k ...` logs some new level-0/1 messages to stderr.
Improved support for assert()/debugs() triggered from within the Debug
module: Besides crashes, such "internal" debugs() could be logged before
earlier "external" messages and some assert() messages could be lost.
[^1]: The restriction to only buffer level-0/1 messages can be easily
removed (after research and discussion) if there is consensus that the
actual memory required to accumulate all typical level-2+ early messages
is worth spending on making `squid -X` write all messages to cache.log.
[^2]: Squid has thousands of debugs() calls (and counting), including
many calls on performance-sensitive paths. Most debugs() calls should do
nothing by default. Thus, the speed at which the debugs() macro can skip
logging is an important common case on the performance sensitive path.
Similarly, disruption to CPU processing pipeline due to top-level
debugs() checks may be important. Actually writing the message to the
cache.log may not be that important -- at that time, the performance
battle can be considered lost -- but the initial rejection is.
[^3]: This change is not about the levels of any specific messages.
Wrong message levels (if any) should be fixed separately. This change
does not imply that the newly discovered messages have wrong levels.
squidadm [Thu, 3 Feb 2022 12:44:53 +0000 (12:44 +0000)]
Source Format Enforcement (#963)
Besides routine formatting enforcement, this change contains flag-day
updates and code polishing from code removals performed in the latter
half of 2021 (e.g., HERE removal).
(summary) log from the script execution:
UPDATE COPYRIGHT for ...
NOTICE: File ... changed: by scripts/maintenance/HERE-obsolete
NOTICE: File src/Makefile.am changed: by
scripts/format-makefile-am.pl
NOTICE: File src/sbuf/Stream.h changed: by
scripts/maintenance/sort-includes.pl
NOTICE: File src/tests/stub_store_client.cc changed: by
scripts/maintenance/sort-includes.pl
If you are worried about hitting all this at once when rebasing a
large patch you can split the rebase into steps as follows:
git fetch --all
git rebase [ commit ID prior to this one ]
git rebase [ this commit's ID ]
git rebase master
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 [Fri, 28 Jan 2022 12:05:43 +0000 (12:05 +0000)]
Bug 5194: Remove all unused debug sections (#969)
... that we can find quickly.
Naming/documenting debugging sections is a good idea, but that should
not be done in every source code file that relies on that section. We
cannot remove all such DEBUG: declarations without developing a proper
way to name/document sections, but we can (and, given Bug 5194
existence, probably should) remove the unused ones -- their removal does
not remove any immediately usable info.
Bug 5194 report was specific to Section 56: Folks misinterpret section
56 "HTTP Message Body" title as promising to dump message bodies to
cache.log, which is not a functionality that should be driven by a
debugging section. Currently, Squid lacks such functionality.
Alex Rousskov [Wed, 26 Jan 2022 16:39:38 +0000 (16:39 +0000)]
Avoid reverse DNS lookups when logformat %>A is unused (#912)
Initially, the log_fqdn directive decided whether to do reverse DNS
lookups of client IP addresses ASAP, to improve our chances of logging
the resolved-by-then client FQDN.
Since commit 7684c4b, Squid started violating log_fqdn configuration,
enabling early reverse lookups if %>A was used in some logformat. Seven
years later, commit c581e96 completely removed the log_fqdn directive.
Unfortunately, the idea that seeing %>A somewhere means early lookups
are needed is flawed because
1. Some logformats containing %>A are never parsed. For example, the
icap_squid logformat is still hard-coded as a printf()-based code.
Using that format should enable early reverse lookups, but does not.
2. Some parsed logformats containing %>A may be unused. This is
especially true for _default_ logformats that admins cannot control
at all. Initially, no default logformats were parsed, but four years
later, commit b11724b converted some hard-coded printf()s into parsed
default logformats, inadvertently enabling reverse lookups in all
Squid configurations!
There is no way an admin could turn those lookups off because our
DEFAULT: lines are parsed unconditionally; overwriting those defaults
simply means that the corresponding logformat directive is parsed
twice, once when parsing defaults (enabling the lookups) and then
when parsing admin settings (the lookups state remains unchanged).
Transactions do not wait for these DNS lookup queries to be answered
and, due to DNS caching, not every client connection triggers a new DNS
query, but the total volume of these lookups may be significant in
environments with many client IP addresses, especially after a fresh
Squid start (or during a flash crowd arrival), when Squid DNS cache is
empty (or has not been primed with a lot of new addresses yet).
This change replaces the "parsed %>A enables early lookups" logic with
the "used %>A enables early lookups" approach. Logging of FQDNs is still
not guaranteed -- the lookups may not be enabled early enough and may
not complete by the time we use %>A -- but Squid instances not using %>A
are now guaranteed to avoid useless reverse DNS lookups of client IPs.
Also fixed logging of client FQDNs in the default ICAP access log format
(a.k.a. icap_squid): That format has the equivalent of a %>A field.
Thus, the icap_squid logging code must _always_ attempt to log FQDN and
enable early reverse DNS lookups. Neither was happening.
Alex Rousskov [Fri, 21 Jan 2022 04:45:11 +0000 (04:45 +0000)]
Categorize level-0/1 messages; part 1: "easy" problem messages (#946)
Today, it is impractical to reliably identify important cache.log
messages in automated sysadmin notification systems. No regular
expression is guaranteed to match all important messages and none of the
unimportant ones. While message importance is naturally subjective (with
cache_log_message and notification system regexes available to customize
handling of specific messages), even reliable detection of _potentially_
important messages (to bootstrap admin customization efforts) is
impractical today. Any notification configuration is likely to miss an
important message (in today's or future code) and/or spam admins with
unimportant messages.
Fully addressing this problem requires many changes. This change lays a
foundation for that work by standardizing three well-known message
prefixes (`FATAL:`, `ERROR:`, and `WARNING:`) for debugs() messages
reporting various problems. Eventually, all problem-reporting messages
(and only those) will use one of the standard prefixes. This change
converts 300+ debugs(), bringing the total to 900+ messages using a
a standard prefix. There are more than 5'000 debugs() in Squid.
The scope of this change are problem-reporting debugs() statements that
can be detected and converted by a simple script. We also applied a few
easy-to-automate polishing touches to modified debugs(), like replacing
debug level 0 with DBG_CRITICAL, expanding some contractions (e.g.,
"can't"), and fixing grammar (e.g., "is has").
Alex Rousskov [Thu, 13 Jan 2022 22:38:24 +0000 (22:38 +0000)]
Maintenance: De-duplicate PackableStreamBuf and SBufStreamBuf (#959)
SBufStreamBuf was almost identical to PackableStreamBuf. Both classes
are pass-through write-only streambufs that lack their own put area. Now
their common code lives in AppendingStreamBuf.
No functionality changes intended.
The removed std::streambuf method descriptions were imprecise and some
were misleading. STL documentation describes this standard/parent API.
While also similar, PackableStream and SBufStream have important
differences in API and implementation. For example, unlike
PackableStream, SBufStream owns the sink buffer and does not modify the
buffer it was created with, providing a safe content extraction method
instead. The two classes cannot be merged without changing their users,
and it may be impossible to justify SBufStream users exposure to the
dangers of forgetting to sync() the streambuf before accessing the sink.
Remove ConfigParser::Undo() hack to improve ACL flags parsing (#960)
Since `acl ... -n` support was added in commit 33810b1, flag-agnostic
parseFlags() extracts ACL flags, applies supported ones, and rejects the
rest. However, that extraction code does not apply the supported `-i`
flag! Instead, the flag is put "back" via Undo() as if we did not see it
at all. Later, the ACL data parser re-parses and applies it.
That "undo" hack avoided ACL data parsing changes but caused several
problems, including:
* The global Undo_ storage could "leak" the stored flag to the wrong ACL
on certain ACL parsing errors. The global nature of that storage also
blocked serious preprocessing/reconfiguration support improvements.
* AclData::parse() did not distinguish (previously undone and now
"redone") flags from (post-"--") ACL data, blindly assuming that the
first token is a flag and treating the remaining tokens as ACL data.
* Increasingly inconsistent handling of `-i` and `+i` flags.
* Related `ident -i` parsing code had severe performance problems: Some
tests timed out due to exponential growth of `-i` parsing delays (with
the number of parsed ACLs) caused by excessive userDataNames copying.
This change removes the "undo" hack for good. We now parse all leading
ACL options once, using a single Acl::Option API for both "global" (i.e.
applicable to the entire named ACL object) and "line" scoped options.
The parsed line-scoped flags (e.g., `-i`) are reset before parsing each
ACL directive line. They are delivered to the (ACL data) parsing code
using the existing Acl::Option linkedWith() mechanism.
TODO: This change does not fix all ACL data flag handling problems. For
example, ACL data parsing methods should be refactored to reuse the
now-generalized Acl::Option API for handling flags located _between_ ACL
parameters. Those non-trivial fixes are unrelated to Undo() removal and
will fix/improve ACL data handling, so they deserve dedicated commits.
Alex Rousskov [Mon, 10 Jan 2022 21:36:46 +0000 (21:36 +0000)]
Fix gawk v5 warning (#961)
gawk: ./mk-globals-c.awk:24:
warning: regexp escape sequence \" is not a known regexp operator
Some awk variants complain about what they perceive as invalid escape
sequences. Gawk v5+ works but complains about us escaping double quotes.
The corresponding awk statement (added in commit 42c674f) is unnecessary
since commit 582c2af. Removing that statement is better than trying to
guess its portable spelling in the gray zone of awk escape sequences.
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).
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.
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).
Amos Jeffries [Sun, 5 Dec 2021 21:39:14 +0000 (21:39 +0000)]
Remove m88k-specific support (#944)
Modern support for m88k hardware is only available through
OpenBSD and NetBSD which have their own compatibility handling.
We no longer need to detect m88k ourselves.
Amos Jeffries [Sat, 4 Dec 2021 17:54:35 +0000 (17:54 +0000)]
Remove unused/broken ACL copying support (#941)
This code is unused. The clone() methods were the only use of
copy construction and operator. Most ACL related classes lack
implementation of the copy and/or assert so even if used this
code would be quite dangerous.
Explicitly forbid copy/move at the hierarchy base classes
ACL and ACLData. Removing all child specific copy
implementations and prohibitions (now unnecessary).
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.
I removed the whole "we have a valid TLS connection" condition because
the code in question no longer uses the TLS connection object, and the
condition itself may change _after_ this code runs. The condition is
important to higher-level code (that calls the code that uses
allowSplice/allowBump state set in the affected low-level BIO method),
but there are many such conditions on that code path; there is no good
reason to single out this specific condition in this low-level code.
Amos Jeffries [Thu, 11 Nov 2021 21:38:31 +0000 (21:38 +0000)]
Remove CPU profiler mechanism (#931)
The old CPU profiler has not been updated in many years. As a result,
the statistics provided are deceptively incomplete and not sufficient
for their intended purpose of profiling Squids CPU usage. External tools
such as oprofile do a better job despite their differences and some
limitations.
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.
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.
We have a const-correctness bug in std::unordered_map when supplying an
allocator that OpenBSD is strict about. Fix it.
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.
BUG: Unexpected state while connecting to ... server, part 1 (#916)
These BUG messages (discussed and removed in a recent commit 2b6b1bc)
exposed several bugs. This change fixes a case where a BUG message was
correctly triggered by a Must() violation in Ssl::ServerBio::write():
The code expectations reflected in that Must() were wrong: Instead of
sending ClientHello, OpenSSL may also send a TLS Alert (Level: Fatal,
Description: Internal Error), at least. We believe that alert is sent
when SslBump configures OpenSSL to negotiate using unsupported ciphers
or something like that. This change relaxes ServerBio code expectations,
preventing the Must() violation.
The Must() violation was causing OpenSSL-related memory leaks. A more
comprehensive solution is needed to avoid similar leaks, but this small
fix helps in a specific (and a fairly common) case.
Remove step2+ stare-and-splice and peek-and-bump support (#926)
Support for these features was successfully disabled (at build time) for
five years. See commit 88a300c for the list of reasons. We did not even
provide an ./configure --enable-... option for it. The corresponding
code does not compile with modern OpenSSL versions, but its mere
presence complicates related code logic and significantly increases
related development efforts. This code is not worth keeping.
This change addresses a known problem that triggered unwanted C++
exceptions every time Squid selected a to-server persistent connection
as the primary Happy Eyeballs destination/answer. The bug existed since
HappyConnOpener inception (commit 5562295). It did not seem to affect
the connection requestor directly because the HappyConnOpener job sends
the selected pconn to the requestor _before_ throwing.
Also adjusted Happy Eyeballs state documentation to reflect a successful
termination state. Before and after this change, we may enter that state
in the middle of checkForNewConnection(). The less we think about done()
as an _exceptional_ "only on error" or "only at the end of processing"
state, the fewer similar bugs we will create.
The code also improved after we abandoned the idea of documenting all
primary state changes in checkForNewConnection(). There are too many
nuances/changes to document everything anyway, and moving primary track
handling into a dedicated function significantly improves readability.
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: Unexpected state while connecting to ... server, part 2 (#917)
These BUG messages (discussed and removed in a recent commit 2b6b1bc)
exposed several bugs. This change fixes a case where a BUG message was
triggered by the following Must() violation:
The above Must() assumed that PeekingPeerConnector always changes the
SslBump step to step3. However, that assumption was wrong because
PeekingPeerConnector may run multiple times (and the step is recorded
outside the connector object). When FwdState reforwarded a failed
attempt, PeekingPeerConnector would find itself at step3.
Instead of fixing the Must(), we fixed a bigger bug: SslBump step3 must
start when we decide to communicate with the server, not in the middle
of that communication. This fix may affect some esoteric configurations
that use at_step ACLs outside ssl_bump _and_ rely on the wrong step
change timing, but, technically, such configurations are not officially
supported.
More step boundary fixes are needed. There is a (much bigger) ongoing
project dedicated to those changes.
Properly track (and mark) truncated store entries (#909)
## Responses with truncated bodies
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.
## Related access-logging improvements
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.
## Other
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().
Treat responses with truncated HTTP headers (i.e. no CRLF after all the
field-lines) as malformed, replacing them with an HTTP 502
ERR_INVALID_RESP error (same as any other HTTP response with malformed
headers would get).
Since Bug 2879 (commit da6c841 and earlier v2-only changes), Squid was
"fixing" such truncated headers by adding an extra CRLF sequence and
re-parsing them. Depending on the truncation circumstances, that old
workaround could result in rather bad outcomes for Squid and its
clients. Hopefully, we no longer need to work around such bugs. If we
do, we should do it in a safer manner and with admin permission.
...using ALE cache.code instead of its partial
ClientSideRequest::logType duplicate.
The need to de-duplicate these two LogTag members became apparent when
we tried to update/detail LogTags in forwarding code (e.g., to report
peer timeouts) and noticed that ALE cache.code value could be
overwritten by (stale) ClientSideRequest::logType information. Those
updates needed other significant changes, so we factored this small
stand-alone improvement out.
Fortunately, ClientSideRequest::logType lifetime does not exceed ALE's,
making ClientHttpRequest::logTags removal straightforward.
Also removed a paranoid TunnelStateData::al check after verifying that
the ALE pointer is never nil.
Improved const-correctness of ALE-driven APIs (#904)
Also made ClientHttpRequest::al constant. This work actually started
with changing this data member, to make some ALE-related hack safer.
That change required the API fixes. We have since got rid of the hack,
but all these const-correctness changes are valuable on their own.
Fixed 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.
Alex Rousskov [Sun, 12 Sep 2021 19:03:39 +0000 (19:03 +0000)]
Fix "mem_obj->inmem_lo == 0" assertion in StoreEntry::swapOut() (#896)
Squid may stop writing to disk well before receiving the entire miss
response (e.g., because of swapout errors). When that happens, Squid
continues to call swapOut() to mem-cache the incoming response body.
Avoid the inapplicable disk-writing part of swapOut() in those cases.
Fix MAX_PKT{4,6}_SZ to account for icmpEchoData padding (#887)
The bug was exposed by GCC v11 on Ubuntu Impish:
Icmp4.cc:116:11: error: array subscript icmpEchoData[0] is partly
outside array bounds of char[282] [-Werror=array-bounds]
echo->opcode = (unsigned char) opcode;
The array the compiler is talking about is the pkt buffer. That buffer
size (i.e. MAX_PKT4_SZ) was calculated under the faulty assumption that
a compiler cannot add padding after icmphdr (when doing "icmp+1") and/or
between icmpEchoData data members. When compiler padded, the old
MAX_PKT4_SZ math stopped working.
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 were 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
would only be a matter of time before the next bug bites 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. Without them, we will keep chasing deadly side effects of
poorly implemented closure callbacks.
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
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.
Amos Jeffries [Thu, 19 Aug 2021 04:56:04 +0000 (04:56 +0000)]
Cleanup dependencies for testEvent unit test (#886)
Many objects linked to the testEvent unit test were there for
legacy reasons and no longer needed.
Converting Event::dump() to Packable interface additionally
removes all dependency on the Store and StoreEntry API. Vastly
simplifying the complexity of the test setup and clarifying
(lack of) code coverage by this test.
If the compiler supports it, add the `-Wextra` compiler flag to enable
extra checks. Do not enable extra checks with GCC v4 because that old
GCC version reports too many false positives.
Explicitly disable those `-Wextra` checks that offer few insights while
creating a lot of noise which cannot be easily avoided.
Fix warnings reported by GCC or clang with the new settings.
Rework syslog-related code in debug.cc, making it clearer and with less
dependencies on some syslog levels.
Support "file" syntax for 'squid_error' and 'has' ACL parameters (#874)
The 'squid_error' and 'has' are the only ACLs that do not support
loading their parameters from a file using the "path/filename" syntax.
We see no justification for this exception, and it stands in the way of
(unrelated) configuration improvements we are working on (that will,
among other things, prevent such accidents from happening again).
This change causes no upgrade problems because it does not change
handling of previously accepted configurations. It only expands the set
of acceptable configurations, better matching documentation and admin
expectations with regard to universal "path/filename" syntax support.
Amos Jeffries [Sun, 1 Aug 2021 12:58:25 +0000 (12:58 +0000)]
Fix TCP keepalive (#853)
Setting TCP keep-alive flags at accept(2) time resolves issues with
client sockets timing out while waiting for the ::Server handler to run.
Also resolves a bug with FTP DATA connections not having keep-alive set.
These connections would truncate objects if the data transfer connection
paused for too long and became timed out by the network routing system.
cache_log_message ID 62 was missing from doc/debug-messages.dox because
the code generating that file did not recognize debugs() statements with
Critical() or Important() macro inside a conditional operator.
Also fixed a related sed regex: sed does not support non-greedy matches.
Alex Rousskov [Wed, 28 Jul 2021 12:21:45 +0000 (12:21 +0000)]
Remove legacy context-based debugging in favor of CodeContext (#866)
Added in 1998, context-based debugging has been neglected and
- only covers two relatively small contexts
- unsafe in the presence of exceptions
- produces noise (e.g., "ctx: exit level 0" messages)
- delayed "ctx: exit" messages confuse admins
- uses deprecated urlXXX() API
- difficult to extend to more contexts without performance overheads
- usually provides less info in fewer contexts (than CodeContext)
Jonathan Newton [Fri, 23 Jul 2021 07:03:26 +0000 (07:03 +0000)]
Allow sending "squid -k ..." signals to PID 1 (#863)
Inside a container, Squid usually runs with PID=1. Since "squid -k"
feature was added in 1996 (commit 7690e8e), Squid refused to signal a
PID=1 instance. We do not know why, and it could have been an accident.
We cannot find a good reason to keep prohibiting such signalling.
Reject different HTTP requests with unusual framing (#753)
... and remove support for request_entities.
Squid now follows the following (approximate) rules when checking HTTP
request framing. The first matching rule wins.
* HTTP requests with a Transfer-Encoding:chunked header, including GET
and HEAD requests with that header, are accepted. No changes here.
* HTTP requests with unsupported Transfer-Encoding values are rejected
(Squid replies with HTTP 501 "Not Implemented"). No changes here.
* HTTP requests having conflicting Content-Length values are rejected
(Squid replies with HTTP 400 "Bad Request"). No changes here.
* HTTP/1.0 and HTTP/0.9 POST and PUT requests without a valid
Content-Length header are now rejected (Squid replies with HTTP 411
"Length Required"). All of these were allowed before.
* HTTP/1.0 GET and HEAD requests with a Content-Length:0 header are now
rejected (Squid replies with HTTP 400 "Bad Request"). All of these
were allowed before.
* HTTP/1.0 GET and HEAD requests with a positive Content-Length header
are now rejected (Squid replies with HTTP 400 "Bad Request"). All of
these were allowed before if and only if the request_entities
directive was explicitly set to "on".
There are no other framing-related HTTP request restrictions. Prior to
these changes, HTTP/1.1 GET and HEAD requests with a positive
Content-Length header were rejected unless the request_entities
directive was explicitly set to "on". The following configuration sketch
keeps rejecting those requests:
acl getOrHead method GET HEAD
acl withContentLength req_header Content-Length .
http_access deny getOrHead withContentLength
The new restrictions were added due to possibility of cache corruption
attacks and other security issues related to HTTP request framing.
The request_entities directive was removed to simplify decision logic.
Some developers believe that these changes should be accompanied by
configuration options that allow admins to bypass (most of) the
previously absent restrictions. However, these developers do not know of
any important use cases that these changes break, and such cases may not
even exist. The authors insist on these security-driven changes.
Alex Rousskov [Thu, 15 Jul 2021 05:58:19 +0000 (05:58 +0000)]
Remove bogus "found KEY_PRIVATE" WARNINGs (#862)
... triggered by private bumped StoreEntry unlock()ed in ~ServerBump().
The WARNINGs were added long time ago (commit fc8b9fc) because, AFAICT,
earlier Store code expected StoreEntry owners to release() uncachable
entries, including KEY_PRIVATE ones, right before unlocking them.
However, there is no compile-time enforcement of that expectation, and
unlocking code does not always know whether the entry is cachable (as
ServerBump constructor/destructor RAII code illustrates).
This change stops tying release and unlocking decisions/actions together
but makes sure that idle KEY_PRIVATE entries are still released (because
we do not want to index unneeded/unusable Store entries).
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.
The incoming_sockets_accepted counter is used by Comm I/O modules to
count the number of successfully accept()ed FDs during a single
select(2) (or equivalent) scan of ready descriptors. For TCP
connections, the official code incremented that counter in the async
TcpAcceptor callbacks. Those callbacks run outside the counting Comm I/O
context, rendering counter increments useless and resulting in
under-reporting of actual acceptance activity.
Also, some successfully accepted TCP connections fire no callbacks, so
it is conceptually wrong to count accepted sockets in callbacks,
regardless of the timing problems.
static_assert ... "Sum() return type can fit its (unsigned) result"
in ClpMap member instantiation inside sslCrtvdHandleReplyWrapper()
Honor Sum<T>() caller's explicit request to use a specific accumulation
type T instead of guessing the right type for inner sum iterations
(based on the second, third, etc. arguments) and hitting static
assertions when those guesses were wrong because some of those arguments
used types smaller than T.
This fix also allows Sum() callers to avoid explicit T when the first
argument is already the largest one. Callers should not be forced to be
explicit at all, but computing the largest type is a complex known TODO
outside this fix scope.
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.
This directive controls cache.log printing of important messages.
Rationale: In any given deployment environment, many of the cache.log
messages that Squid logs by default are noise. Some of them should
probably never be logged by default, but some are useful or even
critical in certain other environments. No single approach to logging is
going to satisfy all needs. The existing debug_options directive
operates on large message groups and is not suitable for controlling
individual critical or important messages.
The current implementation uses hard-coded manually-assigned opaque
message IDs. We have also experimented with an automated generation of
message IDs, including meaningful IDs and compiler-generated IDs created
by various advanced C++ preprocessing and template tricks. All of the
automation considered (and some partially implemented) were rejected
for being overall worse than the current manual approach.
TODO: The usefulness of ID ranges will diminish with time. We could
support named (hard-coded) sets of message IDs (e.g., "startup",
"shutdown", and "configuration"). However, sets make it easier to
accidentally suppress an important new message during an upgrade.
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.
Avoid "BUG #3329: Lost orphan ..." during accept problems (#780)
Comm::TcpAcceptor creates a Comm::Connection with an open FD. Lots of
things could go wrong while that connection object travels to its
intended owner (e.g., Ftp::Server). A Connection object abandoned during
that travel will auto-close, triggering level-4 "BUG #3329" messages.
TODO: The manual enter/leaveOrphanage() tracking is unreliable. We need
to design and implement a true connection ownership concept that does
not require such tracking. These changes highlight handover spots.
Also made a few TcpAcceptor members protected to reduce the chance of
missing a Connection recipient (i.e. TcpAcceptor subscriber). Most of
these members should be (at least) protected for other reasons as well.
We examined most `#if...` clauses and removed those we decided are
clearly unwanted now. Most of the removed code snippets were unused for
many years or represented stale ideas. We tried to check (at least
superficially) all clauses, but most likely missed some good candidates.
Alex Rousskov [Mon, 24 May 2021 20:02:52 +0000 (20:02 +0000)]
Add tls_key_log to report TLS communication secrets (#759)
The new log is meant for triage using traffic inspection tools like
Wireshark. Also known as SSLKEYLOGFILE, it is a common feature across
TLS-capable applications. By default, the new feature is disabled and
has negligible performance impact.
We have rejected the idea of adding an access_log %code instead, as
detailed in squid-dev thread titled "RFC: tls_key_log: report TLS
pre-master secrets, other key material". Such %code can still be added.
http://lists.squid-cache.org/pipermail/squid-dev/2020-July/009605.html
----
Generalized StoreClient::fillChecklist() API into Acl::ChecklistFiller
Code that establishes TLS connections outsources logging of secrets to
Security::KeyLogger. Security::KeyLogger::shouldLog() must consult
tls_key_log ACLs before logging the secrets but it does not know much
about the code/context that delegated this logging to KeyLogger. That
code could fill the checklist in advance, but that is an inferior
solution because it
* prevents outsourcing a rather mundane responsibility to KeyLogger
* creates a potentially stale Checklist, not reflecting TLS progress
* wastes resources on filling a checklist that may not be needed
While adding ConnStateData::fillChecklist() and looking through
ConnStateData code for examples on how to fill a checklist, it became
clear that there is a lot of code duplication in this area, including
some subtle problems with various checklist.foo assignments possibly
missing in some places (it is difficult to know for sure because some
checklist methods may extract the same information from _multiple_
sources). For example,
* on_unsupported_protocol and ssl_bump ignored acl_uses_indirect_client
* ident_lookup_access and delay_pool_access lacked port info and
other ConnStateData details beyond src/dst addresses
Now, most of that duplicated code is replaced with unified
fillChecklist() calls. This unification is a significant improvement
that provides additional justification for the fillChecklist() design.
Also renamed ACLFilledChecklist::conn() to setConn(). Renaming is not
necessary for the compiler but will help detect no longer necessary
conn() setting calls when this code is ported to other Squid versions.
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.
Alex Rousskov [Thu, 20 May 2021 03:11:17 +0000 (03:11 +0000)]
Make CustomLog more reusable (#824)
The future tls_key_log directive supports most access_log features but
should not inherit deprecated configuration styles, multi-directive
support complications, and legacy top-level configuration parsing code.
The shared/reusable bits were extracted from CustomLog and cache_cf.cc
functions into the new FormattedLog class. Some of them were polished.
All existing access_log/etc. directives continue to use CustomLog, so
nothing in the code justifies FormattedLog existence right now.
Fixed at least these old bugs:
* access_log config dumps were missing the destination parameter
* "access_log none" config dump included a bogus "logformat=none" option
* "access_log buffer-size=..." config dump were missing size units
* built-in logformat re-definition check missed the "icap_squid" format
* "access_log rotate=N" accepted (but then ignored) negative N values
* "access_log rotate=N" was stored as short but dumped as full integer
Admin-visible changes:
* access_log key=value strings are now case sensitive. This change
improves consistency with squid.conf directives that are all case
sensitive and most other directive options. The logformat name was
already case sensitive. I see no good reason to keep this undocumented
exception.
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.
Joshua Rogers [Mon, 17 May 2021 03:43:48 +0000 (03:43 +0000)]
Simplified Http::Message::parse() (#814)
size_t hdr_len cannot be negative, and sanityCheckStartLine() already
rejects zero values. We now use (and clearly document) the latter fact.
Also replaced a level-1 warning about "Too large" headers with a level-3
debugging message because huge headers is not a Squid problem, and the
problem should already be visible in access.logs records.
Alex Rousskov [Sat, 8 May 2021 07:51:32 +0000 (07:51 +0000)]
API to separate Config.x users/parsers from Config.y details (#811)
The new API avoids several kinds of unwanted code dependencies:
* Config.x users do not need to know the type of any Config.y component.
Forward declarations of Config.y types are sufficient.
* Config.x parsers do not need to know about Config.y parsers. Config.x
parsers can also be hidden from Config.x users.
* The centralized parsing code does not need to contain Config.x parsing
code (for any x) and does not maintain "generic" parser registration.
The compiler/linker do that for us _without_ losing C++ type safety.
Correct API implementation may also help separate the active Config
object from the being-parsed configuration, to eventually support safe
"hot" reconfiguration.
This change does not convert any existing Config fields and does not
imply a policy that existing fields must be converted when touched.
The API was tested on a typical new Config.x component (to be merged
separately). It is likely to evolve, especially when support for
multi-directive components is added.
Activated by using TYPE names with two colons (e.g., TYPE: Security::X).
projects / thirdparty / squid.git / log
