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) <sbaigal@cisco.com>
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 ae7f2ea7920b3fe99b9b2d89cad368214ee47b14..d70ea9f6739698cbaf36c5d1fb5ee868e0487741 100644 (file)
--- 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 b38a0983c42a0bd47a2c11e1ad6a612b51d18226..501ff29b03e36d11f8f2d46a718e5f5400c093bf 100644 (file)
--- 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 09507d4c87bcfb45e8f24661185a84ded40ba5a7..9367c870f2cce5454eae4e4e152c09502f4cf16d 100644 (file)
--- 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 1b71a3306acb77851144afd228eb95c1c9a64d39..5d3164aeeb27aa7d7349d5a4bf931e8abe147a85 100644 (file)
--- 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<snort::SfIp, HostAttributesDescriptor, HostAttributesCacheKey> 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 cb852e5b2010058f219d0b4acafb68d25a618c66..e776fce4f4c7b021e13d1f74354ad8dc4eb24035 100644 (file)
--- 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*);