From: Alex Rousskov Date: Fri, 14 Jun 2024 16:15:16 +0000 (+0000) Subject: Fix reporting of unrecognized directives (#1841) X-Git-Tag: SQUID_7_0_1~105 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=3e310519fedc7ad1c52dd925641823c6fe06d942;p=thirdparty%2Fsquid.git Fix reporting of unrecognized directives (#1841) When Squid does not recognize a directive name, it reports the problem and proceeds to parse the remaining configuration lines. When parsing is complete, Squid quits if it has encountered such directives. This change preserves this overall behavior while fixing related reporting problems. Fixed problems depend on whether Squid discovered an unrecognized directive at startup or during reconfiguration. Startup configuration case was missing both an ERROR classification and a FATAL message, resulting in a somewhat mysterious death: Processing Configuration File: squid.conf (depth 0) squid.conf(7): unrecognized: 'http_accesses' Starting Authentication on port [::]:4443 Disabling Authentication on port [::]:4443 (interception enabled) If other non-fatal ERRORs were present, that startup death reporting became more problematic because admins would naturally (but incorrectly) assume that Squid died due to those irrelevant ERRORs: Processing Configuration File: squid.conf (depth 0) squid.conf(7): unrecognized: 'http_accesses' ERROR: Unsupported TLS option SINGLE_DH_USE Reconfiguration case was worse because Squid clearly attributed failure to the wrong (innocent and default) directive that was not even present in the configuration file (among other problems like saying "null"): Reconfiguring Squid Cache (version 7.0.0-VCS)... squid.conf(7): unrecognized: 'http_accesses' ERROR: Unsupported TLS option SINGLE_DH_USE FATAL: Bungled (null) line 3: sslproxy_cert_sign signTrusted all Squid Cache (Version 7.0.0-VCS): Terminated abnormally. Squid now reports unrecognized directives using the standardized ERROR prefix and prints correct FATAL message in both cases: Processing Configuration File: squid.conf (depth 0) ERROR: unrecognized directive near 'http_accesses' directive location: squid.conf(7) FATAL: reconfiguration failure: Found 1 unrecognized directive(s) exception location: cache_cf.cc(610) Parse advice: Run 'squid -k parse' and check for ERRORs. Squid Cache (Version 7.0.0-VCS): Terminated abnormally. Also reduced the number of nested try/catch blocks, simplifying configuration code. However, this change is just a small step forward. More fatal (re)configuration error handling improvements are needed, including removing unwanted and noisy fatal() side effects and possibly recognizing other kinds of errors that should not immediately terminate configuration parsing. --- diff --git a/src/ConfigParser.h b/src/ConfigParser.h index f85c20ea3f..4b588bcaee 100644 --- a/src/ConfigParser.h +++ b/src/ConfigParser.h @@ -232,7 +232,10 @@ protected: static enum ParsingStates {atParseKey, atParseValue} KvPairState_; ///< Parsing state while parsing kv-pair tokens }; -int parseConfigFile(const char *file_name); +namespace Configuration { +/// interprets (and partially applies) squid.conf or equivalent configuration +void Parse(); +} #endif /* SQUID_SRC_CONFIGPARSER_H */ diff --git a/src/cache_cf.cc b/src/cache_cf.cc index d88fc0dcf1..c9e3d5994b 100644 --- a/src/cache_cf.cc +++ b/src/cache_cf.cc @@ -561,7 +561,8 @@ parseOneConfigFile(const char *file_name, unsigned int depth) } else { try { if (!parse_line(tmp_line)) { - debugs(3, DBG_CRITICAL, ConfigParser::CurrentLocation() << ": unrecognized: '" << tmp_line << "'"); + debugs(3, DBG_CRITICAL, "ERROR: unrecognized directive near '" << tmp_line << "'" << + Debug::Extra << "directive location: " << ConfigParser::CurrentLocation()); ++err_count; } } catch (...) { @@ -595,12 +596,9 @@ parseOneConfigFile(const char *file_name, unsigned int depth) return err_count; } -static -int -parseConfigFileOrThrow(const char *file_name) +void +Configuration::Parse() { - int err_count = 0; - debugs(5, 4, MYNAME); configFreeMemory(); @@ -608,7 +606,7 @@ parseConfigFileOrThrow(const char *file_name) ACLMethodData::ThePurgeCount = 0; default_all(); - err_count = parseOneConfigFile(file_name, 0); + const auto unrecognizedDirectives = parseOneConfigFile(ConfigFile, 0); defaults_if_none(); @@ -621,28 +619,21 @@ parseConfigFileOrThrow(const char *file_name) */ configDoConfigure(); + // TODO: Throw before configDoConfigure(). Doing that would reduce the set + // of configuration errors Squid can detect in one execution, but we should + // not apply a clearly broken configuration, especially when we are going to + // quit anyway. While some legacy parse_foo() functions apply significant + // changes before configDoConfigure(), we cannot easily stop them. We can + // easily stop configDoConfigure(). + if (unrecognizedDirectives) + throw TextException(ToSBuf("Found ", unrecognizedDirectives, " unrecognized directive(s)"), Here()); + if (opt_send_signal == -1) { Mgr::RegisterAction("config", "Current Squid Configuration", dump_config, 1, 1); } - - return err_count; -} - -// TODO: Refactor main.cc to centrally handle (and report) all exceptions. -int -parseConfigFile(const char *file_name) -{ - try { - return parseConfigFileOrThrow(file_name); - } - catch (const std::exception &ex) { - debugs(3, DBG_CRITICAL, "FATAL: bad configuration: " << ex.what()); - self_destruct(); - return 1; // not reached - } } /* diff --git a/src/main.cc b/src/main.cc index 5185977503..60744a8131 100644 --- a/src/main.cc +++ b/src/main.cc @@ -884,6 +884,18 @@ mainReconfigureStart(void) false); } +/// error message to log when Configuration::Parse() fails +static SBuf +ConfigurationFailureMessage() +{ + SBufStream out; + out << (reconfiguring ? "re" : ""); + out << "configuration failure: " << CurrentException; + if (!opt_parse_cfg_only) + out << Debug::Extra << "advice: Run 'squid -k parse' and check for ERRORs."; + return out.buf(); +} + static void mainReconfigureFinish(void *) { @@ -896,18 +908,13 @@ mainReconfigureFinish(void *) if (Config2.onoff.enable_purge) Config2.onoff.enable_purge = 2; - // parse the config returns a count of errors encountered. const int oldWorkers = Config.workers; + try { - if (parseConfigFile(ConfigFile) != 0) { - // for now any errors are a fatal condition... - self_destruct(); - } + Configuration::Parse(); } catch (...) { // for now any errors are a fatal condition... - debugs(1, DBG_CRITICAL, "FATAL: Unhandled exception parsing config file. " << - " Run squid -k parse and check for errors."); - self_destruct(); + fatal(ConfigurationFailureMessage().c_str()); } if (oldWorkers != Config.workers) { @@ -1568,8 +1575,6 @@ SquidMain(int argc, char **argv) /* parse configuration file * note: in "normal" case this used to be called from mainInitialize() */ { - int parse_err; - if (!ConfigFile) ConfigFile = xstrdup(DEFAULT_CONFIG_FILE); @@ -1594,16 +1599,20 @@ SquidMain(int argc, char **argv) RunRegisteredHere(RegisteredRunner::bootstrapConfig); try { - parse_err = parseConfigFile(ConfigFile); + Configuration::Parse(); } catch (...) { - // for now any errors are a fatal condition... - debugs(1, DBG_CRITICAL, "FATAL: Unhandled exception parsing config file." << - (opt_parse_cfg_only ? " Run squid -k parse and check for errors." : "")); - parse_err = 1; + auto msg = ConfigurationFailureMessage(); + if (opt_parse_cfg_only) { + debugs(3, DBG_CRITICAL, "FATAL: " << msg); + return EXIT_FAILURE; + } else { + fatal(msg.c_str()); + return EXIT_FAILURE; // unreachable + } } - if (opt_parse_cfg_only || parse_err > 0) - return parse_err; + if (opt_parse_cfg_only) + return EXIT_SUCCESS; } setUmask(Config.umask);