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.
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 [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.
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.
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.
SslBump: Disable OpenSSL TLSv1.3 support for older TLS traffic (#620)
* 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.
This is a Measurement Factory project
* Fixed TLS selected_version parsing and debugging
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)
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.
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.
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.
Change time_units test to also work on 32bit systems (#563)
With a 64-bit maximum value, "make check" failed on 32-bit platforms
(e.g., arm7l) with:
configuration error: directive supports time values up to 2147483647
but is given 9223372036 seconds
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.
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.
huaraz [Sat, 25 Jan 2020 03:36:49 +0000 (03:36 +0000)]
kerberos_ldap_group: fix encryption type for cross realm check (#542)
Newer setups require AESxxx encryption but old Crossrealm
tickets are still using RC4. Remove the use of the cached client
ticket encryption type and use the configured default list
(which must include AESxxx and RC4).
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.
Alex Rousskov [Fri, 24 Jan 2020 03:41:38 +0000 (03:41 +0000)]
Preserve caller context across Store data delivery (#543)
StoreEntry::invokeHandlers() sends a recently loaded response data to
the waiting store_clients. During concurrent cache hits (including, but
not limited to collapsed ones), the response can be loaded into Store by
one transaction and delivered to several different transactions (i.e.
store_clients). This Store "hit sharing service" must restore the
context of the transactions it serves.
Marcos Mello [Thu, 23 Jan 2020 12:07:40 +0000 (12:07 +0000)]
Bug 5016: systemd thinks Squid is ready before Squid listens (#539)
Use systemd API to send start-up completion notification if built
with libsystemd support. New configure option --with-systemd
can be used to force enable or disable the feature (default:
auto-detect on Linux platforms).
Do not stall if xactions overwrite a recently active cache entry (#516)
After the last transaction that cached or read the reply R1 ended, its
Transients entry T1 was not freed. Subsequent requests (with different
public keys) could occupy the same shared memory and rock slots (purging
unlocked R1), preventing Squid from attaching a T1-derived StoreEntry to
the cache. Another request for R1 would receive T1 and stall because its
worker W1 kept waiting for a notification from another worker W2,
incorrectly assuming that W2 exists and is going to fetch R1 for W1.
That request was aborted after a timeout.
A Transients entry represents active transaction(s). Broadcasts stop
when there are no transactions to inform. We must remove idle (i.e.
unlocked) Transients entries to avoid feeding new transactions with
stale info. We now do that when unlocking a Transients entry and also
double check that a found unattached Transients entry has a writer.
DrDaveD [Mon, 30 Dec 2019 20:43:33 +0000 (20:43 +0000)]
Bug 4735: Truncated chunked responses cached as whole (#528)
Mark responses received without the last chunk as responses that have
bad (and, hence, unknown) message body length (i.e. ENTRY_BAD_LENGTH).
If they were being cached, such responses will be released and will stop
being shareable.
Fix server_cert_fingerprint on cert validator-reported errors (#522)
The server_cert_fingerprint ACL mismatched when sslproxy_cert_error
directive was applied to validation errors reported by the certificate
validator because the ACL could not find the server certificate.
Centralized PagePool/PageStack ID generation (#525)
Easy-to-find-in-cache.log and predictable/stable stack IDs for shared
memory pages and/or index slot numbers are very useful when debugging
cache metadata corruption issues because they allow to track related
(e.g. same-stack) operations across huge SMP logs.
Fixed prohibitively slow search for new SMP shm pages (#523)
The original Ipc::Mem::PageStack algorithm used an optimistic linear
search to locate the next free page. Measurements showed that, in
certain cases, that search could take seconds on busy caches, iterating
over millions of page index items and effectively stalling all workers
while showing 100% CPU utilization.
The new code uses a deterministic stack. It is still lock-free. The spin
loops around stack head pointer updates are expected to quit after at
most few iterations, even with a large number of workers. These loops do
not have ABA update problems. They are not spin locks.
Sergey Kirpa [Mon, 23 Dec 2019 08:01:21 +0000 (08:01 +0000)]
Smarter auth_param utf8 handling, including CP1251 support (#480)
When forwarding authentication credentials to authentication helpers:
* With auth_param utf8 parameter: Squid assumed that the received
credentials are encoded with Latin-1 (and re-encoded them with UTF-8).
This assumption is wrong for Internet Explorer running with CP1251
regional settings. Now Squid uses HTTP Accept-Language request header
to guess the received credentials encoding (Latin-1, CP1251, or UTF-8)
and converts the first two encodings into UTF-8.
* Without auth_param utf8 parameter: No changes. Squid sends credentials
in their original encoding, only applying RFC 1738 escaping on top.
Chrome and Firefox should not be affected because they always use UTF-8
encoding when sending authentication credentials.
Amos Jeffries [Sat, 21 Dec 2019 21:14:11 +0000 (21:14 +0000)]
Fix some compile errors from Windows MinGW (#71)
MinGW compiler is a bit more limited than most GCC or Clang available.
The types used on Windows for some API declarations differ from those
commonly used on Linux/BSD systems.
TODO: There are additional compile issues to resolve.
Fix the parsing of the received listing from FTP services.
Also relaxed size/filename grammar used for DOS listings: Tolerate
multiple spaces between the size and the filename.
Fix shared memory size calculation on 64-bit systems (#520)
Since commit 2253ee0, the wrong type (uint32 instead of size_t) was used
to calculate the PagePool::theLevels size. theLevels memory (positioned
by different and correct code) did not overlap with the raw pages
buffer, but the raw pages buffer could, in some cases, be 32 bits short,
placing the last 4 bytes of the last page outside of allocated memory.
In practice, shared memory allocations are page-aligned, and the
difference in 4 bytes was probably compensated by the extra allocated
bytes in most (or perhaps even all) cases.
Send HTTP/500 (Internal Server Error) when lacking peers (#490)
... instead of sending HTTP/503 (Service Unavailable) in
tunneling cases.
Tunneling code reported 503 while regular forwarding code reported 500
errors under identical "no peers to try" circumstances.
For consistency sake, we need to use one of these codes in both places.
We considered and rejected 503 because RFC7231 limits its usage to
several specific cases, not including 'lacking peers' case.
jijiwawa [Sat, 23 Nov 2019 10:24:41 +0000 (10:24 +0000)]
Bug 5008: SIGBUS in PagePool::level() with custom rock slot size (#515)
SMP Squids were crashing on arm64 due to incorrect memory alignment of
Ipc::Mem::PagePool::theLevels array. The relative position of the array
depends on the number of workers and the number of pages (influenced by
the cache capacity and slot size), so some configurations worked OK.
We have to manually align manually positioned fields inside shared
memory segments. Thankfully, C++11 provides alignment-computing APIs.
Alex Rousskov [Sat, 23 Nov 2019 09:18:24 +0000 (09:18 +0000)]
Bug 5009: Build failure with older clang libc++ (#514)
Older clang libc++ implementations correctly reject implicit usage of an
explicit (in C++11) std::map copy constructor with "chosen constructor
is explicit in copy-initialization" errors. The same code becomes legal
in C++14[1], so newer libc++ implementation allow implicit usage (even
in C++11), but there is no need for copy-initialization here at all.
Evidently, libstdc++ has never declared constructors explicit.
The bug was seen with Apple clang in Xcode 5.1.1 (roughly upstream clang
3.4) and Xcode 6.2 (roughly upstream clang 3.5), both using libc++.
Happy Eyeballs: Do not wait for already exhausted spares (#509)
ResolvedPeers::findSpareOrNextPeer() never returned the next peer. That
bug made doneWithSpares() return false when the next peer was present,
even though no spares were expected. The exact effects of this bug are
not known, but wrong false doneWithSpares() outcome may force Squid to
wait for more spare addresses that would never come, possibly slowing
down transactions.
Amos Jeffries [Mon, 18 Nov 2019 12:06:56 +0000 (01:06 +1300)]
Fix detection of sys/sysctl.h detection (#511)
Make sure we test the EUI specific headers using same flags
chosen for final build operations. This should make the
test detect the header as unavailable if the user options
would make the compiler #warning be a fatal error later.
Preserve caller context across Happy Eyeballs connection attempts (#499)
To efficiently enforce various global and local limits, Happy Eyeballs
jobs uses two stand-alone HappyOrderEnforcer services that create job
calls. Thus, they need manual adjustments to preserve job context.
If similar changes are required in many places, we may want to add a
CodeContext member to the AsyncJob itself so that callbacks can
magically restore their context without service modifications (assuming
the job was created in or somehow provided the right context before
those callbacks).
squidcontrib [Sun, 20 Oct 2019 18:59:08 +0000 (18:59 +0000)]
Hash Digest noncedata (#491)
These commits together
1. Hash the noncedata for Digest nonces before encoding,
to match the documentation.
2. Encode Digest nonces using hex, rather than base64.
Alex Rousskov [Sun, 20 Oct 2019 07:35:40 +0000 (07:35 +0000)]
Preserve caller context across DNS lookups (#496)
For now, the context is restored when communicating with _individual_
callers waiting for their DNS lookup results. Eventually, we might also
support establishing a multi-caller context during DNS answer parsing,
before individual callers are notified. That feature would most likely
require making idns_query refcount-based (a serious change). Or we could
realize that maintaining a very basic query ID-based context is enough.
Alex Rousskov [Sat, 19 Oct 2019 16:19:10 +0000 (16:19 +0000)]
Fix build after ccfbe8f (Report context of cache.log messages) (#495)
In some environments (e.g., Ubuntu 14 without libcppunit-dev), GCC
correctly complained that it does not know how to print "[unknown]",
probably because none of the other headers imported `<ostream>` before
importing base/InstanceId.h.
Alex Rousskov [Sat, 12 Oct 2019 00:40:35 +0000 (00:40 +0000)]
Re-enabled updates of stored headers on HTTP 304 responses (#485)
Commit 60ba25f disabled header updates (introduced in commit abf396e)
after we discovered critical inconsistencies in related metadata
updates. Finding a way to keep metadata consistent proved to be very
difficult. The primary challenge is the multitude of often concurrent
and semi-dependent activities associated with a single StoreEntry object
(e.g., writing an incoming miss response into RAM, caching the response,
loading a cache hit into RAM, and sending a response to the client).
Concurrent activities (re)set or use overlapping sets of 304-dependent
members, including StoreEntry "basics" (e.g. StoreEntry::swap_file_sz
and timestamp), "reply" (MemObject::reply_ including its hdr_sz member),
and "data" (MemObject::data_hdr). A data member update by one activity
affects other activities.
Splitting one StoreEntry object into two internally consistent and
"constant" StoreEntry objects (one old and one updated) does not work
well because there is no mechanism to share StoreEntry "data" storage
and invokeHandlers() call streams after the split.
To avoid crashes and response corruption due to inconsistent sizes and
offsets, all size-related data members must be based on the same entry
"version". If we update one such member, we must update all others.
Furthermore, due to copying of information into activity-local
variables/state, we cannot update anything while an activity is still
running. For example, two HTTP clients may use the same StoreEntry to
receive data, and one of them may already be in the middle of response
sending, using old response offsets/sizes, when a 304 update arrives for
the other.
With any updates of size-related StoreEntry data members ruled out, the
only remaining solution for preserving consistency is to keep all those
members constant/stale despite the 304 update! The updated size-related
info has to go somewhere else (inside the same StoreEntry object).
The updated headers are now stored in a new MemObject::updatedReply
field. The updated headers are swapped out, but the old StoreEntry is
not (and was not before these changes) associated with the new store
entry anchor. After the old StoreEntry is abandoned, new local hits will
use the updated anchors. Other worker hits will use the updated anchors
immediately, but they will create fresh StoreEntry objects.
We update freshness-related data members because the associated instant
decisions should not lead to inconsistencies, and using the latest info
is preferable. If any counter-examples are found, we may have to split
StoreEntry::timestamp/etc. fields into stale and fresh groups.
We do not update Vary-related data members. See rationale below[1].
Also removed HttpHeader::update() code that disabled needUpdate() checks
for non-CF configurations. The check is expensive but storing the
updated response is a lot more expensive so even if a small fraction of
checks prevents updates, we may improve performance. Also moved the
corresponding code into HttpReply so that most Header::update() callers
(that have nothing to do with 304 updates) do not spend time on it.
Also adjusted CheckQuickAbortIsReasonable(): The old expectlen setting
code did not check for unknown content length, relying on "-1 + hdr_sz"
to be negative only when no data has been received. We now use a more
direct (but possibly still broken for HTTP/0) test (hdr_sz <= 0) and
avoid using unknown content_length in arithmetic expressions. HTTP/0
aside, responses without Content-Length should still be aborted but now
with a correct "unknown content length" debug message.
MemObject is always constructed with an (empty) base reply. We now also
assert that MemObject always has a (possibly empty) base reply after
checking that all [indirect] replaceBaseReply() callers either
* supply a non-nil reply or
* call replaceHttpReply() with a true andStartWriting parameter, going
through an existing assert(rep) in StoreEntry::startWriting().
[1] Why exclude Vary response headers from 304 updates?
RFC 7234 Section 4.3.4 requires that Squid updates cached Vary. However,
reacting to changed variance requires either forgetting all cached
entries attached to the old Vary mark entry (bad for caching) or
re-keying all those entries using a new variance and a new Vary mark.
Re-keying requires both maintaining a list of cached Vary-controlled
entries and caching _request_ headers for every such entry!
Whether HTTP compliance is worth this complexity is debatable. Perhaps
origins should not return 304s to change variance? FWIW, Fastly folks
decided that it is _not_ worth it for them; look for the "Side note" in
https://www.smashingmagazine.com/2017/11/understanding-vary-header/
Alex Rousskov [Fri, 4 Oct 2019 14:14:09 +0000 (14:14 +0000)]
Report context of level-0/1 cache.log messages (#483)
Most runtime level-0/1 cache.log messages do not carry information
sufficient to identify the transaction that caused the problem. Admins
are forced to guess the transaction based on message timestamp and, if
they are lucky to get one, request URL. The associated triage and
mitigation delay costs are often significant and can be huge, especially
when administering busy proxies in complex deployment environments.
The focus of this change is associating level-0/1 cache.log messages
with access.log records, but the same API is also used for associating
master transactions with (sections of) debugging cache.log messages.
Since level-0/1 messages are rare, association code usually wastes
resources. This performance overhead is reduced by saving pointers to
the existing transaction information (e.g., ALE). ALE gives access to
HttpRequest and MasterXaction (where available), is available in
contexts where HttpRequest and MasterXaction do not exist, and (unlike
HttpRequest) represents the whole master transaction rather than its
(often repeated) component.
CodeContext::Current() represents the current master transaction (or a
similar "primary processing task" context). A new context is created
when the master transaction (or a similar "primary processing task")
starts. Context changes usually happen in low-level transaction-unaware
callback-calling "context switching" code such as DoSelect().
The vast majority of AsyncCalls, including AsyncCall-based callbacks,
should run in their creator's context. This association is easily
automated. The primary difficulty is in handling C-style typeless calls
that prohibit context storage and restoration automation:
* In our design, the "context switching" code is ultimately responsible
for associating the being-saved callback with the current code context
and for restoring CodeContext::Current() when calling that callback.
* An alternative design would task the higher-level callback creator and
callback recipient with saving/restoring CodeContext::Current(). That
design is inferior because there are a lot more callback creators and
recipients than "context switchers". That alternative would require a
lot more manual changes.
The code context remains unknown if the context creator is not
instrumented to set CodeContext::Current(). TODO: Instrument ICP query
listener. Check for others.
The code context gets forgotten if the context switcher dealing with
C-style callbacks does not remember and restore CodeContext::Current().
TODO: Instrument remaining DoSelect()s, event.cc, as well as DNS, ICP,
HTCP, and IPC listeners/handlers. Check for others.
This change covers epoll DoSelect(), TcpAcceptor, ConnStateData, and SMP
disk I/O (IpcIoFile). It already annotates several level-0/1 messages
and significantly improves complex debugging. The remaining
instrumentation TODOs are likely to use similar techniques.
Squid might report wrong context until all context switchers are
instrumented, but the vast majority of uninstrumented cases result in
benign loss of context knowledge rather than mis-attribution. No design
guarantees correct attribution until all C-style callbacks are gone.
TODO: Remove legacy ctx_enter()/exit() debugging that covers very little
while suffering from worse mis-attribution problems.
Also log "-" instead of the made-up method "NONE".
Craig Gowing [Thu, 3 Oct 2019 23:11:48 +0000 (23:11 +0000)]
Bug 4989: Leaking StoreEntry objects on Cache Digest rebuilds (#487)
When writing a newly generated Cache Digest to cache, Squid relied on a
cache key collision to purge the old digest entry. Since 4310f8b, the
collision resolution method -- forcePublicKey() -- leaked an idle (i.e.
lock_count=0) digest entry. If Squid still had unlocked entries lying
around, then the problem could extend to clashes unrelated to Digests.
Until 4310f8b, StoreEntry::forcePublicKey() called setPrivateKey()
before releasing the old entry. That explicit call was wasteful in many
cases, but, unbeknownst to its removal authors, it allowed release() to
destroy an idle Cache Digest entry by effectively disabling the
ENTRY_SPECIAL hack in StoreEntry::locked().
This change removes the ENTRY_SPECIAL hack in StoreEntry::locked(),
addressing an old TODO. The two ENTRY_SPECIAL creators (icons and Cache
Digests) now lock their entries to prevent their unwanted destruction.
Also explicitly release the old Cache Digest entry (instead of relying
on the implicit key collision) to avoid the unchecked assumption that
the Cache Digest key never changes.
Add GeneratingCONNECT step for the existing at_step ACL (#484)
The new step allows admins to customize CONNECT request generation using
request_header_access and request_header_replace. As detailed below,
matching the request method does not work for those purposes.
request_header_access controls what HTTP request headers Squid _sends_.
However, this directive, like most other ACL-driven Squid directives,
uses the received request, _not_ the being-formed to-be-sent request.
This can be confusing, but it is the correct design/behavior.
When Squid is about to send a CONNECT request to a cache peer, what
_received_ request should request_header_access ACLs look at? There are
several ways to answer that question:
1. The received CONNECT request (or, for intercepted TLS connections,
its faked equivalent). This is how the current CONNECT-sending code
works when establishing the tunnel for a not-yet-bumped client
connection. Problems:
1. The CONNECT request received by Squid was not meant for the cache
peer. It was meant specifically for this Squid. While it may have
info useful for request_header_access checks, it is conceptually
wrong to think of whats happening as "forwarding of that received
CONNECT to the cache peer". Unlike GET requests, the CONNECT
request that this Squid will send to the cache peer is dedicated
to the Squid-peer connection. It is generated, not forwarded.
2. That CONNECT request may have been received a long time ago, and
Squid may have forwarded many bumped GET requests since then. It
feels strange to consult such an old/"disconnected" message.
2. The received (and bumped) GET request. This is how the current
CONNECT-sending code works when establishing the tunnel for an
already bumped client connection. Problem:
1. Squid is about to send a generated CONNECT request, not to
forward a received GET request (the latter will happen later,
after the CONNECT transaction). The two requests may differ a
lot. Using a GET request when generating a CONNECT request is
confusing: "Why is my CONNECT method ACL does not match when
Squid sends a CONNECT request?!"
3. No request. Problems:
1. Some old configurations that use request-specific ACLs with
request_header_access will generate runtime "missing request"
warnings and may fail to work correctly.
2. Extra admin work is required to store request-specific
information as connection annotations that request_header_access
ACLs can still access.
3. Conceptually, there is a request that Squid is establishing this
CONNECT tunnel for. Squid will access-log that request. Hiding
that information from ACLs feels odd/wrong. And some of that info
would still be accessible to ACLs (via ALE/etc.). This hiding
does not really hide all of the details.
Our solution preserves what received request Squid is looking at. Items
1 and 2 above still apply (depending on the configuration and on the
request being processed), with their unique problems intact. Those
problems are not as bad as the problems associated with the item 3!
The at_step ACL was added for SslBump but, IIRC, we knew that it may
eventually cover other request processing steps. Generating a CONNECT
request is one of those other steps.
Fix the SQUID_CC_REQUIRE_ARGUMENT autoconf function (#478)
Inside AC_DEFUN(), autoconf replaces `$1` with the first argument of the
function. In this case, the first argument is a variable name. To get
the _value_ of that variable, one has to use `$$1`.
One known effect of this fix (in many build environments) is the
disappearance of the following annoying extra error when a build fails
for some other reason:
unrecognized command line option -Wno-deprecated-register
Set ALE::reply to the 200 (Connection established) (#476)
... thus addressing a TODO. Lack of reply may cause
"ACL is used in context without an HTTP response" errors in some
contexts. These contexts (if any) should be fixed separately.
urnHandleReply() may be called several times while copying the entry
from the store. Each time it must use the buffer length that is left
(from the previous call).
Also do not abandon a urn entry, still having clients attached.
Also allow urnHandleReply() to produce a reply if it receives a
zero-sized buffer. This may happen after the entry has been fully
stored.
Currently, knowing master transaction ID can be very helpful in triage,
especially when dealing with flash crowds on busy proxies. Upcoming
changes will also tie many current "anonymous" level-0/1 messages to
logged transactions via this ID.
FX Coudert [Wed, 11 Sep 2019 05:12:04 +0000 (05:12 +0000)]
Fix detection of OpenSSL built w/o deprecated features support (#470)
SSL_library_init() is deprecated since OpenSSL v1.1 and is absent in
OpenSSL built without deprecated features. Several distributions (e.g.
Homebrew) ship OpenSSL built without deprecated features.
RFC 7230: server MUST reject messages with BWS after field-name (#445)
Obey the RFC requirement to reject HTTP requests with whitespace
between field-name and the colon delimiter. Rejection is
critical in the presence of broken HTTP agents that mishandle
malformed messages.
Also obey requirement to always strip such whitespace from HTTP
response messages. The relaxed parser is no longer necessary for
this response change.
For now non-HTTP protocols retain the old behaviour of removal
only when using the relaxed parser.
Initial replacement of URI/URL parse method internals with
SBuf and Tokenizer based parse.
For now this parsing only handles the scheme section of
URL. With this we add the missing check for alpha character
as first in the scheme name for unknown schemes and
prohibit URL without any scheme (previously accepted).
Also polishes the documentation, URN and asterisk-form
URI parsing.
Also, adds validation of URN NID portion characters to
ensure valid authority host names are generated for
THTTP lookup URLs.
Supply ALE with HttpReply before checking http_reply_access (#398)
Before this fix, Squid warned "ALE missing HttpReply object",
because ALE::reply was initialized too late. This problem affected
both HTTP and FTP code paths.
To avoid these problems, ALE::reply is initialized early with the
available (received) response.
Also:
* Fixed logging control (1xx) responses in case these responses
are final. Before this fix, '-' was logged.
* All reply headers ('<h') were logged with a single CR separator,
instead of CRLF.
* Supply ALE for send_hit ACL. cb36505 already covered many (but not
all) similar cases.
Fix "BUG: Lost previously bumped from-Squid connection" (#460)
FwdState assumed that PeerSelector always returns the connection pinned
by previous SslBump steps. That assumption was wrong in two cases:
1. The previously pinned connection got closed.
2. PeerSelector policies prevent pinned connection reuse. For example,
connection destination is now denied by cache_peer_access rules.
PeerSelector now returns a PINNED selection even if a pinned connection
cannot or should not be used. The initiator is now fully responsible for
checking the returned connection usability, including the new
ConnStateData::pinning::peerAccessDenied flag. Unusable pinned
connection is now treated as any other fatal (for the transaction)
forwarding problem rather than an internal BUG.
The above changes do not change traffic on the wire but remove bogus
level-1 BUG messages from cache.log.
We also polished which error page is returned depending on the pinning
validation problem: ERR_ZERO_SIZE_OBJECT is returned when the validation
failed because of the peer disappearance or to-server connection
closure. Other cases use ERR_CANNOT_FORWARD. Eventually, these errors
can be detailed further to distinguish various problems. We may also
want to generalize ERR_CONFLICT_HOST and/or ERR_FORWARDING_DENIED to
make them applicable in this context.
Bug 4918: Crashes when using OpenSSL prior to v1.0.2 (#465)
The implementation of x509_get0_signature() replacement in 24b30fd was
based on OpenSSL v1.1.0 where `signature` and `sig_alg` members of
`x509_st` structure stopped being raw pointers and became structures.
The mismatch caused segfaults when using OpenSSL versions that lacked
x509_get0_signature() -- anything earlier than OpenSSL v1.0.2.
A site with both AAAA and A DNS records may only be available via one of
the IP address families; attempts to connect to that site over the other
IP family will fail, sometimes after painful timeouts. Prior to these
changes, Squid tried to connect to resolved addresses sequentially,
which often resulted in unhappy user eyeballs and aborted (by clients)
client-Squid connections, especially when the first DNS answer contained
multiple unusable IP addresses.
To reduce user-visible delays, Squid now follows the Happy Eyeballs (RFC
8305) strategy: Start opening a to-server TCP connection using IPvX and,
if that "primary" connection was not established fast enough, initiate a
parallel "spare" connection opening attempt using IPvY. As before, X is
the IP protocol family in the first/fastest DNS response received by
Squid. As more IP addresses (from each family) become known, they feed
subsequent connection opening attempts on primary and spare tracks.
No changes in peer selection. No changes in peer usage: Squid still
exhausts all paths to peer[N] before using peer[N+1] IPs, even if it
means waiting for DNS A answer for peer[N] while sitting on an AAAA
answer for peer[N+1].
Happy Eyeballs implementations must balance the desire to improve
response times (by opening as many parallel connections as fast as
possible) with the dangers of triggering DoS alarms and creating
significant traffic overheads. To control that balance, Squid never uses
more than two parallel tracks for forwarding a single request and
provides three admin-configurable parameters (with reasonable defaults).
* happy_eyeballs_connect_timeout forces spare connection establishment
track to wait a little (to give the primary track a chance to
establish a connection).
Fixed parsing of TLS messages that span multiple records (#457)
Squid fed the TLS message parser with one TLS record fragment
at a time but allowed InsufficientInput exceptions to bubble up
beyond the TLS message parsing code. If a server handshake
message spans multiple TLS records, and Squid reads all those
records together with the end of the TLS server handshake, then
the higher-level code interprets InsufficientInput as the need
for more TLS records for the record parser (rather than more
fragments for the TLS message parser). The affected transaction
would then time out or otherwise fail while waiting for those
non-existent TLS records to come from the server.
We now parse TLS messages only after accumulating all same-type
TLS records. For truncated handshakes, this may reduce the
level of information extracted by Squid in some cases, but
this approach keeps the code simple. The handshake is still
available for logging if that partial info is needed for triage.
Test case: 1000-sans.badssl.com which sends a huge server certificate.
Instead of tunneling traffic, a matching on_unsupported_protocol
"tunnel" action resulted in a Squid error response sent to the client
(or, where an error response was not possible, in a connection closure).
The following three cases were fixed:
Also, when on_unsupported_protocol was configured, Squid wasted RAM and
CPU cycles to buffer client HTTP requests beyond the point of no return
(i.e., roughly, beyond the first HTTP request on a connection or in a
tunnel), when on_unsupported_protocol settings no longer apply.
Client handshake accumulation is now driven by preservingClientData_. We
set that data member when the connection is accepted (because we may
decide to start preserving bytes right away) and reset it whenever that
decision may change, including when switching to a new protocol inside
CONNECT tunnel and confirming the expected/supported protocol by
successfully parsing its handshake.
Squid does not stop handshake preservation when on_unsupported_protocol
gets disabled during reconfiguration, but Squid will not tunnel
preserved bytes if that happens (and will not tunnel a partial handshake
if on_unsupported_protocol configuration keeps changing).
Also changed how IPv6-based certificates are generated. Their CN field
value is no longer surrounded by [square brackets]. This change was done
to improve Squid code that had to be modified to fix
on_unsupported_protocol. It affects certificate cache key so old
IPv6-based certificates will never be found (and will eventually be
purged) while new ones will be generated and cached instead. We believe
these IPv6-based certificates are rare and untrusted by browsers so the
change in their CN should not have a significant affect on users.
* The "mem-loaded all" message was printing -1 instead of the
accumulated object size. It also deserves a lower debugging level
because it happens at most once per transaction.
projects / thirdparty / squid.git / log
