From: Mike Stepanek (mstepane) Date: Tue, 14 Aug 2018 15:38:19 +0000 (-0400) Subject: Merge pull request #1321 in SNORT/snort3 from appid_data_races to master X-Git-Tag: 3.0.0-246~6 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=54388e99875c029e2bc11406847068c107f3514e;p=thirdparty%2Fsnort3.git Merge pull request #1321 in SNORT/snort3 from appid_data_races to master Squashed commit of the following: commit de23fde0a250955859238a3d614d60604b014b94 Author: Devendra Dahiphale 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 66a7590c9..7fa4fbe8b 100644 --- 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 AF_indicators; // list of "indicator apps" +static THREAD_LOCAL std::map *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; } -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 98a70810e..2232f59d4 100644 --- a/src/network_inspectors/appid/app_forecast.h +++ b/src/network_inspectors/appid/app_forecast.h @@ -24,6 +24,7 @@ #include #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 26db30049..63a58094b 100644 --- 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 a94787333..51ef5b594 100644 --- 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 40313b549..28c067bfc 100644 --- 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();