[PATCH] abort initialization on abrt.conf parsing errors

[Denys Vlasenko]

LoadSettings() is too liberal: it only warns about parse errors.
With this patch, obviously broken config files are detected
and abrtd fails to start. Example: I removed = sign from
"Database = SQLite3" line, and:

# abrtd -dvv
abrtd: Loading settings
abrtd: abrt.conf: Invalid syntax on line 23
abrtd: Error while initializing daemon
abrtd: Exiting

-- 
vda


diff -x '*.po' -d -urpN abrt.5/src/Daemon/Daemon.cpp abrt.6/src/Daemon/Daemon.cpp
--- abrt.5/src/Daemon/Daemon.cpp	2010-07-13 12:55:39.000000000 +0200
+++ abrt.6/src/Daemon/Daemon.cpp	2010-07-13 13:53:48.216072655 +0200
@@ -842,7 +842,8 @@ int main(int argc, char** argv)
         init_daemon_logging(&watcher);
 
         VERB1 log("Loading settings");
-        LoadSettings();
+        if (LoadSettings() != 0)
+            throw 1;
 
         VERB1 log("Initializing XML-RPC library");
         xmlrpc_env env;
diff -x '*.po' -d -urpN abrt.5/src/Daemon/Settings.cpp
abrt.6/src/Daemon/Settings.cpp
--- abrt.5/src/Daemon/Settings.cpp	2010-06-22 18:19:11.000000000 +0200
+++ abrt.6/src/Daemon/Settings.cpp	2010-07-13 14:14:40.054822644 +0200
@@ -99,7 +99,7 @@ static set_string_t ParseList(const char
 }
 
 /* Format: name, name(param),name("param with spaces \"and quotes\"")
*/
-static vector_pair_string_string_t ParseListWithArgs(const char *pValue)
+static vector_pair_string_string_t ParseListWithArgs(const char *pValue, int *err)
 {
     VERB3 log(" ParseListWithArgs(%s)", pValue);
 
@@ -136,7 +136,10 @@ static vector_pair_string_string_t Parse
                 is_arg = true;
             }
             else
+            {
+                *err = 1;
                 error_msg("Parser error: Invalid syntax on column %d in
\"%s\"", ii, pValue);
+            }
 
             continue;
         }
@@ -151,7 +154,10 @@ static vector_pair_string_string_t Parse
                 action = "";
             }
             else
+            {
+                *err = 1;
                 error_msg("Parser error: Invalid syntax on column %d in
\"%s\"", ii, pValue);
+            }
 
             continue;
         }
@@ -169,10 +175,16 @@ static vector_pair_string_string_t Parse
     }
 
     if (is_quote)
+    {
+        *err = 1;
         error_msg("Parser error: Unclosed quote in \"%s\"", pValue);
+    }
 
     if (is_arg)
+    {
+        *err = 1;
         error_msg("Parser error: Unclosed argument in \"%s\"",
pValue);
+    }
     else if (item != "")
     {
         VERB3 log(" adding (%s,%s)", item.c_str(), "");
@@ -181,7 +193,7 @@ static vector_pair_string_string_t Parse
     return pluginsWithArgs;
 }
 
-static void ParseCommon()
+static int ParseCommon()
 {
     map_string_t::const_iterator end = s_mapSectionCommon.end();
     map_string_t::const_iterator it = s_mapSectionCommon.find("OpenGPGCheck");
@@ -217,26 +229,34 @@ static void ParseCommon()
     it = s_mapSectionCommon.find("ActionsAndReporters");
     if (it != end)
     {
-        g_settings_vectorActionsAndReporters = ParseListWithArgs(it->second.c_str());
+        int err = 0;
+        g_settings_vectorActionsAndReporters = ParseListWithArgs(it->second.c_str(),
&err);
+        if (err)
+            return err;
     }
     it = s_mapSectionCommon.find("ProcessUnpackaged");
     if (it != end)
     {
         g_settings_bProcessUnpackaged = string_to_bool(it->second.c_str());
     }
+    return 0; /* no error */
 }
 
