From: Tom Peters (thopeter) Date: Wed, 6 Feb 2019 21:41:38 +0000 (-0500) Subject: Merge pull request #1505 in SNORT/snort3 from ~MDAGON/snort3:clean to master X-Git-Tag: 3.0.0-251~55 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=caa22f45087e9824bcbbcca212e1c9904abba6f4;p=thirdparty%2Fsnort3.git Merge pull request #1505 in SNORT/snort3 from ~MDAGON/snort3:clean to master Squashed commit of the following: commit defc62939f9af82d37f3460815bb1d61a5c25dfa Author: Maya Dagon 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 504b1edd8..3061b2dd9 100644 --- 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 f35d481bf..e4fad47b2 100644 --- 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 ade3ac59f..b7b656ef0 100644 --- 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 2897a443a..237e0696f 100644 --- 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 c2c7ede60..b0c3ccb54 100644 --- 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 22cfc64ca..2999d9bc2 100644 --- 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 5bfe48718..cfd236ee5 100644 --- 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 be543679d..f5bd119f9 100644 --- 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 74065aa15..14891b7e4 100644 --- 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[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 fa2806e41..f01540937 100644 --- 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 )