Use added X509_check_issued() replacements. The only case left is in
src/ssl/gadgets.cc which is used by certificate helpers that cannot be
linked with libsecurity yet.
Use added X509_NAME_oneline() replacements, where feasible. This change
speeds up ssl_verify_cb() and other functions that used to extract and
copy certificate name into a buffer even when that name was unused
because debugging levels were not elevated enough, including by default.
Also fixes memory leak when debugging section 83 at level 3+ of an
OpenSSL-using Squid (missing name cleanup in clientNegotiateSSL()).
Also fixes a (usually symptom-free) sslcrtd bug: C strings allocated by
OpenSSL were freed by xfree() instead of OPENSSL_free().
ERROR: failure while establishing TLS connection on FD: 70x123456*1
* FD value was malformed. Now report all Connection details instead.
* Error details were not reported (a pointer value was reported).
Also, for error message prefix, use a single/constant phrase instead of
error-dependent phrases. This principle is for level-0/1 messages, but
it is OK to apply it here as well, especially given the related TODO.
Also improved RawPointer reporting so that we can use it for this fix.
Some of these changes are not necessary (for the final code state), but
all of them except the last one were necessary at some point during the
work on this fix, and they all may become handy in the future:
* Print nothing if ptr is nil. Both existing callers benefit from this.
* Allow the caller to set the label/object delimiter. Also faster.
* Support reporting object values on a dedicated Debug::Extra line.
* Support (and do not attempt to print) nil RawPointer() labels.
One master transaction may encounter several certificate
validation-related errors because Squid may need to validate several
server certificates for one connection and may even need to open several
server connections. Since 2012 commit 4fb72cb, ssl_verify_cb()
incorrectly assumed that no past errors were possible. The callback
started to assert when recent code changes (commit e227da8?) improved
delivery of past errors to ACL checks.
While fixing this bug, we discovered that ServerBump::serverSession
ignored new TLS connection when Squid connected to another destination
after a failed TLS handshake. That stale value was then used to extract
stale information about past TLS errors. In some cases, that would
prevent Squid from hitting the now-fixed assertion. In other cases, it
could result in wrong sslproxy_cert_sign and sslproxy_cert_adapt
decisions. Now also fixed; see Ssl::ServerBump::attachServerSession().
A nil PeerConnector::check->sslErrors value may get stale because
PeerConnector::check is filled just once: sslError will remain nil
during the second validation pass (after PeerConnector fetches missing
intermediate certificates and revalidates), despite validation errors
discovered during the first pass. Today, ssl_verify_cb() does not really
use FilledChecklist::sslErrors preset by its PeerConnector, so a stale
value currently has no effect. Fixing this waiting-to-happen problem is
not trivial (we tried). A proper fix deserves a dedicated PR. Added XXX.
While working on these changes, we discovered that Squid implements two
mutually exclusive ssl_error (FilledChecklist::sslErrors) definitions:
* In sslproxy_cert_error context, ssl_error matches the last or current
error being examined by the sslproxy_cert_error code.
* In sslproxy_cert_sign and sslproxy_cert_adapt context, ssl_error
matches any previously observed validation error.
This change updates Squid documentation to reflect the above findings.
Unfortunately, it is not feasible to remove this unwanted directive
"sensitivity" without deprecating/replacing sslproxy_cert_error and
ssl_error: Selecting any one of the two ssl_error definitions above may
seriously (and silently) break existing configurations:
# If all-errors semantics is adopted for sslproxy_cert_error, then
# this configuration will start allowing certificates with _any_
# error as long as that certificate also has a minorError:
acl minorError ssl_error ...
sslproxy_cert_error allow minorError
# If last-error semantics is adopted for sslproxy_cert_sign, then
# this configuration will stop properly signing certificates that
# mimic self-signed real certificates if the very last error during
# that real certificate validation was _not_ certSelfSigned:
sslproxy_cert_sign signSelf ssl::certSelfSigned
Alex Rousskov [Mon, 16 May 2022 12:53:26 +0000 (12:53 +0000)]
Fixed (and renamed) Ssl::ReadX509Certificate() API (#1048)
This pointer-returning non-throwing function was used to load an unknown
number of certificates from a file. A nil result normally indicated the
end of the certificate sequence but also a certificate loading failure.
The caller could not distinguish the two nil-result outcomes without
OpenSSL error stack analysis, which is difficult to do correctly, and
which none of the callers bothered to do, resulting in some certificate
configuration errors silently ignored by Squid.
The adjusted API uses exceptions to report errors. A nil result is still
used to report the end of the certificate sequence. To help callers that
must have at least one certificate, one function is now dedicated to
loading required certificate(s) while a companion function is added to
load optional ones.
Avoid exponential growth in the number of changed functions by wrapping
callers inside the not-ready-to-throw code chains (that cannot be easily
converted) with try/catch-return-false blocks.
Also removed the need for Ssl::readCertFromMemory() by adding
Ssl::ReadOnlyBioTiedTo(). Functions reading things (from various input
sources) should be kept separately from functions creating input sources
(for various reading functions). Otherwise, we will end up creating a
lot more functions (at least readers*creators) with virtually no gain in
performance or convenience. OpenSSL BIO API exists specifically to
separate I/O mechanisms from I/O users. Use it to our advantage.
We now also clear old/stale OpenSSL error stack before performing the
certificate loading operation that may trigger one or more errors.
Also forget stale errno when forgetting OpenSSL errors. This reset
avoids misleading error reports based on stale errno values.
Also detail certificate loading errors, now that they are not ignored.
ReportSysError was added to avoid SysErrorDetail::NewIfAny() memory
allocations when reporting basic system call errors. Perhaps there is a
way to reuse SysErrorDetail here via (enhanced) Optional, but the
resulting complexity and the current SBuf-based reporting overheads do
not justify the reuse of "if errno then strerror(errno)" logic. We may
revisit this when Optional supports non-trivial classes.
Also moved most SysErrorDetail implementation details to the newly added
.cc file (to reduce user exposure), replacing strerror() with xstrerr().
ForgetErrors guts were moved from Security to Ssl because we cannot link
helpers with src/security (yet). We may have to refactor Security to
make such reuse possible in the future.
Preserve caller context across (and improve) deferred reads (#1025)
The transaction context was not saved/restored when dealing with
deferred reads initiated by events like the DelayPools::Update() event.
To fix this, we refactored MemObject::delayRead() and its descendants to
use an AsyncCall, which automatically stores/restores code context.
Using explicit async callbacks highlighted the danger of passing
Connection object via CommRead that does not maintain a closure
callback. There was also a related "stuck transaction" suspicion
documented in DeferredReadManager::kickARead(). Fortunately, all these
problems could now be solved by removing DeferredRead and CommRead
classes! The delayed readers already store the Connection object,
maintain closure callbacks, and have to check stored Connection validity
before reading anyway. The general/centralized delayed reading logic is
not really about reading and Connections (those parts are handled by
transaction-specific code) but about triggering reading attempts.
Asynchronous calls are perfect (and sufficient) for doing that.
Also fixed Delay Pools for Gopher: delayAwareRead() was initiated only
once from gopherSendComplete() and the subsequent read calls were
delay-unaware (i.e. immediate) reads.
Also fixed a Delay Pools problem with active transactions: A transaction
started with Delay Pools on becomes stuck if a reconfiguration turns
Delay Pools off.
Also refactored the existing AsyncCall FIFO intrusive storage, making
its reuse possible (and marked one candidate with a TODO).
Ensure initClient MasterXactions have listening ports (#993)
MasterXaction creation code that forgets to supply a port for an
initClient transaction will now be rejected by the compiler.
This safety improvement hit two problems described below.
The TcpAcceptor class was incorrectly creating a MasterXaction object
for FTP DATA connections. We moved MasterXaction creation to TcpAcceptor
callbacks because only they know whether accepting a TCP connection
implies a new master transaction start (it does not for FTP DATA cases).
The Downloader class was implemented to spawn its own MasterXaction
using XactionInitiator info supplied by the download requestor. That
design is incompatible with the new static assertion because each
MasterXaction creator must now hard-code XactionInitiator value for the
assertion to work at _compile_ time. All other contexts naturally
hard-code that value.
We saw two ways to resolve this conflict:
a) Let the download requestor create the MasterXaction object for the
Downloader. The primary advantage of this (implemented) approach is
that it allows (future) client request processing code to satisfy
client requests using Downloader. Such request satisfaction requires
sharing the master transaction between the client transaction and the
downloader transaction, and this change enables such sharing. This
change moves the "Which master transaction does this download belong
to?" question from the Downloader into download requestors.
b) Move initClient-has-port enforcement (and squidPort) from
MasterXaction into XactionInitiator. The primary advantage of this
(not implemented) approach is that it places that enforcement inside
the type it is meant to police (essentially) -- XactionInitiator.
The two changes are complementary. We did not implement (b) because it
requires significant XactionInitiator redesign, moving _all_ originating
client information from MasterXaction to XactionInitiator (currently
squidPort and tcpClient).
Ensure null-termination of string in negotiate_wrapper (#1031)
Coverity identified a theoretical chance that a buffer may not be
null-terminated in negotiate_wrapper. The code flow is clean, adding a
forced null termination to apply defensive programming practices.
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.
Fix CID 1461131 Invalid type in argument to printf (#1027)
in ext_edirecory_userip, persist_timeout is defined
as a time_t, wihich doesn't fly well with printf.
Cast it to int for printing; since it is set using atoi
it is guaranteed not to overflow anyway
If fwd->serverConnection is null, _peer is read before being
initialized. We now make sure all HttpStateData data members are
initialized (using in-class member initialization).
Detected by Coverity. CID 1494356: Uninitialized pointer read (UNINIT).
Undocumented Ipc::Mem::PageId::maxPurpose is used to mark freed pages.
Freed pages do not participate in estimating future memory needs, so the
Limits array does not need to store their needs. However, some of the
needs estimation code is written without that assumption in mind because
maxPurpose is a legitimate/used purpose value (rather than an enum end
marker). This change improves this code in case our assumption change.
Alex Rousskov [Tue, 26 Apr 2022 15:11:44 +0000 (15:11 +0000)]
sslcrtd_program: Use (more) Squid-wide APIs (#1021)
This switch allows us to use Squid-native debugging and error handling
code in ssl/libsslutil.la. Such use is necessary to fix and enhance
certificate handling code currently located in that library. For now, we
only did straightforward adjustments like fixing parseBytesOptionValue()
and Ssl::CrtdMessage::parseRequest() error detection/handling. More
serious changes deserve dedicated PRs.
This change also converts the remaining bare std::runtime_error uses in
runtime Squid code and almost all std::cerr uses in sslcrtd_program:
* std::runtime_error: TextException provides the source code location of
the thrower and will be enhanced along with Squid improvements.
* std::cerr: debugs() provides much better runtime logging, on many
levels. The usage() dump still uses std::cerr because debugs()
decorations are not useful in that special case and because, IMO, that
case should be using std::cout instead (it is not reporting an error).
Removing STL APIs (that have Squid-native alternatives) from the old
helper code also reduces the temptation to use wrong APIs in new code,
especially when authors are not familiar with Squid conventions/plans.
Also replaced sslcrtd_program's `Here` hack with SourceLocation.
Also report all started helpers (at debug level 2) because a successful
helper start is a significant event worth reporting (when level-2
debugging is enabled) across all helpers.
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.
Alex Rousskov [Fri, 15 Apr 2022 18:10:49 +0000 (18:10 +0000)]
Add Assure() as a replacement for problematic Must() (#864)
The now-deprecated Must() has several interdependent flaws:
1. Must() logs at debug level 3, hiding some important bugs from humans.
2. Must() has been (ab)used for both checking code logic and validating
input, making purpose-specific implementation changes impractical.
3. Must() does not honor the standard NDEBUG macro, complicating runtime
cost assessment and surprising some developers that are used to that
standard assert() semantics.
The new Assure() is a throwing version of a POSIX assert(3):
* Meant for detecting Squid code logic bugs (not input validation).
* Informs admins about bugs by logging their info to cache.log.
* Completely disabled in NDEBUG builds.
* Kills the current component (e.g., a Squid-origin HTTP transaction).
The killed component boundary is essentially defined by the location of
the handling try/catch statement. Some Assure() failures will kill a
Squid process, but the throwing code should not worry (or even know)
about the catcher location and handling logic.
This change also optimizes the compiled Must()/Assure() caller code
size, which may help a bit with runtime performance: With the new
Assure()/Must() implementation approach, the total stripped Squid
executable size in one reasonable configuration goes down by 5%. For
comparison, removing all Must()s completely gives 6% size reduction.
Automatically replacing Must() calls with Assure() is not practical due
to the second flaw itemized above.
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.
Alex Rousskov [Tue, 12 Apr 2022 22:32:19 +0000 (22:32 +0000)]
Improve debugs() handling in helpers (#1011)
This change also reduces libdebug dependency on globals.cc, improving
libdebug reusability.
Also do not default-reset debug sections after they were explicitly set:
* In most sbin/squid contexts, DebugModule constructor is called before
Debug::parseOptions(). That call order results in the Levels array
being reset to default values before it is reset to configured values.
No problem.
* In sbin/squid -X context, DebugModule constructor is called after
Debug::parseOptions(), but the override_X flag protected the Levels
array from being reset to default values in this case. No problem.
* In helper contexts, DebugModule constructor may be called after
Debug::parseOptions(), and the override_X flag stays false. This order
results in the parseOptions() effects erased by ResetSections() called
from the constructor.
This bug was detected while trying to understand why pinger's hard-coded
(and wrong) ALL,10 default has no effect on pinger's debugging. The two
bugs cancelled each other.
The following changes affect pinger (now) and other helpers that will
eventually use libdebug and its new NameThisHelper() API:
* Label helper debugs() lines with the helper name (e.g., "pinger"),
similar to how we already label SMP debugs() lines with "kidN". This
change improves cache.log readability, distinguishes output from
different helpers, and distinguishes helper output from sbin/squid
output in non-SMP logs.
* Make sure level-1 debugs() messages are logged.
* Stop pointless accumulation of cache.log and syslog channel messages.
* Automatically honor SQUID_DEBUG environment variable set by the parent
Squid process. The pinger helper was already honoring it.
The following changes are specific to pinger:
* Removed the now-duplicated references to "pinger" in pinger debugs().
* Do not default pinger debugging levels to ALL,10. The default pinger
debugging levels should be the same as the default Squid debugging
levels (i.e. "ALL,1"). Bugs in the debugging module prevented the
hard-coded "ALL,10" (or any other elevated setting) from having an
effect, but we now fixed the last of those bugs. AFAICT, the change of
default from ALL,1 to ALL,10 in commit cc192b5 was accidental.
Alex Rousskov [Sat, 9 Apr 2022 04:42:51 +0000 (04:42 +0000)]
Remove failing-to-build and not-built-by-default ufsdump (#1013)
The program was not built by default since 2010 (see commit 2d94e2d).
Its build has been failing since before 2017 (see commit 4c2f8b7).
Judging by commit log, other ufsdump problems were discovered and often
left unaddressed throughout the years. Addressing some of them probably
requires serious work. The program source code and its dependencies have
been neglected for a long time and are a distraction. If we decide the
Project should have this tool, it should be rewritten from scratch.
The program does not support newer STORE_META_ TLVs and rock cache_dirs.
There are no signs that the program is in demand. No official bug
reports mention ufsdump since 2013. I could only find irrelevant
squid-dev references and no squid-users references at all since 2012.
Alex Rousskov [Sun, 3 Apr 2022 04:18:54 +0000 (04:18 +0000)]
comm/libminimal.la to facilitate helper use of convenience libs (#1009)
Helpers may not (want to) use fd_open() and fd_close() directly, but
they (want to) use libdebug which does use fd_open() and fd_close().
Currently, the Comm "importing" fd_open() and Comm "delisting"
fd_close() APIs are not implemented inside src/comm/ but they do belong
to the Comm module and use its internals. Moving fd.h to src/comm/ will
require a lot of noisy out-of-scope changes deserving a dedicated PR.
Despite their names, the minimally-implemented functions do not open(2)
and close(2) file descriptors in their full implementations either: That
full implementation just updates fd-associated Squid-specific metadata
that Squid helpers do not need/use.
Do not use stub_fd.cc in the deployed/non-test pinger program.
Avoid build-breaking copying of stub_fd.cc source file in test-suite/.
Alex Rousskov [Fri, 1 Apr 2022 20:14:42 +0000 (20:14 +0000)]
Honor assertions during shutdown (#1007)
From code correctness/guarantees point of view, it is much better to
assert than to exhibit undefined behavior, especially since the
asserting code is usually not shutdown-specific and the shutdown state
often lasts through hundreds of transactions.
We are not aware of any frequent assertions during shutdown, and we want
to fix the ones that do exist (instead of not knowing about them). Thus,
this change is unlikely to introduce a lot of problems and might trigger
other positive changes.
Bypassing assertion failures does not guarantee the code will keep
running: In many (most?) cases, the asserting code will still crash or
seriously misbehave. In those cases, this change clearly improves Squid.
We ignored assertion failures during shutdown since Squid started
customizing assert() in commit 54f742e. Back in 1998, Squid was known to
often crash while shutting down, the crashes were often "benign" (the
code was just mishandling disappearing modules), and Squid could not
always start after a crash, complicating startup scripts. With a focus
on adding new features, we probably felt it is best to ignore these
usually minor but often annoying failures.
This change reduces libdebug dependency on globals.cc, addressing commit 6249367 TODO, and improving libdebug reusability.
Alex Rousskov [Wed, 30 Mar 2022 15:51:20 +0000 (15:51 +0000)]
Logformat %lp expands to "-" in wildcard listening port configs (#997)
FindListeningPortAddress() and its helpers look for "local" address of
an accepted connection. When FindListeningPortAddress() is called in %la
context, we must skip Ip::Address objects with INADDR_ANY IP addresses
because we are looking for a specific IP address, not a wildcard.
However, when called in %lp context, skipping those Ip::Address objects
may, in some cases, result in skipping the only object that actually
contains the port information, resulting in %lp expanding to "-".
Similarly, zero-port Ip::Address objects (with non-any IPs) could be, in
theory, returned instead of continuing the search for an object with a
non-zero port number, although this case was not observed in tests.
Now we configure the address searching helpers with a "good Ip::Address
object" filter so that each %code code path can customize its search.
The problem was introduced in commit ea35939 that expanded INADDR_ANY
check scope from %la to %lp.
Alex Rousskov [Mon, 28 Mar 2022 15:56:24 +0000 (15:56 +0000)]
mem/libminimal.la to facilitate helper reuse of convenience libs (#1004)
Squid helpers/tools cannot reuse features like SBuf because, in part,
the corresponding convenience libraries depend on libmem.la which drags
in heavy dependencies, including Store and time-based Event modules. The
new mem/libminimal convenience library implements enough of src/mem/
APIs to make the new memory library usable by helpers/tools without
causing a dependency explosion.
Do not use tests/stub_libmem.cc in the deployed/non-test squidclient,
cachemgr.cgi, and pinger programs.
Kid restart leads to persistent queue overflows, delays/timeouts (#706)
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...").
Alex Rousskov [Fri, 25 Mar 2022 16:17:54 +0000 (16:17 +0000)]
Detach libsbuf from StatHist to facilitate SBuf reuse (#1003)
The stated purpose of sbuf/DetailedStats was to "avoid adding external
dependencies to the SBuf code". DetailedStats failed to accomplish that
because it introduced a dependency on StatHist (which depends on Store).
To break the unwanted dependency, we outsource at-destruction-time size
statistics collection to external-to-SBuf code, configurable via the new
SBufStats fields. This also allows to avoid non-trivial SBuf statistics
collection in programs that do not need that statistics.
Since the new SBufStats fields are set at cache manager configuration
time in mainInitialize(), earlier SBuf and MemBlob destructions are not
accounted for. With more code changes/complications, we could initialize
the fields much earlier, but this delay may be considered a _positive_
change because it removes unusual SBuf stats from long-term histograms.
Also do not use the SBuf STUB in the deployed/non-test pinger program.
TODO: Convert more helpers to use SBuf. This change only adjusts pinger
because that adjustment did not require other significant changes.
... assertion when preceded by the following error message:
ERROR: Inconsistent service method ... in dynamic adaptation chain
The assertion is triggered by the following chain of events. During
SslBump step1, a REQMOD adaptation service returns a dynamic
(X-Next-Services) plan containing a RESPMOD service P. Then, during
SslBump step2 (after obtaining TLS client SNI):
* Adaptation::AccessCheck::start() discovers P in the "future services"
storage (Adaptation::History::theFutureServices) and returns it.
* The adaptation routing code correctly concludes that P is not
applicable to the current vectoring point, logs the above ERROR, and
returns the untouched virgin message object to the adaptation
initiator. See thePlan.exhausted() in Adaptation::Iterator::step().
* ClientHttpRequest asserts because it expects a new message object.
Fixed Adaptation::AccessCheck code no longer assumes that it cannot be
activated twice for the same vectoring point. It leaves services
applicable to future vectoring points in theFutureServices instead of
always suggesting them for the current vectoring point.
TODO: We can and should optimize adaptation requesting code to stop
requiring a new message object when no adaptation is necessary, but that
change is difficult (we tried!) and independent from the bug fixed here.
Amos Jeffries [Mon, 28 Feb 2022 18:03:54 +0000 (18:03 +0000)]
Improve modularity of IDENT protocol code (#991)
Simple changes to further isolate the IDENT code
in ident/libident.la and redux the mainInitialize() sequence.
Ident::Start() is already checking for and triggering ident_hash
initialization. No need to do so in main.cc if there is never
any use of IDENT. Removing one unnecessary memory allocation.
Alex Rousskov [Thu, 24 Feb 2022 02:17:14 +0000 (02:17 +0000)]
Fix Sum() by replacing it with a safer NaturalSum() (#869)
While testing future cache entry expiration computation code, we found a
Sum() bug: Sum(0l, -1lu) returns -1 instead of overflowing (long cannot
hold the maximum unsigned long value). This discovery triggered an
investigation that discovered several Sum() flaws, even inside the
supposed-to-be-trivial implementation branch for unsigned operands:
0. AllUnsigned-based optimization path selection was based on the
detection of unsigned raw S and T types, but the actual s+t sum
used integral-promoted types that could be signed!
1. "sizeof(T) >= sizeof(U)" assertion missed that same-size types have
very different maximums when exactly one of the types is signed.
2. "auto sum = a + b" assignment missed that "auto" may be bigger than
the return type "T" and, hence, the sum may not overflow when it does
not fit T. The overflow would go undetected and the result will be
truncated (to fit T) in the return statement. This automatic sum type
enlargement may be due to signed->unsigned integral conversion
related to the previous bullet, but it can also happen because of
integral _promotions_ (e.g., those that convert "char" into "int").
3. Sum() silently truncated its arguments to fit T. Before commit 1fba9ab, that silent truncation only applied to the first argument
(if T did not match its actual type). After that commit, all other
arguments could be silently truncated (except the last one).
4. It is trivial for the caller to do everything right when calling
Sum() but then assign the result to a variable that cannot hold Sum()
value, essentially undoing all the overflow detection work.
Fortunately, none of these bugs affected two existing Sum() callers.
Some of these problems were very difficult to fix! Eventually, a new,
simplified concept emerged that was easier to implement and that was a
better match for Squid current and foreseeable needs: NaturalSum().
NaturalSum() is designed for cases where we want an exact sum of
arguments but do not consider negative arguments as valid numbers. This
both simplifies the implementation and protects typical callers from
adding "-1" (e.g., a special "no delay" option setting) to "3600" (e.g.,
configured TTL) and getting a meaningless result of 3599.
NaturalSum() requires the caller to specify the summation type that will
be used to accumulate the sum value as Squid iterates over arguments.
Sum() wanted to automatically use the largest type that can accommodate
(partial) sums, but that complicated task was not implemented and
becomes unnecessary when dealing with only natural numbers -- there is
no need to temporary inflate the partial sum (beyond what the resulting
type can hold) in case some negative operand will decrease it later.
Also added SetToNaturalSumOrMax() to allow the caller to reset a
variable without guessing its type, avoiding silent sum truncation at
assignment time.
Amos Jeffries [Wed, 23 Feb 2022 18:42:41 +0000 (18:42 +0000)]
Maintenance: code style updates in libcompat (#880)
The libcompat code is targeted here specifically because it is
rarely touched so is not likely to be soon updated by our
"upgrade when modified" policy for source style changes.
Doing this part of the code semi-manually also allows us
to trivially exclude the directory from automated conversion
attempts and not bother with automating the detection of
whether a .h is included in C files.
Alex Rousskov [Tue, 22 Feb 2022 21:58:02 +0000 (21:58 +0000)]
SourceLayout: Move debugs()-related code into src/debug/ (#984)
The new library is needed to facilitate debugs() reuse and, perhaps more
importantly, reuse of other Squid code that already uses debugs() -- all
without build-breaking hacks of copying debug.cc source files and very
dangerous stub use in production running code.
The Raw class, asHex(), and other I/O manipulators are not specific to
debugs() streams. They are now in src/base/.
Also removed test-suite/debug, addressing an XXX. That test was
misplaced and not really doing anything useful -- the compiler already
tests what test-suite/debug was testing (and much more).
Also removed the no longer necessary xassert() stub from test-suite/.
TODO: Adjust the moved code to use Debug namespace. Those adjustments
are likely to trigger debug/Stream.h reorganization, simplifying exposed
debugging API. They deserve a dedicated PR.
TODO: Convert more helpers to use the new debugging library. This change
only adjusts pinger because that adjustment did not require other
significant changes.
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.
Do not stop listening after "ERROR: NAT/TPROXY lookup failed..." (#837)
_Intermittent_ NAT/TPROXY lookup failures can happen for several reasons
outside Squid control. For example, flushing conntrack tables under load
is likely to trigger such errors. Squid should not stop listening if
there is a good change that the next accept(2) will be fully successful.
We already apply the same logic to several other accept problems today.
Also added an XXX that TcpAcceptor::mustStop() does not work as expected
on error.
Alex Rousskov [Fri, 11 Feb 2022 16:32:27 +0000 (16:32 +0000)]
Do not skip problematic regexes in ACLs (#979)
This change has two partially overlapping parts:
* Reject configurations with ACLs containing failed-to-compile regexes.
* Do not skip ACL regexes with BUFSIZ or more characters.
Invalid or long ACL regexes were skipped (with an ERROR message),
resulting in a dangerous mismatch between admin (mis)interpretation of
their squid.conf and the actual configuration of the running instance.
Given the volume of ERRORs busy Squids are often reporting, including
transaction errors that admins may consider irrelevant, admins are not
paying enough attention to non-fatal configuration ERRORs, especially
reconfiguration ERRORs. Squid should reject erroneous configs instead.
And until we find (and this time document!) a specific reason to
artificially restrict individual regex length, let the Squid
configuration tokenizer and the regex library limit RE lengths. KISS.
Also deliver what commit 0fa036e promised but failed to do: "Squid no
longer reports REs _optimization_ failure when it is an individual RE
that is broken (and reported as such)". In that commit, I overlooked the
fact that Squid still ignored broken REs at the time, fooling the
higher-level code into thinking that they were OK (and triggering
unnecessary reporting of misleading optimization errors).
Besides rejection of invalid REs, there are two known side effects:
1. Squid may silently start using previously skipped long REs[^1].
2. Squid may fail/succeed/change regex optimization because the
previously skipped long REs now participate in optimization attempts.
In nearly all cases, this does not affect the ACL matching outcome.
[^1]: The regex library is unlikely to reject regexes based on their
length alone because libraries ought to accept regexes much longer than
longest Squid configuration tokens. For example, glibc accepts 4MB-long
regexes on 32-bit systems AFAICT based on a quick scan of regcomp code.
David CARLIER [Wed, 9 Feb 2022 19:38:21 +0000 (19:38 +0000)]
Improve coredump_dir on FreeBSD and Solaris based OS (#974)
Disclose that coredump_dir may also make the process "traceable",
including enabling core dumps and ptrace(2) attachments in certain
environments.
Add support for making the process traceable in FreeBSD- and
Solaris-like environments.
When Squid does not know how to make the process traceable in a given
environment, it does nothing under the assumption that the process is
already traceable enough (by default) to dump cores. Alternatively, we
could warn the admin about the lack of tracing support for that
environment. It is not clear which option is better, but we are betting
on the processes being traceable by default in most not explicitly
covered environments.
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.
Also included a missing header providing IPPROTO_TCP definition.
ConnStateData::tunnelOnError() ignored its method parameter and always
called initiateTunneledRequest() with METHOD_NONE. buildFakeRequest()
then set HttpRequest::method to METHOD_NONE. Squid does not support such
HttpRequest objects well because quite a bit of code assumes that
HttpRequest::method must be known. For example, depending on
configuration and other factors, Squid may assert.
Moreover, many Squids using the on_unsupported_protocol directive also
have special rules for handling tunnels and those rules may not work as
intended for these METHOD_NONE transactions.
Squid now uses CONNECT method when it creates a CONNECT-like request
that facilitates on_unsupported_protocol tunneling. This helps meet code
expectations about HttpRequest::method being defined and natural admin
expectations about tunneled requests having a CONNECT method.
Admins that want to distinguish on_unsupported_protocol tunnels from
other tunnels can use ACL annotations (for now). If needed, one can add
a better/dedicated way of identifying on_unsupported_protocol tunnels.
Also removed the method parameter from clientTunnelOnError() and related
methods. That method was extracted from a low-level parser field and
- for cases where the higher-level code deemed input to be non-HTTP, it
was wrong to use essentially garbage/non-HTTP chars as a method name;
- for other cases, the method is available via HttpRequest::method.
TODO: We may be able to remove more duplicated parameters or unnecessary
checks on this code path: Perhaps clientReplyContext::setReplyToError()
method parameter can be retrieved from errstate->request? Perhaps
errstate->request itself is always the same as http->request?
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.
projects / thirdparty / squid.git / log
