From: Mike Stepanek (mstepane) <mstepane@cisco.com>
Date: Wed, 30 May 2018 00:21:10 +0000 (-0400)
Subject: Merge pull request #1242 in SNORT/snort3 from TPLibHandler_Reload to master
X-Git-Tag: 3.0.0-246~78
X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=4f058b2b987c2497c8ddbbd31b94b833ec649e5f;p=thirdparty%2Fsnort3.git

Merge pull request #1242 in SNORT/snort3 from TPLibHandler_Reload to master

Squashed commit of the following:

commit 4081128f8c8bae834226f42212d4a8442cafff3d
Author: Silviu Minut <sminut@cisco.com>
Date:   Fri May 25 18:34:55 2018 -0400

    appid: make TPLibHandler survive reloads without memory leaks.
---

diff --git a/src/network_inspectors/appid/appid_config.cc b/src/network_inspectors/appid/appid_config.cc
index c374d2858..4297b71ac 100644
--- a/src/network_inspectors/appid/appid_config.cc
+++ b/src/network_inspectors/appid/appid_config.cc
@@ -42,7 +42,6 @@
 #include "tp_lib_handler.h"
 #endif
 
-
 #define ODP_PORT_DETECTORS "odp/port/*"
 #define CUSTOM_PORT_DETECTORS "custom/port/*"
 #define MAX_DISPLAY_SIZE   65536
@@ -121,38 +120,6 @@ AppIdConfig::~AppIdConfig()
     cleanup();
 }
 
