Merge pull request #1505 in SNORT/snort3 from ~MDAGON/snort3:clean to master

Squashed commit of the following:

commit defc62939f9af82d37f3460815bb1d61a5c25dfa
Author: Maya Dagon <mdagon@cisco.com>
Date:   Tue Jan 29 15:21:48 2019 -0500

    reload: differentiate between restart required and bad config

diff --git a/doc/reload_limitations.txt b/doc/reload_limitations.txt
index 504b1edd88d0a39627a310520978c6de30d54aae..3061b2dd9541898959abae29a3c7539eb52ad0d9 100644 (file)
--- a/doc/reload_limitations.txt
+++ b/doc/reload_limitations.txt
@@ -46,4 +46,6 @@ In addition, the following scenarios require a restart:
  currently enabled
 * Adding/removing stream_* inspectors if stream was already configured
 
-In all of these cases reload will fail, and the original config will remain in use.
+In all of these cases reload will fail with the following message: "reload
+ failed - restart required". The original config will remain in use.
+

diff --git a/src/file_api/file_module.cc b/src/file_api/file_module.cc
index f35d481bf2736d02dcaf099e63bd0436db481c2e..e4fad47b2242416a389e076cc8a2db3e834db8f6 100644 (file)
--- a/src/file_api/file_module.cc
+++ b/src/file_api/file_module.cc
@@ -270,7 +270,7 @@ bool FileIdModule::set(const char*, Value& v, SnortConfig*)
         {
             if (Snort::is_reloading() && !FileService::is_file_capture_enabled())
             {
-                ParseError("Enabling file capture requires a restart\n");
+                ReloadError("Enabling file capture requires a restart\n");
                 return false;
             }
             fp.set_file_capture(true);
@@ -364,7 +364,7 @@ bool FileIdModule::set(const char*, Value& v, SnortConfig*)
         if (file_rule.use.capture_enabled && Snort::is_reloading()
             && !FileService::is_file_capture_enabled())
         {
-            ParseError("Enabling file capture requires a restart\n");
+            ReloadError("Enabling file capture requires a restart\n");
             return false;
         }
     }

diff --git a/src/file_api/file_service.cc b/src/file_api/file_service.cc
index ade3ac59f9cc149b04f6d5c031cf5f818ccbf4e1..b7b656ef0d31dbd54789e50afb79dfc1d8e281bf 100644 (file)
--- a/src/file_api/file_service.cc
+++ b/src/file_api/file_service.cc
@@ -87,14 +87,14 @@ void FileService::verify_reload(SnortConfig* sc)
         return;
 
     if (max_files_cached != conf->max_files_cached)
-        ParseError("Changing file_id:max_files_cached requires a restart\n");
+        ReloadError("Changing file_id:max_files_cached requires a restart\n");
 
     if (file_capture_enabled)
     {
         if (capture_memcap != conf->capture_memcap)
-            ParseError("Changing file_id:capture_memcap requires a restart\n");
+            ReloadError("Changing file_id:capture_memcap requires a restart\n");
         if (capture_block_size != conf->capture_block_size)
-            ParseError("Changing file_id:capture_block_size requires a restart\n");
+            ReloadError("Changing file_id:capture_block_size requires a restart\n");
     }
 }
 

diff --git a/src/log/messages.cc b/src/log/messages.cc
index 2897a443ae771a76ef583075f90361076f0cb8c9..237e0696f69634e8d0cf1ce9564bbed1e3d33b05 100644 (file)
--- a/src/log/messages.cc
+++ b/src/log/messages.cc
@@ -39,10 +39,12 @@ static int already_fatal = 0;
 
 static unsigned parse_errors = 0;
 static unsigned parse_warnings = 0;
+static unsigned reload_errors = 0;
 
 void reset_parse_errors()
 {
     parse_errors = 0;
+    reload_errors = 0;
 }
 
 unsigned get_parse_errors()
@@ -59,6 +61,11 @@ unsigned get_parse_warnings()
     return tmp;
 }
 
+unsigned get_reload_errors()
+{
+    return reload_errors;
+}
+
 static void log_message(FILE* file, const char* type, const char* msg)
 {
     const char* file_name;
@@ -119,6 +126,21 @@ void ParseError(const char* format, ...)
     parse_errors++;
 }
 
