From: Russ Combs (rucombs) Date: Wed, 23 Nov 2016 21:24:58 +0000 (-0500) Subject: Merge pull request #722 in SNORT/snort3 from appid_stl_thread_safety to master X-Git-Tag: 3.0.0-233~173 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=20bf27cf1d5492fe2183a909bd35db2428cdf7fc;p=thirdparty%2Fsnort3.git Merge pull request #722 in SNORT/snort3 from appid_stl_thread_safety to master Squashed commit of the following: commit 991eb29ae7f85b1e9e1b72f334eb96536c568b10 Author: davis mcpherson Date: Wed Nov 23 14:09:00 2016 -0500 use std::lock_guard to manage life cycle of mutex ownership when accessing app info tables. commit a09a573489bf2b69930b6aa58006699fd3ab0681 Author: davis mcpherson Date: Wed Nov 23 10:33:49 2016 -0500 add lock around read accesses to app info tables. there is a single lock that is global to all app info tables, more granular locking on a per table basis may be implemented in the future to improve performance remove caching AppIdServiceIDState object pointers in the AppIdSession object. The service state object may get deleted without the knowledge of appid sessions that have cached the pointer. For now a get using the ip/port/protocol tuple of the destination is used to get the service state object. This is short term solution until a move to the host cache can be implemented. --- diff --git a/src/network_inspectors/appid/app_info_table.cc b/src/network_inspectors/appid/app_info_table.cc index beeb41a18..1e6a65c77 100644 --- a/src/network_inspectors/appid/app_info_table.cc +++ b/src/network_inspectors/appid/app_info_table.cc @@ -102,23 +102,23 @@ AppInfoTableEntry* AppInfoManager::get_app_info_entry(AppId appId, const AppInfo { AppId tmp; AppInfoTable::const_iterator app; + AppInfoTableEntry* entry = nullptr; + std::lock_guard lock(app_info_tables_rw_mutex); if ((tmp = get_static_app_info_entry(appId))) { app = lookup_table.find(tmp); if( app != lookup_table.end() ) - return app->second; - else - return nullptr; + entry = app->second; } else { app = custom_app_info_table.find(appId); if( app != custom_app_info_table.end() ) - return app->second; - else - return nullptr; + entry = app->second; } + + return entry; } AppInfoTableEntry* AppInfoManager::get_app_info_entry(AppId appId) @@ -134,7 +134,7 @@ AppInfoTableEntry* AppInfoManager::add_dynamic_app_entry(const char* app_name) return nullptr; } - custom_app_mutex.lock(); + std::lock_guard lock(app_info_tables_rw_mutex); AppInfoTableEntry* entry = find_app_info_by_name(app_name); if (!entry) { @@ -147,7 +147,6 @@ AppInfoTableEntry* AppInfoManager::add_dynamic_app_entry(const char* app_name) add_entry_to_app_info_hash(entry->app_name_key, entry); } - custom_app_mutex.unlock(); return entry; } diff --git a/src/network_inspectors/appid/app_info_table.h b/src/network_inspectors/appid/app_info_table.h index de1bba309..9e9210c02 100644 --- a/src/network_inspectors/appid/app_info_table.h +++ b/src/network_inspectors/appid/app_info_table.h @@ -154,7 +154,7 @@ private: AppInfoManager() {} void load_appid_config(const char* path); AppInfoTableEntry* get_app_info_entry(AppId appId, const AppInfoTable& lookup_table); - std::mutex custom_app_mutex; + std::mutex app_info_tables_rw_mutex; }; #endif diff --git a/src/network_inspectors/appid/appid_session.cc b/src/network_inspectors/appid/appid_session.cc index df1389daa..1d48f9e9e 100644 --- a/src/network_inspectors/appid/appid_session.cc +++ b/src/network_inspectors/appid/appid_session.cc @@ -1574,6 +1574,8 @@ void AppIdSession::do_application_discovery(Packet* p) LogMessage("AppIdDbg %s new session\n", asd->session_logging_id); } + // FIXIT-L - from this point on we always have a valid ptr to an AppIdSession and a Packet + // refactor to pass these as refs and delete any checks for null appid_stats.processed_packets++; asd->session_packet_count++; @@ -1620,7 +1622,7 @@ void AppIdSession::do_application_discovery(Packet* p) } AppIdServiceIDState* id_state = AppIdServiceState::get(ip, IpProtocol::TCP, port, - AppIdServiceDetectionLevel(asd)); + get_service_detect_level(asd)); if (id_state) { @@ -1629,7 +1631,7 @@ void AppIdSession::do_application_discovery(Packet* p) else if ((packet_time() - id_state->reset_time) >= 60) { AppIdServiceState::remove(ip, IpProtocol::TCP, port, - AppIdServiceDetectionLevel(asd)); + get_service_detect_level(asd)); asd->set_session_flags(APPID_SESSION_SERVICE_DELETED); } } diff --git a/src/network_inspectors/appid/appid_session.h b/src/network_inspectors/appid/appid_session.h index d62fb9f7b..7819728f3 100644 --- a/src/network_inspectors/appid/appid_session.h +++ b/src/network_inspectors/appid/appid_session.h @@ -249,7 +249,6 @@ public: char* serviceVendor = nullptr; char* serviceVersion = nullptr; RNAServiceSubtype* subtype = nullptr; - AppIdServiceIDState* id_state = nullptr; char* netbios_name = nullptr; SF_LIST* candidate_service_list = nullptr; unsigned int num_candidate_services_tried = 0; diff --git a/src/network_inspectors/appid/service_plugins/service_base.cc b/src/network_inspectors/appid/service_plugins/service_base.cc index 30bce6970..964737446 100644 --- a/src/network_inspectors/appid/service_plugins/service_base.cc +++ b/src/network_inspectors/appid/service_plugins/service_base.cc @@ -354,7 +354,7 @@ static inline RNAServiceElement* AppIdGetNexServiceByPort( IpProtocol protocol, RNAServiceElement* service = nullptr; SF_LIST* list = nullptr; - if (AppIdServiceDetectionLevel(asd) == 1) + if (get_service_detect_level(asd) == 1) { unsigned remappedPort = sslPortRemap(port); if (remappedPort) @@ -1102,7 +1102,7 @@ static int AppIdServiceAddServiceEx(AppIdSession* asd, const Packet* pkt, int di uint16_t port = 0; const sfip_t* ip = nullptr; - if (!asd || !pkt || !svc_element) + if ( !pkt || !svc_element ) { ErrorMessage("Invalid arguments to absinthe_add_appId"); return SERVICE_EINVALID; @@ -1159,21 +1159,11 @@ static int AppIdServiceAddServiceEx(AppIdSession* asd, const Packet* pkt, int di // If UDP reversed, ensure we have the correct host tracker entry. if (asd->get_session_flags(APPID_SESSION_UDP_REVERSED)) - { - asd->id_state = AppIdServiceState::get(ip, asd->protocol, port, - AppIdServiceDetectionLevel(asd)); - } + id_state = AppIdServiceState::get(ip, asd->protocol, port, get_service_detect_level(asd)); - if (!(id_state = asd->id_state)) + if ( !id_state ) { - if (!(id_state = AppIdServiceState::add(ip, asd->protocol, port, - AppIdServiceDetectionLevel(asd)))) - { - ErrorMessage("Add service failed to create state"); - return SERVICE_ENOMEM; - } - - asd->id_state = id_state; + id_state = AppIdServiceState::add(ip, asd->protocol, port, get_service_detect_level(asd)); asd->service_ip = *ip; asd->service_port = port; } @@ -1299,23 +1289,16 @@ int AppIdServiceInProcess(AppIdSession* asd, const Packet* pkt, int dir, asd->get_session_flags(APPID_SESSION_IGNORE_HOST | APPID_SESSION_UDP_REVERSED)) return SERVICE_SUCCESS; - if (!(id_state = asd->id_state)) + const sfip_t* ip = pkt->ptrs.ip_api.get_src(); + uint16_t port = asd->service_port ? asd->service_port : pkt->ptrs.sp; + id_state = AppIdServiceState::get(ip, asd->protocol, port, get_service_detect_level(asd)); + if ( !id_state ) { - const sfip_t* ip = pkt->ptrs.ip_api.get_src(); - uint16_t port = asd->service_port ? asd->service_port : pkt->ptrs.sp; - - if (!(id_state = AppIdServiceState::add(ip, asd->protocol, port, - AppIdServiceDetectionLevel(asd)))) - { - ErrorMessage("In-process service failed to create state"); - return SERVICE_ENOMEM; - } - - asd->id_state = id_state; - asd->service_ip = *ip; - asd->service_port = port; + id_state = AppIdServiceState::add(ip, asd->protocol, port, get_service_detect_level(asd)); id_state->state = SERVICE_ID_NEW; id_state->svc = svc_element; + asd->service_ip = *ip; + asd->service_port = port; } else { @@ -1350,8 +1333,6 @@ int AppIdServiceInProcess(AppIdSession* asd, const Packet* pkt, int dir, int AppIdServiceIncompatibleData(AppIdSession* asd, const Packet* pkt, int dir, const RNAServiceElement* svc_element, unsigned flow_data_index, const AppIdConfig*) { - AppIdServiceIDState* id_state; - if (!asd || !pkt) { ErrorMessage("Invalid arguments to service_incompatible_data"); @@ -1363,15 +1344,18 @@ int AppIdServiceIncompatibleData(AppIdSession* asd, const Packet* pkt, int dir, /* If we're still working on a port/pattern list of detectors, then ignore * individual fails until we're done looking at everything. */ + const sfip_t* ip = pkt->ptrs.ip_api.get_src(); + uint16_t port = asd->service_port ? asd->service_port : pkt->ptrs.sp; + AppIdServiceIDState* id_state = AppIdServiceState::get(ip, asd->protocol, port, get_service_detect_level(asd)); if ( (asd->serviceData == nullptr) && (asd->candidate_service_list != nullptr) - && (asd->id_state != nullptr) ) + && (id_state != nullptr) ) { if (sflist_count(asd->candidate_service_list) != 0) { return SERVICE_SUCCESS; } else if ((asd->num_candidate_services_tried >= MAX_CANDIDATE_SERVICES) - || (asd->id_state->state == SERVICE_ID_BRUTE_FORCE) ) + || (id_state->state == SERVICE_ID_BRUTE_FORCE) ) { return SERVICE_SUCCESS; } @@ -1392,22 +1376,11 @@ int AppIdServiceIncompatibleData(AppIdSession* asd, const Packet* pkt, int dir, return SERVICE_SUCCESS; } - if (!(id_state = asd->id_state)) + if ( !id_state ) { - const sfip_t* ip = pkt->ptrs.ip_api.get_src(); - uint16_t port = asd->service_port ? asd->service_port : pkt->ptrs.sp; - - if (!(id_state = AppIdServiceState::add(ip, asd->protocol, port, - AppIdServiceDetectionLevel(asd)))) - { - ErrorMessage("Incompatible service failed to create state"); - return SERVICE_ENOMEM; - } - - //id_state->service_list = nullptr; + id_state = AppIdServiceState::add(ip, asd->protocol, port, get_service_detect_level(asd)); id_state->state = SERVICE_ID_NEW; id_state->svc = svc_element; - asd->id_state = id_state; asd->service_ip = *ip; asd->service_port = port; } @@ -1447,21 +1420,22 @@ int AppIdServiceIncompatibleData(AppIdSession* asd, const Packet* pkt, int dir, int AppIdServiceFailService(AppIdSession* asd, const Packet* pkt, int dir, const RNAServiceElement* svc_element, unsigned flow_data_index) { - AppIdServiceIDState* id_state; + const sfip_t* ip = pkt->ptrs.ip_api.get_src(); + uint16_t port = asd->service_port ? asd->service_port : pkt->ptrs.sp; + AppIdServiceIDState* id_state = AppIdServiceState::get(ip, asd->protocol, port, get_service_detect_level(asd)); if (flow_data_index != APPID_SESSION_DATA_NONE) asd->free_flow_data_by_id(flow_data_index); /* If we're still working on a port/pattern list of detectors, then ignore * individual fails until we're done looking at everything. */ - if ( (asd->serviceData == nullptr) - && (asd->candidate_service_list != nullptr) - && (asd->id_state != nullptr) ) + if ( (asd->serviceData == nullptr) && (asd->candidate_service_list != nullptr) + && (id_state != nullptr) ) { if (sflist_count(asd->candidate_service_list) != 0) return SERVICE_SUCCESS; else if ( (asd->num_candidate_services_tried >= MAX_CANDIDATE_SERVICES) - || (asd->id_state->state == SERVICE_ID_BRUTE_FORCE) ) + || (id_state->state == SERVICE_ID_BRUTE_FORCE) ) return SERVICE_SUCCESS; } @@ -1486,22 +1460,11 @@ int AppIdServiceFailService(AppIdSession* asd, const Packet* pkt, int dir, return SERVICE_SUCCESS; } - if (!(id_state = asd->id_state)) + if ( !id_state ) { - const sfip_t* ip = pkt->ptrs.ip_api.get_src(); - uint16_t port = asd->service_port ? asd->service_port : pkt->ptrs.sp; - - if (!(id_state = AppIdServiceState::add(ip, asd->protocol, port, - AppIdServiceDetectionLevel(asd)))) - { - ErrorMessage("Fail service failed to create state"); - return SERVICE_ENOMEM; - } - - //id_state->service_list = nullptr; + id_state = AppIdServiceState::add(ip, asd->protocol, port, get_service_detect_level(asd)); id_state->state = SERVICE_ID_NEW; id_state->svc = svc_element; - asd->id_state = id_state; asd->service_ip = *ip; asd->service_port = port; } @@ -1629,8 +1592,8 @@ void FailInProcessService(AppIdSession* asd, const AppIdConfig*) if (asd->get_session_flags(APPID_SESSION_SERVICE_DETECTED | APPID_SESSION_UDP_REVERSED)) return; - AppIdServiceIDState* id_state = AppIdServiceState::get(&asd->service_ip, - asd->protocol, asd->service_port, AppIdServiceDetectionLevel(asd)); + AppIdServiceIDState* id_state = AppIdServiceState::get(&asd->service_ip, asd->protocol, + asd->service_port, get_service_detect_level(asd)); APPID_LOG_FILTER_SERVICE_PORT(asd->service_port, "FailInProcess %" PRIx64 ", %08X:%u proto %u\n", asd->common.flags, @@ -1720,7 +1683,7 @@ static const RNAServiceElement* get_next_service(const Packet* p, const int dir, const sfip_t* reverse_ip = p->ptrs.ip_api.get_src(); asd->tried_reverse_service = true; if ((reverse_id_state = AppIdServiceState::get(reverse_ip, proto, p->ptrs.sp, - AppIdServiceDetectionLevel(asd)))) + get_service_detect_level(asd)))) { reverse_service = reverse_id_state->svc; } @@ -1793,26 +1756,10 @@ int AppIdDiscoverService(Packet* p, const int dir, AppIdSession* asd) } /* Get host tracker state. */ - AppIdServiceIDState* id_state = asd->id_state; + AppIdServiceIDState* id_state = AppIdServiceState::get(ip, proto, port, + get_service_detect_level(asd)); if (id_state == nullptr) - { - id_state = AppIdServiceState::get(ip, proto, port, AppIdServiceDetectionLevel(asd)); - - /* Create it if it doesn't exist yet. */ - if (id_state == nullptr) - { - if (!(id_state = AppIdServiceState::add(ip, proto, port, - AppIdServiceDetectionLevel(asd)))) - { - ErrorMessage("Discover service failed to create state"); - return SERVICE_ENOMEM; - } - memset(id_state, 0, sizeof(*id_state)); - } - - //id_state->service_list = nullptr; - asd->id_state = id_state; - } + id_state = AppIdServiceState::add(ip, proto, port, get_service_detect_level(asd)); if (asd->serviceData == nullptr) { diff --git a/src/network_inspectors/appid/service_plugins/service_base.h b/src/network_inspectors/appid/service_plugins/service_base.h index d3f8070d8..ec0af5fc9 100644 --- a/src/network_inspectors/appid/service_plugins/service_base.h +++ b/src/network_inspectors/appid/service_plugins/service_base.h @@ -86,7 +86,7 @@ inline bool compareServiceElements(const RNAServiceElement* first, return (first->validate != second->validate || first->userdata != second->userdata); } -inline uint32_t AppIdServiceDetectionLevel(AppIdSession* asd) +inline uint32_t get_service_detect_level(AppIdSession* asd) { if (asd->get_session_flags(APPID_SESSION_DECRYPTED)) return 1;