Alex Rousskov [Tue, 28 Feb 2023 21:40:14 +0000 (21:40 +0000)]
CI: Remove irrelevant branch from master GitHub Actions config (#1292)
The original GitHub Actions configuration (see commit 2ed8cc4)
incorrectly assumed that GitHub will use that configuration (stored in
the master branch) for testing _all_ repository branches. In reality,
GitHub uses configuration from the being-tested branch. Thus, we do not
need to (and, hence, should not) list any branches in the X branch
configuration, with the exception of the X branch itself and the "auto"
branch where all PRs (including all X-targeting PRs) are staged for the
final "as if merged" testing.
Alex Rousskov [Mon, 27 Feb 2023 10:56:57 +0000 (10:56 +0000)]
Improve AnyP::Uri::port_ and related port storage types (#1255)
The old "unsigned short" type is used for various things all over the
code and cannot distinguish a parsed port zero from an absent port. In
theory, it could also hold values exceeding 65535, getting out of sync
with various (poorly duplicated) maximum port value checks.
No significant runtime changes are expected, but some transactions with
unknown request URI ports may now be reported correctly, showing "-"
instead of "0" for the missing port number. In some very special/rare
on_unsupported_protocol cases, Squid might no longer attempt to tunnel
non-HTTP requests to a zero port. Such attempts were failing anyway.
This change leaves many "unsigned short" port use cases untouched. Most
of them should be converted, but most (if not all) of those conversions
should be done in dedicated projects. Here are a few TODO highlights:
* ACLIntRange: Stores integers, is usable for integers, but assumes it
is being configured with port numbers. We probably should enhance that
class to handle "all" integers instead of expanding/deepening that
rather limiting "input is just port numbers" assumption.
* CachePeer and internalRemoteUri(): Likely runtime changes related to
squid.conf validity.
* Ftp::Server::listenForDataConnection() and friends: Triggers a few
changes with runtime effects related to comm_local_port() failures.
* Ip::Address, Snmp::Session, fde: Too many low-level changes, some of
which would be difficult to test.
* Config.Port.icp: Possible runtime changes related to squid.conf and
command-line arguments validity.
Also used "%hu" instead of "%u" or "%d" to printf() ports. These changes
arguably improve code and might make some pedantic compiler happier, but
they do not fix any "real" bugs because variadic printf() arguments are
automatically promoted from short to int and, hence, printf()
implementations never get and never expect shorts.
Maintenance: Change unit test class names to CamelCase (#1288)
Besides applying Project CamelCase class naming rules to all CppUnit
test class names, these script-driven changes also adjust affected class
name-dependent code, improve consistency across unit test declarations a
little, and rename a few test classes (e.g. testURL is not TestUri). The
corresponding source code filenames are left unchanged (for now).
Alex Rousskov [Sun, 19 Feb 2023 00:23:31 +0000 (00:23 +0000)]
Ensure StoreEntry presence during store_client lifetime (#1281)
Most likely, all store_client users already lock their StoreEntry
objects, so this change is unlikely to affect any current use cases.
However, objects like store_client that store StoreEntry objects for
long-term/asynchronous reuse should lock their entries rather than rely
on others to do that for them. Being able to rely on StoreEntry locking
also clarifies existing logic in several store_client methods.
Alex Rousskov [Tue, 14 Feb 2023 21:31:11 +0000 (21:31 +0000)]
CI: Disable optimizations in "minimal" build tests (#1278)
Our "minimal" layer tests had to stop disabling optimizations in 2019
commit 4f3c41b due to problems specific to GCC v9 (v9.1 or earlier). GCC
v9.4 (and later) appear to work OK (at least on Ubuntu 22.04), so let's
try to resume testing --disable-optimizations ./configure option.
Enable compiler checks for printf-like SBuf methods (#1270)
After commit 0f17e25, these two are the only known printf()-like
functions not already covered by these compiler checks. Some helper
debugging hacks do not have PRINTF_FORMAT_ARG2 or similar attributes,
but they are covered because their `__GNUC__` variants use standard
fprintf().
Since inception (2017 commit e99fa72), FileOpeningConfig::RetryGapUsec
was declared but not defined in any translation unit (a bug). The build
probably "worked" because compilers simply inlined the value of the
constant data member when calling xusleep(). Commit 5eee0e4 removed code
that passed RetryGapUsec to that external "C" function. Now, the same
member is passed to a heavily-inlined and convoluted STL code, and at
least some compilers (e.g., GCC v10-v12 on Ubuntu 22.04) stopped
inlining RetryGapUsec unless optimization is enabled. Our CI tests
passed because we do not test --disable-optimizations builds (yet).
We do not need FileOpeningConfig::RetryGapUsec to be static, breaking
builds, requiring extra code, and triggering questions. We do not even
need it to be constant, but removing "const" is an arguably out-of-scope
change, as is improving its type.
File::InvalidHandle is missing a definition as well, except on Windows,
but we are leaving that code "as is" for now, until we can test whether
Windows is OK with removing that "static" keyword.
Alex Rousskov [Tue, 14 Feb 2023 10:03:34 +0000 (10:03 +0000)]
CI: Extend GitHub Actions build-tests to doc/release-notes/ (#1275)
"make dist[check]" does nothing in doc/release-notes unless a linuxdoc
tool has been detected during ./configure, missing errors like this:
make[6]: Entering directory /squid/doc/release-notes
linuxdoc -B html -T 2 --split=0 release-6.sgml
Processing file release-6.sgml
onsgmls: ...E: end tag for element "E" which is not open
Alex Rousskov [Tue, 14 Feb 2023 00:00:20 +0000 (00:00 +0000)]
Place more msgs under cache_log_message control, downgrade some (#1273)
Also support cache_log_message-controlled messages in ::Config-unaware
libraries (e.g., libip) used by tools (e.g., cachemgr.cgi).
Restricting the very first (i.e. "depth 0") "Processing Configuration
File" message does not work during startup (before that file is parsed),
and does not work during reconfiguration (because the old configuration
is reset shortly before logging that line). Future reconfiguration
support improvements may fix the reset problem. Restrictions do work as
expected for included files (i.e. positive "depth" levels).
Restricting "BCP 177 violation" WARNINGs does not work because the
warnings are printed _before_ Squid configuration is parsed. Future
initialization improvements may fix this.
Also downgraded the importance of a few (re)configuration
progress-reporting messages from level 1 to level 2.
Run cache_peer probes without transaction context (#1256)
Even when triggered by a master transaction, these background probes
should not be associated with it. That transaction just wants to know
whether the neighbor is "up" right now. The transaction does not wait
(and is not aware of) the background probe its question may or may not
have triggered.
Amos Jeffries [Fri, 10 Feb 2023 12:25:07 +0000 (12:25 +0000)]
Cleanup: remove xusleep() hacks (#613)
usleep() has been obsolete for a long time. C++11 provides better and
portable waiting mechanism we can use easily instead of these hacks
trying to replicate and extend usleep().
The original text contained a "last modification age" typo and was
misinterpreted by some admins as if Squid applied the configured percent
to the current object age.
The text also did not make it clear that the percent-based heuristic is
effectively only applied to responses with a Last-Modified header (in
addition to the usual "without an explicit expiry time" precondition).
squidclient: Drop cache_object protocol support (#1251)
Use `http` URL scheme for `mgr:command` cache manager requests instead.
Also fixed a bug: When `mgr:command` was used together with a `-w X`
command-line option, squidclient sent that proxy authentication password
`X` in the `cache_object` URL instead of sending the `Y` password from
the `-W Y` option (or no password at all if no `-W` option was given).
If no Authorization header had been sent, Squid's cachemgr_passwd
honored the `-w X` password sent in the cache_object URL. If
Authorization header had been sent, Squid's cachemgr_passwd honored the
corresponding `-W Y` password, ignoring any password sent in the
cache_object URL.
Now, squidclient uses Authorization HTTP header for sending cache
manager authentication credentials with `mgr:command` requests. Those
credentials are taken either from the `-U` and `-W` command-line options
(if `mgr:command` parameter lacks an embedded password) or from the `-U`
command line option and from the `mgr:command@password` parameter
(otherwise).
squidclient now sends Proxy authentication credentials if and only if
`-u` and `-w` command line options are given. These credentials may be
required to authenticate with the proxy, but they do not affect cache
manager authentication driven by the cachemgr_passwd directive.
Also prohibit specifying a command-line option with a password (`-w` or
`-W`) without specifying the corresponding user name (`-u` or `-U`).
Prior to this change, squidclient did not send the Proxy-Authorization
or Authorization header if the corresponding user name was missing but
did not complain about the problem.
Remove broken -sha1 option from server_cert_fingerprint (#1249)
server_cert_fingerprint support for the sha1 parameter has been broken
for years, probably since its inception (2012 commit 42d3334). The bug
was known since at least 2018 when it was mentioned in Bug 4847
discussion. The single-dash syntax violates the double-dash pattern
used for other --long ACL options. If fixed, using the option would not
change Squid behavior because SHA1 is the default (and the only
supported) fingerprinting algorithm.
The option was meant to allow admins to be explicit about that default
in case it changes in the future, but implementation bugs derailed that
plan. The fix is not straightforward, and we should be focusing on more
important things.
Improve error message storage in Dns::LookupDetails (#1241)
Removed one more deprecated String usage and optimized handling of a
common "no error, no lookup" case as well as portions of the "no-error
lookup" code paths. Further optimizations need similar ipcache_entry and
fqdncache_entry error storage changes (at least).
Co-authored-by: Alex Rousskov <rousskov@measurement-factory.com>
Fix logging of responses truncated by premature EOF (#1244)
When a transaction failed due to a premature EOF while receiving an HTTP
response, %Ss logformat code was missing the ABORTED suffix (e.g.,
logging TCP_MISS instead of TCP_MISS_ABORTED). When premature EOF
truncated the header, the default "squid" access log format contained a
502 sent status code (%>Hs), hinting at the problem. When the body was
truncated, even that weak hint was usually absent because the actual
status code returned by the server (usually 200) was usually logged.
Similarly, %err_code/%err_detail logformat codes for HTTP responses with
truncated by a premature EOF bodies carried no information. Now they
expand to ERR_READ_ERROR/SRV_PREMATURE_EOF in those cases.
No changes to requests truncated by Squid-server read timeouts. They
were and are still logged with TIMEDOUT %Ss suffix and
ERR_READ_TIMEOUT/WITH_SERVER %err_code/%err_detail.
Also adjusted WHOIS to mark zero-length responses with
ERR_ZERO_SIZE_OBJECT instead of default ERR_READ_ERROR. This affects
forwarded WHOIS transactions and AS number queries generated during
(re)configuration.
Reduce code duplication in ACLCertificateData::parse() (#1242)
ACLs ca_cert and user_cert already prohibit key changes. And
server_cert_fingerprint ACL only supports one option name spelling, so,
while "-sha1" is not, technically, a "key", it still cannot be
"changed". Adjust SetAclKey() (added in recent commit 4a3b853) and reuse
it to implement those existing "key change" checks.
Fix build with clang v15.0.6 [-Wdeprecated-non-prototype] (#1246)
clang v15.0.6 distributed with Fedora Rawhide complains about function
definitions using K&R style:
lib/snmplib/mib.c:229:1: error: a function definition without a
prototype is deprecated in all versions of C and is not supported in
C2x [-Werror,-Wdeprecated-non-prototype]
Amos Jeffries [Tue, 17 Jan 2023 02:19:01 +0000 (02:19 +0000)]
Removed tcp-banger* and pconn-banger tools (#1236)
We do not have enough resources/demand for maintaining these tools, they
do require maintenance, and there are better tools available.
* tcp-banger2 is not built by default and probably could not be built at
all since at least 2006 commit 5679212.
* tcp-banger3 lacked build rules since inception (1998 commit 2742510)
and probably could not be built manually (by mimicking tcp-banger2
build commands) without warnings since 2002 commit 88d8a2a.
* pconn-banger lacked build rules since inception (1997 commit eb5f55b)
and probably could not be built manually since at least 2007 commit cc192b5.
* tcp-banger.pl has portability and code quality issues; its basic
functionality is supported by squidclient, wget, curl, and others.
Co-authored-by: Alex Rousskov <rousskov@measurement-factory.com>
Alex Rousskov [Mon, 16 Jan 2023 23:24:41 +0000 (23:24 +0000)]
Maintenance: Check for expected squid-conf-tests notifications (#1231)
Sometimes, Squid must accept a configuration file while also logging a
WARNING or even an ERROR message. The new "expect-message" test case
instruction tells test-suite/test-squid-conf.sh to check for a given
expected message while still expecting configuration acceptance.
Two test cases (mgr_passwd.conf and time_units.conf) were enhanced. We
plan to use this feature in future tests already under development.
Alex Rousskov [Sat, 14 Jan 2023 18:29:18 +0000 (18:29 +0000)]
Move Namespace::Foo printing operators to Namespace (#1218)
... where C++ Argument-Dependent Lookup (ADL) can find them.
These specific printing operators did not encounter build problems yet,
but they were misplaced. See commit XXX for rationale and details.
Also removed unimplemented Mgr::ActionParams printing operator.
Also removed excessive "Namespace::" prefixes inside Namespace in
several printing operators declarations that were already properly
placed in their argument's namespaces (e.g., Ipc::ReadWriteLock).
Alex Rousskov [Fri, 13 Jan 2023 22:33:35 +0000 (22:33 +0000)]
Bug 5256: Intercepting port fails to accept (#1238)
ERROR: NF getsockopt(ORIGINAL_DST) failed on ... Bad file descriptor
ERROR: NAT lookup failed to locate original IPs on ...
Ip::Interceptor.LookupNat() needs an open Connection, but commit 7185c9e
supplied a half-baked details object, resulting in ERRORs (showing a
closed connection -- no FD... field) for every otherwise successful
accept(2) attempt on an intercepting port.
Refactored oldAccept() to use exceptions for error handling and false
return result for no-error-but-no-connection cases (Comm::NOMESSAGE).
This refactoring allows us to centralize connection closing code instead
of chasing individual COMM_ERROR and NOMESSAGE cases while still missing
connection closure (if a function called by oldAccept() throws). With
connection closure handled, we can now open the details Connection
earlier, as we have done before commit 7185c9e, meeting current
LookupNat() and other/future code expectations.
Positive side effects of this fix include elimination of the following
old error reporting problems that left admins puzzled why Squid is not
handling new connections with no error messages in non-debugging
cache.log (and with the socket listening queue growing).
* Silent lack of accept() after an ENFILE or EMFILE failure.
* Silent lack of accept() if some function used by oldAccept() throws.
* Silent at level-0 lack of accept() after logging a level-1 ERROR. This
problem was specific to ALL,0 and similar (rare) configurations.
Alex Rousskov [Mon, 9 Jan 2023 21:12:02 +0000 (21:12 +0000)]
Maintenance: Consistent use of C++11 "override" specifier (#1224)
Start following CppCoreGuidelines recommendation C.128: Virtual
functions should specify exactly one of virtual, override, or final.
Used clang-tidy modernize-use-override check to mark overridden methods.
Clang-tidy uses clang AST, so we can only modernize code that clang can
see, but we can enable enough ./configure options for clang to reach
most (possibly all) relevant classes in a single build.
Manual gradual conversion of modified (for other reasons) method
declarations is impractical because some compilers complain when a class
uses an inconsistent _mix_ of virtual and override declarations:
warning: 'toCbdata' overrides a member function but is not marked
'override' [-Winconsistent-missing-override]
### cbdata changes
The following manual changes avoid -Winconsistent-missing-override
warnings. Doing these separately, either before or after applying
modernize-use-override, would trigger those warnings.
* Replaced CbdataParent-CBDATA_CLASS with CbdataParent-CBDATA_CHILD
class inheritance sequences. This fix does not really change anything
except for making toCbdata() specifiers consistent.
* Replaced CbdataParent-CBDATA_CLASS-CBDATA_CHILD with
CbdataParent-CBDATA_INTERMEDIATE-CBDATA_CHILD class inheritance
sequences. This fix removes unused new/delete operators in
intermediate classes (and associated unused static memory pools) and
makes toCbdata() specifiers consistent. This fix was difficult to get
right because of the old design problem discussed below.
While working on the second bullet changes, we first tried to simply
drop the intermediate CBDATA_CLASS sequence element because it should
never be needed/used in class hierarchies ending with CBDATA_CHILD.
Fortunately, CI tests discovered that dropping CBDATA_CLASS converts an
old design problem into runtime bugs: Those intermediate class
constructors (e.g., Mgr::StoreToCommWriter) actually call toCbdata()
when creating connection closure callbacks and such!
During intermediate constructor execution, the class virtual table does
not yet point to toCbdata() defined by CBDATA_CHILD. If we simply remove
CBDATA_CLASS, then, during that intermediate constructor execution, the
virtual table would still point to pure CbdataParent::toCbdata(). Those
intermediate constructors would then crash Squid:
FATAL: Dying from an exception handling failure;
exception: [no active exception]
in OnTerminate () at main.cc:1310
in std::terminate() () from libstdc++.so.6
in __cxa_pure_virtual () from libstdc++.so.6
in CbcPointer<Mgr::StoreToCommWriter>::CbcPointer at CbcPointer.h:94
in Mgr::StoreToCommWriter::StoreToCommWriter at ...oCommWriter.cc:29
We now use lighter CBDATA_INTERMEDIATE() instead of intermediate
CBDATA_CLASS. The resulting code is as risky as it used to be, but at
least we are marking this (old) design problem (in some cases).
We rejected the idea of defining CbdataParent::toCbdata() instead. It
would work fine for linear class inheritance where "this" does not
change -- in those simple class hierarchies, we do not need toCbdata()!
However, such a change would make any constructor-time bugs in multiple
inheritance hierarchies much more difficult to find because the faulty
constructor will work instead of triggering the above FATAL error. The
crashes would happen later, when wrong cbdata pointer is used. While
CBDATA_INTERMEDIATE() cannot fix this design problem, it does not make
it _worse_, so we prefer that solution.
Alex Rousskov [Sun, 8 Jan 2023 12:43:48 +0000 (12:43 +0000)]
Warn about some bad from-helper annotations (#1221)
From-helper annotations may belong to one or more of the following
(partially overlapping) categories:
* A1: Annotations with a name ending with an underscore. Those
annotations are reserved for admin use. Example: "category_".
* A2: Annotations that this Squid version knows about and treats
specially. Example: "clt_conn_tag".
* B1: Annotations that this Squid version does not know about at all.
Future Squids may start using them specially. Example: "detail".
* B2: Annotations unused by this Squid build. Example: Squid
./configured with --disable-auth does not use "user" and "password"
annotations received from an external ACL helper.
* B3: Annotations incompatible with the current helper response
type/kind. Example: Digest authentication code does not use "ha1"
annotation in ERR responses received from digest authentication
helpers.
* B4: Annotations with repeated names (the first of them will be used).
Example: "user" in OK replies from negotiate authentication helpers.
Warn about received helper annotations in the B1 category.
It is possible to also warn about B2 and B3 annotations, but several
draft implementations imply that such an improvement requires complex
code changes that may also increase Squid code maintenance overheads.
Warning about B4 annotations is not that difficult, but we must decide:
* Whether custom repeated annotation are actually bad (and should be
reported or even ignored).
* Whether Squid-recognized repeated annotations are bad enough to
warrant the decreased performance and increased code complexity that
reporting them would require. In other words, do we want to penalize
good helpers that do _not_ send any repeated Squid-recognized
annotations (except, perhaps, "message") to warn about bad helpers
that do?
Squid retries certain request forwarding failures. When the problem is
persistent, and there are many peers (or the origin server has many
IPs), many retries are possible. Absence of Squid-server transaction log
hides these retries from admins. The new logformat code helps discover
and triage problems, especially when Squid retries many times.
Alex Rousskov [Tue, 3 Jan 2023 12:20:27 +0000 (12:20 +0000)]
%busy_time: Time spent in transaction-related code (#1216)
Configuration tuning, problems triage, and performance optimization
often benefit from knowing how much time Squid spent on a particular
transaction (or a typical transaction in a category like "cache hits").
We already log total transaction response time (%tr), but that time
includes transaction queuing time (i.e. the time when Squid could
advance our transaction but was advancing other transactions) and
various waiting periods (e.g., waiting for a helper response). The new
measurement focuses on time periods when Squid was actively working on
(as in "was blocked on") our transaction.
CodeContext coverage problems do affect this measurement, but the
existing coverage is already good enough for usable measurements in some
cases, and it is getting better. It will take us a long time to cover
all cases; we should not wait for that to happen (as long as we warn
admins about these problems).
- Allow passing astyle program name via ASTYLE environment variable.
- By default, use "astyle-3.1" if present or "astyle" otherwise.
- Add an option to format changed source files only.
No changes to the default behavior except for environments where a
program named "astyle-3.1" is present but a program named "astyle" was
previously used.
Commit a1c2236 forgot to quote AS_CASE argument of AC_ARG_WITH(),
resulting in the following ./configure-time errors:
error: --with-filedescriptors expects a numeric argument
We now quote AS_CASE argument of AC_ARG_WITH(), as recommended by
Autoconf and for consistency sake. More (hopefully automated) work is
needed to quote all function call arguments (that should be quoted).
Also use m4 quadrigraphs for quoting safety sake and to attract readers
attention to a risky (from writing in m4 point of view) expression. This
particular improvement is not required to fix the bug.
The other two AC_ARG_WITH() are modified for consistency sake but they
were not broken and their compiled versions have not changed.
Alex Rousskov [Thu, 22 Dec 2022 02:12:12 +0000 (02:12 +0000)]
Fix GCC v12 build errors related to ADL in "printing" code (#1215)
This change contains three fixes/adjustments described below.
### Fix Store::SwapMetaView printing
src/sbuf/Stream.h:66:20: error: no match for 'operator<<' (operand
types are 'std::basic_ostream<char>' and 'const
Store::SwapMetaView'): (out << ... << args);
Disclaimer: This explanation omits many details that become important in
some other contexts. Most Argument-Dependent Lookup details are
documented at https://en.cppreference.com/w/cpp/language/adl
// In some printer.h, we define a printing function for any type A.
// This is similar to an ToSBuf() declaration in sbuf/Stream.h.
template <typename A>
void print(ostream &os, const A &argument) { os << argument; }
// In some n/t.h, we define how to print objects of type N::T.
// This is similar to store/SwapMetaView.h.
operator <<(ostream &os, const N::T &argument);
// In some caller1.cc, we include both headers and call print():
#include "printer.h"
#include "n/t.h"
void caller1(const N::T &argument) { print(std::cout, argument); }
// In some caller2.cc, we do the same but change #include order:
#include "n/t.h"
#include "printer.h"
void caller2(const N::T &argument) { print(std::cout, argument); }
When looking at "os << argument", the compiler considers two sets of
argument printing operators, formed by the following two sources:
* The usual unqualified name lookup. This set includes N::T printing
operator if that operator is declared in global namespace somewhere
above the print() _template declaration_. In the example above, only
caller2() will have that printing operator in this set, provided that
operator is declared in global namespace (as it used to be). None of
the callers will have that printing operator in this set otherwise.
* ADL. This set is computed from the caller point of view. It includes
N::T printing operator if that operator is declared inside namespace N
somewhere above the print() _caller_. In the example above, both
caller1() and caller2() will have that printing operator in this set,
provided that operator is declared in namespace N (as it is now). None
of the callers will have that printing operator in this set otherwise.
For code to compile, one of the sets must contain the printing operator.
Given the above outcomes, there is only one sane solution that allows
any caller to instantiate print() with an N::T argument: The argument
printing operator must be declared inside namaspace N! Declaring in
global namespace would require maintaining certain #include order (that
will cause headaches and, eventually, circular dependencies).
In other words, we must rely on ADL, and that means declaring operators
in the namespace of one of their argument types.
### Fix std::optional<Ip::Address> printing (in src/icap/Xaction.cc)
src/base/AsyncJobCalls.h:115:61: error: no match for 'operator<<'
(operand types are 'std::basic_ostream<char>' and 'const
std::optional<Ip::Address>'):
void print(std::ostream &os) const { os << '(' << arg1 << ')'; }
In this context, both printing operator argument types are in std, but
ADL also looks at template parameters of argument types (if an argument
type is a template). That recursion adds the Ip namespace to the search.
This is a minimal fix. We should move both Ip::Address printers into
ip/print.h or similar, away from Ip::Address users that print nothing.
### Do not declare an overly general std::optional printer
The removed declaration:
* evidently became unused (after the other changes described above);
* places std::optional<N::T> printers in the wrong namespace (global
instead of N), where ADL cannot find them;
* exposes all I/O manipulators to a, technically, unrelated
std::optional interface.
Alex Rousskov [Tue, 20 Dec 2022 15:41:40 +0000 (15:41 +0000)]
Support "negative" squid-conf-tests (#1214)
The test suite can now check that Squid rejects a given malformed
configuration file and that the rejection reason matches the expected
one. The latter reduces the probability that a successful "negative"
test outcome would hide a bug (because Squid has rejected a malformed
file but for a reason unrelated to what the test was trying to verify).
For now, enable just one acl regex test case, addressing an old TODO.
To improve "negative" test coverage, we would need to generate test
configurations using a configuration template and a list of problematic
expressions (with corresponding failure messages), but this hard-coded
approach is already better than nothing.
Alex Rousskov [Tue, 13 Dec 2022 23:17:48 +0000 (23:17 +0000)]
Require C++17 (#1212)
Modern environments support C++17 well. We are wasting significant
amounts of time on emulating such basic C++17 features as std::optional.
We are writing worse code than we can because we lack access to such
C++14 and C++17 features like std::make_unique and std::byte.
The following language-standard-dependent TODOs were completed:
* Switched to [[fallthrough]] attributes.
* Replaced Optional with std::optional.
* Replaced EnableIfType with std::enable_if_t.
* Removed old C++11 type replacements (e.g., xuniform_int_distribution).
* Removed std::allocator declarations deprecated in C++17.
* Made ToSBuf() readable using a fold expression.
* Simplified compile-time "natural type" assertions.
* Used std::container::emplace*() return value where warranted.
Moved BUILDCXX-setting code after AX_CXX_COMPILE_STDCXX adds -std=c++17
to CXX (rather than CXXFLAGS!). Our cf_gen was built with compiler's
default C++ standard version! Ideally, BUILDCXX block should be lowered
further -- other macros might alter CXX -- but its CXXFLAGS adjustment
may affect subsequent code, warranting a careful/dedicated change.
Alex Rousskov [Mon, 12 Dec 2022 13:34:12 +0000 (13:34 +0000)]
Ban acl key changes in req_header, rep_header, and note ACLs (#1210)
* req_header and rep_header ACLs; the key is the header-name argument:
acl barred req_header X-1 bar
acl barred req_header X-2 bar # never matches since commit a0b240c
http_access deny barred
* the "note" ACL; the key is the annotation name argument:
acl banned note color green # never matches since commit 39baccc
acl banned note weight heavy
http_access deny banned
Squid did write a cache.log ERROR for req_header/rep_header key changes
but was silent about the preceding "note" ACL rules being ineffective
after a key change (e.g., Squid was not denying "green" requests above).
Now, Squid rejects configurations containing such ACL key changes.
Also significantly reduced the corresponding parsing code duplication.
ext_kerberos_ldap_group_acl: Support -b with -D (#1207)
When both '-b' (i.e. bind DN) and '-D' (i.e. Kerberos domain) options
are specified, '-b' is ignored completely. This breaks the helper when a
search subtree has to be limited (e.g., when using FreeIPA).
Fix it to take '-b' into account if it was specified with '-D'.
Alex Rousskov [Fri, 9 Dec 2022 11:51:39 +0000 (11:51 +0000)]
squid-conf-tests should test installed Squid (#1204)
FATAL: ..._inst/etc/mime.conf: (2) No such file or directory
FATAL: ..._inst/share/icons: (2) No such file or directory
Our squid-conf-tests are running "squid -k parse" that requires icons
and other supplementary files to be _installed_. Thus, squid-conf-tests
must run during "make installcheck" rather than during "make check".
With this change, "make distcheck" triggers squid-conf-tests at the
right time, after "all", "check", and "install" targets are built.
Also, we are now testing installed Squid binaries, as we should.
Eric Walters [Fri, 9 Dec 2022 05:58:43 +0000 (05:58 +0000)]
Update Host, Via, and other headers in-place when possible (#1203)
This change optimizes Host header synchronization code that usually does
not need to change the Host header field but was always deleting the old
field and generating/appending a new one. Appending Host also violated
an RFC 9110 section 7.2 rule (on behalf of the user agent whose Host we
were rewriting): "A user agent that sends Host SHOULD send it as the
first field in the header section".
Also, removing old header fields and appending new ones introduced HTTP
header changes that broke otherwise working peers. For example, some
origins denied Squid-proxied messages exclusively due to an unusual
(from those origins point of view) Host header position. This change
reduces (but does not eliminate) such unnecessary and risky changes.
Two issues here causing no helpers to be detected as buildable when
./configure parameters are missing entirely (eg "./configure ").
These were not detected by our build farm because missing helpers
is not an error, they are optional, and we lack tests validating
optional helpers.
1) Over-escaping of "$" variable prefix (M4 "$$" expands to "$")
prevented the path string being expanded to correct value for
the helper directory test. Causing it to always fail.
2) The $ac_top_srcdir variable internal to autoconf may not be set
at the time AC_DEFUN macros are expanded. Making it unreliable.
The $top_srcdir variable which should be used instead has the
same problem with "top" not yet being determined. As such we
use $srcdir instead and hope it expands correctly at ./configure
run time.
Alex Rousskov [Thu, 8 Dec 2022 08:25:44 +0000 (08:25 +0000)]
Do not overwrite caching bans (#1189)
To ban caching, Squid made RequestFlags::cachable false in several
places (e.g., when a "cache deny" rule matched). To permit caching,
Squid also made the flag true in several places (e.g., when
maybeCacheable() was true). That combination worked as intended only
when the cachable=false veto always came after all the cachable=true
support votes. The code did not (and should not) enforce such a
complicated/fragile timing invariant.
Squid now correctly honors caching bans regardless of the updates order.
This change addresses an old XXX, but we are not aware of any specific
bugs fixed by this change. The primary purpose of this change is to make
the existing baseline ban-honoring functionality easy to maintain.
Also reduced code duplication across cachable=false,noCache=true code.
Amos Jeffries [Mon, 5 Dec 2022 15:52:26 +0000 (15:52 +0000)]
Drop CBDATA debugging (#1199)
This was occasionally (but still rare) useful when CBDATA was the
only method of passing callback parameters. Much of that has now
been replaced with AsyncJob/AsyncCall logic. Making this debug
much less useful.
Remove the --enable-debug-cbdata build option and two cache manager
reports only enabled when that option is built. Greatly reducing
the cbdata logic complexity and removing one of its five APIs
permutations entirely.
Avoid level-3 "failed to find or read error text file" WARNINGs (#1176)
... when (re)configuring ERR_REQUEST_PARSE_TIMEOUT and ERR_RELAY_REMOTE
error templates. These errors cannot be customized externally and should
have been listed as such in commits 9ce4a1e and f5e1794, respectively.
Bug 5162: mgr:index URL do not produce MGR_INDEX template (#1191)
Satisfy mgr:index requests using
* a 200 OK response with a body derived from the MGR_INDEX template (if
that template file was found during (re)configuration) or
* a 404 (Not Found) error response (otherwise).
Broken in 2019 commit 7e6eabb, when Squid started replying using a 200
OK response with a hard-coded "mgr_index" text as a body, ignoring any
configured MGR_INDEX template.
Alex Rousskov [Thu, 1 Dec 2022 05:49:57 +0000 (05:49 +0000)]
Make Optional<bool> trivially copyable (#1188)
ipc/TypedMsgHdr.h: static assertion failed: putPod() used for a POD
ActionParams.cc:44: required from here [with Pod = RequestFlags]
The known XXX in Optional destructor has started to bite us because
pending changes expose memcpy(3)-based IPC serialization code to
Optional<bool> flags. It is possible to mimic standard std::optional
implementations, avoiding that XXX, but that requires rather
sophisticated C++ tricks with placement new() and such. Specializing the
whole Optional is a better alternative for this _temporary_ class IMO.
Amos Jeffries [Mon, 21 Nov 2022 05:20:32 +0000 (05:20 +0000)]
Remove Mem::Report() and polish memory_pools_limit storage (#1184)
The official code should not call Mem::Report(). There is no
justification for main.cc treating memory module in such a
special way. The reporting code itself has problems.
Removing all this code is better than leaving or polishing it.
Do not blame cache_peer for 4xx CONNECT responses (#1166)
To avoid using a problematic cache_peer, Squid penalizes cache_peers for
a variety of problems. However, a problem like an HTTP 403 Forbidden
response may be caused by the client or other factors unrelated to the
cache_peer that sent the response to Squid. In those cases, the
cache_peer is not at fault and should not be marked dead, even after
many such responses.
This change stops blaming cache_peers for HTTP 4xx outcomes of Squid
CONNECT requests. Currently, such outcomes only happen when a cache_peer
responds with a 4xx reply, but the new code also treats Squid-generated
4xx error responses (while trying to establish a connection to the
cache_peer) the same way.
This hard-coded logic covers known use cases. If different use cases
surface, we can make Squid behavior configurable.
Amos Jeffries [Thu, 10 Nov 2022 05:56:30 +0000 (05:56 +0000)]
Remove --enable-kill-parent-hack (#1178)
This feature has been deprecated for some time.
The useful functionality can more safely be implemented in the parent
script by using the --foreground command line option and waiting for
the Squid process to exit.
It has always been labeled dangerous as the parent process can be
PID 1 or some other vital system process than the intended
RunCache script. Currently it breaks SMP support for manually
killing a single worker or disker process.
Alex Rousskov [Thu, 10 Nov 2022 02:01:29 +0000 (02:01 +0000)]
Sort CA certificates in tls-cert=bundle (#1177)
... as opposed to requiring the bundle to contain CA certificates in the
correct on-the-wire/issuing order and warning about (and then ignoring)
out-of-order bundled certificates.
This enhancement makes it easier to configure Squid to send the right
intermediate certificates, especially during certificate upgrades when
intermediate certificates (which may be obtained from a source not
tightly coordinated with the signing certificate source) is likely to
contain a mix of old and new intermediate certificates.
Squid has to (and did) check the certificate issuing order anyway. The
new code uses very similar checks to sort intermediate certificates.
No on-the-wire changes expected for Squid configurations that have
correctly ordered certificate bundles. Other configurations may start
sending the previously missing intermediate certificates.
Alex Rousskov [Wed, 9 Nov 2022 21:29:33 +0000 (21:29 +0000)]
Improve cache_peer reporting and NetDB cache_peer search (#1174)
Squid used several cache_peer configuration components and CachePeer
fields to identify cache peers in code and cache.log messages: (the
lowercase version of) cache_peer hostname, cache_peer (hostname,
http_port) tuple, and cache_peer name=value (which uses cache_peer
hostname by default).
Inconsistent identification led to admin confusion (e.g., a message
about cache_peer A was misinterpreted as being about cache_peer B) and
bugs (e.g., indexing storage by component X, but searching using
component Y). This change cements a single component as a unique
cache_peer identifier: CachePeer::name.
Also reject configurations with cache_peer naming errors in
cache_peer_access and neighbor_type_domain directives (instead of
reporting but otherwise ignoring the problematic configuration lines).
The following CachePeer reporting principles were used:
* To identify a specific cache_peer p (among all the configured ones) in
a cache.log message, print that peer (i.e. use `os << *p` or
equivalent). CachePeer printing code will print that peer name, which
is guaranteed to be unique across all configured cache_peers.
* After identifying a cache_peer, do not dump its various details like
ICP port numbers, especially when they are not relevant to the message
(i.e. provide no useful context-related information going beyond that
cache_peer identification).
The following bugs were fixed:
* NetDB code is storing cache_peer hostname[1] but was searching for a
cache_peer with a matching name[2] and, hence, usually failing to find
explicitly named cache_peers. Probably broken since 2003 commit be75332 that added name=value support and updated peerFindByName() to
use CachePeer::name but left storage code[1] unchanged.
[1] netdbPeerAdd(): p->peername = netdbPeerName(e->host);
[2] netdbClosestParent(): p = peerFindByName(h->peername);
* netdbClosestParent() could not find the closest cache_peer in certain
cases involving multiple cache_peers with the same hostname because
the code incorrectly stopped looking for eligible same-hostname
cache_peers after correctly discarding the first one.
The above NetDB problems may imply that NetDB code needs to be
refactored to store CachePeer pointers instead of CachePeer hostnames,
but that serious change deserves more analysis (and a dedicated PR).
* cache_peer name=value configurations with empty/missing name value
could dereference a null name pointer while misleading developers into
thinking that a previously set name could be erased/forgotten. Squid
now rejects such configurations.
* neighborIgnoreNonPeer() was allocating CachePeer::host using new() but
~CachePeer is freeing its host data member using xfree().
Also removed peerFindByNameAndPort() unused since inception at db1cd23.
Mimic GET reforwarding decisions when our CONNECT fails (#1168)
Use FwdState::reforwardableStatus() logic when deciding whether to
reforward our CONNECT request after a failed tunneling attempt.
Also honor forward_max_tries limit when retrying tunneling attempts.
These TunnelStateData fixes deal with ordinary CONNECT traffic (no
SslBump). FwdState also handles CONNECT requests (with SslBump). We make
that CONNECT handling more consistent across these classes (in addition
to making it more consistent across CONNECT and GET/etc. methods).
Co-authored-by: Alex Rousskov <rousskov@measurement-factory.com>
Alex Rousskov [Fri, 21 Oct 2022 15:57:49 +0000 (15:57 +0000)]
Maintenance: Drop an unused, misleading condition (#1169)
According to Autoconf NEWS, AC_USE_SYSTEM_EXTENSIONS is defined with
AC_DEFUN_ONCE since Autoconf v2.63b: "This means a subtle change in
semantics; previously, an AC_DEFUN macro could expand [this macro]
multiple times or surround the macro inside shell conditional text to
bypass [its] effects, but now the macro will expand exactly once, and
prior to the start of any enclosing AC_DEFUN macro".
In the generated ./configure script, the if statement no longer had any
code inside it, and AC_USE_SYSTEM_EXTENSIONS was unconditionally
expanded above the if statement.
Fix milliseconds in certain cache.log messages (#1167)
This change fixes two bugs:
1. The milliseconds part of an early message time was wrong because
debugLogTime() got it from the current_time global instead of the
saved message timestamp. This problem probably affected only early
saved messages in rare situations where current_time was updated
between message creation and message writing. This fix adjusts
DebugMessageHeader::timestamp to store message time with microsecond
precision.
2. Level-0/1 messages that followed same-second level-2+ messages were
logged with an unwanted (and stale) milliseconds value. This fix
forces a reset of the message line format buffer in relevant cases.
Amos Jeffries [Mon, 17 Oct 2022 17:27:07 +0000 (17:27 +0000)]
CI: Install all known dependencies for test-builds.sh on GitHub (#1162)
The apt-get build-dep trick makes us dependent on Ubuntu decisions, but
it saves us work, and we can always add more dependencies manually if we
discover holes in the Ubuntu-maintained list.
Co-authored-by: Alex Rousskov <rousskov@measurement-factory.com>
Alex Rousskov [Mon, 17 Oct 2022 13:22:34 +0000 (13:22 +0000)]
Bug 5241: Block all non-localhost requests by default (#1164)
This change goes one step further than commit 6d2f8ed in satisfying the
same "by default, introduce no new attack vectors" principle.
Commit 6d2f8ed blocked access to localhost and to-link-local services
but still allowed access to potentially vulnerable popular cloud
instance metadata services running on site-local IPv6 addresses. It also
still allowed external access to localnet services that could be
completely unprepared for such dangers! This change closes those holes.
This default configuration has two extra deny rules. These rules become
necessary only when the admin adds an "http_access allow" rule below
them. We enabled these rules despite their overheads, including more DNS
queries in some environments, so that the admin does not have to
remember to enable them when following our "INSERT...HERE" instructions.
Alex Rousskov [Sun, 16 Oct 2022 21:59:28 +0000 (21:59 +0000)]
Fix ./configure detection of various LDAP functions (#1163)
Commit 0d57ed2 started using $LIBLDAP_LIBS as AC_CHECK_LIB() argument,
but that does not work because AC_CHECK_LIB() expects a library name (X)
rather than a linker option (-lX). The macro adds a "-l" prefix itself:
checking for ldap_url_desc2str in -l-lldap -llber... no
checking for ldap_url_parse in -l-lldap -llber... no
checking for ldap_start_tls_s in -l-lldap -llber... no
Alex Rousskov [Mon, 10 Oct 2022 23:07:02 +0000 (23:07 +0000)]
Bug 5241: Block to-localhost, to-link-local requests by default (#1161)
Squid suggested blocking to-localhost access since 2001 commit 4cc6eb1.
At that time, the default was not adjusted because some use cases were
known to require to-localhost access. However, the existence of special
cases should not affect defaults! The _default_ configuration should
either block all traffic or only allow traffic that is unlikely to
introduce new attack vectors into the network.
Also block to-link-local traffic (by default), for very similar reasons:
Popular cloud services use well-known IPv4 link-local (i.e. RFC 3927)
addresses (a.k.a. APIPA), to provide sensitive instance metadata
information, via HTTP, to instance users and scripts. Given cloud
popularity, those special addresses become nearly as ubiquitous as
127.0.0.1. Cloud provider networks shield metadata services from
external accesses, but proxies like Squid that forward external HTTP
requests may circumvent that protection.
projects / thirdparty / squid.git / log
