Alex Rousskov [Tue, 7 Nov 2023 02:50:47 +0000 (02:50 +0000)]
Fix ipv4 and expand ipv6 ACL parameter matching (#1568)
The undocumented "ipv4", "ipv6", and "all" parameters can be used in
src, dst, and localip ACLs. This refactoring fixes "ipv4" matching and
expands "ipv6" scope as detailed further below. These parameters remain
undocumented.
Together, the fixed ipv4 parameter and the adjusted ipv6 parameter now
match any IP address known to Squid. So does the undocumented "all"
parameter (which is currently used to implement the "all" dst ACL).
Also speed up matching of ACLs containing "ipv4", "ipv6", and "all"
parameters (at the cost of adding two boolean tests to other src, dst,
and localip ACLs). This performance improvement is a side effect of the
corresponding code improvements and is not the focus of these changes.
Also improved mgr:config reporting of ACLs containing these special
parameters. They are now reported using their names (e.g., "all") rather
than IP addresses (e.g., `::/0` instead of "all").
### Fix ipv4 ACL parameter to match (any IPv4 address)
The "ipv4" parameter is supposed to match any IPv4 address, but it
matched none because an unmasked[^1] IPv4 input address was compared to
an all-0s address[^2], while any IPv4 address in Squid has some 1s due
to IPv4-to-IPv6 mapping used by Ip::Address. Those 1s could not match.
The "ipv4" parameter did match one address -- an IPv6 address `::`[^2].
This matching was possibly broken since inception (2009 commit 7764e92).
[^1]: Calling mask.setNoAddr() sets all sin6_addr bits to 1 while
calling mask.applyMask(0, AF_INET) does nothing at all.
[^2]: We did not set q->addr1. Ip::Address default constructor sets all
Ip::Address bits to 0. For ACL IP comparison purposes, that is
equivalent to an IPv6 `::` address (all sin6_addr bits are zero).
### Expand ipv6 ACL parameter scope to match all IPv6 addresses
The "ipv6" parameter was not matching many IPv6 addresses, including
some frequently seen ones like `::1` (i.e. IPv6 localhost). Now it
matches any IP address that Squid code itself considers an IPv6 address,
reducing surprises among admins that need to treat all IPv6 addresses
specially.
Squids receives IPv6 addresses from traffic, configuration, or
hard-coded constants even when ./configured with --disable-ipv6 or when
IPv6 support was automatically disabled at startup after failing IPv6
tests. To handle IPv6 correctly, such Squids must support basic IPv6
operations like recognizing an IPv6 address in a request-target or
reporting an unsolicited IPv6 DNS record. At least for now, such Squids
must also correctly parse configuration-related IPv6 addresses.
All those activities rely on various low-level operations like filling
addrinfo structure with IP address information. Since 2012 commit c5fbbc7, Ip::Address::getAddrInfo() was failing for IPv6 addresses when
Ip::EnableIpv6 was falsy. That change correctly recognized[^1] the need
for such Squids to handle IPv6, but to support basic operations, we need
to reject IPv6 addresses at a higher level and without asserting.
That high-level rejection work is ongoing, but initial attempts have
exposed difficult problems that will take time to address. For now, we
just avoid the assertion while protecting IPv6-disabled Squid from
listening on or opening connections to IPv6 addresses. Since Squid
already expects (and usually correctly handles) socket opening failures,
disabling those operations is better than failing in low-level IP
manipulation code.
The overall IPv6 posture of IPv6-disabled Squids that lack http_access
or other rules to deny IPv6 requests will change: This fix exposes more
of IPv6-disabled Squid code to IPv6 addresses. It is possible that such
exposure will make some IPv6 resources inside Squid (e.g., a previously
cached HTTP response) accessible to external requests. Squids will not
open or accept IPv6 connections but may forward requests with raw IPv6
targets to IPv4 cache_peers. Whether these and similar behavior changes
are going to be permanent is open for debate, but even if they are
temporary, they are arguably better than the corresponding assertions.
These changes do not effect IPv6-enabled Squids.
The assertion in IPv6-disabled Squid was reported by Joshua Rogers at
https://megamansec.github.io/Squid-Security-Audit/ipv6-assert.html where
it was filed as "Assertion on IPv6 Host Requests with --disable-ipv6".
Alex Rousskov [Wed, 1 Nov 2023 03:16:12 +0000 (03:16 +0000)]
Remove mem_hdr::freeDataUpto() assertion (#1562)
stmem.cc:98: "lowestOffset () <= target_offset"
The assertion is conceptually wrong: The given target_offset parameter
may have any value; that value does not have to correlate with mem_hdr
state in any way. It is freeDataUpto() job to preserve nodes at or above
the given offset and (arguably optionally) remove nodes below it, but
the assertion does not actually validate that freeDataUpdo() did that.
The old mem_hdr::freeDataUpto() assertion incorrectly assumed that,
after zero or more unneeded memory nodes were freed, the remaining
memory area never starts after the given target_offset parameter. That
assumption fails in at least two use cases, both using target_offset
values that do not belong to any existing or future mem_hdr node:
1. target_offset is points to the left of the first node. freeDataUpto()
correctly keeps all memory nodes in such calls, but then asserts. For
example, calling freeDataUpto(0) when mem_hdr has bytes [100,199)
triggers this incorrect assertion.
2. target_offset is in the gap between two nodes. For example, calling
freeDataUpto(2000) when mem_hdr contains two nodes: [0,1000) and
[3000,3003) will trigger this assertion (as happened in Bug 5309).
Such gaps are very common for HTTP 206 responses with a Content-Range
header because such responses often specify a range that does not
start with zero and create a gap after the node(s) with HTTP headers.
Bugs notwithstanding, it is unlikely that relevant calls exist today,
but they certainly could be added, especially when freeDataUpto() stops
preserving the last unused node. The current "avoid change to [some
unidentified] part of code" hoarding excuse should not last forever.
Prior to commit 122a6e3, Squid did not (frequently) assert in gap cases:
Callers first give target_offset 0 (which results in freeDataUpto()
doing nothing, keeping the header node(s)) and then they give
target_offset matching the beginning of the first body node (which
results in freeDataUpto() freeing the header nodes(s) and increasing
lowerOffset() from zero to target_offset). A bug in commit 122a6e3
lowered target_offset a bit, placing target_offset in the gap and
triggering frequent (and incorrect) assertions (Bug 5309).
Recent commit 122a6e3 left store_client::readOffset() unchanged but
should have adjusted it to match changed copyInto.offset semantics:
Starting with that commit, storeClientCopy() callers supply HTTP
response _body_ offset rather than HTTP response offset.
This bug decreased readOffset() values (by the size of stored HTTP
response headers), effectively telling Store that we are not yet done
with some of the MemObject/mem_hdr bytes. This bug could cause slightly
higher transaction memory usage because the same response bytes are
trimmed later. This bug should not have caused any assertions.
However, the old mem_hdr::freeDataUpto() code that uses readOffset() is
also broken -- the assertion in that method only "works" when
readOffset() returns values matching a memory node boundary. The smaller
values returned by buggy readOffset() triggered buggy assertions.
This minimal fix removes the recent store_client::readOffset() bug
described above. We will address old mem_hdr problems separately.
Alex Rousskov [Tue, 31 Oct 2023 11:35:02 +0000 (11:35 +0000)]
Fix infinite recursion when parsing HTTP chunks (#1553)
This change stops infinite HttpStateData recursion with at-max-capacity
inBuf. Such inBuf prevents progress in the following call chain:
* processReply()
* processReplyBody() and decodeAndWriteReplyBody()
* maybeReadVirginBody()
* maybeMakeSpaceAvailable() -- tries but fails to quit processing
* processReply()
HttpStateData::maybeMakeSpaceAvailable() no longer calls processReply(),
preventing recursion.
maybeReadVirginBody() now aborts transactions that would otherwise get
stalled due to full read buffer at its maximum capacity. This change
requires that all maybeReadVirginBody() callers do actually need more
response data to make progress. AFAICT, that (natural) invariant holds.
We moved transaction stalling check from maybeMakeSpaceAvailable() into
its previous callers. Without that move, maybeMakeSpaceAvailable() would
have to handle both abortTransaction() and delayRead() cases. Besides
increased code complexity, that would trigger some premature delayRead()
calls (at maybeReadVirginBody() time). Deciding whether to delay socket
reads is complicated, the delay mechanism is expensive, and delaying may
become unnecessary by the time the socket becomes readable, so it is
best to continue to only delayRead() at readReply() time, when there is
no other choice left.
maybeReadVirginBody() mishandled cases where progress was possible, but
not _immediately_ -- it did nothing in those cases, probably stalling
transactions when maybeMakeSpaceAvailable() returned false but did not
call processReply(). This is now fixed: maybeReadVirginBody() now starts
waiting for the socket to be ready for reading in those cases,
effectively passing control to readReply() that handles them.
maybeReadVirginBody() prematurely grew buffer for future socket reads.
As a (positive) side effect of the above refactoring, we now delay
buffer growth until the actual read(2) time, which is best for
performance. Most likely, this premature buffer growth was an accident:
maybeReadVirginBody() correctly called maybeMakeSpaceAvailable() with
doGrow set to false. However, maybeMakeSpaceAvailable() misinterpreted
doGrow as a "do not actually do it" parameter. That bug is now gone.
This recursion bug was discovered and detailed by Joshua Rogers at
https://megamansec.github.io/Squid-Security-Audit/
where it was filed as "Chunked Encoding Stack Overflow".
Alex Rousskov [Fri, 27 Oct 2023 21:27:20 +0000 (21:27 +0000)]
Exit without asserting when helper process startup fails (#1543)
... to dup() after fork() and before execvp().
Assertions are for handling program logic errors. Helper initialization
code already handled system call errors correctly (i.e. by exiting the
newly created helper process with an error), except for a couple of
assert()s that could be triggered by dup(2) failures.
This bug was discovered and detailed by Joshua Rogers at
https://megamansec.github.io/Squid-Security-Audit/ipc-assert.html
where it was filed as 'Assertion in Squid "Helper" Process Creator'.
Alex Rousskov [Wed, 25 Oct 2023 19:41:45 +0000 (19:41 +0000)]
RFC 1123: Fix date parsing (#1538)
The bug was discovered and detailed by Joshua Rogers at
https://megamansec.github.io/Squid-Security-Audit/datetime-overflow.html
where it was filed as "1-Byte Buffer OverRead in RFC 1123 date/time
Handling".
Alex Rousskov [Wed, 25 Oct 2023 11:47:19 +0000 (11:47 +0000)]
Improve handling of expanding HTTP header values (#1536)
Squid manipulations often increase HTTP header value length compared to
the corresponding raw value received by Squid. Raw header length is
checked against request_header_max_size and reply_header_max_size that
default to 64KB, making the raw value safe to store in a String object
(by default). However, when the increased length of a manipulated value
exceeds String class limits, Squid leaks memory, asserts, or possibly
stalls affected transactions. The long-term fix for this problem is a
complete String elimination from Squid sources, but that takes time.
Known manipulations may effectively concatenate headers and/or increase
header value length by 50%. This workaround makes such known increases
safe by essentially tripling String class limits:
(64KB + 64KB) * 150% = 3 * 64KB
This bug was discovered and detailed by Joshua Rogers at
https://megamansec.github.io/Squid-Security-Audit/response-memleaks.html
where it was filed as "Memory Leak in HTTP Response Parsing".
Since recent commit a4e35bd removed cache_object support, popular
clients like wget and curl can do everything squidclient can, with one
exception: They cannot expand `mgr:foo` macros into
`http://localhost:3128/squid-internal-mgr/foo` URLs. That single feature
is easily emulated and not worth keeping (fairly heavy) squidclient for,
especially since recent Squid security improvements often require
customizing squidclient commands with more and more options (to address
problems tracked in Squid bug 5283).
Alex Rousskov [Fri, 20 Oct 2023 22:24:45 +0000 (22:24 +0000)]
negotiate_wrapper_auth: protect from responses over 64KB (#1530)
... received from NTLM and Kerberos helpers.
This code uses MAX_AUTHTOKEN_LEN (~64KB) buffers to read response lines.
fgets(3) guarantees to terminate the supplied buffer, but it does not
return nil when the input line is larger than the buffer. We have
already detected such "Oversized message" cases for fgets(stdin) calls,
but not for fgets(FDNOUT) and fgets(FDKOUT) calls.
Alex Rousskov [Thu, 19 Oct 2023 02:34:28 +0000 (02:34 +0000)]
Fix "overriding recipe" gmake warnings in src/html/Makefile.am (#1528)
Makefile:1254: warning: overriding recipe for target 'testHeaders'
Makefile:1233: warning: ignoring old recipe for target 'testHeaders'
Makefile:1257: warning: overriding recipe for target '.h.hdrtest'
Makefile:1236: warning: ignoring old recipe for target '.h.hdrtest'
Makefile.am included a second TestHeaders.am since inception in recent
commit 1d0bc8e: Common.am already includes TestHeaders.am.
Andreas Weigel [Wed, 18 Oct 2023 04:14:31 +0000 (04:14 +0000)]
Fix validation of certificates with CN=* (#1523)
The bug was discovered and detailed by Joshua Rogers at
https://megamansec.github.io/Squid-Security-Audit/
where it was filed as "Buffer UnderRead in SSL CN Parsing".
Alex Bason [Sun, 15 Oct 2023 13:04:47 +0000 (13:04 +0000)]
Fix stack buffer overflow when parsing Digest Authorization (#1517)
The bug was discovered and detailed by Joshua Rogers at
https://megamansec.github.io/Squid-Security-Audit/digest-overflow.html
where it was filed as "Stack Buffer Overflow in Digest Authentication".
Recognize internal requests created by adaptation/redirection (#1504)
Before this fix, Squid set flags.internal for virgin requests but not
for adapted/redirected requests, leaving post-adaptation request
processing code in an inconsistent state, forwarding the
internalCheck()-compliant adapted/redirected requests as regular
requests, triggering forwarding loops and complicating code refactoring.
Martin Grimm [Mon, 9 Oct 2023 17:10:43 +0000 (17:10 +0000)]
Rewrite SplayNode to eliminate recursive calls (#1431)
Recursive method calls in SplayNode can lead to a stack overflow with
large (degenerate) trees, e.g. after creating a large dst acl from a
sorted ip list.
A "make check" testHeaders target is supposed to check that C/C++ header
files can be compiled "autonomously" (i.e. without any other code except
the required squid.h). Since 2010 commit a0fdc9b, existing testHeaders
were not detecting problems in any headers due to a use of a shell
heredoc with echo, a command that does not read from standard input.
This fix ensures that all C/C++ header files used by "make" are tested
in the corresponding "make check".
This change also improves parallel testing of individual header files.
Avoid truncation errors when printing time_t-based squid.conf directives
on platforms with 32-bit int and 64-bit time_t. Also avoid similar
errors when printing time_msec-based directives on platforms with 32-bit
int.
Detected by Coverity. CID 1529622: Use of 32-bit time_t (Y2K38_SAFETY).
Do not use raw pointers to index userhash CachePeers (#1496)
Simplified and improved code safety by using CbcPointers for userhash
cache_peers, as we have done for CARP peers in recent commit e7959b5.
Also fixed mgr:userehash Cache Manager reports to detail relevant
cache_peers instead of all cache_peers. This problem existed since
inception (2008 commit f7e1d9c) as detailed in recent commit e7959b5.
Y2038: Use time_t for commSetConnTimeout() timeout parameter (#1492)
Change commSetConnTimeout() "timeout" parameter from int to time_t, to
match the common caller type and improve Year 2038-safety on systems
with 32-bit int.
Detected by Coverity. CID 1545129: Use of 32-bit time_t (Y2K38_SAFETY).
Kill helpers that speak without being spoken to (#1488)
ERROR: helperHandleRead: unexpected read from ...
ERROR: helperStatefulHandleRead: unexpected read from ...
Squid ignored bytes received from both stateful and stateless helper
processes that had no outstanding helper requests at the time of
read(2). In stateful helpers, the implementation also resulted in
undefined behavior: Calling std::list::front() with an empty list.
Ignoring these "early" bytes also complicates code improvements.
Detecting early bytes cannot be done reliably because Squid cannot know
whether some early bytes were sent just before Squid created a helper
request and, hence, could be mistaken for a helper response to that
request. Incorrectly mapping helper responses could lead to serious
problems. When Squid is lucky to detect a buggy helper that sends early
bytes, the safest and simplest action is to kill the helper process.
At high levels of build parallelism on systems using GNU coreutils,
sometimes "make check" hangs on a request to confirm
removal of a read-only file temporary file.
Force-remove temporary test files to ensure removal is noniteractive.
Do not use raw pointers to index sourcehash CachePeers (#1474)
Simplified and improved code safety by using CbcPointers for sourcehash
cache_peers, as we have done for CARP peers in recent commit e7959b5.
Also fixed mgr:sourcehash Cache Manager reports to detail relevant
cache_peers instead of all cache_peers. This problem existed since
inception (2008 commit f7e1d9c) as detailed in recent commit e7959b5.
Also applied the new "no new globals" policy to CARP peering code, to
keep improved CARP and sourcehash peering code in sync.
Bug 5301: cachemgr.cgi not showing new manager interface URLs (#1479)
Also fix several related UI issues uncovered during testing:
* Prune the list of servers accessible via the CGI tool login.
Their responses would be badly mangled if accessed via
the old tools parse logic.
Also, hide the old login form if all servers use the new
manager interface.
* Ensure the 'menu' report is always used by default after
the CGI tool login. This prevents errors about MGR_INDEX
not being available on recent Squid releases. Restoring the
expected CGI tool behavior.
Extend cache_log_message to problematic from-helper annotations (#1481)
WARNING: Unsupported or unexpected from-helper annotation
with a name reserved for Squid use
The above message is emitted for every helper response containing
problematic annotations. Let admins control this reporting using
cache_log_message directive and message id 69.
This bug is specific to "half_closed_clients on" configurations.
assertion failed: ... "isOpen(fd) && !commHasHalfClosedMonitor(fd)"
location: comm.cc:1583 in commStartHalfClosedMonitor()
Squid asserts because Server schedules comm_read() after receiving EOF:
That extra read results in another EOF notification, and an attempt to
start monitoring an already monitored half-closed connection.
Upon detecting a potentially half-closed connection,
Server::doClientRead() should clear flags.readMore to prevent Server
from scheduling another comm_read(), but it does not and cannot do that
(without significant refactoring) because
* Server does not have access to flags.readMore
* flags.readMore hack is used for more than just "read more"
We worked around the above limitation by re-detecting half-closed
conditions and clearing flags.readMore after clientParseRequests(). That
fixed the bug but further increased poor code duplication across
ConnStateData::afterClientRead() and ConnStateData::kick() methods. We
then refactored by merging and moving that duplicated code into
clientParseRequests() and renamed that method to make backports safer.
Alex Rousskov [Sat, 9 Sep 2023 03:01:52 +0000 (03:01 +0000)]
Log %err_code for ERR_RELAY_REMOTE transactions (#1472)
For ERR_RELAY_REMOTE transactions, Squid was logging %err_code as "-"
because BuildHttpReply() was not updating HttpRequest::error or ALE.
That update was missing because the pre-computed response in those
transactions triggered a premature exit from BuildHttpReply().
BuildHttpReply() should not be updating errors at all, but significant
code refactoring required to fix that problem needs a dedicated change.
Also enabled regression testing for the fixed bug and the bug fixed in
recent commit ea3f56e.
Alex Rousskov [Fri, 8 Sep 2023 06:05:36 +0000 (06:05 +0000)]
Restore errno in %err_detail for ERR_CONNECT_FAIL (#1368)
Squid was sometimes logging %err_code/%err_detail as
ERR_CONNECT_FAIL/WITH_SERVER. It now logs
ERR_CONNECT_FAIL/WITH_SERVER+errno=111 (or similar).
When dealing with two error details, Squid was ignoring the latter one.
The new ErrorDetails code combines multiple details, reusing the
existing Security::ErrorDetail::brief() "a+b+c" syntax.
The new detail accumulation functionality may help detail other errors.
At least some of the logged errno details were lost in commit ba3fe8d.
Do not use invasive lists to store CachePeers (#1424)
Using invasive lists for CachePeer objects gives no advantages, but
requires maintaining non-standard error-prone code that leads to
excessive locking and associated memory overheads.
Also fixed a neighbors_init() bug that resulted in accessing an already
deleted "looks like this host" CachePeer object.
Amos Jeffries [Thu, 31 Aug 2023 14:09:50 +0000 (14:09 +0000)]
Allow creation of RefCountable objects via Make() (#1458)
Compared to calling new() directly, using a Pointer-returning Make()
reduces memory leaks related to freshly allocated heap objects. Perfect
forwarding of constructor parameters through Make() minimizes overhead.
const auto foo1 = new Foo(...); // XXX: leak-prone
const auto foo2 = Foo::Pointer::Make(...); // OK
Future changes will prohibit RefCountable object creation via direct
new() calls and require using Make() or an equivalent safety API.
Alex Rousskov [Wed, 16 Aug 2023 17:53:41 +0000 (17:53 +0000)]
Cover OnTerminate() calls unrelated to exception handling (#1430)
The C++ standard lists many reasons[^1] for calling std::terminate().
All of them deal with an "exception handling failure". However, when
Squid bugs lead to an undefined behavior (e.g., by calling a pure
virtual function), some compilers also call std::terminate(). In those
cases, there may be no active exception, and our std::terminate()
handler's report about an "exception handling failure" (with "no active
exception") was confusing and misleading.
Also do not describe an exception when there is none. Callers that know
that the exception exists may continue to use CurrentException(),
especially if they do not need to report that exception on a dedicated
debugs() line. Other callers, including OnTerminate(), should use the
new CurrentExceptionExtra() manipulator.
Keep ::helper objects alive while in use by helper_servers (#1389)
Squid creates ::helper objects to manage a given configured helper. For
each ::helper object, Squid may create one or more helper_server objects
to manage communication with individual helper processes. A
helper_server object uses its so called "parent" ::helper object to
access configuration and for helper process death notification purposes.
There are no checks that the "parent" object is still alive when used.
The same problem applies to statefulhelper and helper_stateful_server.
helper_server code evidently attempted to extend their "parent" lifetime
using cbdata, but that does not work, especially in C++ world where the
object is destructed (and, hence, enters an undefined state) even though
its top-level memory is preserved by cbdata. Non-trivial members like
helper::queue (std::queue) and statefulhelper::reservations
(std::unordered_map) release their internal memory when destructed.
We now refcount ::helper objects to keep them alive until the last
object using them is gone. This does not result in reference loops
because the ::helper object uses raw dlink_node pointers to store its
helper_servers.
The following helpers (listed here by their container names) were
destructed while possibly still in use by their helper_server objects:
external_acl::theHelper, Ssl::CertValidationHelper::ssl_crt_validator,
Ssl::Helper::ssl_crtd. The following helpers are not destructed during
reconfiguration: redirectors, storeIds, basicauthenticators,
ntlmauthenticators, negotiateauthenticators, and digestauthenticators
(even though casual reading of relevant code may suggest otherwise).
This bug fix does not address or mark many remaining helper bugs.
We cannot test cbdata validity of CachePeer pointers stored in
Config.peers because Config.peers own CachePeer objects. By definition,
owners should not lock their objects and do not need to test object
validity: Owners determine the lifetime of those objects! Naturally,
unlocked objects must not be tested for validity by others because to
test an object validity one has to have a lock that preserves object
metadata even after the object is invalidated (by its owner).
Alex Rousskov [Sat, 12 Aug 2023 15:40:12 +0000 (15:40 +0000)]
Bug 5294: ERR_CANNOT_FORWARD returned instead of ERR_DNS_FAIL (#1453)
Since 2017 commit fd9c47d, peer selection code stopped reporting
ERR_DNS_FAIL cases because PeerSelector::noteIps() treated DNS answers
without IP addresses as if at least one IP address was received. Without
seeing a DNS resolution error, the ultimate recipient of the DNS
resolution results (e.g., CONNECT tunneling or regular forwarding code)
used ERR_CANNOT_FORWARD to indicate a failure to find a forwarding path.
PeerSelector::noteIps() code mimicked legacy IPH code with regard to
handling of the addresses parameter. However, IPH caller had a special
emptyIsNil adjustment that was missing from the noteIps() call! We now
apply that adjustment to both noteIps() and IPH code paths.
Long-term, we should probably remove nil address container pointers.
Having two different ways to signal lack of IPs is dangerous. Currently,
there is only one known supplier of nil address container:
IpcacheStats.invalid code that validates ipcache_nbgethostbyname() name
parameter. Either the corresponding nil/empty name check should be
converted into an assertion (blaming the ipcache_nbgethostbyname()
caller for not validating the name) OR that checking code should supply
an empty address container to finalCallback().
Shmaya [Fri, 11 Aug 2023 17:41:30 +0000 (17:41 +0000)]
Bug 4981: Work around in-call job invalidation bugs (#1428)
Bug 4981 is one known case of such invalidation, but this workaround is
much broader than that bug context. We can speculate that architectural
problems described in commit e3b6f15 are behind (some of) these bugs.
Alex Rousskov [Tue, 8 Aug 2023 13:36:09 +0000 (13:36 +0000)]
Replaced ACLStrategised, enabling other ACL improvements (#1392)
The ACLStrategised class stands in the way of several ACL bug fixes and
improvements because its design forces us to place ACL-like match()
methods inside non-ACL classes, creating two parallel matching
hierarchies: one rooted in the ACL class and one rooted in the Strategy
template. The two APIs have similar methods, but Strategy-based objects
lie outside the ACL hierarchy and cannot be treated as ACL objects.
ACLStrategised pairs an ACL-like matching algorithm (a Strategy-derived
class) with an ACLData-derived class. The need to combine the two is
genuine, but the same combination is best supported without creating a
parallel hierarchy of Strategy classes. The new ACL-derived
ParameterizedNode base class accomplishes that, addressing the old
ACLStrategised design XXX.
Strategy-derived classes were not pooled at all! With these changes, all
formerly ACLStrategised classes get individual memory pools, typically
one per acltype, with proper acltype-based naming in mgr:mem reports. No
other functionality changes intended.
Do not use raw pointers to index CARP CachePeers (#1381)
Simplified and improved code safety by using CbcPointers instead.
Also fixed mgr:carp Cache Manager reports to detail relevant
cache_peers instead of all cache_peers. When mgr:carp report was added
in 2000 commit 8ee9b49, Squid did not index (or even distinguish!)
CARP cache_peers, and the reporting loop naturally iterated through all
cache_peers. 2002 commit b399543 added identification and indexing of
CARP peers but forgot to adjust the reporting loop.
Very similar changes will be applied to userhash and sourcehash
cache_peers. 2008 commit 63104e2 simply copied problematic CARP code to
add userhash and sourcehash cache_peer support. This change adds a few
reusable types with those upcoming improvements in mind.
Alex Rousskov [Wed, 2 Aug 2023 05:00:49 +0000 (05:00 +0000)]
CI: More HTTP caching and revalidation tests (#1440)
Use Daft cache-response test to monitor for bugs in basic caching code,
including bugs like the one fixed by commit c203754. Unlike the old
proxy-collapsed-forwarding test that uses concurrent requests, this test
varies basic response properties.
Use Daft accumulate-headers-after-304 test to monitor for bugs like the
one fixed by commit 55e1c6e. Unlike old proxy-update-headers-after-304,
this test focuses on certain _problematic_ HTTP 304 header updates.
Due to poor code duplication, commit 92a5adb accidentally classified
URLs without a trailing slash in the magical prefix as valid cache
manager URLs, triggering the above ERRORs. We were denying such
"slashless" cache manager URLs (as invalid internal URLs) prior to that
commit. Since that commit, the ERRORs triggered by that commit
effectively denied them as well. Denying them properly results in
simpler/smaller code (than allowing them would), so we should avoid a UI
change and continue to deny them, at least for now.
This change also reduces duplication of magic prefix definitions. Other
pending work will completely eliminate that duplication in src/ code.
Alex Rousskov [Sun, 30 Jul 2023 02:44:04 +0000 (02:44 +0000)]
Remove dead "String debugging" code (#1436)
The corresponding src/String.cc code does not build since 2013 commit 5082718 (at least): That commit removed StringRegistry class declaration
the debugging code relied on. Since inception, the code could not be
enabled using ./configure options or make flags -- one had to modify
Squid sources to alter hard-coded `#define DEBUGSTRINGS 0` setting.
Alex Rousskov [Sat, 29 Jul 2023 17:44:59 +0000 (17:44 +0000)]
Bug 5290: pure virtual call in Ftp::Client constructor (#1429)
FATAL: Dying from an exception handling failure;
exception: [no active exception]
Converting `this` to CbcPointer in a constructor of an abstract class
like Ftp::Client does not work because our virtual toCbdata() method
remains pure until the final/child class constructor runs.
Conceptually, the bug was probably introduced in 2013 commit 434a79b,
when FTP class hierarchy grew, making Ftp::Client an abstract class, but
the trigger was recent commit 337b9aa that removed CBDATA_CLASS() from
Ftp::Client class declaration. We discovered, described, and addressed
several such bugs in that commit, but we missed this case.
Alex Rousskov [Sun, 23 Jul 2023 01:38:19 +0000 (01:38 +0000)]
Fix memory leak when reconfiguring multiline all-of ACLs (#1425)
Normally, Acl::InnerNode::add() automatically registers stored ACL nodes
for future cleanup, but when we find the second all-of rule/line (with
the same ACL name), we do not add() the newly created OrNode and have to
explicitly register it to avoid memory leaks on reconfiguration.
Leaking since all-of ACL support was added in 2013 commit 6f58d7d.
Alex Rousskov [Sat, 22 Jul 2023 22:30:39 +0000 (22:30 +0000)]
Allow unit tests to customize their initialization code (#1423)
CppUnit provides TestFixture::setUp() and tearDown() methods, but those
methods are executed before and after _each_ test case. Many single
TestFixture-derived classes register many test cases that need to run
some test program setup/initialization code just once. Counting past
setUp() calls would increase noise and likely introduce bugs.
Some of our test cases may actually need before/after cleanup -- the
functionality already provided by TestFixture::setUp()/tearDown()
methods. Moreover, when multiple TestFixture-derived classes share the
same test executable, their collections of test cases are executed in
essentially random order (see commit 27685ef). In those use cases,
insuring one-time initialization via setUp() hacks would be especially
awkward and error-prone.
With this solution, a test program with custom needs just needs to
define a TestProgram-derived class that implements its one-time startup
logic. The same TestProgram class hierarchy might also prove useful in
future custom test execution adjustments.
Also migrated away from the "include main()" unit test design to avoid
adding risky hacks that register custom TestProgram-derived classes and
to reduce "magic" in test code (while following our style guidelines).
Every test programs now declares its own (trivial) main() rather than
getting it from include/unitTestMain.h.
projects / thirdparty / squid.git / log
