Merge pull request #1321 in SNORT/snort3 from appid_data_races to master

Squashed commit of the following:

commit de23fde0a250955859238a3d614d60604b014b94
Author: Devendra Dahiphale <ddahipha@cisco.com>
Date:   Tue Aug 14 09:43:36 2018 -0400

    appid: fix multithreading issues (data races) from app_forecast

diff --git a/src/network_inspectors/appid/app_forecast.cc b/src/network_inspectors/appid/app_forecast.cc
index 66a7590c9f15d3186bbd14491d14c2d167d279be..7fa4fbe8b3ba375c860a6a0786db6e1467b5b89f 100644 (file)
--- a/src/network_inspectors/appid/app_forecast.cc
+++ b/src/network_inspectors/appid/app_forecast.cc
@@ -25,56 +25,38 @@
 
 #include "app_forecast.h"
 
-#include "hash/xhash.h"
 #include "log/messages.h"
-#include "protocols/packet.h"
 #include "time/packet_time.h"
 #include "appid_session.h"
 
 using namespace snort;
 
-static AFActKey master_key;
-static XHash* AF_indicators = nullptr;     // list of "indicator apps"
-static XHash* AF_actives = nullptr;        // list of hosts to watch
+static std::unordered_map<AppId, AFElement> AF_indicators;     // list of "indicator apps"
+static THREAD_LOCAL std::map<AFActKey, AFActVal> *AF_actives;        // list of hosts to watch
 
-int init_appid_forecast()
+void appid_forecast_tinit()
 {
-    if (!(AF_indicators = xhash_new(1024, sizeof(AppId), sizeof(AFElement),
-            0, 0, nullptr, nullptr, 0)))
-    {
-        ErrorMessage("Config: failed to allocate memory for an AF Indicators hash.");
-        return 0;
-    }
-
-    if (!(AF_actives = xhash_new(1024, sizeof(AFActKey), sizeof(AFActVal),
-            (sizeof(XHashNode)*2048), 1, nullptr,  nullptr, 1)))
-    {
-        xhash_delete(AF_indicators);
-        ErrorMessage("Config: failed to allocate memory for an AF Actives hash.");
-        return 0;
-    }
-    else
-        return 1;
+    AF_actives = new std::map<AFActKey, AFActVal>;
 }
 
-void clean_appid_forecast()
+void appid_forecast_tterm()
 {
-    if (AF_indicators)
-    {
-        xhash_delete(AF_indicators);
-        AF_indicators = nullptr;
-    }
-
-    if (AF_actives)
+    if(nullptr != AF_actives)
     {
-        xhash_delete(AF_actives);
+        AF_actives->clear();
+        delete AF_actives;
         AF_actives = nullptr;
     }
 }
 
+void appid_forecast_pterm()
+{
+    AF_indicators.clear();
+}
+
 void add_af_indicator(AppId indicator, AppId forecast, AppId target)
 {
-    if (xhash_find(AF_indicators, &indicator))
+    if (AF_indicators.find(indicator) != AF_indicators.end())
     {
         ErrorMessage("LuaDetectorApi:Attempt to add more than one AFElement per appId %d",
             indicator);
@@ -82,66 +64,46 @@ void add_af_indicator(AppId indicator, AppId forecast, AppId target)
     }
 
     AFElement val;
-    val.indicator = indicator;
     val.forecast = forecast;
     val.target = target;
-    if (xhash_add(AF_indicators, &indicator, &val))
+    if (false == AF_indicators.insert({indicator, val}).second)
         ErrorMessage("LuaDetectorApi:Failed to add AFElement for appId %d", indicator);
 }
 
