From d7952b9bcbde1b012e4eb66943a535884cc55ce5 Mon Sep 17 00:00:00 2001 From: Helmut Grohne Date: Mon, 1 Jul 2024 11:01:06 +0000 Subject: [PATCH 01/17] Fix cross-compile detecting kerberos due to AC_RUN_IFELSE (#1851) --- CONTRIBUTORS | 1 + acinclude/krb5.m4 | 8 ++++---- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/CONTRIBUTORS b/CONTRIBUTORS index cc104be4e85..26a754f17df 100644 --- a/CONTRIBUTORS +++ b/CONTRIBUTORS @@ -219,6 +219,7 @@ Thank you! Hasso Tepper Heinrich Schuchardt helix84 + Helmut Grohne Henrik Nordstrom Henrik Nordstrom Hide Nagaoka diff --git a/acinclude/krb5.m4 b/acinclude/krb5.m4 index 6b68eb9e102..876825a7779 100644 --- a/acinclude/krb5.m4 +++ b/acinclude/krb5.m4 @@ -78,7 +78,7 @@ AC_DEFUN([SQUID_CHECK_KRB5_HEIMDAL_BROKEN_KRB5_H], [ AC_CACHE_CHECK([for broken Heimdal krb5.h],squid_cv_broken_heimdal_krb5_h, [ SQUID_STATE_SAVE(squid_krb5_heimdal_test) CPPFLAGS="-I${srcdir:-.} $CPPFLAGS" - AC_RUN_IFELSE([AC_LANG_SOURCE([[ + AC_LINK_IFELSE([AC_LANG_SOURCE([[ #include int main(void) @@ -88,7 +88,7 @@ main(void) return 0; } ]])], [ squid_cv_broken_heimdal_krb5_h=no ], [ - AC_RUN_IFELSE([AC_LANG_SOURCE([[ + AC_LINK_IFELSE([AC_LANG_SOURCE([[ #define HAVE_BROKEN_HEIMDAL_KRB5_H 1 #include "compat/krb5.h" int @@ -172,7 +172,7 @@ int main(int argc, char *argv[]) dnl checks that gssapi is ok, and sets squid_cv_working_gssapi accordingly AC_DEFUN([SQUID_CHECK_WORKING_GSSAPI], [ AC_CACHE_CHECK([for working gssapi], squid_cv_working_gssapi, [ - AC_RUN_IFELSE([AC_LANG_SOURCE([[ + AC_LINK_IFELSE([AC_LANG_SOURCE([[ #if HAVE_GSS_H #include #endif @@ -255,7 +255,7 @@ AC_DEFUN([SQUID_CHECK_WORKING_KRB5],[ AC_CACHE_CHECK([for working krb5], squid_cv_working_krb5, [ SQUID_STATE_SAVE(squid_krb5_test) CPPFLAGS="-I${srcdir:-.} $CPPFLAGS" - AC_RUN_IFELSE([AC_LANG_SOURCE([[ + AC_LINK_IFELSE([AC_LANG_SOURCE([[ #include "compat/krb5.h" int main(void) From 314e43047145123e0ea018186445fd720ba6107c Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Tue, 2 Jul 2024 17:35:14 +0000 Subject: [PATCH 02/17] 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. Co-authored-by: Amos Jeffries --- src/cache_cf.cc | 23 +++++++++++---- src/cf_gen.cc | 2 +- test-suite/squidconf/bad-pp-space.conf | 28 +++++++++++++++++++ .../squidconf/bad-pp-space.conf.instructions | 6 ++++ 4 files changed, 53 insertions(+), 6 deletions(-) create mode 100644 test-suite/squidconf/bad-pp-space.conf create mode 100644 test-suite/squidconf/bad-pp-space.conf.instructions diff --git a/src/cache_cf.cc b/src/cache_cf.cc index 2f5ad01eb24..dd7eff5dbdb 100644 --- a/src/cache_cf.cc +++ b/src/cache_cf.cc @@ -22,6 +22,7 @@ #include "auth/Config.h" #include "auth/Scheme.h" #include "AuthReg.h" +#include "base/CharacterSet.h" #include "base/PackableStream.h" #include "base/RunnersRegistry.h" #include "cache_cf.h" @@ -289,10 +290,22 @@ SetConfigFilename(char const *file_name, bool is_pipe) cfg_filename = file_name; } +/// Determines whether the given squid.conf character is a token-delimiting +/// space character according to squid.conf preprocessor grammar. That grammar +/// only recognizes two space characters: ASCII SP and HT. Unlike isspace(3), +/// this function is not sensitive to locale(1) and does not classify LF, VT, +/// FF, and CR characters as token-delimiting space. However, some squid.conf +/// directive-specific parsers still define space based on isspace(3). +static bool +IsSpace(const char ch) +{ + return CharacterSet::WSP[ch]; +} + static const char* skip_ws(const char* s) { - while (xisspace(*s)) + while (IsSpace(*s)) ++s; return s; @@ -370,7 +383,7 @@ trim_trailing_ws(char* str) { assert(str != nullptr); unsigned i = strlen(str); - while ((i > 0) && xisspace(str[i - 1])) + while ((i > 0) && IsSpace(str[i - 1])) --i; str[i] = '\0'; } @@ -387,7 +400,7 @@ FindStatement(const char* line, const char* statement) str += len; if (*str == '\0') return str; - else if (xisspace(*str)) + else if (IsSpace(*str)) return skip_ws(str); } @@ -498,7 +511,7 @@ parseOneConfigFile(const char *file_name, unsigned int depth) if (file == token) continue; /* Not a valid #line directive, may be a comment */ - while (*file && xisspace((unsigned char) *file)) + while (*file && IsSpace(*file)) ++file; if (*file) { @@ -556,7 +569,7 @@ parseOneConfigFile(const char *file_name, unsigned int depth) fatalf("'else' without 'if'\n"); } else if (if_states.empty() || if_states.back()) { // test last if-statement meaning if present /* Handle includes here */ - if (tmp_line_len >= 9 && strncmp(tmp_line, "include", 7) == 0 && xisspace(tmp_line[7])) { + if (tmp_line_len >= 9 && strncmp(tmp_line, "include", 7) == 0 && IsSpace(tmp_line[7])) { err_count += parseManyConfigFiles(tmp_line + 8, depth + 1); } else { try { diff --git a/src/cf_gen.cc b/src/cf_gen.cc index 3ffe4da46c4..2294668b68f 100644 --- a/src/cf_gen.cc +++ b/src/cf_gen.cc @@ -649,7 +649,7 @@ gen_parse(const EntryList &head, std::ostream &fout) "parse_line(char *buff)\n" "{\n" "\tchar\t*token;\n" - "\tif ((token = strtok(buff, w_space)) == NULL) \n" + "\tif ((token = strtok(buff, \" \\t\")) == NULL) \n" "\t\treturn 1;\t/* ignore empty lines */\n" "\tConfigParser::SetCfgLine(strtok(nullptr, \"\"));\n"; diff --git a/test-suite/squidconf/bad-pp-space.conf b/test-suite/squidconf/bad-pp-space.conf new file mode 100644 index 00000000000..e56d3409e1a --- /dev/null +++ b/test-suite/squidconf/bad-pp-space.conf @@ -0,0 +1,28 @@ +## Copyright (C) 1996-2024 The Squid Software Foundation and contributors +## +## Squid software is distributed under GPLv2+ license and includes +## contributions from numerous individuals and organizations. +## Please see the COPYING and CONTRIBUTORS files for details. +## + +# The following line has leading and trailing SP and HT characters around +# directive name. The preprocessor is supposed to silently strip them. + memory_pools off + +# The following line has an ASCII VT character (0x0B) after a recognized +# directive name. VT character is followed by a regular SP character to +# make sure preprocessor can isolate (some) directive name. +http_access deny all + +# The following line has UTF-8 No-Break Space character (0xC2A0) after a +# recognized directive name. No SP character follows NBSP to test that +# preprocessor does not isolate recognized directive names based on their +# spelling (it should isolate the name token based on spaces instead). +debug_options ALL,1 2,3 + +# The following line has UTF-8 Wavy Low Line character (0xEFB98F) instead of +# the first ASCII underscore in what would otherwise be a recognized directive +# name. This test validates that preprocessor does not isolate unrecognized +# directive names based on their spelling (it should isolate the name token +# based on spaces instead). +forward﹏max_tries 13 diff --git a/test-suite/squidconf/bad-pp-space.conf.instructions b/test-suite/squidconf/bad-pp-space.conf.instructions new file mode 100644 index 00000000000..8a06ee7faf0 --- /dev/null +++ b/test-suite/squidconf/bad-pp-space.conf.instructions @@ -0,0 +1,6 @@ +expect-failure FATAL:.*Found.3.unrecognized.directive.* +expect-messages < Date: Sun, 7 Jul 2024 03:03:00 +0000 Subject: [PATCH 03/17] 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: ERROR: configuration failure: NfMarkConfig: invalid value '0/10'... exception location: NfMarkConfig.cc(23) getNfmark Probably broken since 2014 commit 01f2137d. --- src/parser/Tokenizer.cc | 1 - src/tests/testTokenizer.cc | 309 +++++++++++++++++++++++++++++++++++++ 2 files changed, 309 insertions(+), 1 deletion(-) diff --git a/src/parser/Tokenizer.cc b/src/parser/Tokenizer.cc index 51654bae90d..6544fb44823 100644 --- a/src/parser/Tokenizer.cc +++ b/src/parser/Tokenizer.cc @@ -264,7 +264,6 @@ Parser::Tokenizer::int64(int64_t & result, int base, bool allowSign, const SBuf: if (base == 0) { if ( *s == '0') { base = 8; - ++s; } else { base = 10; } diff --git a/src/tests/testTokenizer.cc b/src/tests/testTokenizer.cc index e69fec4f951..28532bce0bd 100644 --- a/src/tests/testTokenizer.cc +++ b/src/tests/testTokenizer.cc @@ -248,6 +248,285 @@ TestTokenizer::testTokenizerInt64() CPPUNIT_ASSERT(t.buf().isEmpty()); } + // When interpreting octal numbers, standard strtol() and Tokenizer::int64() + // treat leading zero as a part of sequence of digits rather than a + // character used _exclusively_ as base indicator. Thus, it is not possible + // to create an invalid octal number with an explicit octal base -- the + // first invalid character after the base will be successfully ignored. This + // treatment also makes it difficult to define "shortest valid octal input". + // Here, we are just enumerating interesting "short input" octal cases in + // four dimensions: + // 1. int64(base) argument: forced or auto-detected; + // 2. base character ("0") in input: absent or present; + // 3. post-base digits in input: absent, valid, or invalid; + // 4. input length limits via int64(length) argument: unlimited or limited. + + // forced base; input: no base, no post-base digits, unlimited + { + int64_t rv; + Parser::Tokenizer t(SBuf("")); + CPPUNIT_ASSERT(!t.int64(rv, 8)); + CPPUNIT_ASSERT_EQUAL(SBuf(""), t.buf()); + } + + // forced base; input: no base, no post-base digits, limited + { + int64_t rv; + Parser::Tokenizer t(SBuf("7")); + CPPUNIT_ASSERT(!t.int64(rv, 8, false, 0)); + CPPUNIT_ASSERT_EQUAL(SBuf("7"), t.buf()); + } + + // forced base; input: no base, one valid post-base digit, unlimited + { + int64_t rv; + Parser::Tokenizer t(SBuf("4")); + const int64_t benchmark = 04; + CPPUNIT_ASSERT(t.int64(rv, 8)); + CPPUNIT_ASSERT_EQUAL(benchmark, rv); + CPPUNIT_ASSERT_EQUAL(SBuf(""), t.buf()); + } + + // forced base; input: no base, one valid post-base digit, limited + { + int64_t rv; + Parser::Tokenizer t(SBuf("46")); + const int64_t benchmark = 04; + CPPUNIT_ASSERT(t.int64(rv, 8, false, 1)); + CPPUNIT_ASSERT_EQUAL(benchmark, rv); + CPPUNIT_ASSERT_EQUAL(SBuf("6"), t.buf()); + } + + // forced base; input: no base, one invalid post-base digit, unlimited + { + int64_t rv; + Parser::Tokenizer t(SBuf("8")); + CPPUNIT_ASSERT(!t.int64(rv, 8)); + CPPUNIT_ASSERT_EQUAL(SBuf("8"), t.buf()); + } + + // forced base; input: no base, one invalid post-base digit, limited + { + int64_t rv; + Parser::Tokenizer t(SBuf("80")); + CPPUNIT_ASSERT(!t.int64(rv, 8, false, 1)); + CPPUNIT_ASSERT_EQUAL(SBuf("80"), t.buf()); + } + + // repeat the above six octal cases, but now with base character in input + + // forced base; input: base, no post-base digits, unlimited + { + int64_t rv; + Parser::Tokenizer t(SBuf("0")); + const int64_t benchmark = 0; + CPPUNIT_ASSERT(t.int64(rv, 8)); + CPPUNIT_ASSERT_EQUAL(benchmark, rv); + CPPUNIT_ASSERT_EQUAL(SBuf(""), t.buf()); + } + + // forced base; input: base, no post-base digits, limited + { + int64_t rv; + Parser::Tokenizer t(SBuf("07")); + const int64_t benchmark = 0; + CPPUNIT_ASSERT(t.int64(rv, 8, false, 1)); + CPPUNIT_ASSERT_EQUAL(benchmark, rv); + CPPUNIT_ASSERT_EQUAL(SBuf("7"), t.buf()); + } + + // forced base; input: base, one valid post-base digit, unlimited + { + int64_t rv; + Parser::Tokenizer t(SBuf("04")); + const int64_t benchmark = 04; + CPPUNIT_ASSERT(t.int64(rv, 8)); + CPPUNIT_ASSERT_EQUAL(benchmark, rv); + CPPUNIT_ASSERT_EQUAL(SBuf(""), t.buf()); + } + + // forced base; input: base, one valid post-base digit, limited + { + int64_t rv; + Parser::Tokenizer t(SBuf("046")); + const int64_t benchmark = 04; + CPPUNIT_ASSERT(t.int64(rv, 8, false, 2)); + CPPUNIT_ASSERT_EQUAL(benchmark, rv); + CPPUNIT_ASSERT_EQUAL(SBuf("6"), t.buf()); + } + + // forced base; input: base, one invalid post-base digit, unlimited + { + int64_t rv; + Parser::Tokenizer t(SBuf("08")); + const int64_t benchmark = 00; + CPPUNIT_ASSERT(t.int64(rv, 8)); + CPPUNIT_ASSERT_EQUAL(benchmark, rv); + CPPUNIT_ASSERT_EQUAL(SBuf("8"), t.buf()); + } + + // forced base; input: base, one invalid post-base digit, limited + { + int64_t rv; + Parser::Tokenizer t(SBuf("08")); + const int64_t benchmark = 00; + CPPUNIT_ASSERT(t.int64(rv, 8, false, 2)); + CPPUNIT_ASSERT_EQUAL(benchmark, rv); + CPPUNIT_ASSERT_EQUAL(SBuf("8"), t.buf()); + } + + // And now repeat six "with base character in input" octal cases but with + // auto-detected base. When octal cases below say "auto-detected base", they + // describe int64() base=0 parameter value. Current int64() implementation + // does auto-detect base as octal in all of these cases, but that might + // change, and some of these cases (e.g., "0") can also be viewed as a + // non-octal input case as well. These cases do not attempt to test base + // detection. They focus on other potential problems. + + // auto-detected base; input: base, no post-base digits, unlimited + { + int64_t rv; + Parser::Tokenizer t(SBuf("0")); + const int64_t benchmark = 00; + CPPUNIT_ASSERT(t.int64(rv, 0)); + CPPUNIT_ASSERT_EQUAL(benchmark, rv); + CPPUNIT_ASSERT_EQUAL(SBuf(""), t.buf()); + } + + // auto-detected base; input: base, no post-base digits, limited + { + int64_t rv; + Parser::Tokenizer t(SBuf("07")); + const int64_t benchmark = 0; + CPPUNIT_ASSERT(t.int64(rv, 0, false, 1)); + CPPUNIT_ASSERT_EQUAL(benchmark, rv); + CPPUNIT_ASSERT_EQUAL(SBuf("7"), t.buf()); + } + + // auto-detected base; input: base, one valid post-base digit, unlimited + { + int64_t rv; + Parser::Tokenizer t(SBuf("04")); + const int64_t benchmark = 04; + CPPUNIT_ASSERT(t.int64(rv, 0)); + CPPUNIT_ASSERT_EQUAL(benchmark, rv); + CPPUNIT_ASSERT_EQUAL(SBuf(""), t.buf()); + } + + // auto-detected base; input: base, one valid post-base digit, limited + { + int64_t rv; + Parser::Tokenizer t(SBuf("046")); + const int64_t benchmark = 04; + CPPUNIT_ASSERT(t.int64(rv, 0, false, 2)); + CPPUNIT_ASSERT_EQUAL(benchmark, rv); + CPPUNIT_ASSERT_EQUAL(SBuf("6"), t.buf()); + } + + // auto-detected base; input: base, one invalid post-base digit, unlimited + { + int64_t rv; + Parser::Tokenizer t(SBuf("08")); + const int64_t benchmark = 00; + CPPUNIT_ASSERT(t.int64(rv, 0)); + CPPUNIT_ASSERT_EQUAL(benchmark, rv); + CPPUNIT_ASSERT_EQUAL(SBuf("8"), t.buf()); + } + + // auto-detected base; input: base, one invalid post-base digit, limited + { + int64_t rv; + Parser::Tokenizer t(SBuf("08")); + const int64_t benchmark = 00; + CPPUNIT_ASSERT(t.int64(rv, 0, false, 2)); + CPPUNIT_ASSERT_EQUAL(benchmark, rv); + CPPUNIT_ASSERT_EQUAL(SBuf("8"), t.buf()); + } + + // this ends four-dimensional enumeration of octal cases described earlier + + // check octal base auto-detection + { + int64_t rv; + Parser::Tokenizer t(SBuf("0128")); + const int64_t benchmark = 012; + CPPUNIT_ASSERT(t.int64(rv, 0)); + CPPUNIT_ASSERT_EQUAL(benchmark, rv); + CPPUNIT_ASSERT_EQUAL(SBuf("8"), t.buf()); + } + + // check that octal base auto-detection is not confused by repeated zeros + { + int64_t rv; + Parser::Tokenizer t(SBuf("00000000071")); + const int64_t benchmark = 00000000071; + CPPUNIT_ASSERT(t.int64(rv)); + CPPUNIT_ASSERT_EQUAL(benchmark,rv); + CPPUNIT_ASSERT_EQUAL(SBuf(""), t.buf()); + } + + // check that forced octal base is not confused by hex prefix + { + int64_t rv; + Parser::Tokenizer t(SBuf("0x5")); + const int64_t benchmark = 0; + CPPUNIT_ASSERT(t.int64(rv, 8)); + CPPUNIT_ASSERT_EQUAL(benchmark, rv); + CPPUNIT_ASSERT_EQUAL(SBuf("x5"), t.buf()); + } + + // autodetect decimal base in shortest valid input + { + int64_t rv; + Parser::Tokenizer t(SBuf("1")); + const int64_t benchmark = 1; + CPPUNIT_ASSERT(t.int64(rv)); + CPPUNIT_ASSERT_EQUAL(benchmark,rv); + CPPUNIT_ASSERT(t.buf().isEmpty()); + } + + // autodetect hex base in shortest valid input + { + int64_t rv; + Parser::Tokenizer t(SBuf("0X1")); + const int64_t benchmark = 0X1; + CPPUNIT_ASSERT(t.int64(rv)); + CPPUNIT_ASSERT_EQUAL(benchmark,rv); + CPPUNIT_ASSERT(t.buf().isEmpty()); + } + + // invalid (when autodetecting base) input matching hex base + { + int64_t rv; + Parser::Tokenizer t(SBuf("0x")); + CPPUNIT_ASSERT(!t.int64(rv)); + CPPUNIT_ASSERT_EQUAL(SBuf("0x"), t.buf()); + } + + // invalid (when forcing hex base) input matching hex base + { + int64_t rv; + Parser::Tokenizer t(SBuf("0x")); + CPPUNIT_ASSERT(!t.int64(rv, 16)); + CPPUNIT_ASSERT_EQUAL(SBuf("0x"), t.buf()); + } + + // invalid (when autodetecting base and limiting) input matching hex base + { + int64_t rv; + Parser::Tokenizer t(SBuf("0x2")); + CPPUNIT_ASSERT(!t.int64(rv, 0, true, 2)); + CPPUNIT_ASSERT_EQUAL(SBuf("0x2"), t.buf()); + } + + // invalid (when forcing hex base and limiting) input matching hex base + { + int64_t rv; + Parser::Tokenizer t(SBuf("0x3")); + CPPUNIT_ASSERT(!t.int64(rv, 16, false, 2)); + CPPUNIT_ASSERT_EQUAL(SBuf("0x3"), t.buf()); + } + // API mismatch: don't eat leading space { int64_t rv; @@ -264,6 +543,36 @@ TestTokenizer::testTokenizerInt64() CPPUNIT_ASSERT_EQUAL(SBuf(" 1234"), t.buf()); } + // zero corner case: repeated zeros + { + int64_t rv; + Parser::Tokenizer t(SBuf("00")); + const int64_t benchmark = 00; + CPPUNIT_ASSERT(t.int64(rv)); + CPPUNIT_ASSERT_EQUAL(benchmark,rv); + CPPUNIT_ASSERT_EQUAL(SBuf(""), t.buf()); + } + + // zero corner case: "positive" zero + { + int64_t rv; + Parser::Tokenizer t(SBuf("+0")); + const int64_t benchmark = +0; + CPPUNIT_ASSERT(t.int64(rv)); + CPPUNIT_ASSERT_EQUAL(benchmark,rv); + CPPUNIT_ASSERT_EQUAL(SBuf(""), t.buf()); + } + + // zero corner case: "negative" zero + { + int64_t rv; + Parser::Tokenizer t(SBuf("-0")); + const int64_t benchmark = -0; + CPPUNIT_ASSERT(t.int64(rv)); + CPPUNIT_ASSERT_EQUAL(benchmark,rv); + CPPUNIT_ASSERT_EQUAL(SBuf(""), t.buf()); + } + // trailing spaces { int64_t rv; From f69f4cee5cbe44bf60f8acb30b777a24e631d8ff Mon Sep 17 00:00:00 2001 From: Eduard Bagdasaryan Date: Mon, 8 Jul 2024 10:28:08 +0000 Subject: [PATCH 04/17] 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. --- src/ClientRequestContext.h | 1 - src/PeerSelectState.h | 1 - src/SnmpRequest.h | 1 - src/adaptation/AccessCheck.cc | 5 ++--- src/adaptation/AccessCheck.h | 1 - src/client_side_request.cc | 22 +++++++++------------- src/peer_select.cc | 18 ++++-------------- 7 files changed, 15 insertions(+), 34 deletions(-) diff --git a/src/ClientRequestContext.h b/src/ClientRequestContext.h index 893a9c6710c..5657cf1eb40 100644 --- a/src/ClientRequestContext.h +++ b/src/ClientRequestContext.h @@ -62,7 +62,6 @@ class ClientRequestContext : public RefCountable #endif ClientHttpRequest *http; - ACLChecklist *acl_checklist = nullptr; ///< need ptr back so we can unregister if needed int redirect_state = REDIRECT_NONE; int store_id_state = REDIRECT_NONE; diff --git a/src/PeerSelectState.h b/src/PeerSelectState.h index 86c168dfed3..c11d79968ab 100644 --- a/src/PeerSelectState.h +++ b/src/PeerSelectState.h @@ -158,7 +158,6 @@ class PeerSelector: public Dns::IpReceiver */ CachePeer *hit; peer_t hit_type; - ACLChecklist *acl_checklist; typedef CbcPointer Initiator; Initiator initiator_; ///< recipient of the destinations we select; use interestedInitiator() to access diff --git a/src/SnmpRequest.h b/src/SnmpRequest.h index a9113e8b9b7..9adeb9089bf 100644 --- a/src/SnmpRequest.h +++ b/src/SnmpRequest.h @@ -28,7 +28,6 @@ class SnmpRequest Ip::Address from; struct snmp_pdu *PDU; - ACLChecklist *acl_checklist; u_char *community; struct snmp_session session; diff --git a/src/adaptation/AccessCheck.cc b/src/adaptation/AccessCheck.cc index a28df6b2368..475ecf25881 100644 --- a/src/adaptation/AccessCheck.cc +++ b/src/adaptation/AccessCheck.cc @@ -46,8 +46,7 @@ Adaptation::AccessCheck::Start(Method method, VectPoint vp, Adaptation::AccessCheck::AccessCheck(const ServiceFilter &aFilter, Adaptation::Initiator *initiator): AsyncJob("AccessCheck"), filter(aFilter), - theInitiator(initiator), - acl_checklist(nullptr) + theInitiator(initiator) { #if ICAP_CLIENT Adaptation::Icap::History::Pointer h = filter.request->icapHistory(); @@ -129,7 +128,7 @@ Adaptation::AccessCheck::checkCandidates() while (!candidates.empty()) { if (AccessRule *r = FindRule(topCandidate())) { /* BUG 2526: what to do when r->acl is empty?? */ - acl_checklist = new ACLFilledChecklist(r->acl, filter.request); + const auto acl_checklist = new ACLFilledChecklist(r->acl, filter.request); acl_checklist->updateAle(filter.al); acl_checklist->updateReply(filter.reply); acl_checklist->syncAle(filter.request, nullptr); diff --git a/src/adaptation/AccessCheck.h b/src/adaptation/AccessCheck.h index 1807c4da80f..eb65e30bf54 100644 --- a/src/adaptation/AccessCheck.h +++ b/src/adaptation/AccessCheck.h @@ -46,7 +46,6 @@ class AccessCheck: public virtual AsyncJob private: const ServiceFilter filter; CbcPointer theInitiator; ///< the job which ordered this access check - ACLFilledChecklist *acl_checklist; typedef int Candidate; typedef std::vector Candidates; diff --git a/src/client_side_request.cc b/src/client_side_request.cc index 09027b6cf99..1d432a8f524 100644 --- a/src/client_side_request.cc +++ b/src/client_side_request.cc @@ -438,13 +438,13 @@ clientFollowXForwardedForCheck(Acl::Answer answer, void *data) if ((addr = asciiaddr)) { request->indirect_client_addr = addr; request->x_forwarded_for_iterator.cut(l); - calloutContext->acl_checklist = clientAclChecklistCreate(Config.accessList.followXFF, http); + const auto ch = clientAclChecklistCreate(Config.accessList.followXFF, http); if (!Config.onoff.acl_uses_indirect_client) { /* override the default src_addr tested if we have to go deeper than one level into XFF */ - Filled(calloutContext->acl_checklist)->src_addr = request->indirect_client_addr; + ch->src_addr = request->indirect_client_addr; } if (++calloutContext->currentXffHopNumber < SQUID_X_FORWARDED_FOR_HOP_MAX) { - calloutContext->acl_checklist->nonBlockingCheck(clientFollowXForwardedForCheck, data); + ch->nonBlockingCheck(clientFollowXForwardedForCheck, data); return; } const auto headerName = Http::HeaderLookupTable.lookup(Http::HdrType::X_FORWARDED_FOR).name; @@ -666,14 +666,14 @@ ClientRequestContext::clientAccessCheck() http->request->x_forwarded_for_iterator = http->request->header.getList(Http::HdrType::X_FORWARDED_FOR); /* begin by checking to see if we trust direct client enough to walk XFF */ - acl_checklist = clientAclChecklistCreate(Config.accessList.followXFF, http); + const auto acl_checklist = clientAclChecklistCreate(Config.accessList.followXFF, http); acl_checklist->nonBlockingCheck(clientFollowXForwardedForCheck, this); return; } #endif if (Config.accessList.http) { - acl_checklist = clientAclChecklistCreate(Config.accessList.http, http); + const auto acl_checklist = clientAclChecklistCreate(Config.accessList.http, http); acl_checklist->nonBlockingCheck(clientAccessCheckDoneWrapper, this); } else { debugs(0, DBG_CRITICAL, "No http_access configuration found. This will block ALL traffic"); @@ -690,7 +690,7 @@ void ClientRequestContext::clientAccessCheck2() { if (Config.accessList.adapted_http) { - acl_checklist = clientAclChecklistCreate(Config.accessList.adapted_http, http); + const auto acl_checklist = clientAclChecklistCreate(Config.accessList.adapted_http, http); acl_checklist->nonBlockingCheck(clientAccessCheckDoneWrapper, this); } else { debugs(85, 2, "No adapted_http_access configuration. default: ALLOW"); @@ -712,7 +712,6 @@ clientAccessCheckDoneWrapper(Acl::Answer answer, void *data) void ClientRequestContext::clientAccessCheckDone(const Acl::Answer &answer) { - acl_checklist = nullptr; Http::StatusCode status; debugs(85, 2, "The request " << http->request->method << ' ' << http->uri << " is " << answer << @@ -821,7 +820,6 @@ clientRedirectAccessCheckDone(Acl::Answer answer, void *data) { ClientRequestContext *context = (ClientRequestContext *)data; ClientHttpRequest *http = context->http; - context->acl_checklist = nullptr; if (answer.allowed()) redirectStart(http, clientRedirectDoneWrapper, context); @@ -837,7 +835,7 @@ ClientRequestContext::clientRedirectStart() debugs(33, 5, "'" << http->uri << "'"); http->al->syncNotes(http->request); if (Config.accessList.redirector) { - acl_checklist = clientAclChecklistCreate(Config.accessList.redirector, http); + const auto acl_checklist = clientAclChecklistCreate(Config.accessList.redirector, http); acl_checklist->nonBlockingCheck(clientRedirectAccessCheckDone, this); } else redirectStart(http, clientRedirectDoneWrapper, this); @@ -852,7 +850,6 @@ clientStoreIdAccessCheckDone(Acl::Answer answer, void *data) { ClientRequestContext *context = static_cast(data); ClientHttpRequest *http = context->http; - context->acl_checklist = nullptr; if (answer.allowed()) storeIdStart(http, clientStoreIdDoneWrapper, context); @@ -874,7 +871,7 @@ ClientRequestContext::clientStoreIdStart() debugs(33, 5,"'" << http->uri << "'"); if (Config.accessList.store_id) { - acl_checklist = clientAclChecklistCreate(Config.accessList.store_id, http); + const auto acl_checklist = clientAclChecklistCreate(Config.accessList.store_id, http); acl_checklist->nonBlockingCheck(clientStoreIdAccessCheckDone, this); } else storeIdStart(http, clientStoreIdDoneWrapper, this); @@ -1313,7 +1310,7 @@ void ClientRequestContext::checkNoCache() { if (Config.accessList.noCache) { - acl_checklist = clientAclChecklistCreate(Config.accessList.noCache, http); + const auto acl_checklist = clientAclChecklistCreate(Config.accessList.noCache, http); acl_checklist->nonBlockingCheck(checkNoCacheDoneWrapper, this); } else { /* unless otherwise specified, we try to cache. */ @@ -1335,7 +1332,6 @@ checkNoCacheDoneWrapper(Acl::Answer answer, void *data) void ClientRequestContext::checkNoCacheDone(const Acl::Answer &answer) { - acl_checklist = nullptr; if (answer.denied()) { http->request->flags.disableCacheUse("a cache deny rule matched"); } diff --git a/src/peer_select.cc b/src/peer_select.cc index 988fe60bbba..ead91c0f991 100644 --- a/src/peer_select.cc +++ b/src/peer_select.cc @@ -243,11 +243,6 @@ PeerSelector::~PeerSelector() entry->ping_status = PING_DONE; } - if (acl_checklist) { - debugs(44, DBG_IMPORTANT, "ERROR: Squid BUG: peer selector gone while waiting for a slow ACL"); - delete acl_checklist; - } - HTTPMSGUNLOCK(request); if (entry) { @@ -342,7 +337,6 @@ PeerSelectionInitiator::startSelectingDestinations(HttpRequest *request, const A void PeerSelector::checkNeverDirectDone(const Acl::Answer answer) { - acl_checklist = nullptr; debugs(44, 3, answer); never_direct = answer; switch (answer) { @@ -370,7 +364,6 @@ PeerSelector::CheckNeverDirectDone(Acl::Answer answer, void *data) void PeerSelector::checkAlwaysDirectDone(const Acl::Answer answer) { - acl_checklist = nullptr; debugs(44, 3, answer); always_direct = answer; switch (answer) { @@ -622,18 +615,16 @@ PeerSelector::selectMore() /** check always_direct; */ const auto ch = new ACLFilledChecklist(Config.accessList.AlwaysDirect, request); ch->al = al; - acl_checklist = ch; - acl_checklist->syncAle(request, nullptr); - acl_checklist->nonBlockingCheck(CheckAlwaysDirectDone, this); + ch->syncAle(request, nullptr); + ch->nonBlockingCheck(CheckAlwaysDirectDone, this); return; } else if (never_direct == ACCESS_DUNNO) { debugs(44, 3, "direct = " << DirectStr[direct] << " (never_direct to be checked)"); /** check never_direct; */ const auto ch = new ACLFilledChecklist(Config.accessList.NeverDirect, request); ch->al = al; - acl_checklist = ch; - acl_checklist->syncAle(request, nullptr); - acl_checklist->nonBlockingCheck(CheckNeverDirectDone, this); + ch->syncAle(request, nullptr); + ch->nonBlockingCheck(CheckNeverDirectDone, this); return; } else if (request->flags.noDirect) { /** if we are accelerating, direct is not an option. */ @@ -1119,7 +1110,6 @@ PeerSelector::PeerSelector(PeerSelectionInitiator *initiator): closest_parent_miss(), hit(nullptr), hit_type(PEER_NONE), - acl_checklist (nullptr), initiator_(initiator) { ; // no local defaults. From f036532e9c0691fe41d7a7c2f50ea5f3e257c882 Mon Sep 17 00:00:00 2001 From: Eduard Bagdasaryan Date: Mon, 8 Jul 2024 21:58:29 +0000 Subject: [PATCH 05/17] Fix PeerDigest lifetime management (#1857) 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). --- src/CachePeer.cc | 4 +- src/PeerDigest.h | 6 +- src/cache_cf.cc | 6 +- src/peer_digest.cc | 167 ++++++++++++++------------------------------- 4 files changed, 59 insertions(+), 124 deletions(-) diff --git a/src/CachePeer.cc b/src/CachePeer.cc index 98a750cb418..11d77164045 100644 --- a/src/CachePeer.cc +++ b/src/CachePeer.cc @@ -41,9 +41,7 @@ CachePeer::~CachePeer() aclDestroyAccessList(&access); #if USE_CACHE_DIGESTS - void *digestTmp = nullptr; - if (cbdataReferenceValidDone(digest, &digestTmp)) - peerDigestNotePeerGone(static_cast(digestTmp)); + delete digest; xfree(digest_url); #endif diff --git a/src/PeerDigest.h b/src/PeerDigest.h index ed5ed57c274..2d760e56964 100644 --- a/src/PeerDigest.h +++ b/src/PeerDigest.h @@ -49,7 +49,7 @@ class DigestFetchState DigestFetchState(PeerDigest *,HttpRequest *); ~DigestFetchState(); - PeerDigest *pd; + CbcPointer pd; StoreEntry *entry; StoreEntry *old_entry; store_client *sc; @@ -79,6 +79,10 @@ class PeerDigest PeerDigest(CachePeer *); ~PeerDigest(); + /// reacts to digest transfer completion + /// \prec DigestFetchState stats were finalized (by calling peerDigestFetchSetStats()) + void noteFetchFinished(const DigestFetchState &, const char *outcomeDescription, bool sawError); + CbcPointer peer; ///< pointer back to peer structure, argh CacheDigest *cd = nullptr; /**< actual digest structure */ const SBuf host; ///< copy of peer->host diff --git a/src/cache_cf.cc b/src/cache_cf.cc index dd7eff5dbdb..10e15d270e6 100644 --- a/src/cache_cf.cc +++ b/src/cache_cf.cc @@ -2392,10 +2392,8 @@ parse_peer(CachePeers **peers) p->connect_fail_limit = 10; #if USE_CACHE_DIGESTS - if (!p->options.no_digest) { - const auto pd = new PeerDigest(p); - p->digest = cbdataReference(pd); // CachePeer destructor unlocks - } + if (!p->options.no_digest) + p->digest = new PeerDigest(p); #endif if (p->secure.encryptTransport) diff --git a/src/peer_digest.cc b/src/peer_digest.cc index 7d290cc9013..0f375ac4c79 100644 --- a/src/peer_digest.cc +++ b/src/peer_digest.cc @@ -44,9 +44,7 @@ int peerDigestSwapInMask(void *, char *, ssize_t); static int peerDigestFetchedEnough(DigestFetchState * fetch, char *buf, ssize_t size, const char *step_name); static void peerDigestFetchStop(DigestFetchState * fetch, char *buf, const char *reason); static void peerDigestFetchAbort(DigestFetchState * fetch, char *buf, const char *reason); -static void peerDigestReqFinish(DigestFetchState * fetch, char *buf, int, int, int, const char *reason, int err); -static void peerDigestPDFinish(DigestFetchState * fetch, int pcb_valid, int err); -static void peerDigestFetchFinish(DigestFetchState * fetch, int err); +static void peerDigestReqFinish(DigestFetchState *, char *buf, const char *reason, int err); static void peerDigestFetchSetStats(DigestFetchState * fetch); static int peerDigestSetCBlock(PeerDigest * pd, const char *buf); static int peerDigestUseful(const PeerDigest * pd); @@ -77,7 +75,7 @@ CBDATA_CLASS_INIT(PeerDigest); CBDATA_CLASS_INIT(DigestFetchState); DigestFetchState::DigestFetchState(PeerDigest *aPd, HttpRequest *req) : - pd(cbdataReference(aPd)), + pd(aPd), entry(nullptr), old_entry(nullptr), sc(nullptr), @@ -104,6 +102,14 @@ DigestFetchState::DigestFetchState(PeerDigest *aPd, HttpRequest *req) : DigestFetchState::~DigestFetchState() { + if (old_entry) { + debugs(72, 3, "deleting old entry"); + storeUnregister(old_sc, old_entry, this); + old_entry->releaseRequest(); + old_entry->unlock("DigestFetchState destructed old"); + old_entry = nullptr; + } + /* unlock everything */ storeUnregister(sc, entry, this); @@ -111,8 +117,6 @@ DigestFetchState::~DigestFetchState() entry = nullptr; HTTPMSGUNLOCK(request); - - assert(pd == nullptr); } PeerDigest::~PeerDigest() @@ -168,21 +172,6 @@ peerDigestSetCheck(PeerDigest * pd, time_t delay) debugs(72, 3, "peerDigestSetCheck: will check peer " << pd->host << " in " << delay << " secs"); } -/* - * called when peer is about to disappear or have already disappeared - */ -void -peerDigestNotePeerGone(PeerDigest * pd) -{ - if (pd->flags.requested) { - debugs(72, 2, "peerDigest: peer " << pd->host << " gone, will destroy after fetch."); - /* do nothing now, the fetching chain will notice and take action */ - } else { - debugs(72, 2, "peerDigest: peer " << pd->host << " is gone, destroying now."); - delete pd; - } -} - /* callback for eventAdd() (with peer digest locked) * request new digest if our copy is too old or if we lack one; * schedule next check otherwise */ @@ -196,10 +185,7 @@ peerDigestCheck(void *data) pd->times.next_check = 0; /* unknown */ - if (pd->peer.set() && !pd->peer.valid()) { - peerDigestNotePeerGone(pd); - return; - } + Assure(pd->peer.valid()); debugs(72, 3, "cache_peer " << RawPointer(pd->peer).orNil()); debugs(72, 3, "peerDigestCheck: time: " << squid_curtime << @@ -349,7 +335,11 @@ peerDigestHandleReply(void *data, StoreIOBuffer receivedData) return; } - assert(fetch->pd); + if (!fetch->pd) { + peerDigestFetchAbort(fetch, fetch->buf, "digest disappeared while loading digest reply from Store"); + return; + } + /* The existing code assumes that the received pointer is * where we asked the data to be put */ @@ -448,7 +438,7 @@ static int peerDigestFetchReply(void *data, char *buf, ssize_t size) { DigestFetchState *fetch = (DigestFetchState *)data; - PeerDigest *pd = fetch->pd; + const auto pd = fetch->pd.get(); assert(pd && buf); assert(!fetch->offset); @@ -535,7 +525,7 @@ peerDigestSwapInCBlock(void *data, char *buf, ssize_t size) return -1; if (size >= (ssize_t)StoreDigestCBlockSize) { - PeerDigest *pd = fetch->pd; + const auto pd = fetch->pd.get(); assert(pd); assert(fetch->entry->mem_obj); @@ -566,9 +556,8 @@ int peerDigestSwapInMask(void *data, char *buf, ssize_t size) { DigestFetchState *fetch = (DigestFetchState *)data; - PeerDigest *pd; - - pd = fetch->pd; + const auto pd = fetch->pd.get(); + assert(pd); assert(pd->cd && pd->cd->mask); /* @@ -602,20 +591,16 @@ peerDigestFetchedEnough(DigestFetchState * fetch, char *buf, ssize_t size, const static const SBuf hostUnknown(""); // peer host (if any) SBuf host = hostUnknown; - PeerDigest *pd = nullptr; + const auto pd = fetch->pd.get(); + Assure(pd); const char *reason = nullptr; /* reason for completion */ const char *no_bug = nullptr; /* successful completion if set */ - const int pdcb_valid = cbdataReferenceValid(fetch->pd); - const int pcb_valid = pdcb_valid && fetch->pd->peer.valid(); /* test possible exiting conditions (the same for most steps!) * cases marked with '?!' should not happen */ if (!reason) { - if (!pdcb_valid || !(pd = fetch->pd)) - reason = "peer digest disappeared?!"; - else - host = pd->host; + host = pd->host; } debugs(72, 6, step_name << ": peer " << host << ", offset: " << @@ -624,9 +609,7 @@ peerDigestFetchedEnough(DigestFetchState * fetch, char *buf, ssize_t size, const /* continue checking (with pd and host known and valid) */ if (!reason) { - if (!pd->peer) - reason = "peer disappeared"; - else if (size < 0) + if (size < 0) reason = "swap failure"; else if (!fetch->entry) reason = "swap aborted?!"; @@ -650,11 +633,7 @@ peerDigestFetchedEnough(DigestFetchState * fetch, char *buf, ssize_t size, const if (reason) { const int level = strstr(reason, "?!") ? 1 : 3; debugs(72, level, "" << step_name << ": peer " << host << ", exiting after '" << reason << "'"); - peerDigestReqFinish(fetch, buf, - 1, pdcb_valid, pcb_valid, reason, !no_bug); - } else { - /* paranoid check */ - assert(pdcb_valid && pcb_valid); + peerDigestReqFinish(fetch, buf, reason, !no_bug); } return reason != nullptr; @@ -667,64 +646,43 @@ peerDigestFetchStop(DigestFetchState * fetch, char *buf, const char *reason) { assert(reason); debugs(72, 2, "peerDigestFetchStop: peer " << fetch->pd->host << ", reason: " << reason); - peerDigestReqFinish(fetch, buf, 1, 1, 1, reason, 0); + peerDigestReqFinish(fetch, buf, reason, 0); } -/* call this when all callback data is valid but something bad happened */ +/// diff reducer: handle a peer digest fetch failure static void peerDigestFetchAbort(DigestFetchState * fetch, char *buf, const char *reason) { assert(reason); - debugs(72, 2, "peerDigestFetchAbort: peer " << fetch->pd->host << ", reason: " << reason); - peerDigestReqFinish(fetch, buf, 1, 1, 1, reason, 1); + peerDigestReqFinish(fetch, buf, reason, 1); } /* complete the digest transfer, update stats, unlock/release everything */ static void peerDigestReqFinish(DigestFetchState * fetch, char * /* buf */, - int fcb_valid, int pdcb_valid, int pcb_valid, const char *reason, int err) { assert(reason); - /* must go before peerDigestPDFinish */ - - if (pdcb_valid) { - fetch->pd->flags.requested = false; - fetch->pd->req_result = reason; - } - - /* schedule next check if peer is still out there */ - if (pcb_valid) { - PeerDigest *pd = fetch->pd; - - if (err) { - pd->times.retry_delay = peerDigestIncDelay(pd); - peerDigestSetCheck(pd, pd->times.retry_delay); - } else { - pd->times.retry_delay = 0; - peerDigestSetCheck(pd, peerDigestNewDelay(fetch->entry)); - } - } + debugs(72, 2, "peer: " << RawPointer(fetch->pd.valid() ? fetch->pd->peer : nullptr).orNil() << ", reason: " << reason << ", err: " << err); /* note: order is significant */ - if (fcb_valid) - peerDigestFetchSetStats(fetch); - - if (pdcb_valid) - peerDigestPDFinish(fetch, pcb_valid, err); + peerDigestFetchSetStats(fetch); + if (const auto pd = fetch->pd.get()) + pd->noteFetchFinished(*fetch, reason, err); - if (fcb_valid) - peerDigestFetchFinish(fetch, err); + delete fetch; } -/* destroys digest if peer disappeared - * must be called only when fetch and pd cbdata are valid */ -static void -peerDigestPDFinish(DigestFetchState * fetch, int pcb_valid, int err) +void +PeerDigest::noteFetchFinished(const DigestFetchState &finishedFetch, const char * const outcomeDescription, const bool sawError) { - PeerDigest *pd = fetch->pd; - const auto host = pd->host; + const auto pd = this; // TODO: remove this diff reducer + const auto fetch = &finishedFetch; // TODO: remove this diff reducer + + pd->flags.requested = false; + pd->req_result = outcomeDescription; + pd->times.received = squid_curtime; pd->times.req_delay = fetch->resp_time; pd->stats.sent.kbytes += fetch->sent.bytes; @@ -732,20 +690,19 @@ peerDigestPDFinish(DigestFetchState * fetch, int pcb_valid, int err) pd->stats.sent.msgs += fetch->sent.msg; pd->stats.recv.msgs += fetch->recv.msg; - if (err) { - debugs(72, DBG_IMPORTANT, "" << (pcb_valid ? "temporary " : "" ) << "disabling (" << pd->req_result << ") digest from " << host); + if (sawError) { + debugs(72, DBG_IMPORTANT, "disabling (" << outcomeDescription << ") digest from " << host); + pd->times.retry_delay = peerDigestIncDelay(pd); + peerDigestSetCheck(pd, pd->times.retry_delay); delete pd->cd; pd->cd = nullptr; pd->flags.usable = false; - - if (!pcb_valid) - peerDigestNotePeerGone(pd); } else { - assert(pcb_valid); - pd->flags.usable = true; + pd->times.retry_delay = 0; + peerDigestSetCheck(pd, peerDigestNewDelay(fetch->entry)); /* XXX: ugly condition, but how? */ @@ -754,32 +711,6 @@ peerDigestPDFinish(DigestFetchState * fetch, int pcb_valid, int err) else debugs(72, 2, "received valid digest from " << host); } - - cbdataReferenceDone(fetch->pd); -} - -/* free fetch state structures - * must be called only when fetch cbdata is valid */ -static void -peerDigestFetchFinish(DigestFetchState * fetch, int /* err */) -{ - assert(fetch->entry && fetch->request); - - if (fetch->old_entry) { - debugs(72, 3, "peerDigestFetchFinish: deleting old entry"); - storeUnregister(fetch->old_sc, fetch->old_entry, fetch); - fetch->old_entry->releaseRequest(); - fetch->old_entry->unlock("peerDigestFetchFinish old"); - fetch->old_entry = nullptr; - } - - /* update global stats */ - statCounter.cd.kbytes_sent += fetch->sent.bytes; - statCounter.cd.kbytes_recv += fetch->recv.bytes; - statCounter.cd.msgs_sent += fetch->sent.msg; - statCounter.cd.msgs_recv += fetch->recv.msg; - - delete fetch; } /* calculate fetch stats after completion */ @@ -813,6 +744,10 @@ peerDigestFetchSetStats(DigestFetchState * fetch) std::showpos << (int) (fetch->entry->lastModified() - squid_curtime) << ")"); + statCounter.cd.kbytes_sent += fetch->sent.bytes; + statCounter.cd.kbytes_recv += fetch->recv.bytes; + statCounter.cd.msgs_sent += fetch->sent.msg; + statCounter.cd.msgs_recv += fetch->recv.msg; } static int From a0f0c636abcea3c7f8511b531b39b51b96fc83ad Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Wed, 10 Jul 2024 10:30:33 +0000 Subject: [PATCH 06/17] 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. --- src/dns_internal.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/src/dns_internal.cc b/src/dns_internal.cc index fb09f8cc277..e8c537b9eba 100644 --- a/src/dns_internal.cc +++ b/src/dns_internal.cc @@ -857,6 +857,7 @@ idnsInitVCConnected(const Comm::ConnectionPointer &conn, Comm::Flag status, int, if (vc->ns < nameservers.size()) nameservers[vc->ns].S.toStr(buf,MAX_IPSTRLEN); debugs(78, DBG_IMPORTANT, "ERROR: Failed to connect to nameserver " << buf << " using TCP."); + delete vc; return; } From cb8541b0fa05e7301285712288c3027655b85c8b Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Thu, 11 Jul 2024 20:04:06 +0000 Subject: [PATCH 07/17] Be more careful when updating nameservers global (#1862) Due to I/O delays and timeouts, DNS nsvc objects may be deleted after the `nameservers` global pointing to them was cleared and then populated with new ns objects. Thus, nsvc destructor should check that nsvc and `nameservers` states are still in sync before manipulating the latter. DNS code should use similar validation in a few other places, but they are all about read-only debugging that requires a rather noisy cleanup. --- src/dns_internal.cc | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/dns_internal.cc b/src/dns_internal.cc index e8c537b9eba..f2643015350 100644 --- a/src/dns_internal.cc +++ b/src/dns_internal.cc @@ -886,7 +886,9 @@ nsvc::~nsvc() { delete queue; delete msg; - if (ns < nameservers.size()) // XXX: idnsShutdownAndFreeState may have freed nameservers[] + // we may outlive nameservers version that was pointing to us because + // reconfigurations repopulate nameservers + if (ns < nameservers.size() && nameservers[ns].vc == this) nameservers[ns].vc = nullptr; } From 334a9819a736156c8207530be0165f49c0ad0fa2 Mon Sep 17 00:00:00 2001 From: Francesco Chemolli <5175948+kinkie@users.noreply.github.com> Date: Mon, 15 Jul 2024 17:37:14 +0000 Subject: [PATCH 08/17] Fix use-after-free in statefulhelper::submit() level-9 debug (#1859) A debug statement in helper.cc dereferences a pointer which might have been freed in helperStatefulDispatch. Detected by Semgrep --- src/helper.cc | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/helper.cc b/src/helper.cc index 197cd8f16a2..34267335e37 100644 --- a/src/helper.cc +++ b/src/helper.cc @@ -681,14 +681,12 @@ statefulhelper::submit(const char *buf, HLPCB * callback, void *data, const Help helper_stateful_server *srv; if ((srv = StatefulGetFirstAvailable(this))) { reserveServer(srv); - helperStatefulDispatch(srv, r); + helperStatefulDispatch(srv, r); // may delete r } else StatefulEnqueue(this, r); } - debugs(84, DBG_DATA, "placeholder: '" << r->request.placeholder << - "', " << Raw("buf", buf, (!buf?0:strlen(buf)))); - + // r may be dangling here syncQueueStats(); } From 4216816eb481fb484e6dc8b28dab11babb0ccd87 Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Mon, 15 Jul 2024 21:59:54 +0000 Subject: [PATCH 09/17] Do not drop queued TCP DNS queries when queuing a new one (#1863) Since 2005 commit 032785bf, previously queued serialized DNS query (waiting for a TCP connection to the nameserver or for a write on an already established TCP connection) was erased every time a new query was serialized and appended to the `vc->queue` MemBuf. Thus, at most one TCP query was submitted when the nameserver became available. Non-serialized versions of erased queries remained on the DNS lru_list and would eventually be retried or timeout. This change restores conditional MemBuf initialization that was replaced with an unconditional MemBuf reset in that 2005 commit. It would take more than 5 hours of 1000/s unique request load for 100-byte DNS queries to overflow a 2GB MemBuf in a properly functioning Squid, but this change does not rely on MEM_BUF_MAX_SIZE always being large enough. New code prevents MemBuf::grow() assertions and informs the admin about dropped queries that may indicate a Squid bug or a persistent problem with nameserver communication. --- src/dns_internal.cc | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/src/dns_internal.cc b/src/dns_internal.cc index f2643015350..1dcad7cb770 100644 --- a/src/dns_internal.cc +++ b/src/dns_internal.cc @@ -933,7 +933,20 @@ idnsSendQueryVC(idns_query * q, size_t nsn) return; } - vc->queue->reset(); + if (vc->queue->isNull()) + vc->queue->init(); + + const auto serialiedQuerySize = 2 + q->sz + 1; // payload_length + payload + terminate() + if (vc->queue->potentialSpaceSize() < serialiedQuerySize) { + // header + payload + MemBuf terminator exceed maximum space size + debugs(78, DBG_IMPORTANT, "ERROR: Dropping DNS query due to insufficient buffer space for DNS over TCP query queue" << + Debug::Extra << "query: " << q->name << + Debug::Extra << "nameserver: " << nameservers[nsn].S << + Debug::Extra << "used space: " << vc->queue->contentSize() << + Debug::Extra << "remaining space: " << vc->queue->potentialSpaceSize() << + Debug::Extra << "required space: " << serialiedQuerySize); + return; // the query will timeout and either fail or be retried + } short head = htons(q->sz); From 51c82279ac5aa734c382bc31804e477bf76fe448 Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Tue, 16 Jul 2024 01:38:59 +0000 Subject: [PATCH 10/17] Enable EDNS for DNS A queries and reverse IPv4 lookups (#1864) This change brings Squid code in sync with existing dns_packet_max directive documentation and allows admins to enable a useful performance optimization for still very popular IPv4-related DNS queries. An enabled dns_packet_max feature may now break Squid compatibility with more buggy nameservers (that appeared to "work" before), but we should not penalize many Squid deployments (or complicate configuration and spend a lot of development time) to accommodate a few exceptional ones, at least until such heroic efforts are proven to be necessary. --- doc/release-notes/release-7.sgml.in | 11 +++++++++++ src/dns/rfc3596.cc | 22 +++++++++++++--------- src/dns/rfc3596.h | 15 +++++---------- src/dns_internal.cc | 20 +++++--------------- 4 files changed, 34 insertions(+), 34 deletions(-) diff --git a/doc/release-notes/release-7.sgml.in b/doc/release-notes/release-7.sgml.in index 881ad8f141e..35fc35d9586 100644 --- a/doc/release-notes/release-7.sgml.in +++ b/doc/release-notes/release-7.sgml.in @@ -172,6 +172,17 @@ This section gives an account of those changes in three categories:

Removed the non_peers action. See the Cache Manager for details. + dns_packet_max +

Honor positive dns_packet_max values when sending DNS A queries + and PTR queries containing IPv4 addresses. Prior to this change, Squid did + not add EDNS extension (RFC 6891) to those DNS queries because 2010 tests + revealed compatibility problems with some DNS resolvers. We hope that those + problems are now sufficiently rare to enable this useful optimization for + all DNS queries, as originally intended. Squid still sends EDNS extension + with DNS AAAA queries and PTR queries containing IPv6 addresses (when + dns_packet_max is set to a positive value). Rare deployments that must use + buggy DNS resolvers should not set dns_packet_max. + access_log

Built-in common and combined logformats now always receive a dash character ("-") in the position of what used to be a diff --git a/src/dns/rfc3596.cc b/src/dns/rfc3596.cc index 997e5d94a2f..37a10bf9e07 100644 --- a/src/dns/rfc3596.cc +++ b/src/dns/rfc3596.cc @@ -9,6 +9,7 @@ #include "squid.h" #include "dns/rfc2671.h" #include "dns/rfc3596.h" +#include "SquidConfig.h" #include "util.h" #if HAVE_UNISTD_H @@ -54,7 +55,7 @@ * Returns the size of the query */ ssize_t -rfc3596BuildHostQuery(const char *hostname, char *buf, size_t sz, unsigned short qid, rfc1035_query * query, int qtype, ssize_t edns_sz) +rfc3596BuildHostQuery(const char *hostname, char *buf, size_t sz, unsigned short qid, rfc1035_query * query, int qtype) { static rfc1035_message h; size_t offset = 0; @@ -64,7 +65,10 @@ rfc3596BuildHostQuery(const char *hostname, char *buf, size_t sz, unsigned short h.rd = 1; h.opcode = 0; /* QUERY */ h.qdcount = (unsigned int) 1; + + const auto edns_sz = Config.dns.packet_max; h.arcount = (edns_sz > 0 ? 1 : 0); + offset += rfc1035HeaderPack(buf + offset, sz - offset, &h); offset += rfc1035QuestionPack(buf + offset, sz - offset, @@ -93,9 +97,9 @@ rfc3596BuildHostQuery(const char *hostname, char *buf, size_t sz, unsigned short * \return the size of the query */ ssize_t -rfc3596BuildAQuery(const char *hostname, char *buf, size_t sz, unsigned short qid, rfc1035_query * query, ssize_t edns_sz) +rfc3596BuildAQuery(const char *hostname, char *buf, size_t sz, unsigned short qid, rfc1035_query * query) { - return rfc3596BuildHostQuery(hostname, buf, sz, qid, query, RFC1035_TYPE_A, edns_sz); + return rfc3596BuildHostQuery(hostname, buf, sz, qid, query, RFC1035_TYPE_A); } /** @@ -107,9 +111,9 @@ rfc3596BuildAQuery(const char *hostname, char *buf, size_t sz, unsigned short qi * \return the size of the query */ ssize_t -rfc3596BuildAAAAQuery(const char *hostname, char *buf, size_t sz, unsigned short qid, rfc1035_query * query, ssize_t edns_sz) +rfc3596BuildAAAAQuery(const char *hostname, char *buf, size_t sz, unsigned short qid, rfc1035_query * query) { - return rfc3596BuildHostQuery(hostname, buf, sz, qid, query, RFC1035_TYPE_AAAA, edns_sz); + return rfc3596BuildHostQuery(hostname, buf, sz, qid, query, RFC1035_TYPE_AAAA); } /** @@ -121,7 +125,7 @@ rfc3596BuildAAAAQuery(const char *hostname, char *buf, size_t sz, unsigned short * \return the size of the query */ ssize_t -rfc3596BuildPTRQuery4(const struct in_addr addr, char *buf, size_t sz, unsigned short qid, rfc1035_query * query, ssize_t edns_sz) +rfc3596BuildPTRQuery4(const struct in_addr addr, char *buf, size_t sz, unsigned short qid, rfc1035_query * query) { static char rev[RFC1035_MAXHOSTNAMESZ]; unsigned int i; @@ -133,11 +137,11 @@ rfc3596BuildPTRQuery4(const struct in_addr addr, char *buf, size_t sz, unsigned (i >> 16) & 255, (i >> 24) & 255); - return rfc3596BuildHostQuery(rev, buf, sz, qid, query, RFC1035_TYPE_PTR, edns_sz); + return rfc3596BuildHostQuery(rev, buf, sz, qid, query, RFC1035_TYPE_PTR); } ssize_t -rfc3596BuildPTRQuery6(const struct in6_addr addr, char *buf, size_t sz, unsigned short qid, rfc1035_query * query, ssize_t edns_sz) +rfc3596BuildPTRQuery6(const struct in6_addr addr, char *buf, size_t sz, unsigned short qid, rfc1035_query * query) { static char rev[RFC1035_MAXHOSTNAMESZ]; const uint8_t* r = addr.s6_addr; @@ -152,6 +156,6 @@ rfc3596BuildPTRQuery6(const struct in6_addr addr, char *buf, size_t sz, unsigned snprintf(p,10,"ip6.arpa."); - return rfc3596BuildHostQuery(rev, buf, sz, qid, query, RFC1035_TYPE_PTR, edns_sz); + return rfc3596BuildHostQuery(rev, buf, sz, qid, query, RFC1035_TYPE_PTR); } diff --git a/src/dns/rfc3596.h b/src/dns/rfc3596.h index 10e49e4708f..3ebad864156 100644 --- a/src/dns/rfc3596.h +++ b/src/dns/rfc3596.h @@ -16,29 +16,25 @@ ssize_t rfc3596BuildAQuery(const char *hostname, char *buf, size_t sz, unsigned short qid, - rfc1035_query * query, - ssize_t edns_sz); + rfc1035_query *); ssize_t rfc3596BuildAAAAQuery(const char *hostname, char *buf, size_t sz, unsigned short qid, - rfc1035_query * query, - ssize_t edns_sz); + rfc1035_query *); ssize_t rfc3596BuildPTRQuery4(const struct in_addr, char *buf, size_t sz, unsigned short qid, - rfc1035_query * query, - ssize_t edns_sz); + rfc1035_query *); ssize_t rfc3596BuildPTRQuery6(const struct in6_addr, char *buf, size_t sz, unsigned short qid, - rfc1035_query * query, - ssize_t edns_sz); + rfc1035_query *); /* RFC3596 library implements RFC1035 generic host interface */ ssize_t rfc3596BuildHostQuery(const char *hostname, @@ -46,8 +42,7 @@ ssize_t rfc3596BuildHostQuery(const char *hostname, size_t sz, unsigned short qid, rfc1035_query * query, - int qtype, - ssize_t edns_sz); + int qtype); /* RFC3596 section 2.1 defines new RR type AAAA as 28 */ #define RFC1035_TYPE_AAAA 28 diff --git a/src/dns_internal.cc b/src/dns_internal.cc index 1dcad7cb770..8b4d98e9c88 100644 --- a/src/dns_internal.cc +++ b/src/dns_internal.cc @@ -233,13 +233,6 @@ static hash_table *idns_lookup_hash = nullptr; /* * Notes on EDNS: * - * IPv4: - * EDNS as specified may be sent as an additional record for any request. - * early testing has revealed that it works on common devices, but cannot - * be reliably used on any A or PTR requet done for IPv4 addresses. - * - * As such the IPv4 packets are still hard-coded not to contain EDNS (0) - * * Squid design: * Squid is optimized to generate one packet and re-send it to all NS * due to this we cannot customize the EDNS size per NS. @@ -1296,8 +1289,7 @@ idnsGrokReply(const char *buf, size_t sz, int /*from_ns*/) // Build new query q->query_id = idnsQueryID(); debugs(78, 3, "idnsGrokReply: Trying A Query for " << q->name); - // see EDNS notes at top of file why this sends 0 - q->sz = rfc3596BuildAQuery(q->name, q->buf, sizeof(q->buf), q->query_id, &q->query, 0); + q->sz = rfc3596BuildAQuery(q->name, q->buf, sizeof(q->buf), q->query_id, &q->query); if (q->sz < 0) { /* problem with query data -- query not sent */ idnsCallback(q, "Internal error"); @@ -1740,7 +1732,7 @@ idnsSendSlaveAAAAQuery(idns_query *master) memcpy(q->orig, master->orig, sizeof(q->orig)); q->master = master; q->query_id = idnsQueryID(); - q->sz = rfc3596BuildAAAAQuery(q->name, q->buf, sizeof(q->buf), q->query_id, &q->query, Config.dns.packet_max); + q->sz = rfc3596BuildAAAAQuery(q->name, q->buf, sizeof(q->buf), q->query_id, &q->query); debugs(78, 3, "buf is " << q->sz << " bytes for " << q->name << ", id = 0x" << asHex(q->query_id)); @@ -1796,8 +1788,7 @@ idnsALookup(const char *name, IDNSCB * callback, void *data) debugs(78, 3, "idnsALookup: searchpath used for " << q->name); } - // see EDNS notes at top of file why this sends 0 - q->sz = rfc3596BuildAQuery(q->name, q->buf, sizeof(q->buf), q->query_id, &q->query, 0); + q->sz = rfc3596BuildAQuery(q->name, q->buf, sizeof(q->buf), q->query_id, &q->query); if (q->sz < 0) { /* problem with query data -- query not sent */ @@ -1829,12 +1820,11 @@ idnsPTRLookup(const Ip::Address &addr, IDNSCB * callback, void *data) if (addr.isIPv6()) { struct in6_addr addr6; addr.getInAddr(addr6); - q->sz = rfc3596BuildPTRQuery6(addr6, q->buf, sizeof(q->buf), q->query_id, &q->query, Config.dns.packet_max); + q->sz = rfc3596BuildPTRQuery6(addr6, q->buf, sizeof(q->buf), q->query_id, &q->query); } else { struct in_addr addr4; addr.getInAddr(addr4); - // see EDNS notes at top of file why this sends 0 - q->sz = rfc3596BuildPTRQuery4(addr4, q->buf, sizeof(q->buf), q->query_id, &q->query, 0); + q->sz = rfc3596BuildPTRQuery4(addr4, q->buf, sizeof(q->buf), q->query_id, &q->query); } if (q->sz < 0) { From 61ddaf9d4a17cf236d49d487aa05af1ef6e0b821 Mon Sep 17 00:00:00 2001 From: Eduard Bagdasaryan Date: Tue, 16 Jul 2024 05:45:49 +0000 Subject: [PATCH 11/17] Fix a use-after-free bug in peerDigestFetchReply() (#1865) The problem occurred when handling an HTTP 304 cache digest response. Also removed effectively unused DIGEST_READ_DONE enum value. --- src/enums.h | 3 +-- src/peer_digest.cc | 6 +----- 2 files changed, 2 insertions(+), 7 deletions(-) diff --git a/src/enums.h b/src/enums.h index e852ba4418d..0ac7f3d82f6 100644 --- a/src/enums.h +++ b/src/enums.h @@ -198,8 +198,7 @@ typedef enum { DIGEST_READ_NONE, DIGEST_READ_REPLY, DIGEST_READ_CBLOCK, - DIGEST_READ_MASK, - DIGEST_READ_DONE + DIGEST_READ_MASK } digest_read_state_t; /* CygWin & Windows NT Port */ diff --git a/src/peer_digest.cc b/src/peer_digest.cc index 0f375ac4c79..089a2db8780 100644 --- a/src/peer_digest.cc +++ b/src/peer_digest.cc @@ -386,10 +386,6 @@ peerDigestHandleReply(void *data, StoreIOBuffer receivedData) case DIGEST_READ_NONE: break; - case DIGEST_READ_DONE: - return; - break; - default: fatal("Bad digest transfer mode!\n"); } @@ -491,7 +487,7 @@ peerDigestFetchReply(void *data, char *buf, ssize_t size) // stay with the old in-memory digest peerDigestFetchStop(fetch, buf, "Not modified"); - fetch->state = DIGEST_READ_DONE; + return -1; } else if (status == Http::scOkay) { /* get rid of old entry if any */ From 82e18891262580e1d6ea23e97283ab0dccaa8c1e Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Mon, 5 Aug 2024 12:18:02 +0000 Subject: [PATCH 12/17] Bootstrap libltdl to fix libtool v2.4 + automake v1.17 build (#1877) gmake[3]: Entering directory .../libltdl .../cfgaux/missing: line 85: aclocal-1.16: command not found gmake[3]: *** [Makefile:561: .././../libltdl/aclocal.m4] Error 127 During bootstrap.sh run, libtoolize copies prepackaged configure and Makefile.in files into our libltdl directory: * libltdl/configure from libtool v2.4 has aclocal version set to 1.16; * libltdl/Makefile.in from libtool v2.4 uses configure-set aclocal version to build aclocal.m4 Thus, libltdl/Makefile (generated from libltdl/Makefile.in above) runs aclocal-1.16 if "make" needs to build libltdl/aclocal.m4. Normally, "make" does not need to build libltdl/aclocal.m4 because that file was created by libtoolize. However, libtool v2.4 is packaged with (generated by packaging folks) libltdl/Makefile.in that makes libltdl/aclocal.m4 target dependent on files in libltld/../m4 directory. Squid does not have that ./m4 directory, so "make" attempts to re-generate libltdl/aclocal.m4. When it does, it uses aclocal-1.16. Our bootstrap.sh generated new ./configure but preserved copied libltdl/configure with its aclocal version set to 1.16. In other words, our bootstrap.sh did not bootstrap libltdl sub-project. In build environments without aclocal-1.16, Squid build failed. Several solutions or workarounds have been tried or considered: * Adjust bootstrap.sh to bootstrap libltdl (this change). 2008 attempt to do that was reverted in commit bfd6b6a9 with "better to fix libtool installation" rationale. Another potential argument against this option is that packages should be bootstrapped by their distributors, not "users". We are not distributing libtool, but this is a gray area because we do distribute files that libtoolize creates. Finally, libtool itself does not provide a bootstrapping script and does not explicitly recommend bootstrapping in documentation. * "Fix libtool installation". We failed to find a good way to do that on MacOS (without building and installing newer libtool from sources). * Place m4 files where libtool v2.4 expects to find them. That change fixes MacOS builds that use automake v1.17, but breaks Gentoo builds because Gentoo libtool installs a buggy libltdl/Makefile.in that must be regenerated by automake before it can work. Fixing m4 files location prevents that regeneration. We picked the first option despite its drawbacks because the third option did not work on Gentoo, and asking Squid developers to install libtool from sources (i.e. the second option) felt like a greater evil. This old problem was exposed by recently introduced CI MacOS tests that started to fail when MacOS brew updated automake package from v1.16 without the corresponding libtoolize package changes. Also work around what seems to be a libtool packaging bug affecting MacOS/brew environments, including GitHub Actions runners we use for CI: libtool (2.4.7) : glibtool libtool path : /opt/homebrew/bin Bootstrapping glibtoolize: error: creating 'libltdl/configure.ac' ... failed glibtoolize: error: creating 'libltdl/configure' ... failed glibtoolize failed That workaround will be removed after libtool package is fixed. Also removed a single-iteration "for dir" loop with several stale hacks from bootstrap.sh: With only two directories to bootstrap and with a directory-specific mkdir command, source comments, and progress messages, it is best to unroll that loop. --- .github/workflows/default.yaml | 15 +++++++ bootstrap.sh | 71 +++++++++++++++++++--------------- 2 files changed, 55 insertions(+), 31 deletions(-) diff --git a/.github/workflows/default.yaml b/.github/workflows/default.yaml index f3da5e41e72..0204f95a2c4 100644 --- a/.github/workflows/default.yaml +++ b/.github/workflows/default.yaml @@ -159,6 +159,21 @@ jobs: export CPPFLAGS="-I$HOMEBREW_PREFIX/include${CPPFLAGS:+ $CPPFLAGS}" export LDFLAGS="-L$HOMEBREW_PREFIX/lib${LDFLAGS:+ $LDFLAGS}" export CFLAGS="-Wno-compound-token-split-by-macro${CFLAGS:+ $CFLAGS}" # needed fir ltdl with Xcode + + # libtool package referenced below fails to copy its configure* + # files, possibly due to a packaging/brewing bug. The following sed + # command restores installed libtoolize code to its earlier (and + # working) variation. + echo "brew libtool package details:" + brew info libtool --json | grep -E 'rebuild|tap_git_head' + # This hack was tested on libtoolize package with the following output: + # "rebuild": 2, + # "tap_git_head": "5cede8ea3b7b12c7f68215f75a951430b38d945f", + # + editable=$HOMEBREW_CELLAR/libtool/2.4.7/bin/glibtoolize + sed -i.bak 's@ltdl_ac_aux_dir=""@ltdl_ac_aux_dir="../build-aux"@' $editable || true + diff -u $editable.bak $editable || true + ./test-builds.sh ${{ matrix.layer.name }} - name: Publish build logs diff --git a/bootstrap.sh b/bootstrap.sh index bf0cea53824..fe71d958d04 100755 --- a/bootstrap.sh +++ b/bootstrap.sh @@ -116,38 +116,47 @@ echo "autoconf ($acversion) : autoconf$acver" echo "libtool ($ltversion) : ${LIBTOOL_BIN}${ltver}" echo "libtool path : $ltpath" -for dir in \ - "" -do - if [ -z "$dir" ] || [ -d $dir ]; then - if ( - echo "Bootstrapping $dir" - cd ./$dir - if [ -n "$dir" ] && [ -f bootstrap.sh ]; then - ./bootstrap.sh - elif [ ! -f $dir/configure ]; then - # Make sure cfgaux exists - mkdir -p cfgaux - - if test -n "$ltpath"; then - acincludeflag="-I $ltpath/../share/aclocal" - else - acincludeflag="" - fi - - # Bootstrap the autotool subsystems - bootstrap aclocal$amver $acincludeflag - bootstrap autoheader$acver - bootstrap_libtoolize ${LIBTOOL_BIN}ize${ltver} - bootstrap automake$amver --foreign --add-missing --copy -f - bootstrap autoconf$acver --force - fi ); then - : # OK - else - exit 1 - fi +if test -n "$ltpath"; then + acincludeflag="-I $ltpath/../share/aclocal" +else + acincludeflag="" +fi + +# bootstrap primary or subproject sources +bootstrap_dir() { + dir="$1" + cd $dir || exit $? + + bootstrap aclocal$amver $acincludeflag + bootstrap autoheader$acver + + # Do not libtoolize ltdl + if grep -q '^LTDL_INIT' configure.ac + then + bootstrap_libtoolize ${LIBTOOL_BIN}ize${ltver} fi -done + + bootstrap automake$amver --foreign --add-missing --copy --force + bootstrap autoconf$acver --force + + cd - > /dev/null +} + +echo "Bootstrapping primary Squid sources" +mkdir -p cfgaux || exit $? +bootstrap_dir . + +# The above bootstrap_libtoolize step creates or updates libltdl. It copies +# (with minor adjustments) configure.ac and configure, Makefile.am and +# Makefile.in from libtool installation, but does not regenerate copied +# configure from copied configure.ac and copied Makefile.in from Makefile.am. +# We get libltdl/configure and libltdl/Makefile.in as they were bootstrapped +# by libtool authors or package maintainers. Low-level idiosyncrasies in those +# libtool files result in mismatches between copied code expectations and +# Squid sub-project environment, leading to occasional build failures that +# this bootstrapping addresses. +echo "Bootstrapping libltdl sub-project" +bootstrap_dir libltdl # Make a copy of SPONSORS we can package if test -f SPONSORS.list; then From 0d6454140f07311bac841acdca6eab15548bf747 Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Mon, 5 Aug 2024 19:30:08 +0000 Subject: [PATCH 13/17] Extend in-use ACLs lifetime across reconfiguration (#1826) When Squid is reconfigured, all previously created Acl::Node objects become unusable[^1], resulting in ACLChecklist::resumeNonBlockingCheck() ACCESS_DUNNO outcomes. ACCESS_DUNNO leads to Squid applying the affected directive "default" action rather than the configured one (e.g., an http_access denies a transaction that should be allowed or miss_access allows a transaction that should be denied). These unwanted and surprising effects make hot reconfiguration unreliable. They are also difficult to discover and triage. An Acl::Node object should last as long as the object/scope that saved that ACL for reuse. This change applies this (re)configuration principle to ACLChecklist::accessList by converting that raw Acl::Tree pointer to use reference counting. That change requires reference counting of individual Acl::Node objects and related types. Several TODOs and XXXs asked for reference counted ACLs, but more than 50 explicit raw ACL pointers in Squid code make that transition difficult: * Config.accessList.foo and similar items used raw Acl::Tree pointers to objects destroyed during reconfiguration by aclDestroyAcls(). They now point to refcounted Acl::Trees that ACLChecklist and other configuration-using code can safely store across reconfigurations. The outer Config.accessList.foo pointer is still raw because Squid invasive RefCount smart pointers cannot be stored directly in various configuration objects without significant out-of-scope refactoring. * Acl::Node::next items formed an invasive list of raw ACL pointers (rooted in Config.aclList). The list was used to FindByName() and also to prepareForUse() explicitly named ACLs. Reconfiguration destroyed listed nodes via aclDestroyAcls(). Acl::Node::next is now gone. * RegisteredAcls std::set stored raw ACL pointers to destroy all ACL objects (named and internally-generated) during reconfiguration, after all the free_foo() calls, in aclDestroyAcls(). It is now gone. * InnerNode::nodes std::vector stored raw pointers to internally-generated Acl::Node objects. They are now refcounted. The remaining raw Acl::Node pointers are not actually _stored_ across async calls. They are used to pass ACLs around. Some of them should be replaced with references (where it is known that the ACL must exist and is not expected to be stored), but that is a different polishing issue. For now, we just make sure that no raw Acl::Node pointers are stored across async calls (because any stored raw ACL pointer may be invalidated by reconfiguration). Extending lifetime of in-use ACLs is necessary but not sufficient to make Squid reconfiguration reliable. We need to preserve more in-use resources... [^1]: Prior to these changes, reconfiguration made Acl::Node objects unusable because their C++ destructors were called. Cbdata locks may have kept underlying object memory, but any linked resources freed by destructors were gone, and the destructed objects could not be accessed. --- src/ConfigParser.cc | 4 +- src/ConfigParser.h | 2 +- src/Notes.cc | 2 +- src/SquidConfig.h | 2 +- src/acl/Acl.cc | 100 +++++++++++++++++++--------------- src/acl/Acl.h | 6 ++ src/acl/AllOf.cc | 6 +- src/acl/BoolOps.cc | 6 +- src/acl/Checklist.cc | 47 ++++++++-------- src/acl/Checklist.h | 25 ++++----- src/acl/Gadgets.cc | 82 +++++++++------------------- src/acl/Gadgets.h | 24 +++++--- src/acl/InnerNode.cc | 4 +- src/acl/InnerNode.h | 8 +-- src/acl/Node.h | 17 +++--- src/acl/Tree.cc | 2 - src/acl/Tree.h | 8 +-- src/acl/forward.h | 9 ++- src/cache_cf.cc | 45 +++++++-------- src/cf.data.pre | 2 +- src/clients/Client.cc | 2 +- src/external_acl.cc | 8 +++ src/security/KeyLog.cc | 2 +- src/tests/stub_acl.cc | 2 +- src/tests/testACLMaxUserIP.cc | 9 ++- 25 files changed, 209 insertions(+), 215 deletions(-) diff --git a/src/ConfigParser.cc b/src/ConfigParser.cc index 0e405eb85ae..d9e7a2456ea 100644 --- a/src/ConfigParser.cc +++ b/src/ConfigParser.cc @@ -594,13 +594,13 @@ ConfigParser::skipOptional(const char *keyword) return false; // no more tokens (i.e. we are at the end of the line) } -Acl::Tree * +ACLList * ConfigParser::optionalAclList() { if (!skipOptional("if")) return nullptr; // OK: the directive has no ACLs - Acl::Tree *acls = nullptr; + ACLList *acls = nullptr; const auto aclCount = aclParseAclList(*this, &acls, cfg_directive); assert(acls); if (aclCount <= 0) diff --git a/src/ConfigParser.h b/src/ConfigParser.h index 4b588bcaeed..1ef9577b4ab 100644 --- a/src/ConfigParser.h +++ b/src/ConfigParser.h @@ -72,7 +72,7 @@ class ConfigParser bool skipOptional(const char *keyword); /// parses an [if [!]...] construct - Acl::Tree *optionalAclList(); + ACLList *optionalAclList(); /// extracts and returns a regex (including any optional flags) std::unique_ptr regex(const char *expectedRegexDescription); diff --git a/src/Notes.cc b/src/Notes.cc index 1f60d37f2e6..3f77761f5c8 100644 --- a/src/Notes.cc +++ b/src/Notes.cc @@ -110,7 +110,7 @@ Note::printAsNoteDirective(StoreEntry * const entry, const char * const directiv os << directiveName << ' ' << key() << ' ' << ConfigParser::QuoteString(SBufToString(v->value())); if (v->aclList) { // TODO: Use Acl::dump() after fixing the XXX in dump_acl_list(). - for (const auto &item: v->aclList->treeDump("", &Acl::AllowOrDeny)) { + for (const auto &item: ToTree(v->aclList).treeDump("", &Acl::AllowOrDeny)) { if (item.isEmpty()) // treeDump("") adds this prefix continue; if (item.cmp("\n") == 0) // treeDump() adds this suffix diff --git a/src/SquidConfig.h b/src/SquidConfig.h index 8ac9a47b941..8f5063d7802 100644 --- a/src/SquidConfig.h +++ b/src/SquidConfig.h @@ -353,7 +353,7 @@ class SquidConfig std::chrono::nanoseconds paranoid_hit_validation; - class Acl::Node *aclList; + Acl::NamedAcls *namedAcls; ///< acl aclname acltype ... struct { acl_access *http; diff --git a/src/acl/Acl.cc b/src/acl/Acl.cc index b08ab442e5d..2d07d3fc9ac 100644 --- a/src/acl/Acl.cc +++ b/src/acl/Acl.cc @@ -20,15 +20,24 @@ #include "debug/Stream.h" #include "fatal.h" #include "globals.h" +#include "mem/PoolingAllocator.h" +#include "sbuf/Algorithms.h" #include "sbuf/List.h" #include "sbuf/Stream.h" #include "SquidConfig.h" #include #include +#include namespace Acl { +/// parsed "acl aclname ..." directives indexed by aclname +class NamedAcls: public std::unordered_map > > { +}; + /// Acl::Node type name comparison functor class TypeNameCmp { public: @@ -149,40 +158,30 @@ void Acl::Node::operator delete(void *) fatal ("unusable Acl::Node::delete"); } -/// implements both Acl::Node::FindByName() variations -template -static Acl::Node * -FindByNameT(const SBufOrCString name) +Acl::Node * +Acl::Node::FindByName(const SBuf &name) { - for (auto a = Config.aclList; a; a = a->next) { - if (a->name.caseCmp(name) == 0) { - debugs(28, 8, "found " << a->name); - return a; - } + if (!Config.namedAcls) { + debugs(28, 8, "no named ACLs to find " << name); + return nullptr; } - debugs(28, 8, "cannot find " << name); - return nullptr; -} + const auto result = Config.namedAcls->find(name); + if (result == Config.namedAcls->end()) { + debugs(28, 8, "no ACL named " << name); + return nullptr; + } -Acl::Node * -Acl::Node::FindByName(const SBuf &name) -{ - return FindByNameT(name); + debugs(28, 8, result->second << " is named " << name); + assert(result->second); + return result->second.getRaw(); } -Acl::Node * -Acl::Node::FindByName(const char *name) +Acl::Node::Node() { - return FindByNameT(name); + debugs(28, 8, "constructing, this=" << this); } -Acl::Node::Node() : - cfgline(nullptr), - next(nullptr), - registered(false) -{} - bool Acl::Node::valid() const { @@ -230,7 +229,7 @@ Acl::Node::context(const SBuf &aName, const char *aCfgLine) } void -Acl::Node::ParseAclLine(ConfigParser &parser, Node ** head) +Acl::Node::ParseNamedAcl(ConfigParser &parser, NamedAcls *&namedAcls) { /* we're already using strtok() to grok the line */ char *t = nullptr; @@ -243,15 +242,18 @@ Acl::Node::ParseAclLine(ConfigParser &parser, Node ** head) return; } + if (!namedAcls) + namedAcls = new NamedAcls(); + SBuf aclname(t); CallParser(ParsingContext::Pointer::Make(aclname), [&] { - ParseNamed(parser, head, aclname); + ParseNamed(parser, *namedAcls, aclname); }); } /// parses acl directive parts that follow aclname void -Acl::Node::ParseNamed(ConfigParser &parser, Node ** const head, const SBuf &aclname) +Acl::Node::ParseNamed(ConfigParser &parser, NamedAcls &namedAcls, const SBuf &aclname) { /* snarf the ACL type */ const char *theType; @@ -293,9 +295,9 @@ Acl::Node::ParseNamed(ConfigParser &parser, Node ** const head, const SBuf &acln theType = "client_connection_mark"; } - Node *A = nullptr; + auto A = FindByName(aclname); int new_acl = 0; - if ((A = FindByName(aclname)) == nullptr) { + if (!A) { debugs(28, 3, "aclParseAclLine: Creating ACL '" << aclname << "'"); A = Acl::Make(theType); A->context(aclname, config_input_line); @@ -328,13 +330,27 @@ Acl::Node::ParseNamed(ConfigParser &parser, Node ** const head, const SBuf &acln A->cfgline); } - // add to the global list for searching explicit ACLs by name - assert(head && *head == Config.aclList); - A->next = *head; - *head = A; + const auto insertion = namedAcls.emplace(A->name, A); + Assure(insertion.second); // FindByName() above checked that A is a new ACL +} - // register for centralized cleanup - aclRegister(A); +void +Acl::DumpNamedAcls(std::ostream &os, const char * const directiveName, NamedAcls * const namedAcls) +{ + if (namedAcls) { + for (const auto &nameAndAcl: *namedAcls) { + debugs(3, 3, directiveName << ' ' << nameAndAcl.first); + nameAndAcl.second->dumpWhole(directiveName, os); + } + } +} + +void +Acl::FreeNamedAcls(NamedAcls ** const namedAcls) +{ + assert(namedAcls); + delete *namedAcls; + *namedAcls = nullptr; } bool @@ -450,19 +466,17 @@ Acl::Node::requiresRequest() const Acl::Node::~Node() { - debugs(28, 3, "freeing ACL " << name); + debugs(28, 8, "destructing " << name << ", this=" << this); safe_free(cfgline); } void Acl::Node::Initialize() { - auto *a = Config.aclList; - debugs(53, 3, "Acl::Node::Initialize"); - - while (a) { - a->prepareForUse(); - a = a->next; + debugs(28, 3, (Config.namedAcls ? Config.namedAcls->size() : 0)); + if (Config.namedAcls) { + for (const auto &nameAndAcl: *Config.namedAcls) + nameAndAcl.second->prepareForUse(); } } diff --git a/src/acl/Acl.h b/src/acl/Acl.h index 130682b2c46..5b46121f7d2 100644 --- a/src/acl/Acl.h +++ b/src/acl/Acl.h @@ -125,6 +125,12 @@ operator <<(std::ostream &o, const Answer &a) return o; } +/// report the given list of "acl" directives (using squid.conf syntax) +void DumpNamedAcls(std::ostream &, const char *directiveName, NamedAcls *); + +/// delete the given list of "acl" directives +void FreeNamedAcls(NamedAcls **); + } // namespace Acl /// \ingroup ACLAPI diff --git a/src/acl/AllOf.cc b/src/acl/AllOf.cc index 731f56781b7..56002e7ed37 100644 --- a/src/acl/AllOf.cc +++ b/src/acl/AllOf.cc @@ -10,7 +10,6 @@ #include "acl/AllOf.h" #include "acl/BoolOps.h" #include "acl/Checklist.h" -#include "acl/Gadgets.h" #include "cache_cf.h" #include "MemBuf.h" #include "sbuf/SBuf.h" @@ -37,7 +36,7 @@ Acl::AllOf::doMatch(ACLChecklist *checklist, Nodes::const_iterator start) const if (empty()) return 1; // not 0 because in math empty product equals identity - if (checklist->matchChild(this, start, *start)) + if (checklist->matchChild(this, start)) return 1; // match return checklist->keepMatching() ? 0 : -1; @@ -48,7 +47,7 @@ void Acl::AllOf::parse() { Acl::InnerNode *whole = nullptr; - Acl::Node *oldNode = empty() ? nullptr : nodes.front(); + const auto oldNode = empty() ? nullptr : nodes.front().getRaw(); // optimization: this logic reduces subtree hight (number of tree levels) if (Acl::OrNode *oldWhole = dynamic_cast(oldNode)) { @@ -61,7 +60,6 @@ Acl::AllOf::parse() newWhole->context(ToSBuf('(', name, " lines)"), oldNode->cfgline); newWhole->add(oldNode); // old (i.e. first) line nodes.front() = whole = newWhole; - aclRegister(newWhole); } else { // this is the first line for this acl; just use it as is whole = this; diff --git a/src/acl/BoolOps.cc b/src/acl/BoolOps.cc index e418c567b22..06508d32ec1 100644 --- a/src/acl/BoolOps.cc +++ b/src/acl/BoolOps.cc @@ -36,7 +36,7 @@ Acl::NotNode::doMatch(ACLChecklist *checklist, Nodes::const_iterator start) cons { assert(start == nodes.begin()); // we only have one node - if (checklist->matchChild(this, start, *start)) + if (checklist->matchChild(this, start)) return 0; // converting match into mismatch if (!checklist->keepMatching()) @@ -72,7 +72,7 @@ Acl::AndNode::doMatch(ACLChecklist *checklist, Nodes::const_iterator start) cons { // find the first node that does not match for (Nodes::const_iterator i = start; i != nodes.end(); ++i) { - if (!checklist->matchChild(this, i, *i)) + if (!checklist->matchChild(this, i)) return checklist->keepMatching() ? 0 : -1; } @@ -110,7 +110,7 @@ Acl::OrNode::doMatch(ACLChecklist *checklist, Nodes::const_iterator start) const for (Nodes::const_iterator i = start; i != nodes.end(); ++i) { if (bannedAction(checklist, i)) continue; - if (checklist->matchChild(this, i, *i)) { + if (checklist->matchChild(this, i)) { lastMatch_ = i; return 1; } diff --git a/src/acl/Checklist.cc b/src/acl/Checklist.cc index 4dc5ad7e396..950d6fe0601 100644 --- a/src/acl/Checklist.cc +++ b/src/acl/Checklist.cc @@ -27,17 +27,6 @@ ACLChecklist::prepNonBlocking() return false; } - /** \par - * If the accessList is no longer valid (i.e. its been - * freed because of a reconfigure), then bail with ACCESS_DUNNO. - */ - - if (!cbdataReferenceValid(accessList)) { - cbdataReferenceDone(accessList); - checkCallback("accessList is invalid"); - return false; - } - return true; } @@ -49,7 +38,6 @@ ACLChecklist::completeNonBlocking() if (!finished()) calcImplicitAnswer(); - cbdataReferenceDone(accessList); checkCallback(nullptr); } @@ -79,8 +67,9 @@ ACLChecklist::preCheck(const char *what) } bool -ACLChecklist::matchChild(const Acl::InnerNode *current, Acl::Nodes::const_iterator pos, const Acl::Node *child) +ACLChecklist::matchChild(const Acl::InnerNode * const current, const Acl::Nodes::const_iterator pos) { + const auto &child = *pos; assert(current && child); // Remember the current tree location to prevent "async loop" cases where @@ -191,10 +180,21 @@ ACLChecklist::ACLChecklist() : ACLChecklist::~ACLChecklist() { assert (!asyncInProgress()); + debugs(28, 4, "ACLChecklist::~ACLChecklist: destroyed " << this); +} - changeAcl(nullptr); +void +ACLChecklist::changeAcl(const acl_access * const replacement) +{ + accessList = replacement ? *replacement : nullptr; +} - debugs(28, 4, "ACLChecklist::~ACLChecklist: destroyed " << this); +Acl::TreePointer +ACLChecklist::swapAcl(const acl_access * const replacement) +{ + const auto old = accessList; + changeAcl(replacement); + return old; } /** @@ -269,24 +269,24 @@ ACLChecklist::matchAndFinish() markFinished(accessList->winningAction(), "match"); } -Acl::Answer const & -ACLChecklist::fastCheck(const Acl::Tree * list) +const Acl::Answer & +ACLChecklist::fastCheck(const ACLList * const list) { preCheck("fast ACLs"); asyncCaller_ = false; // Concurrent checks are not supported, but sequential checks are, and they // may use a mixture of fastCheck(void) and fastCheck(list) calls. - const Acl::Tree * const savedList = changeAcl(list); + const auto savedList = swapAcl(list); // assume DENY/ALLOW on mis/matches due to action-free accessList // matchAndFinish() takes care of the ALLOW case - if (accessList && cbdataReferenceValid(accessList)) + if (accessList) matchAndFinish(); // calls markFinished() on success if (!finished()) markFinished(ACCESS_DENIED, "ACLs failed to match"); - changeAcl(savedList); + changeAcl(&savedList); occupied_ = false; return currentAnswer(); } @@ -301,13 +301,11 @@ ACLChecklist::fastCheck() asyncCaller_ = false; debugs(28, 5, "aclCheckFast: list: " << accessList); - const Acl::Tree *acl = cbdataReference(accessList); - if (acl != nullptr && cbdataReferenceValid(acl)) { + if (accessList) { matchAndFinish(); // calls markFinished() on success // if finished (on a match or in exceptional cases), stop if (finished()) { - cbdataReferenceDone(acl); occupied_ = false; return currentAnswer(); } @@ -317,7 +315,6 @@ ACLChecklist::fastCheck() // There were no rules to match or no rules matched calcImplicitAnswer(); - cbdataReferenceDone(acl); occupied_ = false; return currentAnswer(); @@ -328,7 +325,7 @@ ACLChecklist::fastCheck() void ACLChecklist::calcImplicitAnswer() { - const auto lastAction = (accessList && cbdataReferenceValid(accessList)) ? + const auto lastAction = accessList ? accessList->lastAction() : Acl::Answer(ACCESS_DUNNO); auto implicitRuleAnswer = Acl::Answer(ACCESS_DUNNO); if (lastAction == ACCESS_DENIED) // reverse last seen "deny" diff --git a/src/acl/Checklist.h b/src/acl/Checklist.h index 10899916948..664751a1fa9 100644 --- a/src/acl/Checklist.h +++ b/src/acl/Checklist.h @@ -104,15 +104,15 @@ class ACLChecklist * * If there are no ACLs to check at all, the result becomes ACCESS_ALLOWED. */ - Acl::Answer const & fastCheck(const Acl::Tree *list); + const Acl::Answer &fastCheck(const ACLList *); /// If slow lookups are allowed, switches into "async in progress" state. /// Otherwise, returns false; the caller is expected to handle the failure. bool goAsync(AsyncStarter, const Acl::Node &); - /// Matches (or resumes matching of) a child node while maintaning + /// Matches (or resumes matching of) a child node at pos while maintaining /// resumption breadcrumbs if a [grand]child node goes async. - bool matchChild(const Acl::InnerNode *parent, Acl::Nodes::const_iterator pos, const Acl::Node *child); + bool matchChild(const Acl::InnerNode *parent, Acl::Nodes::const_iterator pos); /// Whether we should continue to match tree nodes or stop/pause. bool keepMatching() const { return !finished() && !asyncInProgress(); } @@ -144,15 +144,7 @@ class ACLChecklist virtual void verifyAle() const = 0; /// change the current ACL list - /// \return a pointer to the old list value (may be nullptr) - const Acl::Tree *changeAcl(const Acl::Tree *t) { - const Acl::Tree *old = accessList; - if (t != accessList) { - cbdataReferenceDone(accessList); - accessList = cbdataReference(t); - } - return old; - } + void changeAcl(const acl_access *); /// remember the name of the last ACL being evaluated void setLastCheckedName(const SBuf &name) { lastCheckedName_ = name; } @@ -164,7 +156,12 @@ class ACLChecklist void matchAndFinish(); - const Acl::Tree *accessList; + /// \copydoc changeAcl() + /// \returns old accessList pointer (that may be nil) + Acl::TreePointer swapAcl(const acl_access *); + + Acl::TreePointer accessList; + public: ACLCB *callback; @@ -184,7 +181,7 @@ class ACLChecklist bool operator ==(const Breadcrumb &b) const { return parent == b.parent && (!parent || position == b.position); } bool operator !=(const Breadcrumb &b) const { return !this->operator ==(b); } void clear() { parent = nullptr; } - const Acl::InnerNode *parent; ///< intermediate node in the ACL tree + RefCount parent; ///< intermediate node in the ACL tree Acl::Nodes::const_iterator position; ///< child position inside parent }; diff --git a/src/acl/Gadgets.cc b/src/acl/Gadgets.cc index fb0492cf41d..d12040dc333 100644 --- a/src/acl/Gadgets.cc +++ b/src/acl/Gadgets.cc @@ -28,13 +28,8 @@ #include "SquidConfig.h" #include "src/sbuf/Stream.h" -#include #include -using AclSet = std::set; -/// Accumulates all ACLs to facilitate their clean deletion despite reuse. -static AclSet *RegisteredAcls; // TODO: Remove when ACLs are refcounted - err_type FindDenyInfoPage(const Acl::Answer &answer, const bool redirect_allowed) { @@ -124,8 +119,17 @@ aclParseDenyInfoLine(AclDenyInfoList ** head) *T = A; } +const Acl::Tree & +Acl::ToTree(const TreePointer * const config) +{ + Assure(config); + const auto &treePtr = *config; + Assure(treePtr); + return *treePtr; +} + void -aclParseAccessLine(const char *directive, ConfigParser &, acl_access **treep) +aclParseAccessLine(const char *directive, ConfigParser &, acl_access **config) { /* first expect either 'allow' or 'deny' */ const char *t = ConfigParser::NextToken(); @@ -147,7 +151,7 @@ aclParseAccessLine(const char *directive, ConfigParser &, acl_access **treep) return; } - const int ruleId = ((treep && *treep) ? (*treep)->childrenCount() : 0) + 1; + const int ruleId = ((config && *config) ? ToTree(*config).childrenCount() : 0) + 1; Acl::AndNode *rule = new Acl::AndNode; rule->context(ToSBuf(directive, '#', ruleId), config_input_line); @@ -161,6 +165,11 @@ aclParseAccessLine(const char *directive, ConfigParser &, acl_access **treep) /* Append to the end of this list */ + assert(config); + if (!*config) + *config = new acl_access(); + const auto treep = *config; + assert(treep); if (!*treep) { *treep = new Acl::Tree; @@ -168,13 +177,11 @@ aclParseAccessLine(const char *directive, ConfigParser &, acl_access **treep) } (*treep)->add(rule, action); - - /* We lock _acl_access structures in ACLChecklist::matchNonBlocking() */ } // aclParseAclList does not expect or set actions (cf. aclParseAccessLine) size_t -aclParseAclList(ConfigParser &, Acl::Tree **treep, const char *label) +aclParseAclList(ConfigParser &, ACLList **config, const char *label) { // accommodate callers unable to convert their ACL list context to string if (!label) @@ -184,64 +191,25 @@ aclParseAclList(ConfigParser &, Acl::Tree **treep, const char *label) rule->context(ToSBuf('(', cfg_directive, ' ', label, " line)"), config_input_line); const auto aclCount = rule->lineParse(); - // We want a cbdata-protected Tree (despite giving it only one child node). + // XXX: We have created only one node, and our callers do not support + // actions, but we now have to create an action-supporting Acl::Tree because + // Checklist needs actions support. TODO: Add actions methods to Acl::Node, + // so that Checklist can be satisfied with Acl::AndNode created above. Acl::Tree *tree = new Acl::Tree; tree->add(rule); tree->context(ToSBuf(cfg_directive, ' ', label), config_input_line); - assert(treep); - assert(!*treep); - *treep = tree; + assert(config); + assert(!*config); + *config = new acl_access(tree); return aclCount; } -void -aclRegister(Acl::Node *acl) -{ - if (!acl->registered) { - if (!RegisteredAcls) - RegisteredAcls = new AclSet; - RegisteredAcls->insert(acl); - acl->registered = true; - } -} - -/// remove registered acl from the centralized deletion set -static -void -aclDeregister(Acl::Node *acl) -{ - if (acl->registered) { - if (RegisteredAcls) - RegisteredAcls->erase(acl); - acl->registered = false; - } -} - /*********************/ /* Destroy functions */ /*********************/ -/// called to delete ALL Acls. -void -aclDestroyAcls(Acl::Node ** head) -{ - *head = nullptr; // Config.aclList - if (AclSet *acls = RegisteredAcls) { - debugs(28, 8, "deleting all " << acls->size() << " ACLs"); - while (!acls->empty()) { - auto *acl = *acls->begin(); - // We use centralized deletion (this function) so ~Acl::Node should not - // delete other ACLs, but we still deregister first to prevent any - // accesses to the being-deleted Acl::Node via RegisteredAcls. - assert(acl->registered); // make sure we are making progress - aclDeregister(acl); - delete acl; - } - } -} - void aclDestroyAclList(ACLList **list) { @@ -256,7 +224,7 @@ aclDestroyAccessList(acl_access ** list) { assert(list); if (*list) - debugs(28, 3, "destroying: " << *list << ' ' << (*list)->name); + debugs(28, 3, "destroying: " << *list << ' ' << ToTree(*list).name); delete *list; *list = nullptr; } diff --git a/src/acl/Gadgets.h b/src/acl/Gadgets.h index 1e09be11b11..6df2fa7b16b 100644 --- a/src/acl/Gadgets.h +++ b/src/acl/Gadgets.h @@ -21,26 +21,23 @@ class dlink_list; class StoreEntry; class wordlist; -/// Register an Acl::Node object for future deletion. Repeated registrations are OK. -/// \ingroup ACLAPI -void aclRegister(Acl::Node *acl); /// \ingroup ACLAPI void aclDestroyAccessList(acl_access **list); /// \ingroup ACLAPI -void aclDestroyAcls(Acl::Node **); -/// \ingroup ACLAPI void aclDestroyAclList(ACLList **); + /// Parses a single line of a "action followed by acls" directive (e.g., http_access). -/// \ingroup ACLAPI -void aclParseAccessLine(const char *directive, ConfigParser &parser, Acl::Tree **); +void aclParseAccessLine(const char *directive, ConfigParser &, acl_access **); + /// Parses a single line of a "some context followed by acls" directive (e.g., note n v). /// The label parameter identifies the context (for debugging). /// \returns the number of parsed ACL names -size_t aclParseAclList(ConfigParser &parser, Acl::Tree **, const char *label); +size_t aclParseAclList(ConfigParser &, ACLList **, const char *label); + /// Template to convert various context labels to strings. \ingroup ACLAPI template inline size_t -aclParseAclList(ConfigParser &parser, Acl::Tree **tree, const Any any) +aclParseAclList(ConfigParser &parser, ACLList ** const tree, const Any any) { std::ostringstream buf; buf << any; @@ -68,5 +65,14 @@ void dump_acl_access(StoreEntry * entry, const char *name, acl_access * head); /// \ingroup ACLAPI void dump_acl_list(StoreEntry * entry, ACLList * head); +namespace Acl { +/// convenient and safe access to a stored (and parsed/configured) Tree +/// \returns **cfg or *cfg->getRaw() +/// \prec cfg points to a non-nil TreePointer object; ACL parsing code is +/// written so that ToTree() caller may just check that cfg itself is not nil +/// (because parsing code never stores nil TreePointer objects). +const Tree &ToTree(const TreePointer *cfg); +} + #endif /* SQUID_SRC_ACL_GADGETS_H */ diff --git a/src/acl/InnerNode.cc b/src/acl/InnerNode.cc index 761b56814ca..5b38472ea56 100644 --- a/src/acl/InnerNode.cc +++ b/src/acl/InnerNode.cc @@ -37,7 +37,6 @@ Acl::InnerNode::add(Acl::Node *node) { assert(node != nullptr); nodes.push_back(node); - aclRegister(node); } // kids use this method to handle [multiple] parse() calls correctly @@ -57,7 +56,8 @@ Acl::InnerNode::lineParse() ++t; debugs(28, 3, "looking for ACL " << t); - auto *a = Acl::Node::FindByName(t); + // XXX: Performance regression: SBuf will allocate memory. + const auto a = Acl::Node::FindByName(SBuf(t)); if (a == nullptr) { debugs(28, DBG_CRITICAL, "ERROR: ACL not found: " << t); diff --git a/src/acl/InnerNode.h b/src/acl/InnerNode.h index 63f36b7fa15..0d227f519e8 100644 --- a/src/acl/InnerNode.h +++ b/src/acl/InnerNode.h @@ -15,14 +15,13 @@ namespace Acl { -using Nodes = std::vector; ///< a collection of nodes +/// operands of a boolean ACL expression, in configuration/evaluation order +using Nodes = std::vector; /// An intermediate Acl::Node tree node. Manages a collection of child tree nodes. class InnerNode: public Acl::Node { public: - // No ~InnerNode() to delete children. They are aclRegister()ed instead. - /// Resumes matching (suspended by an async call) at the given position. bool resumeMatchingAt(ACLChecklist *checklist, Acl::Nodes::const_iterator pos) const; @@ -49,8 +48,7 @@ class InnerNode: public Acl::Node /* Acl::Node API */ int match(ACLChecklist *checklist) override; - // XXX: use refcounting instead of raw pointers - Nodes nodes; ///< children nodes of this intermediate node + Nodes nodes; ///< children of this intermediate node }; } // namespace Acl diff --git a/src/acl/Node.h b/src/acl/Node.h index cd264f52244..e1f81997665 100644 --- a/src/acl/Node.h +++ b/src/acl/Node.h @@ -22,21 +22,22 @@ namespace Acl { /// Can evaluate itself in FilledChecklist context. /// Does not change during evaluation. /// \ingroup ACLAPI -class Node +class Node: public RefCountable { public: + using Pointer = RefCount; + void *operator new(size_t); void operator delete(void *); - static void ParseAclLine(ConfigParser &parser, Acl::Node **head); + /// parses acl directive parts that follow directive name (i.e. "acl") + static void ParseNamedAcl(ConfigParser &, NamedAcls *&); + static void Initialize(); /// A configured ACL with a given name or nil. static Acl::Node *FindByName(const SBuf &); - /// \copydoc FindByName() - /// \deprecated Use to avoid performance regressions; remove with the last caller. - static Acl::Node *FindByName(const char *name); Node(); Node(Node &&) = delete; // no copying of any kind @@ -79,9 +80,7 @@ class Node /// See also: context() and FindByName(). SBuf name; - char *cfgline; - Acl::Node *next; // XXX: remove or at least use refcounting - bool registered; ///< added to the global list of ACLs via aclRegister() + char *cfgline = nullptr; private: /// Matches the actual data in checklist against this Acl::Node. @@ -102,7 +101,7 @@ class Node /// \see Acl::Node::options() virtual const Acl::Options &lineOptions() { return Acl::NoOptions(); } - static void ParseNamed(ConfigParser &, Node **head, const SBuf &name); + static void ParseNamed(ConfigParser &, NamedAcls &, const SBuf &name); }; } // namespace Acl diff --git a/src/acl/Tree.cc b/src/acl/Tree.cc index 1ecf291e46e..faa3d05f935 100644 --- a/src/acl/Tree.cc +++ b/src/acl/Tree.cc @@ -11,8 +11,6 @@ #include "acl/Tree.h" #include "wordlist.h" -CBDATA_NAMESPACED_CLASS_INIT(Acl, Tree); - Acl::Answer Acl::Tree::winningAction() const { diff --git a/src/acl/Tree.h b/src/acl/Tree.h index 5e3a3381feb..63999762df6 100644 --- a/src/acl/Tree.h +++ b/src/acl/Tree.h @@ -17,13 +17,11 @@ namespace Acl { -/// An ORed set of rules at the top of the ACL expression tree, providing two -/// unique properties: cbdata protection and optional rule actions. +/// An ORed set of rules at the top of the ACL expression tree with support for +/// optional rule actions. class Tree: public OrNode { - // XXX: We should use refcounting instead, but it requires making ACLs - // refcounted as well. Otherwise, async lookups will reach deleted ACLs. - CBDATA_CLASS(Tree); + MEMPROXY_CLASS(Tree); public: /// dumps tuples diff --git a/src/acl/forward.h b/src/acl/forward.h index d11747ff720..e47095dddf6 100644 --- a/src/acl/forward.h +++ b/src/acl/forward.h @@ -13,7 +13,6 @@ class ACLChecklist; class ACLFilledChecklist; -class ACLList; class AclDenyInfoList; class AclSizeLimit; @@ -27,6 +26,7 @@ class AndNode; class Answer; class ChecklistFiller; class InnerNode; +class NamedAcls; class NotNode; class OrNode; class Tree; @@ -34,14 +34,17 @@ class Tree; /// prepares to parse ACLs configuration void Init(void); +/// reconfiguration-safe storage of ACL rules +using TreePointer = RefCount; + } // namespace Acl typedef void ACLCB(Acl::Answer, void *); // TODO: Consider renaming all users and removing. Cons: hides the difference // between ACLList tree without actions and acl_access Tree with actions. -#define acl_access Acl::Tree -#define ACLList Acl::Tree +using acl_access = Acl::TreePointer; +using ACLList = Acl::TreePointer; class ExternalACLEntry; typedef RefCount ExternalACLEntryPointer; diff --git a/src/cache_cf.cc b/src/cache_cf.cc index 10e15d270e6..b66ba93cd91 100644 --- a/src/cache_cf.cc +++ b/src/cache_cf.cc @@ -1486,26 +1486,23 @@ free_SBufList(SBufList *list) } static void -dump_acl(StoreEntry * entry, const char *name, Acl::Node * ae) +dump_acl(StoreEntry *entry, const char *directiveName, Acl::NamedAcls *config) { PackableStream os(*entry); - while (ae != nullptr) { - debugs(3, 3, "dump_acl: " << name << " " << ae->name); - ae->dumpWhole(name, os); - ae = ae->next; - } + Acl::DumpNamedAcls(os, directiveName, config); } static void -parse_acl(Acl::Node ** ae) +parse_acl(Acl::NamedAcls **config) { - Acl::Node::ParseAclLine(LegacyParser, ae); + assert(config); + Acl::Node::ParseNamedAcl(LegacyParser, *config); } static void -free_acl(Acl::Node ** ae) +free_acl(Acl::NamedAcls **config) { - aclDestroyAcls(ae); + Acl::FreeNamedAcls(config); } void @@ -1513,14 +1510,14 @@ dump_acl_list(StoreEntry * entry, ACLList * head) { // XXX: Should dump ACL names like "foo !bar" but dumps parsing context like // "(clientside_tos 0x11 line)". - dump_SBufList(entry, head->dump()); + dump_SBufList(entry, ToTree(head).dump()); } void dump_acl_access(StoreEntry * entry, const char *name, acl_access * head) { if (head) - dump_SBufList(entry, head->treeDump(name, &Acl::AllowOrDeny)); + dump_SBufList(entry, ToTree(head).treeDump(name, &Acl::AllowOrDeny)); } static void @@ -2011,7 +2008,7 @@ static void dump_AuthSchemes(StoreEntry *entry, const char *name, acl_access *authSchemes) { if (authSchemes) - dump_SBufList(entry, authSchemes->treeDump(name, [](const Acl::Answer &action) { + dump_SBufList(entry, ToTree(authSchemes).treeDump(name, [](const Acl::Answer &action) { return Auth::TheConfig.schemeLists.at(action.kind).rawSchemes; })); } @@ -2019,13 +2016,17 @@ dump_AuthSchemes(StoreEntry *entry, const char *name, acl_access *authSchemes) #endif /* USE_AUTH */ static void -ParseAclWithAction(acl_access **access, const Acl::Answer &action, const char *desc, Acl::Node *acl) +ParseAclWithAction(acl_access **config, const Acl::Answer &action, const char *desc, Acl::Node *acl) { - assert(access); - if (!*access) { - *access = new Acl::Tree; - (*access)->context(ToSBuf('(', desc, " rules)"), config_input_line); + assert(config); + auto &access = *config; + if (!access) { + const auto tree = new Acl::Tree; + tree->context(ToSBuf('(', desc, " rules)"), config_input_line); + access = new acl_access(tree); } + assert(access); + Acl::AndNode *rule = new Acl::AndNode; rule->context(ToSBuf('(', desc, " rule)"), config_input_line); if (acl) @@ -4485,7 +4486,7 @@ static void parse_sslproxy_ssl_bump(acl_access **ssl_bump) static void dump_sslproxy_ssl_bump(StoreEntry *entry, const char *name, acl_access *ssl_bump) { if (ssl_bump) - dump_SBufList(entry, ssl_bump->treeDump(name, [](const Acl::Answer &action) { + dump_SBufList(entry, ToTree(ssl_bump).treeDump(name, [](const Acl::Answer &action) { return Ssl::BumpModeStr.at(action.kind); })); } @@ -4729,7 +4730,7 @@ static void parse_ftp_epsv(acl_access **ftp_epsv) static void dump_ftp_epsv(StoreEntry *entry, const char *name, acl_access *ftp_epsv) { if (ftp_epsv) - dump_SBufList(entry, ftp_epsv->treeDump(name, Acl::AllowOrDeny)); + dump_SBufList(entry, ToTree(ftp_epsv).treeDump(name, Acl::AllowOrDeny)); } static void free_ftp_epsv(acl_access **ftp_epsv) @@ -4900,7 +4901,7 @@ dump_on_unsupported_protocol(StoreEntry *entry, const char *name, acl_access *ac "respond" }; if (access) { - SBufList lines = access->treeDump(name, [](const Acl::Answer &action) { + const auto lines = ToTree(access).treeDump(name, [](const Acl::Answer &action) { return onErrorTunnelMode.at(action.kind); }); dump_SBufList(entry, lines); @@ -4934,7 +4935,7 @@ dump_http_upgrade_request_protocols(StoreEntry *entry, const char *rawName, Http SBufList line; line.push_back(name); line.push_back(proto); - const auto acld = acls->treeDump("", &Acl::AllowOrDeny); + const auto acld = ToTree(acls).treeDump("", &Acl::AllowOrDeny); line.insert(line.end(), acld.begin(), acld.end()); dump_SBufList(entry, line); }); diff --git a/src/cf.data.pre b/src/cf.data.pre index 4ebdf93a515..bf8eb45ee48 100644 --- a/src/cf.data.pre +++ b/src/cf.data.pre @@ -1089,7 +1089,7 @@ DOC_END NAME: acl TYPE: acl -LOC: Config.aclList +LOC: Config.namedAcls IF USE_OPENSSL DEFAULT: ssl::certHasExpired ssl_error X509_V_ERR_CERT_HAS_EXPIRED DEFAULT: ssl::certNotYetValid ssl_error X509_V_ERR_CERT_NOT_YET_VALID diff --git a/src/clients/Client.cc b/src/clients/Client.cc index b5d8a8d2b7c..551e82e936b 100644 --- a/src/clients/Client.cc +++ b/src/clients/Client.cc @@ -551,7 +551,7 @@ Client::haveParsedReplyHeaders() bool Client::blockCaching() { - if (const Acl::Tree *acl = Config.accessList.storeMiss) { + if (const auto acl = Config.accessList.storeMiss) { // This relatively expensive check is not in StoreEntry::checkCachable: // That method lacks HttpRequest and may be called too many times. ACLFilledChecklist ch(acl, originalRequest().getRaw()); diff --git a/src/external_acl.cc b/src/external_acl.cc index 87d60da86f9..e32592bece0 100644 --- a/src/external_acl.cc +++ b/src/external_acl.cc @@ -592,6 +592,14 @@ copyResultsFromEntry(const HttpRequest::Pointer &req, const ExternalACLEntryPoin Acl::Answer ACLExternal::aclMatchExternal(external_acl_data *acl, ACLFilledChecklist *ch) const { + // Despite its external_acl C++ type name, acl->def is not an ACL (i.e. not + // a reference-counted Acl::Node) and gets invalidated by reconfiguration. + // TODO: RefCount external_acl, so that we do not have to bail here. + if (!cbdataReferenceValid(acl->def)) { + debugs(82, 3, "cannot resume matching; external_acl gone"); + return ACCESS_DUNNO; + } + debugs(82, 9, "acl=\"" << acl->def->name << "\""); ExternalACLEntryPointer entry = ch->extacl_entry; diff --git a/src/security/KeyLog.cc b/src/security/KeyLog.cc index 219a229b698..fb97e05a307 100644 --- a/src/security/KeyLog.cc +++ b/src/security/KeyLog.cc @@ -62,7 +62,7 @@ Security::KeyLog::dump(std::ostream &os) const dumpOptions(os); if (aclList) { // TODO: Use Acl::dump() after fixing the XXX in dump_acl_list(). - for (const auto &acl: aclList->treeDump("if", &Acl::AllowOrDeny)) + for (const auto &acl: ToTree(aclList).treeDump("if", &Acl::AllowOrDeny)) os << ' ' << acl; } } diff --git a/src/tests/stub_acl.cc b/src/tests/stub_acl.cc index 29c1027bc62..2d07fcd7188 100644 --- a/src/tests/stub_acl.cc +++ b/src/tests/stub_acl.cc @@ -16,5 +16,5 @@ #include "acl/forward.h" #include "acl/Gadgets.h" -size_t aclParseAclList(ConfigParser &, Acl::Tree **, const char *) STUB_RETVAL(0) +size_t aclParseAclList(ConfigParser &, ACLList **, const char *) STUB_RETVAL(0) diff --git a/src/tests/testACLMaxUserIP.cc b/src/tests/testACLMaxUserIP.cc index fea6a8492b9..2c856c21969 100644 --- a/src/tests/testACLMaxUserIP.cc +++ b/src/tests/testACLMaxUserIP.cc @@ -15,6 +15,7 @@ #include "auth/UserRequest.h" #include "compat/cppunit.h" #include "ConfigParser.h" +#include "SquidConfig.h" #include "unitTestMain.h" #include @@ -74,9 +75,11 @@ TestACLMaxUserIP::testParseLine() char * line = xstrdup("test max_user_ip -s 1"); /* seed the parser */ ConfigParser::SetCfgLine(line); - Acl::Node *anACL = nullptr; ConfigParser LegacyParser; - Acl::Node::ParseAclLine(LegacyParser, &anACL); + Acl::Node::ParseNamedAcl(LegacyParser, Config.namedAcls); + CPPUNIT_ASSERT(Config.namedAcls); + const auto anACL = Acl::Node::FindByName(SBuf("test")); + CPPUNIT_ASSERT(anACL); ACLMaxUserIP *maxUserIpACL = dynamic_cast(anACL); CPPUNIT_ASSERT(maxUserIpACL); if (maxUserIpACL) { @@ -86,7 +89,7 @@ TestACLMaxUserIP::testParseLine() /* the acl must be vaid */ CPPUNIT_ASSERT_EQUAL(true, maxUserIpACL->valid()); } - delete anACL; + Acl::FreeNamedAcls(&Config.namedAcls); xfree(line); } From 2d93cfe715b35839cab989711574522921c7eaf3 Mon Sep 17 00:00:00 2001 From: Francesco Chemolli <5175948+kinkie@users.noreply.github.com> Date: Tue, 6 Aug 2024 17:44:00 +0000 Subject: [PATCH 14/17] ext_time_quota_acl: remove -l option (#1872) Supporting logging to a file complicates upgrading helper code to use debugs() because DebugFile code calls commSetCloseOnExec(), and our comm/libminimal does not currently provide a functioning implementation for that API: The existing implementation is an unconditional assert. To save development time while upgrading helpers, we are dropping this feature. It can probably be emulated using shell redirection tricks. --- doc/release-notes/release-7.sgml.in | 9 +++++++ .../external/time_quota/ext_time_quota_acl.8 | 10 ++----- .../external/time_quota/ext_time_quota_acl.cc | 26 ++++++------------- 3 files changed, 19 insertions(+), 26 deletions(-) diff --git a/doc/release-notes/release-7.sgml.in b/doc/release-notes/release-7.sgml.in index 35fc35d9586..e3150e65ffe 100644 --- a/doc/release-notes/release-7.sgml.in +++ b/doc/release-notes/release-7.sgml.in @@ -286,6 +286,15 @@ This section gives an account of those changes in three categories: +Other changes

+ + Adjusted configuration and format of ext_time_quota_acl helper debugging +

The -l option that enables ext_time_quota_acl to log debug messages + to a custom logfile has been removed, and their format has been + changed to be in line with Squid's cache.log format. + + Copyright

Copyright (C) 1996-2023 The Squid Software Foundation and contributors diff --git a/src/acl/external/time_quota/ext_time_quota_acl.8 b/src/acl/external/time_quota/ext_time_quota_acl.8 index 4abf99890e3..579e0af17d7 100644 --- a/src/acl/external/time_quota/ext_time_quota_acl.8 +++ b/src/acl/external/time_quota/ext_time_quota_acl.8 @@ -7,7 +7,7 @@ Version 1.0 . .SH SYNOPSIS .if !'po4a'hide' .B ext_time_quota_acl -.if !'po4a'hide' .B "[\-b database] [\-l logfile] [\-d] [\-p pauselen] [\-h] configfile +.if !'po4a'hide' .B "[\-b database] [\-d] [\-p pauselen] [\-h] configfile . .SH DESCRIPTION .B ext_time_quota_acl @@ -35,14 +35,8 @@ Pauses shorter than this value will be counted against the quota, longer ones ig Default is 300 seconds (5 minutes). . .if !'po4a'hide' .TP -.if !'po4a'hide' .B "\-l logfile" -.B Filename -where all logging and debugging information will be written. If none is given, -then stderr will be used and the logging will go to Squids main cache.log. -. -.if !'po4a'hide' .TP .if !'po4a'hide' .B "\-d" -Enables debug logging in the logfile. +Enables debug logging to stderr. . .if !'po4a'hide' .TP .if !'po4a'hide' .B "\-h" diff --git a/src/acl/external/time_quota/ext_time_quota_acl.cc b/src/acl/external/time_quota/ext_time_quota_acl.cc index 38fc70a923f..5158e8f4584 100644 --- a/src/acl/external/time_quota/ext_time_quota_acl.cc +++ b/src/acl/external/time_quota/ext_time_quota_acl.cc @@ -76,15 +76,6 @@ static int pauseLength = 300; static FILE *logfile = stderr; static int tq_debug_enabled = false; -static void open_log(const char *logfilename) -{ - logfile = fopen(logfilename, "a"); - if ( logfile == NULL ) { - perror(logfilename); - logfile = stderr; - } -} - static void vlog(const char *level, const char *format, va_list args) { time_t now = time(NULL); @@ -397,11 +388,8 @@ static void processActivity(const char *user_key) static void usage(void) { - log_error("Wrong usage. Please reconfigure in squid.conf.\n"); - - fprintf(stderr, "Usage: %s [-d] [-l logfile] [-b dbpath] [-p pauselen] [-h] configfile\n", program_name); - fprintf(stderr, " -d enable debugging output to logfile\n"); - fprintf(stderr, " -l logfile log messages to logfile\n"); + fprintf(stderr, "Usage: %s [-d] [-b dbpath] [-p pauselen] [-h] configfile\n", program_name); + fprintf(stderr, " -d enable debugging output\n"); fprintf(stderr, " -b dbpath Path where persistent session database will be kept\n"); fprintf(stderr, " If option is not used, then " DEFAULT_QUOTA_DB " will be used.\n"); fprintf(stderr, " -p pauselen length in seconds to describe a pause between 2 requests.\n"); @@ -416,14 +404,11 @@ int main(int argc, char **argv) program_name = argv[0]; - while ((opt = getopt(argc, argv, "dp:l:b:h")) != -1) { + while ((opt = getopt(argc, argv, "dp:b:h")) != -1) { switch (opt) { case 'd': tq_debug_enabled = true; break; - case 'l': - open_log(optarg); - break; case 'b': db_path = optarg; break; @@ -434,6 +419,11 @@ int main(int argc, char **argv) usage(); exit(EXIT_SUCCESS); break; + default: + // getopt() emits error message to stderr + usage(); + exit(EXIT_FAILURE); + break; } } From f5d4fc4f3caa293baae0fec138c1031bc4808774 Mon Sep 17 00:00:00 2001 From: Amos Jeffries Date: Tue, 6 Aug 2024 23:17:16 +0000 Subject: [PATCH 15/17] Maintenance: Replace one self_destruct() with throw (#1871) --- src/acl/Acl.cc | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/src/acl/Acl.cc b/src/acl/Acl.cc index 2d07d3fc9ac..d48b5c0813b 100644 --- a/src/acl/Acl.cc +++ b/src/acl/Acl.cc @@ -61,11 +61,8 @@ Acl::Node * Make(TypeName typeName) { const auto pos = TheMakers().find(typeName); - if (pos == TheMakers().end()) { - debugs(28, DBG_CRITICAL, "FATAL: Invalid ACL type '" << typeName << "'"); - self_destruct(); - assert(false); // not reached - } + if (pos == TheMakers().end()) + throw TextException(ToSBuf("invalid ACL type '", typeName, "'"), Here()); auto *result = (pos->second)(pos->first); debugs(28, 4, typeName << '=' << result); From a1f1b1d287fd9fcd3ea10ec7e05c08e01dd6877f Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Wed, 7 Aug 2024 03:12:30 +0000 Subject: [PATCH 16/17] Maintenance: Remove unused SNPRINTFSOURCE variable (#1878) This Makefile.am variable became unused in 2007 commit d2eebe0f. --- lib/Makefile.am | 1 - 1 file changed, 1 deletion(-) diff --git a/lib/Makefile.am b/lib/Makefile.am index 02fd5e8d75e..0c09a0e57d8 100644 --- a/lib/Makefile.am +++ b/lib/Makefile.am @@ -54,7 +54,6 @@ libmisccontainers_la_SOURCES = \ hash.cc libmiscutil_la_SOURCES = \ - $(SNPRINTFSOURCE) \ Splay.cc \ heap.c \ radix.c \ From a18aabb2d94100cf303deea71e1216bcea2847d7 Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Wed, 7 Aug 2024 06:56:21 +0000 Subject: [PATCH 17/17] Maintenance: Remove unused fde::flags.close_on_exec (#1879) This flag became effectively unused in 2010 commit cfd66529 when copyFDFlags() -- the only function checking the flag -- became unused. That unused function was removed a bit later in commit 5ae21d99. --- src/comm.cc | 6 ------ src/fde.h | 1 - 2 files changed, 7 deletions(-) diff --git a/src/comm.cc b/src/comm.cc index af51538a81a..5c8fbd83f15 100644 --- a/src/comm.cc +++ b/src/comm.cc @@ -555,9 +555,6 @@ comm_import_opened(const Comm::ConnectionPointer &conn, comm_init_opened(conn, note, AI); - if (!(conn->flags & COMM_NOCLOEXEC)) - fd_table[conn->fd].flags.close_on_exec = true; - if (conn->local.port() > (unsigned short) 0) { #if _SQUID_WINDOWS_ if (AI->ai_socktype != SOCK_DGRAM) @@ -1146,9 +1143,6 @@ commSetCloseOnExec(int fd) int xerrno = errno; debugs(50, DBG_CRITICAL, "ERROR: " << MYNAME << "FD " << fd << ": set close-on-exec failed: " << xstrerr(xerrno)); } - - fd_table[fd].flags.close_on_exec = true; - #endif } diff --git a/src/fde.h b/src/fde.h index 61f77d94c2b..7607c99ba0d 100644 --- a/src/fde.h +++ b/src/fde.h @@ -124,7 +124,6 @@ class fde bool ipc = false; bool called_connect = false; bool nodelay = false; - bool close_on_exec = false; /// buffering readMethod_ has data to give (regardless of socket state) bool read_pending = false; //bool write_pending; //XXX seems not to be used