-const TPLibHandler * AppIdConfig::tp_handler() const
-{
-#ifdef ENABLE_APPID_THIRD_PARTY
-    return tph;
-#else
-    return nullptr;
-#endif
-}
-
-bool AppIdConfig::have_tp() const
-{
-#ifdef ENABLE_APPID_THIRD_PARTY
-    return tph != nullptr && tph->have_tp();
-#else
-    return false;
-#endif
-}
-
-void AppIdConfig::tp_appid_module_tinit()
-{
-#ifdef ENABLE_APPID_THIRD_PARTY
-    if(tph) tph->tinit();
-#endif
-}
-
-void AppIdConfig::tp_appid_module_tterm()
-{
-#ifdef ENABLE_APPID_THIRD_PARTY
-    if(tph) tph->tterm();
-#endif
-}
-
 void AppIdConfig::read_port_detectors(const char* files)
 {
     int rval;
@@ -773,8 +740,7 @@ bool AppIdConfig::init_appid(SnortConfig* sc)
     read_port_detectors(CUSTOM_PORT_DETECTORS);
 
 #ifdef ENABLE_APPID_THIRD_PARTY
-    tph=TPLibHandler::get();
-    tph->pinit(mod_config);
+    TPLibHandler::pinit(mod_config);
 #endif
     map_app_names_to_snort_ids(sc);
     return true;
@@ -794,11 +760,6 @@ static void free_port_exclusion_list(AppIdPortExclusions& pe_list)
 
 void AppIdConfig::cleanup()
 {
-#ifdef ENABLE_APPID_THIRD_PARTY
-    tph->pfini(0);
-    TPLibHandler::destroy(tph);
-#endif
-    
     app_info_mgr.cleanup_appid_info_table();
 
 #ifdef USE_RNA_CONFIG
diff --git a/src/network_inspectors/appid/appid_config.h b/src/network_inspectors/appid/appid_config.h
index 2eace7c89..6a641d9cc 100644
--- a/src/network_inspectors/appid/appid_config.h
+++ b/src/network_inspectors/appid/appid_config.h
@@ -39,7 +39,6 @@
 
 struct NetworkSet;
 class AppInfoManager;
-class TPLibHandler;
 
 extern unsigned appIdPolicyId;
 extern uint32_t app_id_netmasks[];
@@ -125,11 +124,6 @@ public:
     AppIdModuleConfig* mod_config = nullptr;
     unsigned appIdPolicyId = 53;
 
-    const TPLibHandler * tp_handler() const;
-    bool have_tp() const;
-    void tp_appid_module_tinit();
-    void tp_appid_module_tterm();
-
 private:
     void read_port_detectors(const char* files);
     void configure_analysis_networks(char* toklist[], uint32_t flag);
@@ -141,11 +135,6 @@ private:
     void display_port_config();
 
     AppInfoManager& app_info_mgr;
-
-// #ifdef ENABLE_APPID_THIRD_PARTY
-    // class that holds pointers to third party objects in thirdparty.so
-    TPLibHandler* tph = nullptr;
-// #endif
 };
 
 #endif
diff --git a/src/network_inspectors/appid/appid_discovery.cc b/src/network_inspectors/appid/appid_discovery.cc
index c54f23858..17729e926 100644
--- a/src/network_inspectors/appid/appid_discovery.cc
+++ b/src/network_inspectors/appid/appid_discovery.cc
@@ -45,12 +45,13 @@
 #include "detector_plugins/http_url_patterns.h"
 #include "host_port_app_cache.h"
 #include "service_plugins/service_discovery.h"
-#ifdef ENABLE_THIRD_PARTY_APPID
-#include "tp_appid_session_api.h"
+#ifdef ENABLE_APPID_THIRD_PARTY
+#include "tp_lib_handler.h"
 #endif
 
 using namespace snort;
 
+
 bool do_tp_discovery(AppIdSession&, IpProtocol, Packet*, AppidSessionDirection&);
 
 AppIdDiscovery::AppIdDiscovery(AppIdInspector& ins)
@@ -808,7 +809,7 @@ bool AppIdDiscovery::do_discovery(Packet* p, AppIdSession& asd, IpProtocol proto
 
     // Third party detection
 #ifdef ENABLE_APPID_THIRD_PARTY
-    if(asd.config->have_tp())
+    if ( TPLibHandler::have_tp() )
         is_discovery_done = do_tp_discovery(asd,protocol,p,direction);
 #endif
 
diff --git a/src/network_inspectors/appid/appid_http_session.cc b/src/network_inspectors/appid/appid_http_session.cc
index 98375ad68..72cc4062b 100644
--- a/src/network_inspectors/appid/appid_http_session.cc
+++ b/src/network_inspectors/appid/appid_http_session.cc
@@ -34,7 +34,7 @@
 #include "detector_plugins/http_url_patterns.h"
 #include "http_xff_fields.h"
 #ifdef ENABLE_APPID_THIRD_PARTY
-#include "tp_appid_session_api.h"
+#include "tp_lib_handler.h"
 #endif
 
 static const char* httpFieldName[ MAX_HTTP_FIELD_ID ] = // for use in debug messages
@@ -393,7 +393,7 @@ int AppIdHttpSession::process_http_packet(AppidSessionDirection direction)
     AppId service_id = APP_ID_NONE;
     AppId client_id = APP_ID_NONE;
     AppId payload_id = APP_ID_NONE;
-    bool have_tp = asd.tpsession != nullptr;
+    bool have_tp = asd.tpsession;
 
     // For fragmented HTTP headers, do not process if none of the fields are set.
     // These fields will get set when the HTTP header is reassembled.
diff --git a/src/network_inspectors/appid/appid_inspector.cc b/src/network_inspectors/appid/appid_inspector.cc
index 6498fd1d1..d669eb0cd 100644
--- a/src/network_inspectors/appid/appid_inspector.cc
+++ b/src/network_inspectors/appid/appid_inspector.cc
@@ -50,6 +50,9 @@
 #include "lua_detector_module.h"
 #include "service_plugins/service_discovery.h"
 #include "service_plugins/service_ssl.h"
+#ifdef ENABLE_APPID_THIRD_PARTY
+#include "tp_lib_handler.h"
+#endif
 
 using namespace snort;
 static THREAD_LOCAL PacketTracer::TracerMute appid_mute;
@@ -163,7 +166,7 @@ void AppIdInspector::tinit()
     if (active_config->mod_config and active_config->mod_config->log_all_sessions)
         appidDebug->set_enabled(true);
 #ifdef ENABLE_APPID_THIRD_PARTY
-    active_config->tp_appid_module_tinit();
+    TPLibHandler::tinit();
 #endif
 }
 
@@ -183,7 +186,7 @@ void AppIdInspector::tterm()
     delete appidDebug;
     appidDebug = nullptr;
 #ifdef ENABLE_APPID_THIRD_PARTY
-    active_config->tp_appid_module_tterm();
+    TPLibHandler::tterm();
 #endif
 }
 
