From: Steve Chew (stechew) Date: Tue, 17 Nov 2020 03:04:45 +0000 (+0000) Subject: Merge pull request #2615 in SNORT/snort3 from ~SBAIGAL/snort3:host_attr_fix to master X-Git-Tag: 3.0.3-6~49 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=5ba3210fd92b6a0fdb119726a7175061dbea7d7c;p=thirdparty%2Fsnort3.git Merge pull request #2615 in SNORT/snort3 from ~SBAIGAL/snort3:host_attr_fix to master Squashed commit of the following: commit e4720b210f3c993e9bf55c1680bfe910c762b810 Author: Steven Baigal (sbaigal) Date: Tue Nov 10 19:09:45 2020 -0500 host_attributes: better error handling for reload to eliminate double free and memory leaks --- diff --git a/src/main.cc b/src/main.cc index ae7f2ea79..d70ea9f67 100644 --- a/src/main.cc +++ b/src/main.cc @@ -369,15 +369,10 @@ int main_reload_config(lua_State* L) else current_request->respond("== reload failed - bad config\n"); + HostAttributesManager::load_failure_cleanup(); return 0; } - if ( !sc->attribute_hosts_file.empty() ) - { - if ( !HostAttributesManager::load_hosts_file(sc, sc->attribute_hosts_file.c_str()) ) - current_request->respond("== reload failed - host attributes file failed to load\n"); - } - int32_t num_hosts = HostAttributesManager::get_num_host_entries(); if ( num_hosts >= 0 ) LogMessage( "host attribute table: %d hosts loaded\n", num_hosts); diff --git a/src/main/modules.cc b/src/main/modules.cc index b38a0983c..501ff29b0 100644 --- a/src/main/modules.cc +++ b/src/main/modules.cc @@ -1835,24 +1835,6 @@ bool RateFilterModule::end(const char*, int idx, SnortConfig* sc) // hosts module //------------------------------------------------------------------------- -class HostAttributesReloadTuner : public snort::ReloadResourceTuner -{ -public: - HostAttributesReloadTuner() = default; - - bool tinit() override - { - HostAttributesManager::initialize(); - return true; - } - - bool tune_packet_context() override - { return true; } - - bool tune_idle_context() override - { return true; } -}; - static const Parameter service_params[] = { { "name", Parameter::PT_STRING, nullptr, nullptr, @@ -1968,14 +1950,6 @@ bool HostsModule::end(const char* fqn, int idx, SnortConfig* sc) host.reset(); host = nullptr; } - else if ( !idx && !strcmp(fqn, "hosts")) - { - if ( HostAttributesManager::activate() ) - { - if ( Snort::is_reloading() ) - sc->register_reload_resource_tuner(new HostAttributesReloadTuner); - } - } return true; } diff --git a/src/main/snort.cc b/src/main/snort.cc index 09507d4c8..9367c870f 100644 --- a/src/main/snort.cc +++ b/src/main/snort.cc @@ -189,6 +189,7 @@ void Snort::init(int argc, char** argv) if ( !HostAttributesManager::load_hosts_file(sc, sc->attribute_hosts_file.c_str()) ) ParseError("host attributes file failed to load\n"); } + HostAttributesManager::activate(sc); // Must be after CodecManager::instantiate() if ( !InspectorManager::configure(sc) ) @@ -520,6 +521,14 @@ SnortConfig* Snort::get_reload_config(const char* fname, const char* plugin_path } InspectorManager::update_policy(sc); + + if ( !sc->attribute_hosts_file.empty() ) + { + if ( !HostAttributesManager::load_hosts_file(sc, sc->attribute_hosts_file.c_str()) ) + LogMessage("== WARNING: host attributes file failed to load\n"); + } + HostAttributesManager::activate(sc); + reloading = false; parser_term(sc); diff --git a/src/target_based/host_attributes.cc b/src/target_based/host_attributes.cc index 1b71a3306..5d3164aee 100644 --- a/src/target_based/host_attributes.cc +++ b/src/target_based/host_attributes.cc @@ -27,6 +27,7 @@ #include "hash/lru_cache_shared.h" #include "main/shell.h" +#include "main/snort.h" #include "main/snort_config.h" #include "main/thread.h" @@ -53,6 +54,24 @@ public: typedef HostLruSharedCache HostAttributesSharedCache; +class HostAttributesReloadTuner : public snort::ReloadResourceTuner +{ +public: + HostAttributesReloadTuner() = default; + + bool tinit() override + { + HostAttributesManager::initialize(); + return true; + } + + bool tune_packet_context() override + { return true; } + + bool tune_idle_context() override + { return true; } +}; + static THREAD_LOCAL HostAttributesSharedCache* active_cache = nullptr; static HostAttributesSharedCache* swap_cache = nullptr; static HostAttributesSharedCache* next_cache = nullptr; @@ -120,11 +139,13 @@ bool HostAttributesManager::load_hosts_file(snort::SnortConfig* sc, const char* Shell sh(fname); if ( sh.configure(sc, false, true) ) + { + activate(sc); return true; + } // loading of host file failed... - delete next_cache; - next_cache = nullptr; + load_failure_cleanup(); return false; } @@ -136,21 +157,33 @@ bool HostAttributesManager::add_host(HostAttributesEntry host, snort::SnortConfi return next_cache->find_else_insert(host->get_ip_addr(), host, true); } -bool HostAttributesManager::activate() +void HostAttributesManager::activate(SnortConfig* sc) { + if ( next_cache == nullptr ) + return; old_cache = active_cache; active_cache = next_cache; swap_cache = next_cache; next_cache = nullptr; - return ( active_cache != old_cache ) ? true : false; + if( active_cache != old_cache and Snort::is_reloading() ) + sc->register_reload_resource_tuner(new HostAttributesReloadTuner); } void HostAttributesManager::initialize() { active_cache = swap_cache; } +void HostAttributesManager::load_failure_cleanup() +{ + delete next_cache; + next_cache = nullptr; +} + void HostAttributesManager::swap_cleanup() -{ delete old_cache; } +{ + delete old_cache; + old_cache = nullptr; +} void HostAttributesManager::term() { delete active_cache; } diff --git a/src/target_based/host_attributes.h b/src/target_based/host_attributes.h index cb852e5b2..e776fce4f 100644 --- a/src/target_based/host_attributes.h +++ b/src/target_based/host_attributes.h @@ -143,9 +143,10 @@ class HostAttributesManager { public: static bool load_hosts_file(snort::SnortConfig*, const char* fname); - static bool activate(); + static void activate(snort::SnortConfig*); static void initialize(); static void swap_cleanup(); + static void load_failure_cleanup(); static void term(); static bool add_host(HostAttributesEntry, snort::SnortConfig*);