Amos Jeffries [Sun, 16 Aug 2020 02:21:22 +0000 (02:21 +0000)]
Improve Transfer-Encoding handling (#702)
Reject messages containing Transfer-Encoding header with coding other
than chunked or identity. Squid does not support other codings.
For simplicity and security sake, also reject messages where
Transfer-Encoding contains unnecessary complex values that are
technically equivalent to "chunked" or "identity" (e.g., ",,chunked" or
"identity, chunked").
RFC 7230 formally deprecated and removed identity coding, but it is
still used by some agents.
Amos Jeffries [Fri, 14 Aug 2020 15:05:31 +0000 (15:05 +0000)]
Forbid obs-fold and bare CR whitespace in framing header fields (#701)
Header folding has been used for various attacks on HTTP proxies and
servers. RFC 7230 prohibits sending obs-fold (in any header field) and
allows the recipient to reject messages with folded headers. To reduce
the attack space, Squid now rejects messages with folded Content-Length
and Transfer-Encoding header field values. TODO: Follow RFC 7230 status
code recommendations when rejecting.
Bare CR is a CR character that is not followed by a LF character.
Similar to folding, bare CRs has been used for various attacks. HTTP
does not allow sending bare CRs in Content-Length and Transfer-Encoding
header field values. Squid now rejects messages with bare CR characters
in those framing-related field values.
When rejecting, Squid informs the admin with a level-1 WARNING such as
obs-fold seen in framing-sensitive Content-Length: ...
Ignore SMP queue responses made stale by worker restarts (#711)
When a worker restarts (for any reason), the disker-to-worker queue may
contain disker responses to I/O requests sent by the previous
incarnation of the restarted worker process (the "previous generation"
responses). Since the current response:request mapping mechanism relies
on a 32-bit integer counter, and a worker process always starts counting
from 0, there is a small chance that the restarted worker may see a
previous generation response that accidentally matches the current
generation request ID.
For writing transactions, accepting a previous generation response may
mean unlocking a cache entry too soon, making not yet written slots
available to other workers that might read wrong content. For reading
transactions, accepting a previous generation response may mean
immediately serving wrong response content (that have been already
overwritten on disk with the information that the restarted worker is
now waiting for).
To avoid these problems, each disk I/O request now stores the worker
process ID. Workers ignore responses to requests originated by a
different/mismatching worker ID.
Alex Rousskov [Mon, 10 Aug 2020 18:46:02 +0000 (18:46 +0000)]
Do not send keep-alive in 101 (Switching Protocols) responses (#709)
... because it breaks clients using websocket_client[1] library and is
redundant in our HTTP/1.1 control messages anyway.
I suspect that at least some buggy clients are confused by a multi-value
Connection field rather than the redundant keep-alive signal itself, but
let's try to follow RFC 7230 Upgrade example more closely this time and
send no keep-alive at all.
trapexit [Sun, 9 Aug 2020 06:14:51 +0000 (06:14 +0000)]
Add http_port sslflags=CONDITIONAL_AUTH (#510)
Enabling this flag removes SSL_VERIFY_FAIL_IF_NO_PEER_CERT from the
SSL_CTX_set_verify callback. Meaning a client certificate verify
occurs iff provided.
Amos Jeffries [Thu, 6 Aug 2020 22:49:34 +0000 (22:49 +0000)]
Fix: cannot stat tests/STUB.h No such file or directory (#707)
Since 2b5ebbe (PR #670), we have been seeing "random" build failures.
The tests/Stub.am generated by source-maintenance.sh has not included
the tests/STUB.h file for explicit distribution. The source file was
built and included only when seen as a dependency of the files using it.
When stubs are copied for use by the extra binaries from tools/
directory, there is a secondary side effect from "make -j":
* When the -j concurrency is small, tests/STUB.h gets copied during the
first cycle, and parallel builds compiling other copied files succeed.
* When the -j concurrency is large, tests/STUB.h may be deferred to a
later copy cycle, and the first actual compile task needing it fails
with `cannot stat 'src/tests/STUB.h': No such file or directory`.
Add tests/STUB.h to src/Makefile.am EXTRA_DIST to restore the previous
distribution behavior (when STUB_SOURCE contained it explicitly).
Update the pinger source paths that were omitted in 2b5ebbe.
Amos Jeffries [Tue, 4 Aug 2020 04:34:32 +0000 (04:34 +0000)]
Enforce token characters for field-name (#700)
RFC 7230 defines field-name as a token. Request splitting and cache
poisoning attacks have used non-token characters to fool broken HTTP
agents behind or in front of Squid for years. This change should
significantly reduce that abuse.
If we discover exceptional situations that need special treatment, the
relaxed parser can allow them on a case-by-case basis (while being extra
careful about framing-related header fields), just like we already
tolerate some header whitespace (e.g., between the response header
field-name and colon).
Do not stall while debugging a scan of an empty store_table (#699)
Non-SMP Squid and each SMP kid allocate a store_table hash. With large
caches, some allocated store_table may have millions of buckets.
Recently we discovered that it is almost impossible to debug SMP Squid
with a large but mostly empty disk cache because the disker registration
times out while store_table is being built -- the disker process is
essentially blocked on a very long debugging loop.
The code suspends the loop every 500 entries (to take care of tasks like
kid registration), but there are no pauses when scanning millions of
empty hash buckets where every bucket prints two debug lines.
Squid now does not report empty store_table buckets explicitly. When
dealing with large caches, the debugged process may still be blocked for
a few hundred milliseconds (instead of many seconds) while scanning the
entire (mostly empty) store_table. Optimizing that should be done as a
part of the complex "store search" API refactoring.
peerDigestHandleReply() was missing a premature EOF check. The existing
peerDigestFetchedEnough() cannot detect EOF because it does not have
access to receivedData.length used to indicate the EOF condition. We did
not adjust peerDigestFetchedEnough() because it is abused to check both
post-I/O state and the state after each digest processing step. The
latter invocations lack access to receivedData.length and should not
really bother with EOF anyway.
Revamped LruMap implementation, adding support for value-specific TTLs,
value-specific memory consumption estimates, memory pooling for value
wrappers, and polishing code. Renamed the result to ClpMap.
Fixed memory leak of generated fake SSL certificates in Squids
configured with dynamic_cert_mem_cache_size=0 (the default is 4MB).
Controversially changed sslcrtvalidator_program cache and TTL defaults:
* After we started accounting for key lengths in validator cache
entries, the old 2048 bytes default size became not just ridiculously
small but insufficient to cache the vast majority of cache entries
because an entry key contains a PEM certificate sent to the validator
(among other validation parameters). A PEM certificate usually
consumes at least a few kilobytes. Living the old 2KB default would
essentially mean that we are disabling caching (by default).
* And if we fix one default, it makes sense to fix the other as well.
The old 60 second TTL default felt too conservative. Most certificate
invalidations last days, not seconds. We picked one hour because that
value is still fairly conservative, used as the default by several
other Squid caches, and should allow for decent cache hit ratio.
Dropped support for the undocumented "sslcrtvalidator_program ttl=-1"
hack that meant unlimited TTL. Large TTL values should be used instead.
TODO: The option parser should be revamped to reject too-large values.
Report SMP store queues state (mgr:store_queues) (#690)
The state of two Store queues is reported: Transients notification queue
(a.k.a. Collapsed Forwarding queue) and SMP I/O request queue (used for
communication with rock diskers). Each worker and disker kid reports its
view of that kid's incoming and outgoing queues state, including a small
sample (up to 7 items) of queued messages. These kid-specific reports
are YAML-compliant.
With the exception of a field labeled "other", each queue report is
self-consistent despite accessing data shared among kids -- the reported
combination of values did exist at the snapshot collection time.
The special "other" field represents a message counter maintained by the
other side of the queue. In most cases, that field will be close to its
correct value, but, due to modifications by the other process of a
non-atomic value, it may be virtually anything. We think it is better to
report (without officially naming) this field because it may be useful
in triage regardless of these caveats. Making the counter atomic just
for these occasional reports is not worth the performance overheads.
Also fixed testStore linking problems (on some platforms) that were
caused by the wrong order of libdiskio and libipc dependencies.
Alex Rousskov [Mon, 6 Jul 2020 08:04:31 +0000 (08:04 +0000)]
Honor on_unsupported_protocol for intercepted https_port (#689)
... when Squid discovers a non-TLS client while parsing its handshake.
For https_port traffic, ConnStateData::switchToHttps() relies on start()
to set preservingClientData_ correctly, but shouldPreserveClientData(),
called by start() to set preservingClientData_, was not preserving TLS
bytes in the https_port start() context. Typical debug messages:
parseTlsHandshake: Got something other than TLS ... Cannot SslBump
tunnelOnError: may have forgotten client data; send error: 40
The failure leads to a cache miss for one of the requests. The miss
transaction will overwrite the already cached entry, possibly creating
more misses due to long write locks used by the disk cache. The problem
does not affect collapsed requests. It affects regular cache hits.
The failure happens because two requests are trying to create a
Transients entry at about the same time. Only one can succeed. With the
current ReadWriteLock-based design, some contention is unavoidable, but
the contention window was rather large/long:
* The critical section essentially started when Controller::find() (in
each request worker) checked Transients and did not find a matching
entry there. That failed lookup put each request into "create a new
Transients entry" mode.
* A worker exited that critical section when openForWriting() in
Transients::addEntry() returned (with a write lock to the
ready-to-be-filled entry for one of the two requests and with a
failure for the other).
This change reduces the contention window to the bare minimum necessary
to create and fill a Transients entry inside openOrCreateForReading().
In the micro-tests that exposed this problem, the probability of a
failure is reduced from more than 70% to less than 3%. YMMV.
Also split addEntry() into two directional methods because while they
are structured similarly they actually have almost no common code now.
This bug was caused by a4d576d. ResolvedPeers::retryPath() used
connection object pointers to find the original path location under the
incorrect assumption that the Connection pointers never change. That
assumption was wrong for persistent connections: ResolvedPeers stores a
half-baked Connection object rather than the corresponding open
Connection object that the unfortunate caller got from fwdPconnPool and
passed to ResolvedPeers::retryPath().
While working on a fix, we discovered a second reason why the pointer
comparison was a bad idea (and why a simpler fix of comparing addresses
rather than pointers was also a bad idea): The peer selection code
promises to deliver unique destinations, but we now suspect that, under
presumably rare circumstances, it may deliver duplicates. This broken
promise is now an out-of-scope XXX in PeerSelector::addSelection().
Squid now eliminates the search (and the previously documented concern
about its slow speed) by remembering Connection's position in the
destination array. The position is remembered in a smart Connection
pointer (compatible with Comm::ConnectionPointer) so that the rest of
the code (that does not really care about all this) is preserved largely
unchanged. Most changes are just renaming the pointer type.
Also freshened a FwdState call and comment made stale by 25b0ce4.
Also marked an out-of-scope problem in maybeGivePrimeItsChance() caller.
Alex Rousskov [Thu, 25 Jun 2020 09:39:45 +0000 (09:39 +0000)]
Avoid FwdState::serverConn while establishing a CONNECT tunnel (#681)
Master/v6 commit 25b0ce4 missed two cases where serverConnection() was
used prematurely: One case requires --enable-delay-pools, and the other
does not actually dereference the prematurely used serverConnection().
Both cases establish a CONNECT tunnel through a cache_peer.
DrDaveD [Tue, 23 Jun 2020 18:41:15 +0000 (18:41 +0000)]
Bug 5051: Some collapsed revalidation responses never expire (#652)
Since negative caching support was repaired in master commit 91870bf, it
has been found to last indefinitely when cache revalidation happens.
New revalidation requests were collapsing on a negatively cached
response forever because handleIMS() logic does not validate response
freshness (still assuming that the reply came in response to the current
request even though that assumption could be false since collapsed
revalidation support was added in master commit 1a210de).
Clearing the ENTRY_REQUIRES_COLLAPSING flag when hitting the negatively
cached collapsed revalidaiton response for the first time works around
this "lack of freshness check" problem. The same solution existed in the
official code for positive responses. However, this solution is partial
and unreliable because there is no guarantee that the clearing code will
be reached (and reached while the cached response is still fresh).
Also added additional partial protections against collapsing on entries
abandoned by their writers, including idle hittingRequiresCollapsing()
StoreEntry objects.
Also fixed a tiny race condition missed in master commit d1d3b4d which
addressed a much bigger (and more frequent) problem. I am not aware of
any real-world cases where this race condition surfaced, but they would
probably manifest in unwarranted failures to collapse.
Amos Jeffries [Mon, 22 Jun 2020 18:01:29 +0000 (18:01 +0000)]
Fix "make check" with GCC-10 (#677)
Latest GCC-10 release on some OS (Debian initially) no
longer locates files in "." by default. Fix Makefile hack
pulling tests/ files into tools/ for stub linking.
Also, Squid #includes under src/ are supposed to always
use path relative to src/. Fix wrong STUB.h references.
Preserve caller context in MemObject::abort callbacks (#671)
Store abused time-based event API to deliver abort notifications, losing
transaction context in the process. Further refactoring may crystallize
the currently vague/obscured "store writer" concept. Such refactoring is
outside this project scope but will benefit from these changes.
Also avoids creation of new source files, as (poorly) illustrated by the
single-byte src/tunnel.cc file created (after being removed) above.
Also improves diagnostic when dealing with misspelled file names:
$ ./scripts/formater.pl src/tunnel.ccsrc/FwdState.cc
Can not open input file: src/tunnel.ccsrc/FwdState.cc.astylebak
Can't open output file: src/tunnel.ccsrc/FwdState.cc
Reforward CONNECT after TLS handshake failure with peer (#489)
When Squid received a CONNECT request and attempted to establish a
secure connection to an SSL cache_peer, it did not try the next
available destination after a TLS negotiation failure.
Why retry such TLS negotiation failures? Peer B may not have the same
misconfiguration that led to the negotiation failure when talking to
peer A. Admins may improve fault tolerance by using pools of different
peers or pools of identical peers that are reconfigured one by one. And
FwdState already does this -- flags.connected_okay is not set until TLS
negotiation is successful.
SslBump: Support parsing GREASEd (and future) TLS handshakes (#663)
A peeking or staring Squid aborted TLS connections containing a GREASE
version in the supported_versions handshake extension (e.g., 0x3a3a).
Here is a sample cache.log error (debug_options ALL,1 83,2):
The same problem would apply to some other "unsupported" (and currently
non-existent) TLS versions (e.g., 0x0400). The growing popularity of
GREASE values exposed the bug (just like RFC 8710 was designed to do).
Squid now ignores all unsupported-by-Squid TLS versions in handshake
extensions. Squid still requires a supported TLS version for
framing-related handshake fields -- no changes there.
It is difficult to define "supported" in this context precisely because
a peeking Squid only observes the TLS handshake. Our handshake parser
may report the following versions: SSL v2.0, SSL v3.0, and TLS v1.x
(with x >= 0). This logic allows us to safely parse the handshake (no
framing errors) and continue to make version-based decisions like
disabling OpenSSL TLS v1.3 support when the agents are negotiating TLS
v1.3+ (master cd29a42). Also, access logs benefit from this "support"
going beyond TLS v1.2 even though Squid cannot bump most TLS v1.3+
handshakes.
Squid was and still is compliant with the "MUST NOT treat GREASE values
differently from any unknown value" requirement of RFC 8710.
Also added source code comments to mark other places where unsupported
(for some definition of "support") TLS values, including GREASE values
may appear.
Folks studying debugging logs (where GREASE values may appear) will
probably either recognize their easy-to-remember 0x?A?A pattern or
not really know/care about RFC 8710 and its special values.
Amos Jeffries [Wed, 10 Jun 2020 21:19:26 +0000 (21:19 +0000)]
Maintenance: Remove FIXME and \todo labels (#647)
Update old ad-hoc FIXME labels and experimental use of doxygen \todo
listings to use XXX or TODO labels for consistency sake.
Approximate current definitions (to be formalized elsewhere):
* XXX - a problem, often a serious one
* TODO - a problem, often a minor one
* BUG - a problem that is worth informing admin about
Existing TODO, XXX, and BUG labels may not fit the above definitions.
Fix clang (with its own libc++) build after 9865de7 (#661)
Compilers that check allocator/container value_type matching detected a
mismatch because multimap::value_type is _not_ std::pair<key,T> but
std::pair<const key,T>!
Symptoms (when compiling files that include src/PingData.h):
Allocator::value_type must be same type as value_type
There are better leak finding tools for most use cases now. The removed
code can be manually added back for experiments in other use cases. The
original leakfinder design required code modifications anyway.
Preserve caller context across ping timeout events (#635)
PeerSelector was losing its transaction context while handling an
event-driven timeout. Also, the old code generating HandlePingTimeout
event was problematic for two reasons:
* it used raw 'this' for cbdata-protected child class. Sooner or later,
code like this leads to "real" bugs similar to those fixed in ce9bb79, 38a9f55 / 9e64d84, and 4299f87.
* It (mis)used event API for the transaction-specific purpose.
We created a new "ping timeout service" that keeps at most one
transaction-agnostic event.h timeout at any given time while maintaining
an internal list of transactions awaiting a ping response. Similar
architecture was used for HappyConnOpener helper services.
We rejected the alternative idea of enhancing event API to support
asynchronous calls because event.cc internals were never meant to handle
thousands of concurrently pending transaction-related timeouts on busy
proxies. While it would be possible to enhance those internals as well,
timeout services offer an even better performance, are much easier to
enhance to support timeout reconfigurations, and offer better "Squid
state" debugging.
SourceLayout: Avoid including cppunit outside unit tests (#649)
compat/cppunit.h is needed on some soon-to-be-deprecated
OSes (centos-6). But it's not necessary to include it
everywhere.
Change so that it is only included in unit tests
Allow upgrading from HTTP/1.1 to other protocols (#481)
Support admin-authorized HTTP Upgrade-driven (RFC 7230 Section 6.7)
protocol switching. The new http_upgrade_request_protocols configuration
directive allows admin to control what client Upgrade offer(s) are
forwarded to the server. Squid does not automatically check whether the
server selection matches one of the client offers, but the admin can
deny unacceptable server responses using http_reply_access.
By default, Squid still does not forward any Upgrade headers,
effectively blocking an upgrade attempt.
Squid itself does not understand the protocols being upgraded to and
participates in the upgraded communication only as a dumb TCP proxy.
Detail TLS and CONNECT cache_peer negotiation failures (#518)
Before PeerConnector and Tunneler were introduced, FwdState and
TunnelStateData naturally owned their to-server connection. When CONNECT
and TLS negotiation were outsourced, we kept that ownership to minimize
changes and simplify negotiation code. That was wrong because FwdState
and TunnelStateData, as connection owners, had to monitor for connection
closures but could not distinguish basic TCP peer closures from complex
CONNECT/TLS negotiation failures that required further detailing. The
user got generic error messages instead of details known to negotiators.
Now, Ssl::PeerConnector and Http::Tunneler jobs own the connection they
work with and, hence, are responsible for monitoring it and, upon
successful negotiation, returning it to the initiators. In case of
problems, these jobs send detailed errors to the initiators instead.
Passing connection ownership to and from a helper job is difficult
because the connection may be either closed or begin to close (e.g. by
shutdown) while the callback is pending without working close handlers.
Many changes focus on keeping Connection::fd in sync with Comm.
Also improved tunnel.cc mimicking of (better) FwdState code: Partially
open connections after Comm::ConnOpener failures are now closed, and
Http::Tunneler failures are now retried.
Amos Jeffries [Thu, 21 May 2020 14:42:02 +0000 (14:42 +0000)]
Add flexible RFC 3986 URI encoder (#617)
Use AnyP::Uri namespace to self-document encoder scope and
coding type.
Use SBuf and CharacterSet for more flexible input and actions
than previous RFC 1738 encoder. Allowing callers to trivially
determine which characters are encoded.
Reduced startup time with large rock cache_dirs (#634)
... addressing an old TODO.
Before scanning the disks to find the actual entries, the old rock
cache_dir initialization code had to populate its index as if all disk
slots were available and then remove all the added entries. Those two
wasteful operations took ~1.5 seconds for a 200GB disk before the
PageStack ABA bug was fixed in a586085. With the tree-based fix, that
time increased to ~15 seconds. The delay is completely gone now,
reducing the total index initialization time (for a 200GB disk) down to
a second.
test-builds.sh: in case of error dump full log (#622)
Change behaviour of the test-builds script to
dump the full output of the current layer's log in
case of error. This will help better diagnose errors
as the root cause of an error may have exited the 20-lines
buffer we currently use
Alex Rousskov [Mon, 18 May 2020 21:42:05 +0000 (21:42 +0000)]
Fix PoolingAllocator build errors with older GCCs (#632)
error: no class template named rebind in class PoolingAllocator
GCC v4.8.4 (at least) does not fully implement C++11 Allocator-related
APIs, forcing the developer to manually provide Allocator traits that
are supposed to come automatically via std::allocator_traits.
The problem may only manifest itself when using a custom allocator with
types such as std::map<T> and std::list<T> that allocate wrapper
objects instead of Ts. For example, std::vector is unaffected.
Alex Rousskov [Wed, 13 May 2020 14:05:00 +0000 (14:05 +0000)]
Validate Content-Length value prefix (#629)
The new code detects all invalid Content-Length prefixes but the old
code was already rejecting most invalid prefixes using strtoll(). The
newly covered (and now rejected) invalid characters are
* explicit "+" sign;
* explicit "-" sign in "-0" values;
* isspace(3) characters that are not (relaxed) OWS characters.
In most deployment environments, the last set is probably empty because
the relaxed OWS set has all the POSIX/C isspace(3) characters but the
new line, and the new line is unlikely to sneak in past other checks.
Thank you, Amit Klein <amit.klein@safebreach.com>, for elevating the
importance of this 2016 TODO (added in commit a1b9ec2).
Replaced a list-based PageStack implementation with a tree-based one.
The new code uses a deterministic binary tree. Inner nodes count the
number of available IDs in their child subtrees. Leaf nodes store IDs
using bitmasks. The root node tells the pop() method whether it is going
to find a free page. The method then adjusts counters in 1-23 nodes
(depending on the tree hight) on the path to the leaf containing a page.
The push() method adds a bit to the leaf node and adjusts the counters
of all the inner nodes (1-23) on the way up to the root one. All the
adjustments are lockless. Push may also be wait-free. No ABA problems.
An alternative fix that preserved list-based implementation was
implemented but ultimately rejected: Avoiding ABA problems required
complex code, and that complexity prevented meaningful validation using
Rust's Loom. Also, dirty performance tests of outside-of-Squid code
showed unexplained significant response time growth of the list-based
implementation when concurrency levels were being increased beyond a few
threads. While these validation and performance concerns could be red
herrings, their existence decreased our confidence in the list-based
algorithm that already had a history of fooling developers.
The tree-based PageStack implementation needs 8-16x less RAM. Roughly:
* list-based: sizeof(uint32_t) * capacity or 4*capacity
* tree-based: sizeof(uint64_t) * 2 * rcapacity/64 or rcapacity/4
where rounded capacity is somewhere between capacity and 2*capacity
The absolute RAM savings are minor for most environments, but the
footprint reduction might be enough to fit a significant part of some
hot index in a modern L1 CPU cache: (e.g., a 32GB rock cache_dir may
have 16GB/16KB = 1M slot IDs = 512KB tree size).
The tree-based structure may make future support for caches with more
than 2^25 entries easier because it does not heavily rely on combining a
cache entry ID and an ABA version/nonce in a single 64-bit atomic.
TODO: While absolute tree- and list-based operation costs are all small
(under 1 microsecond), tree-based implementation cost are higher. Since
rock code pops all disk slots at startup (a known performance bug), rock
startup costs increased significantly. For example, a 200 GB disk cache
test shows ~18 seconds startup time for the tree-based implementation
versus ~4 seconds for list-based. This will be addressed by fixing that
known performance bug. The fix will not alter the tree algorithm.
TODO: The PageStack class should be renamed. Ideally, that renaming
should coincide with refactoring the PagePool/PageStack split, which is
an old XXX also currently exposes a lot of internal PageStack code.
See also: https://en.wikipedia.org/wiki/ABA_problem
gcc-8+ build error: undefined reference to __atomic_is_lock_free (#625)
Compilers warned about AC_SEARCH_LIBS(__atomic_load_8)-generated code.
Newer, stricter compilers (e.g., gcc-8), exit with an error, resulting
in AC_SEARCH_LIBS failure when determining whether libatomic is needed.
More at https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=907277#30
It is unclear whether autoconf will ever handle this case better. Let's
use a custom-made test for now. The current test refuses to build Squid
on platforms where a program using std::atomic<T>::is_lock_free() cannot
be successfully linked (either with or without libatomic) for a the
largest atomic T used by Squid (i.e. a 64 bit integer).
Linking with libatomic may be required for many reasons that we do not
fully understand, but we know that std::atomic<T>::is_lock_free() does
require linking with libatomic in some environments, even where T is an
inlineable atomic. That is why we use that as a test case.
Amos Jeffries [Sun, 10 May 2020 10:29:24 +0000 (10:29 +0000)]
Fix whitespace shell bugs in configure library detection (#628)
Whitespace around shell variable assign and append operators
results in those operations not being performed.
This caused the with_*, *_PATH and *_LIBS variables to not
be set to the correct path when a custom path is used.
Also, lack of condition on the *_CFLAGS assign can pollute
the compiler parameters with non-existent directories.
It should also have been an append like PATH and LIBS to
add the auto-detected value after any user-provided value
instead of overwriting.
Alex Rousskov [Wed, 6 May 2020 01:54:07 +0000 (01:54 +0000)]
Fixed TLS selected_version parsing and debugging (#621)
The description of the expected input was given to the wrong parsing
function. This typo may have affected parsing because it told the TLS
version tokenizer that more data may be expected for the already fully
extracted extension. I believe that the lie could affect error
diagnostic when parsing malformed input, but had no effect on handling
well-formed TLS handshakes (other than less-specific debugging).
Detected by Coverity. CID 1462621: Incorrect expression (NO_EFFECT)
Amos Jeffries [Tue, 5 May 2020 13:57:24 +0000 (13:57 +0000)]
Bug 5046: FreeBSD lacks open(2) O_DSYNC flag (#623)
ext_session_acl built with TrivialDB uses O_DSYNC to ensure
thread-safe manipulation of data within the TDB files in Squid
multi-process environment.
FreeBSD lacks this flag entirely. Use the O_SYNC flag as a
backup, which apparently provides file-level synchronization.
It is not clear whether this flag will prevent duplicate keys or
record overwrites in the case of process write race collisions.
NP: this appears to be FreeBSD specific. Other BSD either define
O_DSYNC or lack support for these POSIX flags entirely.
Add an option to test-builds.sh to emit dots
to enable tracking progress in the test without
being fully verbose
In the context of the build farm, this will save storage
and bandwidth on the build nodes, while still showing
issues if anything were to occur, and showing whether
progress is being made or a build is stuck.
Preserve caller context in commHandleWriteHelper() (#607)
This event handler resumes write operations for descriptors,
queued due to delay pool restraints. Before this fix, the enqueuing
code did not save and the dequeuing code did not restore transaction
contexts.
SslBump: Disable OpenSSL TLSv1.3 support for older TLS traffic (#588)
This change fixes stalled peeked-at during step2 connections from IE11
and FireFox v56 running on Windows 10 (at least), producing "Handshake
with SSL server failed" cache.log errors with this OpenSSL detail:
Disabling TLS v1.3 support for older TLS connections is required
because, in the affected environments, OpenSSL detects and, for some
unknown reason, blocks a "downgrade" when a server claims support for
TLS v1.3 but then accepts a TLS v1.2 connection from an older client.
Streamline ./configure handling of optional libraries (#606)
Squid macros to simplify most AC_ARG_WITH behaviour.
These macros perform the necessary checks for on/off and
custom library path validation for Squid coding policy.
Usage:
* When the parameter is for a build value rather than a
library to link. Use AC_ARG_WITH.
* When a library is experimental and should default disable.
Use SQUID_OPTIONAL_LIB to detect the --with parameter.
* When a library is expected to be available and default enable.
Use SQUID_AUTO_LIB to detect --without parameter.
Sets pkg-config variables to enable more consistent configure.ac
logic for libraries even if pkg-config is not used or supported
for any specific library.
Fixes several trivial bugs where unnecessary or duplicate
library entries were injected into CXXFLAGS, LDFLAGS and similar.
Followup logic for some libraries is adjusted to inject the
pkg-config variables to retain build behaviour for now.
Happy Eyeballs: Do not discard viable reforwarding destinations (#567)
When HappyConnOpener starts opening two connections, both destinations
are removed from the shared destinations list. As soon as one connection
(X) succeeded, the other destination (Y) was essentially forgotten. If
FwdState, after using X, decided to reforward the request, then the
request was never reforwarded to Y. We now return Y to the list.
Also abort the still-active ConnOpener job upon the termination of its
"parent" HappyConnOpener job. Without an explicit termination, those
abandoned child jobs wasted OS and Squid resources while the OS was
trying to open the requested connection. They would terminate on a
timeout or when the connection was finally opened (and discarded).
Waiting for the child ConnOpener job to end is a useful optimization,
but it remains a TODO. It requires complex accumulation avoidance logic.
Also fixed retrying via FwdState::fail() which could reorder addresses.
It incorrectly assumed that only the prime connections were retried.
Maintenance: Move sort-includes.pl to scripts/maintenance/ (#599)
We will no longer get `changed #include order` notices but will get
`changed: by maintenance/sort-includes.pl` notices, including for
whitespace-only changes.
Support plugin-style scripts for source format enforcement (#531)
Allow the source-maintenance script to run arbitrary code or sub-scripts
to perform enforcement of Squid code style and content.
Code placed in the scripts/maintenance/ sub-folder MUST meet the
following criteria:
* be self-executable,
* receive filename of the code file to be touched as one and only
command-line parameter,
* always dump the file contents to stdout (with or without edits),
* not depend on any other code in this sub-folder being run first.
Restore PURGE miss replies to be "404 Not Found", not "0 Init" (#586)
Since commit 6956579, PURGE requests resulted in invalid HTTP responses
with zero status code when Store lacked both GET and HEAD entries with
the requested URI.
Also adjusted Http::StatusLine packing code to avoid generating similar
invalid responses in the future (and to attract developer attention to
their presence in the code logic with a BUG message).
Converting std::chrono::nanoseconds::rep to double may lead to rounding
errors [-Wimplicit-int-float-conversion]. Use explicit cast to signal
that we want to ignore those errors in the max-value checking context.
Create and preserve code context in peerCountMcastPeers* events (#575)
These event handlers initiate multicast cache peer pinging and collect
multicast statistics for the peer in CachePeer::mcast. Before the fix,
both handlers ran without any context, ignoring the existing
ICP transaction context.
DrDaveD [Wed, 18 Mar 2020 17:34:45 +0000 (17:34 +0000)]
Bug 5030: Negative responses are never cached (#566)
Negative caching was blocked by checkCachable().
Since 3e98df2, Squid cached ENTRY_NEGCACHED entries in memory cache
only. Back then, storeCheckSwapable() prevented what later became
ENTRY_NEGCACHED entries from going to disk. The design was obscured by 8350fe9 that renamed storeCheckSwapable() to storeCheckCachable().
Commit 97754f5 violated that (obscured) design by adding a
checkCachable() call to StoreEntry::memoryCachable(), effectively
blocking ENTRY_NEGCACHED entries from the memory cache as well. That
call should have been added, but checkCachable() should not have denied
caching rights to ENTRY_NEGCACHED -- the corresponding check should have
been moved into StoreEntry::mayStartSwapOut().
By removing ENTRY_NEGCACHED from checkCachable(), we now allow
ENTRY_NEGCACHED entries into both memory and disk caches, subject to all
the other checks. We allow ENTRY_NEGCACHED to be cached on disk because
negative responses are fundamentally no different than positive ones:
HTTP allows caching of 4xx and 5xx responses expiring in the future.
Hopefully, the increased disk cache traffic will not be a problem.
Amos Jeffries [Mon, 16 Mar 2020 05:25:42 +0000 (05:25 +0000)]
ESI: convert parse exceptions into 500 status response (#411)
Produce a valid HTTP 500 status reply and continue operations when
ESI parser throws an exception. This will prevent incomplete ESI
responses reaching clients on server errors. Such responses might
have been cacheable and thus corrupted, albeit corrupted consistently
and at source by the reverse-proxy delivering them.
This directive controls whether Squid should perform paranoid validation
of cache entry metadata integrity every time a cache entry is hit. The
client transaction experiences a cache miss instead.
This feature does not protect from the corruption that happens while the
hit is being delivered because the validation runs once per client
request when that client request discovers a cache hit. Future versions
may add more checkpoints.
The paranoid hit validation statistics are available by means of several
new cache manager counters, available by mgr:counters, mgr:5min and
mgr:60min requests. The statistics are reported only when
paranoid_hit_validation is enabled.
Preserve caller context across tunnelDelayed*Read (#560)
tunnel.cc code approximates delay pools functionality using event.h API
that is not meant for transaction-specific events. We (temporary) add a
transaction context data member to TunnelStateData until the class
switches to using transaction-specific deferred reads API.
Amos Jeffries [Fri, 6 Mar 2020 18:40:43 +0000 (18:40 +0000)]
Cleanup: cf_gen code updates (#558)
* cf_gen: fix obsolete yoda stye comparison
* cf.data.pre: replace NOCOMMENT_START/END with
clearer CONFIG_START/END tags
* cf_gen: fix special class constructor/destructors
* Comply with the 'Big Three / Big Five' rule.
* Use C++11 initialization for class Entry member
* Remove unnecessary entries in constructor lists
Supply ALE to request_header_add/reply_header_add (#564)
Supply ALE to request_header_add and reply_header_add ACLs that need it
(e.g., external, annotate_client, and annotate_transaction ACLs). Fixes
"ACL is used in context without an ALE state" errors when external ACLs
are used in the same context (other ACLs do not yet properly disclose
that they need ALE).
Also provides HTTP reply to reply_header_add ACLs.
Ban reserved annotations in "note", "adaptation_meta" directives (#561)
Squid defines a list of names used internally for exchanging name=value
pairs with various helpers. When Squid receives a name=value pair with a
reserved name, Squid stores it as if it was any other annotation; the
information can be checked with a "note" ACL and logged with %note. An
admin who configures a custom annotation with the same reserved name may
see strange/unexpected results such as seemingly corrupted access.log
record fields and mismatching ACLs.
Squid already prohibits reserved annotation names in
annotate_transaction and annotate_client ACLs. This change adds the
missing protection to the "note" and adaptation_meta directives.
Bug 4796: comm.cc !isOpen(conn->fd) assertion when rotating logs (#474)
This long-term solution overrides the short-term fix at 2cd72a2. Now,
debug.cc correctly maintains meta information associated with its file
descriptors.
IMO, the correct place for calling _db_init() is just after locking the
PID file because we want to log ASAP, but cache.log is a common resource
that requires protection. Thus, the two old _db_init() calls were both
excessive and misplaced:
* The first call happens too early, allowing another Squid instance to
pollute cache.log with messages unrelated to the cache.log-owning
instance (e.g., a "FATAL: Squid is already running" message when
attempting to start another instance).
* The second call happens too late, missing earlier debugs() that ought
to be written into cache.log (e.g., debugs() in comm_init()).
Fixing _db_init() calls led to adjustments like moving mainSetCwd() to
be called prior to the relocated _db_init(). However, these changes
should not affect chroot-sensitive code such as UseConfigRunners().
Some early debugs() messages are no longer written into cache.log:
* Exception messages like "Squid is already running" emitted by another
Squid instance. This is an improvement: Messages about other instances
do not belong to the cache.log locked by the running instance.
* Messages (mostly errors and warning) from "finalizeConfig" callers
(e.g., "WARNING: mem-cache size is too small..."). This loss is
regrettable. Long-term, these messages should be reported after
configuration is finalized (TODO). Delayed reporting will also help
when Squid starts rejecting invalid reconfigurations.
* Messages from a few early enter_suid()/leave_suid() calls, such as
"enter_suid: PID" and "leave_suid: PID". This is an improvement: These
debugging messages pollute cache.log.
* A few early SquidMain() messages emitted between parseConfigFile() and
StartUsingConfig() (e.g., "Doing post-config initialization"). This is
an improvement: These debugging messages pollute cache.log.
Also removed outdated 'TEST_ACCESS' hack for simplicity sake.
Also marked an old XXX: Chrooted SMP master process does not db_init().
DrDaveD [Fri, 21 Feb 2020 05:12:04 +0000 (05:12 +0000)]
Bug 5022: Reconfigure kills Coordinator in SMP+ufs configurations (#556)
In these unsupported SMP+ufs configurations, depending on the deployment
specifics, the Coordinator process could exit due to swap state file
opening errors:
kid11| FATAL: UFSSwapDir::openLog: Failed to open swap log.
This patch introduces new time units of microsecond and
nanosecond precision forming a new 'time-units-small' category.
Also found and fixed several problems, related to time parameters
parsing:
* Obscure "integer overflow" fatal messages. For example, passing
"0.0001 second" caused this message. After fixing, Squid reports
that the value "is too small to be used in this context".
* Ignoring possible zero-rounded values after parsing. For example, if
a second-precision parameter was configured with 0.1 second, it
became zero after rounding, which is unexpected. It is treated
as a fatal error now.
* Inconsistent parameter overflow type. For example, parameters
with millisecond and second precision reported that 'time_msec_t'
overflowed. Now we introduce an absolute time maximum allowed,
equal to the maximum of chrono::nanoseconds type which is about
293 years. This absolute maximum allows to keep the time parsing
code simple and at the same time should satisfy any reasonable
configuration need. Note that this solution treats existing
configurations with unreasonably huge time values > 293 years
as fatal errors, such configurations should be fixed accordingly.
* Time overflows for icap_service_failure_limit parameter were not
checked at all. This is probably a result of code duplication.
By fixing the latter problem, the former one was resolved
automatically.
* Unclear fatal message if a time parameter lacked time unit. Now
Squid reports about "missing time unit".
* Improved error reporting when an inapplicable time unit was used, for
example a 'millisecond' instead of a 'second'. For the majority of
time parameters, it reported only a common "FATAL: Bungled..."
message. For url_rewrite_timeout parameter, it reported an irrelevant
"unsupported option ..." message (since it began to treat the faulty
time unit as the next option). Now in both cases it reports about the
underlying time unit problem.
While fixing these bugs I had to refactor and improve time parsing
functions, using safer std::chrono types instead of raw integer types.
Support worker-dedicated listening queues (SO_REUSEPORT) (#369)
This performance optimization has a few cons, including security
concerns, but it improves CPU core utilization/balance in many SMP
environments and is supported by many high-performance servers. Enabled
by the new `*_port worker-queues` configuration option.
Worker-dedicated listening queues reduce client-worker affinity for
requests submitted over different TCP connections. The effect of that
reduction on Squid performance depends on the environment, but many busy
SMP proxies handling modern traffic should benefit.
TODO: Linux tests show load balancing effects of SO_REUSEPORT, but
untested FreeBSD probably needs SO_REUSEPORT_LB to get those effects.
Daris A Nevil [Tue, 18 Feb 2020 14:26:06 +0000 (14:26 +0000)]
Remove 8K limit for single access.log line (#332)
The function logfilePrintf() currently limits a single line
to no more than 8K characters. It allocates an 8K buffer
on the stack, which is not safe. This PR eliminates
the 8K limit, and moves the buffer to the heap.
squidcontrib [Wed, 29 Jan 2020 06:10:04 +0000 (06:10 +0000)]
Remove pointer from the input of Digest nonce hashes (#549)
This is a follow-up to #491 (b20ce97), which hashed what was previously
revealed as plaintext. Removing the pointer from the input to the hash
removes the possibility that someone could recover a pointer by
reversing a hash. Having the pointer as input was not adding anything:
Squid remembers all outstanding nonces, so it really only requires
uniqueness, which is already guaranteed by the
authenticateDigestNonceFindNonce loop.
projects / thirdparty / squid.git / log
