This field can be classified as a 'value which cannot be reused across
(co-existing) Connection objects' (since it contains sensitive user
information which should not be leaked to other transactions).
ICAP: do not set connect_timeout on the established connection
Icap::Xaction code should not bother with connect_timeout at all:
this work is performed by connection establishing jobs
such as ConnOpener and Ssl::IcapPeerConnector.
Moreover, the Xaction::noteCommTimedout() handler does not expect
disconnected Connection and should not be used while connecting.
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.
Alex Rousskov [Tue, 10 Aug 2021 17:25:08 +0000 (13:25 -0400)]
Improved handling of closing-while-in-callback connections
Needs more work, but at least same-callback handlers are (more) similar
now, illustrating the kind of callback reactions the future
unified/automated API will have to support.
When sending a negative answer, we would set answer().conn to an open
connection, async-send the 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.
Probably fixed a bug in the official code where
PeerConnector::negotiate() assumed that a sslFinalized() does not return
true after callBack(). It may (e.g., when CertValidationHelper::Submit()
throws). Same for PeekingPeerConnector::checkForPeekAndSpliceMatched().
Also reduced Security::PeerConnector exposure to a closed Connection
object when serverConn is closed externally.
Alex Rousskov [Mon, 9 Aug 2021 18:33:05 +0000 (14:33 -0400)]
Improved Http::Tunneler::connection management
When sending a negative answer, we would set answer().conn to an open
connection, async-send the 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.
Also reduced Http::Tunneler exposure to a closed Connection object.
Alex Rousskov [Mon, 9 Aug 2021 02:46:31 +0000 (22:46 -0400)]
Keep Connection and other objects in sync with Comm in closure callbacks
There are lots of small bugs, inconsistencies, and other problems in
Connection closure handlers. It is not clear whether any of those
problems could result in serious runtime errors or leaks. In theory, the
rest of the code could neutralize their negative side effects. However,
even in that case, it was just a matter of time before the next bug will
bite us due to stale Connection::fd and such. These changes themselves
carry elevated risk, but I think we have to do them to get closer to a
reliable code as far as Connection maintenance is concerned; otherwise,
we will keep chasing their deadly side effects.
Long-term, all these manual efforts to keep things in sync should become
unnecessary with the introduction of appropriate Connection ownership
APIs that automatically maintain the corresponding environments (TODO).
Also marked a few newly uncovered bugs in the official code.
Alex Rousskov [Fri, 6 Aug 2021 20:20:33 +0000 (16:20 -0400)]
Improved AsyncJob::Start() API and reduced code duplication
Every JobWait::start() call was followed by AsyncJob::Start() call. The
relationship is natural. JobWait::start() should call Start() directly.
AsyncJob::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 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..." comments because the code itself
became clear enough, and those comments were becoming increasingly stale
(because they duplicated the callback names above them).
The problem is real, but there are other similarly problematic places in
the official code. If we fix this one place, then we probably should fix
all other known ones. However,
* the related exceptions ought be rare (there are no known ones);
* Squid will probably recover even if the affected Connection objects
are orphaned; and
* the appropriate fix (i.e. custom try/catch blocks) is not the right
long-term solution -- we need a better Connection storage API that
would do the right thing automatically.
Given the combination of these factors, I think it is best to do nothing
here/now but add the right long-term API in a dedicated branch.
Alex Rousskov [Fri, 6 Aug 2021 15:44:37 +0000 (11:44 -0400)]
Refactored ICAP connection-establishing code
... to delay Connection ownership until the ICAP connection is 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.
Also removed (unused except for debugging) Icap::Xaction::stopReason
that was shadowing AsyncJob::stopReason, effectively breaking debugging.
Alex Rousskov [Thu, 5 Aug 2021 21:56:12 +0000 (17:56 -0400)]
Revised Connection cloning during ConnOpener creation
Item 1 in recent branch commit 21f1082 correctly identified the
Connection sharing problem, but its solution (i.e. let ConnOpener clone
the passed Connection) was not only somewhat expensive but buggy:
ConnOpener assumed that cloneDestinationDetails() is the right cloning
method, but that method missed Connection properties like tos and nfmark
(at least). Another method -- cloneIdentDetails() would copy those, but
calling that method in non-IDENT contexts would look weird, and even
that method could have missed something (now or in the future).
The core problem here was not in the correct cloning method selection,
but that ConnOpener is not in a position to select at all! ConnOpener
should be using/forwarding all the provided Connection details, not
guessing which ones are relevant to the _caller_. After that became
clear, we had two choices:
A. ConnOpener clones everything. This is a simple, safe choice. However,
it means double-cloning: First, the caller would need to clone to
copy just the right set of Connection members (or effectively clear
the irrelevant ones). And then ConnOpener would clone again to break
the Connection ownership link with poorly implemented callers. In
some cases, that cloning was already following ResolvedPeers cloning,
resulting in triple cloning of each used destination on the common
forwarding path! Such cloning is both expensive and complicates
triage by creating long chains of Connection IDs.
B. ConnOpener clones nothing; ConnOpener creator is responsible for
supplying ConnOpener with a non-owned Connection object (that does
not need to be cloned by ConnOpener). This is a clean, efficient,
forward-looking solution but it is a bit less safe because the
compiler will not identify buggy creators that share a Connection
object they own with ConnOpener.
After weighing all these impossible-to-compare factors, we went with B.
Implementing B required adjusting several callers that were still
passing the Connections they owned to ConnOpener.
Also, after comparing the needs of existing and newly 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).
Alex Rousskov [Mon, 2 Aug 2021 01:46:49 +0000 (21:46 -0400)]
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.
Alex Rousskov [Sun, 1 Aug 2021 21:20:48 +0000 (17:20 -0400)]
Adjusted ConnOpener API to fix several problems
1. Many ConnOpener users are witten 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.
ConnOpener now clones the destination parameter, refusing to tie
ConnOpener connection to the ConnOpener creator connection. This
addresses the concern, but requires 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 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.
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.
Apart from Downloader, there are several other doneAll() methods
needing refactoring as well (e.g., ConnOpener::doneAll()). We
should refactor them all at once (probably in a separate PR).
Alex Rousskov [Fri, 16 Jul 2021 15:51:43 +0000 (11:51 -0400)]
Simplified Gopher client code
Its fake deleteThis() and swanSong() methods were not really needed,
swanSong() was doing the cleanup that the destructor should be doing
(because the _real_ swanSong() is not guaranteed to be called), and
swanSong() implementation was actually a little buggy (creating a
dangling StoreEntry pointer).
Alex Rousskov [Thu, 15 Jul 2021 21:58:50 +0000 (17:58 -0400)]
fixup: Moved JobWait classes to their own files
Technically, there may exist some code that uses JobWait but does not
make any JobCalls itself. And the new classes are big/interesting
enough to deserve their own files anyway.
Alex Rousskov [Thu, 15 Jul 2021 20:49:36 +0000 (16:49 -0400)]
fixup: Moved Job_type-agnostic JobWait code to a .cc file
It is possible to make most JobWait users template-free. I have done
that and reverted those changes because they:
* Decrease code readability: The wait member declaration no longer makes
it clear what job kind that wait member is waiting for/dedicated to.
* Decrease JobWait safety (a user may give the wrong job to the
wait.start() call)
* Require strange/ugly tricks to make sure that users of the templated
version do not call the non-templated start() which does not set the
type-aware JobWaitAndCommunicate job_ member.
A couple of other, very minor JobWait polishing touches.
Alex Rousskov [Thu, 15 Jul 2021 18:34:43 +0000 (14:34 -0400)]
Fix PeerConnector handling of last-resort callback requirements
... and simplify the corresponding code.
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 be (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.
The swanSong() method does not need to remember to cancel waits. That
will happen automatically upon imminent job destruction.
Adaptation::Icap::Xaction::noteCommClosed() does not need to remember to
cancel encryption wait because the encryptor will see the same
connection closure notification. The Xaction object should not even have
the connection closure handler while another job is negotiating TLS, but
that is one of the problems I hope we do not have to fix now.
Also added nasty XXXs. This old code needs some love, but fixing these
old problems is far from trivial and is outside this branch scope.
Alex Rousskov [Wed, 14 Jul 2021 21:45:01 +0000 (17:45 -0400)]
fixup: Do not rely on dieOnConnectionFailure() to throw (so much)
That method should probably be rewritten to avoid excessive throwing in
favor of mustStop() or a similar call, but that polishing is outside
this branch scope. Let's just attempt to minimize future code changes
by not relying on dieOnConnectionFailure() unwinding the stack.
Alex Rousskov [Wed, 14 Jul 2021 21:22:50 +0000 (17:22 -0400)]
Simplified PeerPoolMgr by removing excessive conn closure monitoring
... and possibly even fixed branch PeerPoolMgr changes.
Since commit 25b0ce4, Security::BlindPeerConnector (or, to be more
precise, its Security::PeerConnector parent) owns the connection being
secured, including informing the requestor (here, PeerPoolMgr) about
connection closures. This removal should have been done in 25b0ce4.
Removing closure monitoring here/now allows us to easily fix
branch-added callback-related closure handling that might have been racy
with respect to connection closure notification that could have arrived
_after_ we cancelled encryptionWait for other reasons, triggering a
Must() violation.
Alex Rousskov [Wed, 14 Jul 2021 20:58:10 +0000 (16:58 -0400)]
Fix Tunneler handling of last-resort callback requirements
... and simplify the corresponding code.
Events like noteAbort() and callException() terminate the job but are
not bugs that should be reported by swanSong().
Not treating (rare) cancelled callbacks specially in swanSong() allows
us to avoid making cleanup code even more complex than it already is,
undoing branch changes that introduced closeQuietly().
With JobWait API used by the callers, we no longer need to rely on some
unrelated async job call noticing the callback cancellation. We are now
guaranteed to receive a noteAbort() call.
I also removed a call to always-true parent doneAll() because no other
job, not even a parent's job can force Downloader to keep running -- if
we have notified the requestor, we are done! Squid code is not
consistent in application of that principle (yet).
Alex Rousskov [Wed, 14 Jul 2021 20:18:11 +0000 (16:18 -0400)]
fixup: Fix Downloader handling of last-resort callback requirements
... and simplify the corresponding code.
The previous branch commit missed a callException() case that stops the
job but should not be reported as a BUG related to callback maintenance
(it would be up to the callException() implementation to decide how to
report the caught exception). There might be (or there will) be other,
similar cases where the job is stopped prematurely for some non-BUG
reason beyond swanSong() knowledge.
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).
Alex Rousskov [Wed, 14 Jul 2021 19:07:06 +0000 (15:07 -0400)]
Require strict JobWait::start/finish pairing
I hesitated removing !fooWait asserts that many fooWait.start() callers
had at the beginning of their start-waiting methods. The earlier we
assert, the better, and those early asserts looked correct. However,
there is also a danger in asserting too early or for now good reason (as
the code gets refactored) and most of the code separating current
asserts and fooWait.start() calls are relatively simple job
configuration manipulations that are unlikely to significantly alter the
caller state or confuse developers during assertion triage. Keeping
these early asserts consistent across future changes would also be
difficult. Thus, I decided that it is better to remove those asserts
while relying on JobWait asserts, now built into the JobWait class.
I considered making JobWait::finish() forgiving, conditional on being in
waiting() state but succumbed to pressures to address concerns about
possible message corruption or incorrect routing decisions due to
mismatched/out-of-order callbacks.
AFAICT, the only way a caller can decide whether to call fail() or
saveError() is to somehow know that fail() should only be called for
ERR_ZERO_SIZE_OBJECT errors. That knowledge is hidden inside branch
code. Moreover, that distinction may change in the future without
saveError() callers' knowledge. In other words, today, saveError()
callers are correct, but tomorrow some of them will become wrong because
of changes in cleanupOnFail() that they do not call. Developers
modifying cleanupOnFail() can easily miss the hidden fact that some of
the saveError() callers should be kept in sync with cleanupOnFail().
Besides saving CPU cycles on that trivial ERR_ZERO_SIZE_OBJECT check, I
saw no value in this dangerous split of fail() into fail() and
saveError(). The branch commit 0523b93 message did not help me discover
that value.
However, commit 0523b93 was based on code that already had
doFail()/cleanup() methods that seem unnecessary to the current diff
reader like me. Perhaps the prior existence of those methods implies
that I am missing some other reasons behind that split...
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).
Amos Jeffries [Thu, 6 May 2021 08:38:29 +0000 (08:38 +0000)]
Fix GCC -Werror=range-loop-construct (#808)
This warning detects unnecessary object copying in C++ range loops, with
a focus on large objects and copies triggered by implicit type
conversions. Included in -Wall.
error: loop variable ... creates a copy from type ...
Alex Rousskov [Mon, 3 May 2021 21:40:14 +0000 (21:40 +0000)]
Stop processing a response if the Store entry is gone (#806)
HttpStateData::processReply() is usually called synchronously, after
checking the Store entry status, but there are other call chains.
StoreEntry::isAccepting() adds STORE_PENDING check to the ENTRY_ABORTED
check. An accepting entry is required for writing into Store. In theory,
an entry may stop accepting new writes (without being aborted) if
FwdState or another entry co-owner writes an error response due to a
timeout or some other problem that happens while we are waiting for an
I/O callback or some such.
N.B. HTTP and FTP code cannot use StoreEntry::isAccepting() directly
because their network readers may not be the ones writing into Store --
the content may go through the adaptation layer first and that layer
might complete the store entry before the entire peer response is
received. For example, imagine an adaptation service that wants to log
the whole response containing a virus but also replaces that (large)
response with a small error reply.
Use already parsed request-target URL in cache manager and
update CacheManager to Tokanizer based URL parse
Removing use of sscan() and regex string processing which have
proven to be problematic on many levels. Most particularly with
regards to tolerance of normally harmless garbage syntax in URLs
received.
Support for generic URI schemes is added possibly resolving some
issues reported with ftp:// URL and manager access via ftp_port
sockets.
Truly generic support for /squid-internal-mgr/ path prefix is
added, fixing some user confusion about its use on cache_object:
scheme URLs.
TODO: support for single-name parameters and URL #fragments
are left to future updates. As is refactoring the QueryParams
data storage to avoid SBuf data copying.
projects / thirdparty / squid.git / log