-static void ParseCron()
+static int ParseCron()
 {
     map_string_t::iterator it = s_mapSectionCron.begin();
     for (; it != s_mapSectionCron.end(); it++)
     {
-        vector_pair_string_string_t actionsAndReporters =
ParseListWithArgs(it->second.c_str());
+        int err = 0;
+        vector_pair_string_string_t actionsAndReporters =
ParseListWithArgs(it->second.c_str(), &err);
+        if (err)
+            return err;
         g_settings_mapCron[it->first] = actionsAndReporters;
     }
+    return 0; /* no error */
 }
 
-static set_string_t ParseKey(const char *Key)
+static set_string_t ParseKey(const char *Key, int *err)
 {
     unsigned int ii;
     std::string item;
@@ -266,7 +286,10 @@ static set_string_t ParseKey(const char 
                 item = "";
             }
             else
+            {
+                *err = 1;
                 error_msg("Parser error: Invalid syntax on column %d in
\"%s\"", ii, Key);
+            }
         }
         else
         {
@@ -275,6 +298,7 @@ static set_string_t ParseKey(const char 
     }
     if (is_quote)
     {
+        *err = 1;
         error_msg("Parser error: Unclosed quote in \"%s\"", Key);
     }
     else if (item != "")
@@ -291,13 +315,16 @@ static set_string_t ParseKey(const char 
     return set;
 }
 
-static void ParseAnalyzerActionsAndReporters()
+static int ParseAnalyzerActionsAndReporters()
 {
     map_string_t::iterator it = s_mapSectionAnalyzerActionsAndReporters.begin();
     for (; it != s_mapSectionAnalyzerActionsAndReporters.end(); it++)
     {
-        set_string_t keys = ParseKey(it->first.c_str());
-        vector_pair_string_string_t actionsAndReporters =
ParseListWithArgs(it->second.c_str());
+        int err = 0;
+        set_string_t keys = ParseKey(it->first.c_str(), &err);
+        vector_pair_string_string_t actionsAndReporters =
ParseListWithArgs(it->second.c_str(), &err);
+        if (err)
+            return err;
         set_string_t::iterator it_keys = keys.begin();
         for (; it_keys != keys.end(); it_keys++)
         {
@@ -305,6 +332,7 @@ static void ParseAnalyzerActionsAndRepor
             g_settings_mapAnalyzerActionsAndReporters[*it_keys] = actionsAndReporters;
         }
     }
+    return 0; /* no error */
 }
 
 static void LoadGPGKeys()
@@ -332,7 +360,7 @@ static void LoadGPGKeys()
  * Reads configuration from file to s_mapSection* static variables.
  * The file must be opened for reading.
  */
-static void ReadConfigurationFromFile(FILE *fp)
+static int ReadConfigurationFromFile(FILE *fp)
 {
     char line[512];
     std::string section;
@@ -395,26 +423,32 @@ static void ReadConfigurationFromFile(FI
         if (is_quote)
         {
             error_msg("abrt.conf: Invalid syntax on line %d", lineno);
-            continue;
+            return 1; /* error */
         }
 
         if (is_section)
         {
             if (line[ii] != ']') /* section not closed */
+            {
                 error_msg("abrt.conf: Section not closed on line %d", lineno);
+                return 1; /* error */
+            }
             continue;
         }
 
         if (is_key)
         {
             if (!value.empty()) /* the key is stored in value */
+            {
                 error_msg("abrt.conf: Invalid syntax on line %d", lineno);
+                return 1; /* error */
+            }
             continue;
         }
         else if (key.empty()) /* A line without key: " = something" */
         {
             error_msg("abrt.conf: Invalid syntax on line %d", lineno);
-            continue;
+            return 1; /* error */
         }
 
         if (section == SECTION_COMMON)
@@ -436,35 +470,48 @@ static void ReadConfigurationFromFile(FI
             s_mapSectionCron[key] += value;
         }
         else
+        {
             error_msg("abrt.conf: Ignoring entry in invalid section [%s]",
section.c_str());
+            return 1; /* error */
+        }
     }
+    return 0; /* success */
 }
 
 /* abrt daemon loads .conf file */
-void LoadSettings()
+int LoadSettings()
 {
+    int err = 0;
+
     FILE *fp = fopen(CONF_DIR"/abrt.conf", "r");
     if (fp)
     {
-        ReadConfigurationFromFile(fp);
+        err = ReadConfigurationFromFile(fp);
         fclose(fp);
     }
     else
         error_msg("Unable to read configuration file %s",
CONF_DIR"/abrt.conf");
 
-    ParseCommon();
-    ParseAnalyzerActionsAndReporters();
-    ParseCron();
+    if (err == 0)
+        err = ParseCommon();
+    if (err == 0)
+        err = ParseAnalyzerActionsAndReporters();
+    if (err == 0)
+        err = ParseCron();
 
-    /*
-        loading gpg keys will invoke LoadOpenGPGPublicKey() from rpm.cpp
-        pgpReadPkts which makes nss to re-init and thus makes
-        bugzilla plugin work :-/
-    */
+    if (err == 0)
+    {
+        /*
+         * loading gpg keys will invoke LoadOpenGPGPublicKey() from rpm.cpp
+         * pgpReadPkts which makes nss to re-init and thus makes
+         * bugzilla plugin work :-/
+         */
+        //FIXME FIXME FIXME FIXME FIXME FIXME!!!
+        //if(g_settings_bOpenGPGCheck)
+        LoadGPGKeys();
+    }
 
-    //FIXME FIXME FIXME FIXME FIXME FIXME!!!
-    //if(g_settings_bOpenGPGCheck)
-    LoadGPGKeys();
+    return err;
 }
 
 /* dbus call to retrieve .conf file data from daemon */
diff -x '*.po' -d -urpN abrt.5/src/Daemon/Settings.h abrt.6/src/Daemon/Settings.h
--- abrt.5/src/Daemon/Settings.h	2010-06-22 18:19:11.000000000 +0200
+++ abrt.6/src/Daemon/Settings.h	2010-07-13 13:53:05.493822756 +0200
@@ -37,7 +37,7 @@ extern map_cron_t    g_settings_mapCron;
 extern vector_pair_string_string_t g_settings_vectorActionsAndReporters;
 extern map_analyzer_actions_and_reporters_t g_settings_mapAnalyzerActionsAndReporters;
 
-void LoadSettings();
+int LoadSettings();
 void SaveSettings();
 void SetSettings(const map_abrt_settings_t& pSettings, const char * dbus_sender);
 map_abrt_settings_t GetSettings();

[Karel Klic]

On 07/13/2010 03:21 PM, Denys Vlasenko wrote:
>  LoadSettings() is too liberal: it only warns about parse errors.
>  With this patch, obviously broken config files are detected
>  and abrtd fails to start. Example: I removed = sign from
>  "Database = SQLite3" line, and:
>
>  # abrtd -dvv
>  abrtd: Loading settings
>  abrtd: abrt.conf: Invalid syntax on line 23
>  abrtd: Error while initializing daemon
>  abrtd: Exiting
>  
I tested it and went through the code.
Looks good, works well -> please push it to the git.

[Denys Vlasenko]

On Tue, 2010-07-13 at 16:34 +0200, Karel Klic wrote:
>  On 07/13/2010 03:21 PM, Denys Vlasenko wrote:
>  > LoadSettings() is too liberal: it only warns about parse errors.
>  > With this patch, obviously broken config files are detected
>  > and abrtd fails to start. Example: I removed = sign from
>  > "Database = SQLite3" line, and:
>  >
>  > # abrtd -dvv
>  > abrtd: Loading settings
>  > abrtd: abrt.conf: Invalid syntax on line 23
>  > abrtd: Error while initializing daemon
>  > abrtd: Exiting
>  >
>  
>  I tested it and went through the code.
>  Looks good, works well -> please push it to the git. 
Done.
-- 
vda