-static inline void rekey_master_AF_key(Packet* p, AppidSessionDirection dir, AppId forecast)
-{
-    const SfIp* src = dir ? p->ptrs.ip_api.get_dst() : p->ptrs.ip_api.get_src();
-
-    for (int i = 0; i < 4; i++)
-        master_key.ip[i] = src->get_ip6_ptr()[i];
-
-    master_key.forecast = forecast;
-}
-
 void check_session_for_AF_indicator(Packet* p, AppidSessionDirection dir, AppId indicator)
 {
-    AFElement* ind_element;
-    if (!(ind_element = (AFElement*)xhash_find(AF_indicators, &indicator)))
-        return;
-
-    rekey_master_AF_key(p, dir, ind_element->forecast);
+    auto af_indicator_entry = AF_indicators.find(indicator);
 
-    AFActVal* test_active_value;
-    if ((test_active_value = (AFActVal*)xhash_find(AF_actives, &master_key)))
-    {
-        test_active_value->last = snort::packet_time();
-        test_active_value->target = ind_element->target;
+    if (af_indicator_entry == AF_indicators.end())
         return;
-    }
+
+    AFElement ind_element = af_indicator_entry->second;
+    AFActKey master_key(p, dir, ind_element.forecast, master_key);
 
     AFActVal new_active_value;
-    new_active_value.target = ind_element->target;
+    new_active_value.target = ind_element.target;
     new_active_value.last = packet_time();
 
-    xhash_add(AF_actives, &master_key, &new_active_value);
+    (*AF_actives)[master_key] = new_active_value;
 }
 
 AppId check_session_for_AF_forecast(AppIdSession& asd, Packet* p, AppidSessionDirection dir, AppId forecast)
 {
-    AFActVal* check_act_val;
-
-    rekey_master_AF_key(p, dir, forecast);
+    AFActKey master_key(p, dir, forecast, master_key);
 
     //get out if there is no value
-    if (!(check_act_val = (AFActVal*)xhash_find(AF_actives, &master_key)))
+    auto check_act_val = AF_actives->find(master_key);
+    if (check_act_val == AF_actives->end())
         return APP_ID_UNKNOWN;
 
     //if the value is older than 5 minutes, remove it and get out
-    time_t age;
-    age = packet_time() - check_act_val->last;
+    time_t age = packet_time() - check_act_val->second.last;
     if (age < 0 || age > 300)
     {
-        xhash_remove(AF_actives, &master_key);
+        AF_actives->erase(master_key);
         return APP_ID_UNKNOWN;
     }
-
-    asd.payload.set_id(check_act_val->target);
+    asd.payload.set_id(check_act_val->second.target);
     return forecast;
 }
 

diff --git a/src/network_inspectors/appid/app_forecast.h b/src/network_inspectors/appid/app_forecast.h
index 98a70810e2e49c57b55f734f3ebb59da3eba0828..2232f59d4dcb5c7d990003c1291979544fb2679f 100644 (file)
--- a/src/network_inspectors/appid/app_forecast.h
+++ b/src/network_inspectors/appid/app_forecast.h
@@ -24,6 +24,7 @@
 
 #include <ctime>
 #include "flow/flow.h"
+#include "protocols/packet.h"
 #include "appid_types.h"
 #include "application_ids.h"
 
@@ -43,15 +44,30 @@ struct Packet;
 
 struct AFElement
 {
-    AppId indicator;
     AppId forecast;
     AppId target;
 };
 
-struct AFActKey
+class AFActKey
 {
-    uint32_t ip[4];
-    AppId forecast;
+    public:
+        AFActKey(snort::Packet* p, AppidSessionDirection dir, AppId forecast, AFActKey &master_key)
+        {
+            const snort::SfIp* src = dir ? p->ptrs.ip_api.get_dst() : p->ptrs.ip_api.get_src();
+
+            for (int i = 0; i < 4; i++)
+                master_key.ip[i] = src->get_ip6_ptr()[i];
+            master_key.forecast = forecast;
+        }
+
+        bool operator<(const AFActKey &key) const
+        {
+            return (forecast < key.forecast || ip[0] < key.ip[0] ||
+                   ip[1] < key.ip[1] || ip[2] < key.ip[2] || ip[3] < key.ip[3]);
+        }
+    private:
+        uint32_t ip[4];
+        AppId forecast;
 };
 
 struct AFActVal
