Merge pull request #722 in SNORT/snort3 from appid_stl_thread_safety to master

Squashed commit of the following:

commit 991eb29ae7f85b1e9e1b72f334eb96536c568b10
Author: davis mcpherson <davmcphe.cisco.com>
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 <davmcphe.cisco.com>
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 beeb41a18b3e0b5df1692dcdc125f8e203b1991d..1e6a65c77e0a8b19cf9b3345ea5a6d35007cdd37 100644 (file)
--- 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<std::mutex> 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<std::mutex> 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 de1bba309d7d3c194620049c16721e390dc46613..9e9210c02bd4541c59ec542d9775482c4ecd93c8 100644 (file)
--- 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 df1389daa7542258b66e9ee4a306fe322f464860..1d48f9e9ebdafefdc31a8181467c98741865226d 100644 (file)
--- 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 d62fb9f7b0f3568fbb29c97c3eb193f5dd144bda..7819728f3edcb591daf6bbe00ea0196d47489046 100644 (file)
--- 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 30bce6970e2838122f01cef3241720b54808eacf..96473744695c545a9bd7c09fa809e92a9cff1e54 100644 (file)
--- 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 d3f8070d8eae9b13481dd86588cd9d6df28759ff..ec0af5fc90c63616da13566fbd58013000e83eca 100644 (file)
--- 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;