+void ReloadError(const char* format, ...)
+{
+    char buf[STD_BUF+1];
+    va_list ap;
+
+    va_start(ap, format);
+    vsnprintf(buf, STD_BUF, format, ap);
+    va_end(ap);
+
+    buf[STD_BUF] = '\0';
+    log_message(stderr, "ERROR", buf);
+
+    reload_errors++;
+}
+
 [[noreturn]] void ParseAbort(const char* format, ...)
 {
     char buf[STD_BUF+1];

diff --git a/src/log/messages.h b/src/log/messages.h
index c2c7ede60956cc3a7caa33ffe48720ded264ea3e..b0c3ccb540920f9c6f970fd3468e6b1944c5251f 100644 (file)
--- a/src/log/messages.h
+++ b/src/log/messages.h
@@ -48,12 +48,14 @@ enum WarningGroup
 void reset_parse_errors();
 unsigned get_parse_errors();
 unsigned get_parse_warnings();
+unsigned get_reload_errors();
 
 namespace snort
 {
 SO_PUBLIC void ParseMessage(const char*, ...) __attribute__((format (printf, 1, 2)));
 SO_PUBLIC void ParseWarning(WarningGroup, const char*, ...) __attribute__((format (printf, 2, 3)));
 SO_PUBLIC void ParseError(const char*, ...) __attribute__((format (printf, 1, 2)));
+SO_PUBLIC void ReloadError(const char*, ...) __attribute__((format (printf, 1, 2)));
 [[noreturn]] SO_PUBLIC void ParseAbort(const char*, ...) __attribute__((format (printf, 1, 2)));
 
 SO_PUBLIC void LogMessage(const char*, ...) __attribute__((format (printf, 1, 2)));

diff --git a/src/main.cc b/src/main.cc
index 22cfc64ca545a15758273d2f2ecd92b84e4e2433..2999d9bc231649d5a4ac26a84d0e530b17793819 100644 (file)
--- a/src/main.cc
+++ b/src/main.cc
@@ -325,7 +325,10 @@ int main_reload_config(lua_State* L)
 
     if ( !sc )
     {
-        current_request->respond("== reload failed\n");
+        if (get_reload_errors())
+         current_request->respond("== reload failed - restart required\n");
+        else
+          current_request->respond("== reload failed - bad config\n");
         return 0;
     }
 
@@ -334,7 +337,7 @@ int main_reload_config(lua_State* L)
 
     if ( !tc )
     {
-        current_request->respond("== reload failed\n");
+        current_request->respond("== reload failed - bad config\n");
         return 0;
     }
     SnortConfig::set_conf(sc);

diff --git a/src/main/snort.cc b/src/main/snort.cc
index 5bfe48718ea76b8a3dc9e97965ecbcce49830c7b..cfd236ee5da84bc1f10999ee78526fd864471be6 100644 (file)
--- a/src/main/snort.cc
+++ b/src/main/snort.cc
@@ -560,6 +560,13 @@ void Snort::cleanup()
     clean_exit(0);
 }
 
+void Snort::reload_failure_cleanup(SnortConfig* sc)
+{
+    parser_term(sc);
+    delete sc;
+    reloading = false;
+}
+
 // FIXIT-M refactor this so startup and reload call the same core function to
 // instantiate things that can be reloaded
 SnortConfig* Snort::get_reload_config(const char* fname)
@@ -575,9 +582,7 @@ SnortConfig* Snort::get_reload_config(const char* fname)
 
     if ( get_parse_errors() || ModuleManager::get_errors() || !sc->verify() )
     {
-        parser_term(sc);
-        delete sc;
-        reloading = false;
+        reload_failure_cleanup(sc);
         return nullptr;
     }
 
@@ -587,13 +592,16 @@ SnortConfig* Snort::get_reload_config(const char* fname)
     ControlMgmt::reconfigure_controls();
 #endif
 
-    FileService::verify_reload(sc);
-
     if ( get_parse_errors() or !InspectorManager::configure(sc) )
     {
-        parser_term(sc);
-        delete sc;
-        reloading = false;
+        reload_failure_cleanup(sc);
+        return nullptr;
+    }
+
+    FileService::verify_reload(sc);
+    if ( get_reload_errors() )
+    {
+        reload_failure_cleanup(sc);
         return nullptr;
     }
 

diff --git a/src/main/snort.h b/src/main/snort.h
index be543679d00bb21a6bf002ce40031c290d250518..f5bd119f95e3bc58c81ec1e7646a43d567993b5e 100644 (file)
--- a/src/main/snort.h
+++ b/src/main/snort.h
@@ -91,6 +91,7 @@ private:
     static void init(int, char**);
     static void term();
     static void clean_exit(int);
+    static void reload_failure_cleanup(SnortConfig*);
 
 private:
     static bool initializing;

diff --git a/src/main/snort_config.cc b/src/main/snort_config.cc
index 74065aa1591d6357838d0677f0434ae1bb5df300..14891b7e4675ffc522aec91d19ab06d5dcb17757 100644 (file)
--- a/src/main/snort_config.cc
+++ b/src/main/snort_config.cc
@@ -496,7 +496,7 @@ void SnortConfig::merge(SnortConfig* cmd_line)
     state = new std::vector<void*>[num_slots];
 }
 
