Alex Rousskov [Wed, 10 Jul 2024 10:30:33 +0000 (10:30 +0000)]
Recover after failing to open a TCP connection to DNS server (#1861)
ERROR: Failed to connect to nameserver 127.0.0.1 using TCP.
After failing to establish a TCP connection to a DNS server, all DNS
queries that needed a TCP connection to that DNS server would timeout
because the nsvc object representing TCP connectivity got stuck in a
"queuing new queries but too busy to send any right now" state. Such
timeouts typically lead to HTTP 503 ERR_DNS_FAIL responses. This bug was
introduced when Comm closure handler registration was moved/delayed in
2010 commit cfd66529.
With this change, the affected nsvc object is destroyed, and Squid
attempts to open another TCP connection to the DNS server (when needed).
The original query is typically retried (subject to dns_timeout and
dns_retransmit_interval idiosyncrasies).
XXX: This fix increases the surface of reconfiguration and shutdown
problems documented in nsvc class destructor XXX.
This change fixes how cbdata is used for managing CachePeer::digest
lifetime. Prior to these changes, cbdata was (ab)used as a reference
counting mechanism: CachePeer::digest object could outlive CachePeer
(effectively its creator), necessitating complex "is it time to delete
this digest?" cleanup logic. Now, CachePeer is an exclusive digest owner
that no longer locks/unlocks its digest field; it just creates/deletes
the object. Digest fetching code no longer needs to cleanup the digest.
"CachePeer::digest is always valid" invariant simplifies digest fetching
code and, hopefully, reduces the probability of bugs similar to Bug 5329
fixed in minimal commit 4657405 (that promised this refactoring).
Removed effectively unused acl_checklist members (#1860)
Four classes had problematic acl_checklist data members:
* SnmpRequest does not use acl_checklist at all.
* ClientRequestContext and Adaptation::AccessCheck do not use
acl_checklist beyond checklist creation/configuration code. A local
variable works better for creation/configuration, and having a data
member complicates upcoming checklist creation code improvements.
* PeerSelector creates/configures a checklist and uses acl_checklist
during destruction, but the latter code is marked as a Squid BUG and
is itself buggy: Checklist objects are cbdata-protected. They must
have one owner at any time. Starting with a nonBlockingCheck() call,
that owner is the checklist object itself. That owner destructs the
checklist object (i.e. "this"). If that Squid BUG code could be
reached, Squid would delete the same object twice. There are no known
ways to trigger that bug.
Alex Rousskov [Sun, 7 Jul 2024 03:03:00 +0000 (03:03 +0000)]
Fix Tokenizer::int64() parsing of "0" when guessing base (#1842)
Known bug victims in current code were tcp_outgoing_mark,
mark_client_packet, clientside_mark, and mark_client_connection
directives as well as client_connection_mark and (deprecated)
clientside_mark ACLs if they were configured to match a zero mark using
"0" or "0/..." syntax:
Alex Rousskov [Tue, 2 Jul 2024 17:35:14 +0000 (17:35 +0000)]
Restrict squid.conf preprocessor space characters to SP and HT (#1858)
Here, "preprocessor space" refers to a portion of squid.conf grammar
that covers preprocessor directives (e.g., `if` and `include`
statements) and separation of a directive name from directive
parameters. This change replaces all known xisspace() uses in
preprocessor code with code that only recognizes SP and HT as space
characters.
Prior to this change, Squid preprocessor also classified as space ASCII
VT and FF characters as well as any other locale-specific characters
classified as space by isspace(3).
Squid preprocessor still delimits input lines using LF and terminates
each read line at the first CR or LF character (if any), whichever comes
earlier. This behavior precludes CR and LF characters from reaching
individual directive line parsers. However, directive parameter values
loaded by ConfigParser when honoring "quoted filename" or
parameters(filename) syntax are (still) tokenized using w_space which
includes an ASCII CR character. Thus, a "foo_CR_bar" 9-character
sequence is still interpreted as a single "foo_" token by the
preprocessor and as two tokens ("foo_" and "_bar") by
ConfigParser::strtokFile().
After preprocessing, directive parameters are (still) parsed by parsers
specific to that directive type. Many of those parsers are based on
ConfigParser::TokenParse() that effectively uses the same "SP and HT
only" logic for inlined directive parameters. However, some of those
parsers define "space" differently (and may use non-space-like
characters for token delimiters). For example, response_delay_pool still
indirectly uses isspace(3) to parse integer parameters, and the
following "until eol" directives still use xisspace() to skip space
before their parameter value: debug_options, err_html_text,
mail_program, sslcrtd_program, and sslcrtvalidator_program.
More than 100 uses of xisspace() and indirect uses of isspace(3) remain
in Squid code. Most of those use cases are in configuration parsing
code. Restricting all relevant use cases one-by-one may not be
practical. On the other hand, restricting all configuration input lines
to prohibit VT, FF, CR, and locale-specific characters classified as
space by isspace(3) will break rare configs that use those characters in
places like URL paths and regexes.
Due to inconsistencies highlighted above, there is no consensus
regarding this change among Squid developers. This experiment may need
to be reverted or further grammar changes may need to be implemented
based on testing and deployment experience.
Alex Rousskov [Thu, 27 Jun 2024 22:12:40 +0000 (22:12 +0000)]
CI: Upgrade GitHub upload-artifact action to v4 (#1852)
The following artifacts were uploaded using a version of
actions/upload-artifact that is scheduled for deprecation:
"build-logs-macos-14-Object", "build-logs-ubuntu-22.04-Object",
"test-logs".
This change finishes GitHub actions upgrade started in recent commit 8805c474 and removes all remaining GitHub Actions upgrade warnings.
Upgrading to actions/upload-artifact@v4 is not trivial because the new
action version uses "immutable (unless deleted)" artifacts: One GitHub
Actions job cannot add artifacts to a container created by another job.
We now add build layer dimension to the container name created inside
"build-tests" matrix, so that all layer logs are uploaded, with one
artifact container per matrix job.
Also fixed the compiler component in build-tests artifact names: Recent
commit 5a7811af intended to add "compiler dimension to avoid name
clashes" but only added a constant "-Object" suffix, resulting in log
name clashes on Ubuntu runners that test using two compilers.
CI: Use packages from GitHub Actions MacOS runner base-image (#1850)
openldap 2.6.8 is already installed and up-to-date.
openssl@3 3.3.1 is already installed and up-to-date.
Do not "brew install" openldap and openssl packages to avoid GitHub
Actions warnings. If GitHub stops pre-installing these packages, then
MacOS tests will fail because Squid is not compatible with MacOS's
native frameworks for LDAP and OpenSSL.
Scaffolding for YAML-formatted cache manager reports (#1784)
The only YAML-compliant report currently using new code is mgr:pconn.
Non-YAML non-aggregated reports in non-SMP instances are now framed
using "by kid0 {...}" wrappers (for `squid -N`) or "by kid1 {...}"
wrappers (for instances having one worker process and no rock diskers).
This change makes YAML and non-YAML report framing more similar, but may
affect existing report parsing automation.
Also fixed Content-Type value computation for SMP reports: They were
missing ";charset=utf-8" suffix. Broken since 2014 commit 8088f8d0,
possibly due to code duplication created by 2010 commit 8822ebee.
Moved FunActionCreator and ClassActionCreator classes into
src/mgr/Registration.cc because no other code needs to know about them.
This is especially valuable for function-based actions because they do
not need to know about Mgr::FunAction (or even Mgr::Action).
Alex Rousskov [Mon, 24 Jun 2024 17:30:06 +0000 (17:30 +0000)]
Fix parallel "make check": testHeaders for same-basename files (#1846)
header-test: ok - src/ipc/forward.h
header-test: ok - src/ipc/ReadWriteLock.h
cc1plus: fatal error: forward.hdrtest.cc: No such file or directory
header-test: not ok - src/ipc/mem/forward.h
In a parallel build, testHeaders often tests header files using multiple
concurrent "make" processes. When testing same-basename files (e.g.,
src/ipc/forward.h and src/ipc/mem/forward.h), one "make" process may
delete the file generated by another process (leading to random false
"make check" failures like the one shown above) or even substitute bad
file contents with valid one (leading to false positives).
Clashes among temporary generated files can be avoided using unique
temporary directories, but those are difficult to create in a portable
way. Instead, we use "make" PID to guarantee file name uniqueness across
parallel make processes.
Alex Rousskov [Sat, 22 Jun 2024 02:49:41 +0000 (02:49 +0000)]
CI: Upgrade GitHub Setup Node and CodeQL actions to Node 20 (#1845)
These major action version upgrades are recommended by GitHub that is
deprecating Node 16: Node 16 stopped receiving security updates in
September 2023. Node 20 will reach that state around April 2026.
* actions/setup-node@v4: The primary difference is that Setup Node
action v4 uses Node 20, while v3 uses Node 16. Also, Setup Node v4
adds support for arm64 Windows and other secondary improvements.
* github/codeql-action@v3: The only difference is that CodeQL Action v3
runs on Node 20, while CodeQL Action v2 runs on Node 16.
This change is necessary but not sufficient to address current GitHub
Actions upgrade warnings. We also need to upgrade upload-artifact@v3,
but that upgrade has side effects that deserve a dedicate change.
Since 2010 commit 7de94c8c, only worker processes made RegisterAction()
calls associated with the affected cache manager reports. Coordinator
process needs these registrations as well because it uses Mgr::Action
code while iterating report-generating kids[^1].
When fixed Coordinator asks a disker process for an Action report, that
process must recognize the action as well (even when disker has no
information to supply) to avoid triggering similar Action lookup BUGs.
[^1]: Coordinator mostly needs Mgr::Action for stats aggregation, but
even non-aggregating actions (like the fixed three) need registered
Mgr::ActionProfile objects for Coordinator to know whether to aggregate.
Alex Rousskov [Fri, 14 Jun 2024 16:15:16 +0000 (16:15 +0000)]
Fix reporting of unrecognized directives (#1841)
When Squid does not recognize a directive name, it reports the problem
and proceeds to parse the remaining configuration lines. When parsing is
complete, Squid quits if it has encountered such directives. This change
preserves this overall behavior while fixing related reporting problems.
Fixed problems depend on whether Squid discovered an unrecognized
directive at startup or during reconfiguration.
Startup configuration case was missing both an ERROR classification and
a FATAL message, resulting in a somewhat mysterious death:
Processing Configuration File: squid.conf (depth 0)
squid.conf(7): unrecognized: 'http_accesses'
Starting Authentication on port [::]:4443
Disabling Authentication on port [::]:4443 (interception enabled)
If other non-fatal ERRORs were present, that startup death reporting
became more problematic because admins would naturally (but incorrectly)
assume that Squid died due to those irrelevant ERRORs:
Reconfiguration case was worse because Squid clearly attributed failure
to the wrong (innocent and default) directive that was not even present
in the configuration file (among other problems like saying "null"):
Squid now reports unrecognized directives using the standardized ERROR
prefix and prints correct FATAL message in both cases:
Processing Configuration File: squid.conf (depth 0)
ERROR: unrecognized directive near 'http_accesses'
directive location: squid.conf(7)
FATAL: reconfiguration failure: Found 1 unrecognized directive(s)
exception location: cache_cf.cc(610) Parse
advice: Run 'squid -k parse' and check for ERRORs.
Squid Cache (Version 7.0.0-VCS): Terminated abnormally.
Also reduced the number of nested try/catch blocks, simplifying
configuration code. However, this change is just a small step forward.
More fatal (re)configuration error handling improvements are needed,
including removing unwanted and noisy fatal() side effects and possibly
recognizing other kinds of errors that should not immediately terminate
configuration parsing.
Allocate fast-checking ACLFilledChecklists on stack (#1835)
There is no need for dynamic allocation risks and performance overheads
in these simple "immediate fastCheck()" cases.
This change converts all dynamically allocated checklists that use only
fastCheck() calls except one: Security::PeerConnector::initialize() has
to allocate its checklist dynamically to pass it to SSL_set_ex_data().
Alex Rousskov [Thu, 6 Jun 2024 07:24:52 +0000 (07:24 +0000)]
Do not die when parsing obsolete log_access and log_icap (#1832)
All obsolete directives except for log_access and log_icap do not result
in Squid instance death today. Treating those two directives specially
is not necessary but does complicate reconfiguration improvements a bit.
Future refactoring might add support for treating (some or all) obsolete
directives as fatal configuration errors, but we can simplify for now.
Alex Rousskov [Wed, 5 Jun 2024 18:55:06 +0000 (18:55 +0000)]
Improve ErrorState debugging (#1768)
In triage, it is often useful to know that ErrorState was created _when_
it was created rather than waiting for errorAppendEntry() or errorSend()
debugging, especially if those error handling stages are not reached.
Also report HTTP status code of the error response (when available).
TrieNode::add() incorrectly computed an offset of an internal data
structure, resulting in out-of-bounds memory accesses that could cause
corruption or crashes.
This bug was discovered and detailed by Joshua Rogers at
https://megamansec.github.io/Squid-Security-Audit/esi-underflow.html
where it was filed as "Buffer Underflow in ESI".
Ident protocol (RFC 931 obsoleted by RFC 1413) has been considered
seriously insecure and broken since at least 2009 when SANS issued an
update recommending its removal from all networks. Squid Ident
implementation suffered from assertions (since 2021 commit e227da8) and
memory leaks (since 2015 commit fbbea66). Ident implementation design
increased ACLChecklist::goAsync() design complexity.
An external ACL helper can be written to perform Ident transactions.
Configurations using ident/ident_regex ACLs, %ui logformat codes, %IDENT
external_acl_type format code, or ident_lookup_access/ident_timeout
directives are now rejected, leading to fatal startup failures:
To avoid inconveniencing admins that do _not_ use Ident features, access
logs with "common" and "combined" logformats now always receive a dash
in the position of what used to be a %ui record field.
annotate_client and annotate_transaction ACLs must always match (#1820)
WARNING: markAsTunneled ACL is used in context without an HTTP
request. Assuming mismatch.
Our annotate_client and annotate_transaction ACLs are documented as
"always matching", and some existing Squid configurations rely on that
invariant. Both ACLs did not match when they lacked access to the
current transaction information because their requiresRequest() method
returned true; annotate_client ACL also did not match after the
client-to-Squid connection was gone and when the transaction was not
associated with a client-to-Squid connection.
This change makes ACL code conform to documentation. Squid still warns
the admin if an ACL cannot annotate. Such warnings may indicate s Squid
bug or misconfiguration, but mismatching in those cases causes more harm
because it makes it impossible for the admin to rely on the primary
matching invariant. "One reliable invariant plus one unreliable side
effect" is the lesser evil than "two unreliable effects".
# the following must deny even if it cannot annotate
http_access deny markAsDenied
# the following might not log denied traffic (with a prior warning)
access_log syslog:daemon.err markedAsDenied
Also fixed annotate_client to annotate the current transaction even
after ConnStateData destruction. Such annotations may happen when, for
example, Squid continues a large download after the HTTP client is gone.
Alex Rousskov [Mon, 20 May 2024 14:50:19 +0000 (14:50 +0000)]
Docs: REQUIRED in ident_regex, proxy_auth_regex, ext_user_regex (#1818)
The three ACLs were documented as matching any username when configured
with a parameter spelled "REQUIRED". Neither actually treated that
parameter specially -- all interpreted it as an ordinary regex.
This dangerous documentation bug was introduced in 2000 commit 145cf92
that added ident_regex and proxy_auth_regex support. It was then
duplicated in 2003 commit abb929f that added ext_user_regex support.
This minimal documentation fix does not imply that these ACLs should not
treat REQUIRED values specially. Enabling such special treatment
requires significant code changes, especially if we want to do that well
and without duplicating the corresponding code.
Fix build with clang v18 [-Wvla-cxx-extension] (#1813)
src/fs/rock/RockRebuild.cc:356:17: error: variable length arrays
in C++ are a Clang extension [-Werror,-Wvla-cxx-extension]
char hdrBuf[SwapDir::HeaderSize];
note: initializer of 'HeaderSize' is unknown
Ensure ACLChecklist::markFinished() is called (#1809)
For example, ACLChecklist::resumeNonBlockingCheck() missed a
markFinished() call when prepNonBlocking() returned false after a
reconfiguration. Missing markFinished() calls result in the last
evaluated ACL name being unset in nonBlockingCheck() callbacks.
CI: Add clang to GitHub Actions build-tests matrix (#1812)
Add "compiler" dimension to build-tests matrix.
Do not test layer-04-noauth-everything to partially compensate for the
new matrix dimension repeating other layer tests 3 times.
Change build-tests artifacts name pattern to include compiler dimension
to avoid name clashes that would result in logs being overwritten and
lost. Also use target environment instead of runner environment in
anticipation of adding container-based tests where the two differ.
Maintenance: No ptr copy in future CredentialsCache::insert()s (#1807)
Currently, the insert() method calls implicitly convert raw "this"
pointers to RefCount pointers. That self-registration code raises red
flags and may eventually be refactored. If it is refactored, insert()
calls are likely to start using RefCount pointers as parameters. This
change allows those future calls to avoid RefCount pointer copies. This
change does not affect current calls performance.
This change was triggered by Coverity CID 1554665: Unnecessary object
copies can affect performance (COPY_INSTEAD_OF_MOVE). However, the
insert() method is still calling RefCount copy assignment operator
rather than its (cheaper) move assignment operator. Safely removing that
overhead is only possible by reintroducing future parameter copying
overhead described above _and_ using an explicit std::move() call that
we are trying to avoid in general code. It is arguably not worth it.
Alex Rousskov [Fri, 3 May 2024 02:00:08 +0000 (02:00 +0000)]
Bug 5322: Do not leak HttpReply when checking http_reply_access (#1764)
... as well as response_delay_pool and send_hit directives.
auto check = clientAclChecklistCreate(...); // sets check->reply
check->reply = reply; // self-assignment does nothing
HTTPMSGLOCK(check->reply); // an unwanted/extra lock
When ACLFilledChecklist::reply is already set to X, resetting it to X
should not change HttpReply lock count, but some manual locking code did
not check that "already set" precondition and over-locked reply objects
set to ClientHttpRequest::al::reply by clientAclChecklistFill().
Current known leaks were probably introduced in 2021 commit e227da8 that
started supplying HttpReply to ACLChecklist in clientAclChecklistFill().
The added code locked the reply object correctly, but it was
incompatible with unconditional manual locks in three existing indirect
clientAclChecklistFill() callers (calling clientAclChecklistCreate()).
This change removes all known similar leaks and improves
ACLFilledChecklist API to prevent future similar leaks.
Alex Rousskov [Sat, 13 Apr 2024 08:15:00 +0000 (08:15 +0000)]
Bug 5352: Do not get stuck when RESPMOD is slower than read(2) (#1777)
... RESPMOD BodyPipe buffer becomes full ...
maybeMakeSpaceAvailable: will not read up to 0
The AsyncCall Client::noteDelayAwareReadChance constructed
... RESPMOD consumes some buffered virgin body data ...
entering BodyProducer::noteMoreBodySpaceAvailable
leaving BodyProducer::noteMoreBodySpaceAvailable
When RESPMOD does not empty its adaptation BodyPipe buffer fast enough,
readReply() may eventually fill that buffer and call delayRead(),
anticipating a noteDelayAwareReadChance() callback from Store or Server
delay pools. That callback never happens if Store and Server are not
getting any data -- they do not even start working until RESPMOD service
starts releasing adapted/echoed response back to Squid! Meanwhile, our
flags.do_next_read (cleared by readReply() caller) remains false.
When/if RESPMOD service eventually frees some BodyPipe buffer space,
triggering noteMoreBodySpaceAvailable() notification, nothing changes
because maybeReadVirginBody() quits when flags.do_next_read is false.
noteMoreBodySpaceAvailable() could not just make flags.do_next_read true
because that flag may be false for a variety of other/permanent reasons.
Instead, we replaced that one-size-fits-all flag with more specific
checks so that reading can resume if it is safe to resume it. This
change addresses a couple of flag-related XXXs.
The bug was introduced in 2023 commit 50c5af88. Prior that that change,
delayRead() was not called when RESPMOD BodyPipe buffer became full
because maybeMakeSpaceAvailable() returned false in that case, blocking
maybeReadVirginBody() from triggering readReply() via Comm::Read(). We
missed flags.do_next_read dependency and that Store-specific delayRead()
cannot be used to wait for adaptation buffer space to become available.
XXX: To reduce risks, this change duplicates a part of
calcBufferSpaceToReserve() logic. Removing that duplication requires
significant (and risky) refactoring of several related methods.
Alex Rousskov [Mon, 8 Apr 2024 22:06:02 +0000 (22:06 +0000)]
Upgrade Acl::Node::name to SBuf; remove AclMatchedName global (#1766)
AclMatchedName global has been localized into a regular Acl::Answer data
member (Acl::Answer maintains the result of ACLChecklist evaluations).
This long overdue change resolves an old TODO and XXXs, paving the way
for Acl::Node reference counting.
No significant functionality changes are expected, but it is possible
that some deny_info configurations will now be handled better in
reconfiguration corner cases (because Squid no longer forgets the name
of the last checked ACL when a slow ACL check crosses reconfiguration
barrier).
Most of these changes are performance-neutral or -positive because they
eliminate or reduce memory allocations and associated name copying (and
more reduction will become possible after upgrading squid.conf parsers
to use SBuf). This change adds SBuf object copies when Acl::Answer is
propagated to ACLCB callbacks, but those read-only copies are cheap.
Also renamed and polished aclGetDenyInfoPage() because we had to update
its parameter type (to supply the last evaluated ACL). All callers were
also supplying the same first argument (that is unlikely to change in
the foreseeable future); that argument is now gone. We did not fix the
redirect_allowed name and debugs(): Fixing that behavior deserves a
dedicated change.
Also polished legacy aclIsProxyAuth() profile and description because we
have to change the parameter type (to supply the last evaluated ACL).
Also removed 63-character aclname parameter limit for acl directives.
Alex Rousskov [Wed, 3 Apr 2024 09:25:55 +0000 (09:25 +0000)]
Fix const-correctness of ACLHTTPHeaderData::match() parameter (#1771)
ACLHTTPHeaderData::match() required a pointer to non-const HttpHeader
but does not (and should not) modify the supplied HttpHeader.
Also removed support for nil HttpHeader in that method. All callers
already require HttpHeader presence. If that changes, it is the _caller_
that should decide what HttpHeader absence means (match, mismatch,
exception/dunno, etc.); ACLHTTPHeaderData does not have enough
information to make the right choice and, hence, should not be asked to
choose.
Also polished related #includes to remove unnecessary ones.
Alex Rousskov [Tue, 2 Apr 2024 20:37:31 +0000 (20:37 +0000)]
Do not blame cache_peer for CONNECT errors (#1772)
ERROR: Connection to [such-and-such-cache_peer] failed
TCP_TUNNEL/503 CONNECT nxdomain.test:443 FIRSTUP_PARENT
Squid does not alert an admin about (and decrease health level of) a
cache_peer that responded with an error to a GET request. Just like GET
responses from a cache_peer, CONNECT responses may (and often do!)
reflect client or origin server failures. We should not penalize
cache_peers (and alert admins) until we can distinguish these frequent
client/origin failures from (relatively rare) cache_peer problems. This
change absolves cache_peers of CONNECT problems, restoring parity with
GETs and restoring v4 behavior changed (probably by accident) in v5.
Also removed Http::StatusCode parameter from failure notification
functions because it became essentially unused after the primary
Http::Tunneler changes. Tunneler was the only source of status code
information that (in some cases) used received HTTP response to compute
that status code. All other cases extracted that status code from
Squid-generated errors. Those errors were arguably never meant to supply
status code information for "this failure is not our fault" decision,
and they do not supply 4xx status codes driving that decision.
### Problem evolution
2019 commit f5e1794 effectively started blaming cache_peer for all
FwdState CONNECT errors. That functionality change was probably
accidental, likely influenced by the names of noteConnectFailure() and
peerConnectFailed() functions that abbreviated "Connection", making the
functions look as applicable to CONNECT failures. Prior to that commit,
the functions were never used for CONNECT errors. After it, FwdState
started calling peerConnectFailed() for all CONNECT failures.
In 2020 commit 25b0ce4, TunnelStateData started blaming cache_peers as
well (by moving that FwdState-only error handling code into Tunneler).
The same "accidental functionality change" speculations apply here.
In 2022 commit 022dbab, we made an exception for 4xx CONNECT errors as
folks deploying newer code started complaining about cache_peers getting
blamed for client-caused errors (e.g., HTTP 403 Forbidden replies). We
did not realize that the blaming code itself was an unwanted accident.
Now we are getting complaints about cache_peers getting blamed for 502
and 503 CONNECT errors caused by, for example, domain names without IPs:
As these CONNECT error responses are propagated from parent to child
caches, every child cache in the chain logs ERRORs and every cache_peer
in the chain gets its health counter decreased!
Class constructor and destructor sources should not manually duplicate
class names in debugs() statements. Many existing classes do the right
thing, but find-alive.pl still insists on a manually added class name,
forcing its users to add a colon: `find-alive.pl MemBlob:`.
We now accommodate both legacy/manual and modern/auto use cases.
When FwdState decides to re-forward a request, it forgets the original
response[^1] but does not create an error object. Since commit e2bbd3b,
FwdState::noteDestinationsEnd() correctly assumed that we only idly wait
for additional destinations after encountering a problem, but
incorrectly asserted that past problems imply error object existence.
Now Squid generates an HTTP 502 (Bad Gateway) response while setting
%err_code/%err_detail to ERR_CANNOT_FORWARD/REFORWARD_TO_NONE.
TunnelStateData::noteDestinationsEnd() code is similar, but it probably
does not suffer from the same bug because an error object is created
before every retryOrBail() call, and there is no re-forwarding code that
forgets an HTTP error response without creating an error. Those
invariants are not well tracked, and this change mimics FwdState changes
in TunnelStateData just in case and to keep the two methods in sync. In
those hypothetical cases, ERR_CANNOT_FORWARD/RETRY_TO_NONE is logged.
[^1]: Long-term we probably want to preserve that original response so
that we do not have to replace it with a generated error, but doing so
requires significant refactoring and is outside this minimal fix scope.
Alex Rousskov [Wed, 27 Mar 2024 18:41:41 +0000 (18:41 +0000)]
CI: Require source-maintenance.sh application before merging PRs (#1760)
By default, fixing PR-introduced code formatting problems should be that
PR responsibility. We did not enforced formatting because we did not
have CI automation to support that enforcement, and also because it was
tedious even for core developer to maintain the right astyle version.
Neither excuse remains valid these days.
In most development environments, it is not very difficult to install
Astyle v3.1 (and other Squid source maintenance tools). Our GitHub
Actions configuration file (.github/default.yaml) has a recipe. In
exceptional cases, including cases where a pull request was not
formatted because a different astyle version was installed, other
developers can (re)format PR sources (proactively or at author request).
Also fixed wrong error message when running source-maintenance.sh fails.
HTTP: Protect just-parsed responses from accidental destruction (#1752)
The lack of HttpReply locking meant that we could not (safely) pass the
freshly created reply object to ACLFilledChecklist because the checklist
destructor would unwittingly destroy the object (by unlocking it). There
is at least one applicable ACL check in handle1xx(), but we got lucky
because that method adds the reply object to ALE, increasing its
reference counter. This change stops relying on such "external" locks.
This change also protects from HttpReply memory leaks when an exception
is thrown between HttpReply creation and a setVirginReply() call.
TODO: Fix all cases where newly created HttpReply objects are remembered
using raw pointers (but may be passed to locking/unlocking code).
ankor2023 [Tue, 5 Mar 2024 11:13:19 +0000 (11:13 +0000)]
negotiate_kerberos_auth: Support Kerberos PAC-ResourceGroups (#1597)
Parse the ResourceGroupIds pac-data structure to have information
about the user's membership in AD Domain Local groups.
Previously, the helper obtained user groups information only from
GroupIds and ExtraSids pac-data structures (of the
KERB_VALIDATION_INFO structure).
The patch extends the functionality of the helper.
Now it additionally parse the ResourceGroupIds pac-data structure
where Domain Local AD-group rids are located.
It appends these groups to the the list generated by parsing
GroupIds and ExtraSids.
No changes in existing helper deployments are required.
The new parsing functions are similar to those already used for
parsing GroupIds and ExtraSids.
Alex Rousskov [Fri, 1 Mar 2024 22:20:20 +0000 (22:20 +0000)]
Bug 5069: Keep listening after getsockname() error (#1713)
ERROR: Stopped accepting connections:
error: getsockname() failed to locate local-IP on ...
In many cases, these failures are intermittent client-triggered errors
(e.g., client shut down the accepted socket); Squid will successfully
accept other connections and, hence, should keep listening for them.
Store is an essential service, used by a lot of Squid code. As was
already established in 2022 commit 23b7963, this service should be
available during shutdown. That commit correctly removed explicit Store
service termination, but missed the fact that the reference-counting
TheRoot pointer (that provides access to the Store Controller singleton)
gets _automatically_ destroyed during C++ cleanup. This change removes
TheRoot reference counting, making that Controller singleton immortal.
Squid asserted when exiting with active entries in the shared memory
cache because TheRoot destruction leads to Controller destruction,
Controller destructor cleans up cache index, and that cleanup code may
result in calls to Store::Root() that dereferences destroyed TheRoot.
These assertions were seen when Squid was shutdown cleanly (e.g., using
SIGTERM) and when a kid process was exiting due to a fatal() error.
Making Store::Controller singleton immortal means that the class
destructor is never called. Fortunately, the destructor did nothing
particularly useful; Store flushing is performed by Controller::sync()
which is explicitly called during early stages of clean shutdown. The
now-unused destructor code was removed: Implementing this destructor in
a meaningful way (while avoiding accessing a being-destructed global
Store!) requires heroic efforts (which would be wasted since the
destructor is never actually called).
Also made Store Controller singleton available on the first use,
complying with Store's "essential, widely used service" status and
removing the need for an explicit Store::Init(void) call.
Also removed unit tests that required support for dynamic replacement of
Store Controller singleton. The minuscule value of the removed tests did
not justify the costs of supporting a replaceable Store Controller.
Also removed Store::FreeMemory(). Its implementation contradicted Store
status as an essential service. The function was only used by unit
tests, and its removal addresses the corresponding commit 23b7963 TODO.
projects / thirdparty / squid.git / log
