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 f85c20ea3ff16e3fc6e549e8c10a99b1c2c7b645..4b588bcaeed7669a8e92426dc1e9cfeea86dd962 100644 (file)
--- 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 d88fc0dcf13261d754059c613c0bc117d39fe4e0..c9e3d5994b05e4bb0a82371f998f5f7ba80036ac 100644 (file)
--- 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 51859775035534f53d2200ca01402a91544186e4..60744a813191da8259cd872576803b1c614623b6 100644 (file)
--- 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);