-// FIXIT-L this is a work around till snort supports adding/removing 
+// FIXIT-L this is a work around till snort supports adding/removing
 // stream cache during reload
 bool SnortConfig::verify_stream_inspectors()
 {
@@ -529,7 +529,7 @@ bool SnortConfig::verify_stream_inspectors()
                 const bool in_new = InspectorManager::inspector_exists_in_any_policy(name, this);
                 if (orig_inspectors[name] != in_new)
                 {
-                    ErrorMessage("Snort Reload: Adding/removing %s requires a restart.\n", name);
+                    ReloadError("Snort Reload: Adding/removing %s requires a restart.\n", name);
                     return false;
                 }
             }
@@ -543,14 +543,14 @@ bool SnortConfig::verify()
 {
     if (get_conf()->asn1_mem != asn1_mem)
     {
-        ErrorMessage("Snort Reload: Changing the asn1 memory configuration "
+        ReloadError("Snort Reload: Changing the asn1 memory configuration "
             "requires a restart.\n");
         return false;
     }
 
     if ( bpf_filter != get_conf()->bpf_filter )
     {
-        ErrorMessage("Snort Reload: Changing the bpf filter configuration "
+        ReloadError("Snort Reload: Changing the bpf filter configuration "
             "requires a restart.\n");
         return false;
     }
@@ -558,14 +558,13 @@ bool SnortConfig::verify()
     if ( respond_attempts != get_conf()->respond_attempts ||
         respond_device != get_conf()->respond_device )
     {
-        ErrorMessage("Snort Reload: Changing config response "
-            "requires a restart.\n");
+        ReloadError("Snort Reload: Changing config response requires a restart.\n");
         return false;
     }
 
     if (get_conf()->chroot_dir != chroot_dir)
     {
-        ErrorMessage("Snort Reload: Changing the chroot directory "
+        ReloadError("Snort Reload: Changing the chroot directory "
             "configuration requires a restart.\n");
         return false;
     }
@@ -573,7 +572,7 @@ bool SnortConfig::verify()
     if ((get_conf()->run_flags & RUN_FLAG__DAEMON) !=
         (run_flags & RUN_FLAG__DAEMON))
     {
-        ErrorMessage("Snort Reload: Changing to or from daemon mode "
+        ReloadError("Snort Reload: Changing to or from daemon mode "
             "requires a restart.\n");
         return false;
     }
@@ -581,20 +580,20 @@ bool SnortConfig::verify()
     /* Orig log dir because a chroot might have changed it */
     if (get_conf()->orig_log_dir != orig_log_dir)
     {
-        ErrorMessage("Snort Reload: Changing the log directory "
+        ReloadError("Snort Reload: Changing the log directory "
             "configuration requires a restart.\n");
         return false;
     }
 
     if (get_conf()->max_attribute_hosts != max_attribute_hosts)
     {
-        ErrorMessage("Snort Reload: Changing max_attribute_hosts "
+        ReloadError("Snort Reload: Changing max_attribute_hosts "
             "configuration requires a restart.\n");
         return false;
     }
     if (get_conf()->max_attribute_services_per_host != max_attribute_services_per_host)
     {
-        ErrorMessage("Snort Reload: Changing max_attribute_services_per_host "
+        ReloadError("Snort Reload: Changing max_attribute_services_per_host "
             "configuration requires a restart.\n");
         return false;
     }
@@ -602,28 +601,26 @@ bool SnortConfig::verify()
     if ((get_conf()->run_flags & RUN_FLAG__NO_PROMISCUOUS) !=
         (run_flags & RUN_FLAG__NO_PROMISCUOUS))
     {
-        ErrorMessage("Snort Reload: Changing to or from promiscuous mode "
+        ReloadError("Snort Reload: Changing to or from promiscuous mode "
             "requires a restart.\n");
         return false;
     }
 
     if (get_conf()->group_id != group_id)
     {
-        ErrorMessage("Snort Reload: Changing the group id "
-            "configuration requires a restart.\n");
+        ReloadError("Snort Reload: Changing the group id configuration requires a restart.\n");
         return false;
     }
 
     if (get_conf()->user_id != user_id)
     {
-        ErrorMessage("Snort Reload: Changing the user id "
-            "configuration requires a restart.\n");
+        ReloadError("Snort Reload: Changing the user id configuration requires a restart.\n");
         return false;
     }
 
     if (get_conf()->daq_config->mru_size != daq_config->mru_size)
     {
-        ErrorMessage("Snort Reload: Changing the packet snaplen "
+        ReloadError("Snort Reload: Changing the packet snaplen "
             "configuration requires a restart.\n");
         return false;
     }
@@ -631,7 +628,7 @@ bool SnortConfig::verify()
     if (get_conf()->threshold_config->memcap !=
         threshold_config->memcap)
     {
-        ErrorMessage("Snort Reload: Changing the threshold memcap "
+        ReloadError("Snort Reload: Changing the threshold memcap "
             "configuration requires a restart.\n");
         return false;
     }
@@ -639,7 +636,7 @@ bool SnortConfig::verify()
     if (get_conf()->rate_filter_config->memcap !=
         rate_filter_config->memcap)
     {
-        ErrorMessage("Snort Reload: Changing the rate filter memcap "
+        ReloadError("Snort Reload: Changing the rate filter memcap "
             "configuration requires a restart.\n");
         return false;
     }
@@ -647,7 +644,7 @@ bool SnortConfig::verify()
     if (get_conf()->detection_filter_config->memcap !=
         detection_filter_config->memcap)
     {
-        ErrorMessage("Snort Reload: Changing the detection filter memcap "
+        ReloadError("Snort Reload: Changing the detection filter memcap "
             "configuration requires a restart.\n");
         return false;
     }

diff --git a/src/stream/base/stream_module.cc b/src/stream/base/stream_module.cc
index fa2806e41f5ff93e63dcd8dd5271059b38c2fb02..f015409374204c7a3eb2b2f43d99876ff5a9f747 100644 (file)
--- a/src/stream/base/stream_module.cc
+++ b/src/stream/base/stream_module.cc
@@ -183,7 +183,7 @@ static int check_cache_change(const char* fqn, const char* name, const FlowConfi
             or saved_cfg.pruning_timeout != new_cfg.pruning_timeout
             or saved_cfg.nominal_timeout != new_cfg.nominal_timeout )
         {
-            ParseError("Changing of %s requires a restart\n", name);
+            ReloadError("Changing of %s requires a restart\n", name);
             ret = 1;
         }
     }
@@ -209,7 +209,7 @@ bool StreamModule::end(const char* fqn, int, SnortConfig*)
         if ( saved_config.ip_cfg.max_sessions   // saved config is valid
             and config.footprint != saved_config.footprint )
         {
-            ParseError("Changing of stream.footprint requires a restart\n");
+            ReloadError("Changing of stream.footprint requires a restart\n");
             issue_found++;
         }
         if ( issue_found == 0 )