@@ -60,8 +76,9 @@ struct AFActVal
     time_t last;
 };
 
-int init_appid_forecast();
-void clean_appid_forecast();
+void appid_forecast_tinit();
+void appid_forecast_tterm();
+void appid_forecast_pterm();
 void add_af_indicator(AppId, AppId, AppId);
 void check_session_for_AF_indicator(snort::Packet*, AppidSessionDirection, AppId);
 AppId check_session_for_AF_forecast(AppIdSession&, snort::Packet*, AppidSessionDirection, AppId);

diff --git a/src/network_inspectors/appid/appid_config.cc b/src/network_inspectors/appid/appid_config.cc
index 26db300493adc026eb669287ce7c55b1ceea0be9..63a58094b9ccda426620acf20110af4f5a7b88c9 100644 (file)
--- a/src/network_inspectors/appid/appid_config.cc
+++ b/src/network_inspectors/appid/appid_config.cc
@@ -759,7 +759,6 @@ bool AppIdConfig::init_appid(SnortConfig* sc, AppIdInspector *ins)
     {      
         AppIdConfig::app_info_mgr.init_appid_info_table(mod_config, sc);
         HostPortCache::initialize();
-        init_appid_forecast();
         HttpPatternMatchers* http_matchers = HttpPatternMatchers::get_instance();
         AppIdDiscovery::initialize_plugins(ins);
         init_length_app_cache();

diff --git a/src/network_inspectors/appid/appid_discovery.cc b/src/network_inspectors/appid/appid_discovery.cc
index a947873331d50d8549ff3d93346e8e625d3dbc93..51ef5b59440bc2555857c64f65e7625e05d87cce 100644 (file)
--- a/src/network_inspectors/appid/appid_discovery.cc
+++ b/src/network_inspectors/appid/appid_discovery.cc
@@ -1006,6 +1006,8 @@ void AppIdDiscovery::do_post_discovery(Packet* p, AppIdSession& asd,
         {
             asd.past_forecast = check_session_for_AF_forecast(asd, p, direction,
                 (AppId)service_id);
+            asd.set_application_ids(service_id, asd.pick_client_app_id(),
+                asd.pick_payload_app_id(), asd.pick_misc_app_id());
         }
     }
 }

diff --git a/src/network_inspectors/appid/appid_inspector.cc b/src/network_inspectors/appid/appid_inspector.cc
index 40313b5499b874f8bb26edcab54cd234a2a67bab..28c067bfcd43b468fad9fab3ac5cdbcfb255ef2d 100644 (file)
--- a/src/network_inspectors/appid/appid_inspector.cc
+++ b/src/network_inspectors/appid/appid_inspector.cc
@@ -33,7 +33,6 @@
 #include "managers/module_manager.h"
 #include "packet_tracer/packet_tracer.h"
 #include "profiler/profiler.h"
-#include "protocols/packet.h"
 
 #include "app_forecast.h"
 #include "appid_debug.h"
@@ -155,6 +154,7 @@ void AppIdInspector::tinit()
     appid_mute = PacketTracer::get_mute();
 
     AppIdStatistics::initialize_manager(*config);
+    appid_forecast_tinit();
     LuaDetectorManager::initialize(*active_config);
     AppIdServiceState::initialize();
     appidDebug = new AppIdDebug();
@@ -167,6 +167,7 @@ void AppIdInspector::tinit()
 
 void AppIdInspector::tterm()
 {
+    appid_forecast_tterm();
     AppIdStatistics::cleanup();
     LuaDetectorManager::terminate();
     AppIdDiscovery::tterm();
@@ -220,7 +221,7 @@ static void appid_inspector_pterm()
 {
 //FIXIT-M: RELOAD - if app_info_table is associated with an object
     HostPortCache::terminate();
-    clean_appid_forecast();
+    appid_forecast_pterm();
     free_length_app_cache();
     LuaDetectorManager::terminate();
     AppIdDiscovery::release_plugins();