@@ -220,11 +223,17 @@ static void mod_dtor(Module* m)
 static void appid_inspector_pinit()
 {
     AppIdSession::init();
+#ifdef ENABLE_APPID_THIRD_PARTY
+    TPLibHandler::get();
+#endif
 }
 
 static void appid_inspector_pterm()
 {
     openssl_cleanup();
+#ifdef ENABLE_APPID_THIRD_PARTY
+    TPLibHandler::pfini();
+#endif
 }
 
 static void appid_inspector_tinit()
diff --git a/src/network_inspectors/appid/appid_session.cc b/src/network_inspectors/appid/appid_session.cc
index ba20fe2a4..aa5a401ec 100644
--- a/src/network_inspectors/appid/appid_session.cc
+++ b/src/network_inspectors/appid/appid_session.cc
@@ -46,7 +46,7 @@
 #include "appid_utils/ip_funcs.h"
 #include "service_plugins/service_ssl.h"
 #ifdef ENABLE_APPID_THIRD_PARTY
-#include "tp_appid_session_api.h"
+#include "tp_lib_handler.h"
 #endif
 
 using namespace snort;
@@ -895,7 +895,7 @@ AppIdDnsSession* AppIdSession::get_dns_session()
 bool AppIdSession::is_tp_appid_done() const
 {
 #ifdef ENABLE_APPID_THIRD_PARTY
-    if (config->have_tp())
+    if ( TPLibHandler::have_tp() )
     {
         if (!tpsession)
             return false;
@@ -911,12 +911,11 @@ bool AppIdSession::is_tp_appid_done() const
 
 bool AppIdSession::is_tp_processing_done() const
 {
-
 #ifdef ENABLE_APPID_THIRD_PARTY
-    if (config->have_tp() &&
-        !get_session_flags(APPID_SESSION_NO_TPI) &&
-        (!is_tp_appid_done() ||
-        get_session_flags(APPID_SESSION_APP_REINSPECT | APPID_SESSION_APP_REINSPECT_SSL)))
+    if ( TPLibHandler::have_tp() &&
+	 !get_session_flags(APPID_SESSION_NO_TPI) &&
+	 (!is_tp_appid_done() ||
+	  get_session_flags(APPID_SESSION_APP_REINSPECT | APPID_SESSION_APP_REINSPECT_SSL)))
         return false;
 #endif
 
@@ -927,7 +926,7 @@ bool AppIdSession::is_tp_processing_done() const
 bool AppIdSession::is_tp_appid_available() const
 {
 #ifdef ENABLE_APPID_THIRD_PARTY
-    if (config->have_tp())
+    if ( TPLibHandler::have_tp() )
     {
         unsigned state;
 
diff --git a/src/network_inspectors/appid/test/tp_lib_handler_test.cc b/src/network_inspectors/appid/test/tp_lib_handler_test.cc
index 4fdfe3141..650211d51 100644
--- a/src/network_inspectors/appid/test/tp_lib_handler_test.cc
+++ b/src/network_inspectors/appid/test/tp_lib_handler_test.cc
@@ -48,6 +48,7 @@ TEST_GROUP(tp_lib_handler)
 
 TEST(tp_lib_handler, load_unload)
 {
+    tph = TPLibHandler::get();
     tph->pinit(&config);
     CHECK_TRUE(tph->have_tp());
 
@@ -63,8 +64,10 @@ TEST(tp_lib_handler, load_unload)
 
 TEST(tp_lib_handler, tp_lib_handler_get)
 {
+    tph=TPLibHandler::get();
     TPLibHandler* tph2=TPLibHandler::get();
     CHECK_EQUAL(tph,tph2);
+    TPLibHandler::pfini();
 }
 
 TEST(tp_lib_handler, load_error)
@@ -72,10 +75,10 @@ TEST(tp_lib_handler, load_error)
     // Trigger load error:
     AppIdModuleConfig config;
     config.tp_appid_path="nonexistent.so";
-    tph=TPLibHandler::get();
-    tph->pinit(&config);
-    CHECK_FALSE(tph->have_tp());
-    tph->pfini();
+    TPLibHandler::get();
+    TPLibHandler::pinit(&config);
+    CHECK_FALSE(TPLibHandler::have_tp());
+    TPLibHandler::pfini();
 }
 
 TEST(tp_lib_handler, tp_appid_module_pinit_error)
@@ -94,12 +97,9 @@ int main(int argc, char** argv)
 {
     config.tp_appid_path="./libtp_mock.so";
     config.tp_appid_config="./tp.config";
-    tph=TPLibHandler::get();
-
+    
     int rc = CommandLineTestRunner::RunAllTests(argc, argv);
 
-    TPLibHandler::destroy(tph);
-
     return rc;
 }
 
diff --git a/src/network_inspectors/appid/tp_appid_utils.cc b/src/network_inspectors/appid/tp_appid_utils.cc
index 06c7acef6..3851203d0 100644
--- a/src/network_inspectors/appid/tp_appid_utils.cc
+++ b/src/network_inspectors/appid/tp_appid_utils.cc
@@ -43,9 +43,9 @@
 #include "log/messages.h"
 #include "profiler/profiler.h"
 #include "stream/stream.h"
-#include "tp_appid_types.h"
-#include "tp_appid_session_api.h"
+#ifdef ENABLE_APPID_THIRD_PARTY
 #include "tp_lib_handler.h"
+#endif
 
 using namespace std;
 using namespace snort;
@@ -584,8 +584,8 @@ bool do_tp_discovery(AppIdSession& asd, IpProtocol protocol,
     vector<AppId> tp_proto_list;
     bool isTpAppidDiscoveryDone = false;
 
-    if ( !asd.config->have_tp() )
-        return true;
+    if ( !TPLibHandler::have_tp() )
+	return true;
 
     if (asd.tp_app_id == APP_ID_SSH && asd.payload.get_id() != APP_ID_SFTP &&
         asd.session_packet_count >= MIN_SFTP_PACKET_COUNT &&
@@ -624,7 +624,7 @@ bool do_tp_discovery(AppIdSession& asd, IpProtocol protocol,
                 ThirdPartyAppIDAttributeData tp_attribute_data;
                 if (!asd.tpsession)
                 {
-                    const TPLibHandler* tph = asd.config->tp_handler();
+                    const TPLibHandler* tph = TPLibHandler::get();
                     CreateThirdPartyAppIDSession_t tpsf = tph->tpsession_factory();
                     if ( !(asd.tpsession = tpsf()) )
                         FatalError("Could not allocate asd.tpsession data");
diff --git a/src/network_inspectors/appid/tp_lib_handler.cc b/src/network_inspectors/appid/tp_lib_handler.cc
index a19cc1ed7..b463c660d 100644
--- a/src/network_inspectors/appid/tp_lib_handler.cc
+++ b/src/network_inspectors/appid/tp_lib_handler.cc
@@ -37,7 +37,7 @@ using namespace snort;
 #define TP_APPID_MODULE_SYMBOL "create_third_party_appid_module"
 #define TP_APPID_SESSION_SYMBOL "create_third_party_appid_session"
 
-TPLibHandler* TPLibHandler::handler = nullptr;
+TPLibHandler* TPLibHandler::self = nullptr;
 
 int TPLibHandler::LoadCallback(const char* const path, int /* indent */)
 {
@@ -114,59 +114,64 @@ void TPLibHandler::pinit(const AppIdModuleConfig* config)
 {
     int ret;
 
-    if (config->tp_appid_path.empty())
+    if (self->tp_so_handle or config->tp_appid_path.empty())
         return;
 
-    tp_config.tp_appid_config=config->tp_appid_config;
+    self->tp_config.tp_appid_config=config->tp_appid_config;
 
-    LoadCallback(config->tp_appid_path.c_str(),1);
+    self->LoadCallback(config->tp_appid_path.c_str(),1);
 
-    if (tp_appid_module == nullptr)
+    if (self->tp_appid_module == nullptr)
     {
-        // FIXIT-H error message
+        ErrorMessage("Ignoring third party AppId library\n");
         return;
     }
 
-    tp_config.chp_body_collection_max = config->chp_body_collection_max;
-    tp_config.ftp_userid_disabled = config->ftp_userid_disabled;
-    tp_config.chp_body_collection_disabled =
+    self->tp_config.chp_body_collection_max = config->chp_body_collection_max;
+    self->tp_config.ftp_userid_disabled = config->ftp_userid_disabled;
+    self->tp_config.chp_body_collection_disabled =
         config->chp_body_collection_disabled;
-    tp_config.tp_allow_probes = config->tp_allow_probes;
+    self->tp_config.tp_allow_probes = config->tp_allow_probes;
     if (config->http2_detection_enabled)
-        tp_config.http_upgrade_reporting_enabled = 1;
+        self->tp_config.http_upgrade_reporting_enabled = 1;
     else
-        tp_config.http_upgrade_reporting_enabled = 0;
+        self->tp_config.http_upgrade_reporting_enabled = 0;
 
-    tp_config.http_response_version_enabled = config->http_response_version_enabled;
+    self->tp_config.http_response_version_enabled = config->http_response_version_enabled;
 
-    ret = tp_appid_module->pinit(tp_config);
+    ret = self->tp_appid_module->pinit(self->tp_config);
     if (ret != 0)
     {
         ErrorMessage("Unable to initialize 3rd party AppID module (%d)!\n", ret);
-        delete tp_appid_module;
-        dlclose(tp_so_handle);
-        tp_so_handle = nullptr;
-        tp_appid_module = nullptr;
+        delete self->tp_appid_module;
+        dlclose(self->tp_so_handle);
+        self->tp_so_handle = nullptr;
+        self->tp_appid_module = nullptr;
         return;
     }
 }
 
 void TPLibHandler::pfini(bool print_stats_flag)
 {
-    if (tp_appid_module != nullptr)
+    if (self and self->tp_appid_module != nullptr)
     {
         if (print_stats_flag)
-            tp_appid_module->print_stats();
+            self->tp_appid_module->print_stats();
 
-        int ret = tp_appid_module->pfini();
+        int ret = self->tp_appid_module->pfini();
 
         if (ret != 0)
             ErrorMessage("Could not finalize 3rd party AppID module (%d)!\n", ret);
 
-        delete tp_appid_module;
-        tp_appid_module = nullptr;
-        dlclose(tp_so_handle); // after delete, otherwise tpam will be dangling
-        tp_so_handle = nullptr;
+        delete self->tp_appid_module;
+        self->tp_appid_module = nullptr;
+
+        dlclose(self->tp_so_handle); // after delete, otherwise tpam will be dangling
+        self->tp_so_handle = nullptr;
     }
-}
 
+    if ( self ) {
+        delete self;
+        self = nullptr;
+    }
+}
diff --git a/src/network_inspectors/appid/tp_lib_handler.h b/src/network_inspectors/appid/tp_lib_handler.h
index d9f418c2d..0ba2ae482 100644
--- a/src/network_inspectors/appid/tp_lib_handler.h
+++ b/src/network_inspectors/appid/tp_lib_handler.h
@@ -32,33 +32,27 @@ class TPLibHandler
 {
 public:
 
-    bool have_tp() const
+    static bool have_tp()
     {
-	return tp_appid_module != nullptr;
+        return self and self->tp_appid_module != nullptr;
     }
 
     static TPLibHandler* get()
     {
-        if (handler)
-            return handler;
+        if (self)
+            return self;
         else
-            return (handler=new TPLibHandler());
+            return (self=new TPLibHandler());
     }
 
-    static void destroy(TPLibHandler* tph)
-    {
-        delete tph->handler;
-        tph->handler=nullptr;
-    }
-
-    // called from AppIdConfig::init_appid() and cleanup(), respectively.
-    void pinit(const AppIdModuleConfig* config);
-    void pfini(bool print_stats_flag=0);
+    // called from appid_inspector.cc appid_inspector_pinit() / pterm()
+    static void pinit(const AppIdModuleConfig* config);
+    static void pfini(bool print_stats_flag=0);
 
     // called from AppIdInspector tinit/tterm via
     // AppIdConfig::tp_appid_module_tinit/tterm.
-    void tinit() { if ( tp_appid_module ) tp_appid_module->tinit(); }
-    void tterm() { if ( tp_appid_module ) tp_appid_module->tfini(); }
+    static void tinit() { if ( have_tp() ) self->tp_appid_module->tinit(); }
+    static void tterm() { if ( have_tp() ) self->tp_appid_module->tfini(); }
 
     CreateThirdPartyAppIDSession_t tpsession_factory() const
     {
@@ -67,10 +61,10 @@ public:
 
 private:
 
-    TPLibHandler() { }
-    ~TPLibHandler() { }
+    TPLibHandler() = default;
+    ~TPLibHandler() = default;
 
-    static TPLibHandler* handler;
+    static TPLibHandler* self;
     void* tp_so_handle = nullptr;   // output of dlopen(thirdparty.so)
     ThirdPartyAppIDModule* tp_appid_module = nullptr;
     CreateThirdPartyAppIDSession_t createThirdPartyAppIDSession;
@@ -81,4 +75,3 @@ private:
 };
 
 #endif
-