From: Tom Peters (thopeter) Date: Fri, 17 May 2019 14:42:51 +0000 (-0400) Subject: Merge pull request #1610 in SNORT/snort3 from ~SBAIGAL/snort3:perfmon_event_fix to... X-Git-Tag: 3.0.0-256~2 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=683220535f6a584dfcfa08b925b84536e7795b45;p=thirdparty%2Fsnort3.git Merge pull request #1610 in SNORT/snort3 from ~SBAIGAL/snort3:perfmon_event_fix to master Squashed commit of the following: commit a3fcf0a70b39bf05ed8ed9f204fd88a42fd8ea81 Author: Steven Baigal (sbaigal) Date: Wed May 15 13:51:26 2019 -0400 perf_mon: add real timestamp to empty perf_stats data; updated dbus default subscription code and perf_mon event subscirption code to resolve memory leak and invalid event subscription from reloading; moved flow_ip_tracker to thread local --- diff --git a/src/flow/test/flow_stash_test.cc b/src/flow/test/flow_stash_test.cc index 983d9fafd..5fde609de 100644 --- a/src/flow/test/flow_stash_test.cc +++ b/src/flow/test/flow_stash_test.cc @@ -94,13 +94,13 @@ void DataBus::subscribe(const char* key, DataHandler* h) { DB->_subscribe(key, h); } -void DataBus::subscribe_default(const char* key, DataHandler* h) +void DataBus::subscribe_default(const char* key, DataHandler* h, SnortConfig* sc) { DB->_subscribe(key, h); } void DataBus::unsubscribe(const char*, DataHandler*) {} -void DataBus::unsubscribe_default(const char*, DataHandler*) {} +void DataBus::unsubscribe_default(const char*, DataHandler*, SnortConfig* sc) {} void DataBus::publish(const char* key, DataEvent& e, Flow* f) { diff --git a/src/framework/data_bus.cc b/src/framework/data_bus.cc index ac6259fe6..d948cb662 100644 --- a/src/framework/data_bus.cc +++ b/src/framework/data_bus.cc @@ -106,9 +106,12 @@ void DataBus::subscribe(const char* key, DataHandler* h) } // for subscribers that need to receive events regardless of active inspection policy -void DataBus::subscribe_default(const char* key, DataHandler* h) +void DataBus::subscribe_default(const char* key, DataHandler* h, SnortConfig* sc) { - get_default_inspection_policy(SnortConfig::get_conf())->dbus._subscribe(key, h); + if (sc) + get_default_inspection_policy(sc)->dbus._subscribe(key, h); + else + get_default_inspection_policy(SnortConfig::get_conf())->dbus._subscribe(key, h); } void DataBus::unsubscribe(const char* key, DataHandler* h) @@ -116,9 +119,12 @@ void DataBus::unsubscribe(const char* key, DataHandler* h) get_data_bus()._unsubscribe(key, h); } -void DataBus::unsubscribe_default(const char* key, DataHandler* h) +void DataBus::unsubscribe_default(const char* key, DataHandler* h, SnortConfig* sc) { - get_default_inspection_policy(SnortConfig::get_conf())->dbus._unsubscribe(key, h); + if (sc) + get_default_inspection_policy(sc)->dbus._unsubscribe(key, h); + else + get_default_inspection_policy(SnortConfig::get_conf())->dbus._unsubscribe(key, h); } // notify subscribers of event diff --git a/src/framework/data_bus.h b/src/framework/data_bus.h index 5444245b3..44eba7c95 100644 --- a/src/framework/data_bus.h +++ b/src/framework/data_bus.h @@ -38,6 +38,7 @@ namespace snort { class Flow; struct Packet; +struct SnortConfig; class DataEvent { @@ -96,9 +97,9 @@ public: void add_mapped_module(const char*); static void subscribe(const char* key, DataHandler*); - static void subscribe_default(const char* key, DataHandler*); + static void subscribe_default(const char* key, DataHandler*, SnortConfig* = nullptr); static void unsubscribe(const char* key, DataHandler*); - static void unsubscribe_default(const char* key, DataHandler*); + static void unsubscribe_default(const char* key, DataHandler*, SnortConfig* = nullptr); static void publish(const char* key, DataEvent&, Flow* = nullptr); // convenience methods diff --git a/src/network_inspectors/perf_monitor/perf_monitor.cc b/src/network_inspectors/perf_monitor/perf_monitor.cc index 68a54a46f..e53fbd139 100644 --- a/src/network_inspectors/perf_monitor/perf_monitor.cc +++ b/src/network_inspectors/perf_monitor/perf_monitor.cc @@ -50,34 +50,17 @@ THREAD_LOCAL SimpleStats pmstats; THREAD_LOCAL ProfileStats perfmonStats; static THREAD_LOCAL std::vector* trackers; - +static THREAD_LOCAL FlowIPTracker* flow_ip_tracker = nullptr; //------------------------------------------------------------------------- // class stuff //------------------------------------------------------------------------- +class FlowIPDataHandler; class PerfMonitor : public Inspector { public: PerfMonitor(PerfConfig*); - ~PerfMonitor() override - { - if ( perf_idle_handler ) - { - DataBus::unsubscribe_default(THREAD_IDLE_EVENT, perf_idle_handler); - delete perf_idle_handler; - } - if ( perf_rotate_handler ) - { - DataBus::unsubscribe_default(THREAD_ROTATE_EVENT, perf_rotate_handler); - delete perf_rotate_handler; - } - if ( flow_ip_handler ) - { - DataBus::unsubscribe_default(FLOW_STATE_EVENT, flow_ip_handler); - delete flow_ip_handler; - } - delete config; - } + ~PerfMonitor() override { delete config;} bool configure(SnortConfig*) override; void show(SnortConfig*) override; @@ -92,21 +75,9 @@ public: FlowIPTracker* get_flow_ip(); - void reset_perf_idle_handler() - { perf_idle_handler = nullptr; } - - void reset_perf_rotate_handler() - { perf_rotate_handler = nullptr; } - - void reset_flow_ip_handler() - { flow_ip_handler = nullptr; } - private: PerfConfig* const config; - FlowIPTracker* flow_ip_tracker = nullptr; - DataHandler* flow_ip_handler = nullptr; - DataHandler* perf_idle_handler; - DataHandler* perf_rotate_handler; + FlowIPDataHandler* flow_ip_handler = nullptr; void disable_tracker(size_t); }; @@ -114,11 +85,8 @@ private: class PerfIdleHandler : public DataHandler { public: - PerfIdleHandler(PerfMonitor& p) : DataHandler(PERF_NAME), perf_monitor(p) - { DataBus::subscribe_default(THREAD_IDLE_EVENT, this); } - - ~PerfIdleHandler() override - { perf_monitor.reset_perf_idle_handler(); } + PerfIdleHandler(PerfMonitor& p, SnortConfig*& sc) : DataHandler(PERF_NAME), perf_monitor(p) + { DataBus::subscribe_default(THREAD_IDLE_EVENT, this, sc); } void handle(DataEvent&, Flow*) override { perf_monitor.eval(nullptr); } @@ -130,11 +98,8 @@ private: class PerfRotateHandler : public DataHandler { public: - PerfRotateHandler(PerfMonitor& p) : DataHandler(PERF_NAME), perf_monitor(p) - { DataBus::subscribe_default(THREAD_ROTATE_EVENT, this); } - - ~PerfRotateHandler() override - { perf_monitor.reset_perf_rotate_handler(); } + PerfRotateHandler(PerfMonitor& p, SnortConfig* sc) : DataHandler(PERF_NAME), perf_monitor(p) + { DataBus::subscribe_default(THREAD_ROTATE_EVENT, this, sc); } void handle(DataEvent&, Flow*) override { perf_monitor.rotate(); } @@ -146,11 +111,8 @@ private: class FlowIPDataHandler : public DataHandler { public: - FlowIPDataHandler(PerfMonitor& p) : DataHandler(PERF_NAME), perf_monitor(p) - { DataBus::subscribe_default(FLOW_STATE_EVENT, this); } - - ~FlowIPDataHandler() override - { perf_monitor.reset_flow_ip_handler(); } + FlowIPDataHandler(PerfMonitor& p, SnortConfig* sc) : DataHandler(PERF_NAME), perf_monitor(p) + { DataBus::subscribe_default(FLOW_STATE_EVENT, this, sc); } void handle(DataEvent&, Flow* flow) override { @@ -172,7 +134,8 @@ public: return; FlowIPTracker* tracker = perf_monitor.get_flow_ip(); - tracker->update_state(&flow->client_ip, &flow->server_ip, state); + if (tracker) + tracker->update_state(&flow->client_ip, &flow->server_ip, state); } private: @@ -245,6 +208,7 @@ void PerfMonitor::disable_tracker(size_t i) if ( tracker == flow_ip_tracker ) { DataBus::unsubscribe_default(FLOW_STATE_EVENT, flow_ip_handler); + delete flow_ip_handler; flow_ip_tracker = nullptr; } @@ -257,15 +221,13 @@ void PerfMonitor::disable_tracker(size_t i) // type and version fields immediately after timestamp like seconds, usec, // type, version#, data1, data2, ... -bool PerfMonitor::configure(SnortConfig*) +bool PerfMonitor::configure(SnortConfig* sc) { - // DataBus deletes these during shutdown, but we may also need to - // delete manually those subscriptions came from failed reload - perf_idle_handler = new PerfIdleHandler(*this); - perf_rotate_handler = new PerfRotateHandler(*this); + new PerfIdleHandler(*this, sc); + new PerfRotateHandler(*this, sc); if ( config->perf_flags & PERF_FLOWIP ) - flow_ip_handler = new FlowIPDataHandler(*this); + flow_ip_handler = new FlowIPDataHandler(*this, sc); return config->resolve(); } @@ -312,6 +274,7 @@ void PerfMonitor::tterm() trackers->pop_back(); } delete trackers; + flow_ip_tracker = nullptr; } } @@ -378,6 +341,10 @@ bool PerfMonitor::ready_to_process(Packet* p) { if ((cur_time - sample_time) >= config->sample_interval) { + if (cnt == 0) + for (auto& tracker : *trackers) + tracker->update_time(cur_time); + cnt = 0; sample_time = cur_time; return true; @@ -487,8 +454,5 @@ TEST_CASE("Process timing logic", "[perfmon]") REQUIRE((perfmon.ready_to_process(&p) == false)); REQUIRE((perfmon.ready_to_process(&p) == false)); REQUIRE((perfmon.ready_to_process(&p) == true)); - - perfmon.reset_perf_idle_handler(); - perfmon.reset_perf_rotate_handler(); } #endif diff --git a/src/service_inspectors/dce_rpc/dce_smb_utils.cc b/src/service_inspectors/dce_rpc/dce_smb_utils.cc index 4251fa9e7..cd2d4b542 100644 --- a/src/service_inspectors/dce_rpc/dce_smb_utils.cc +++ b/src/service_inspectors/dce_rpc/dce_smb_utils.cc @@ -1580,7 +1580,6 @@ static DCE2_Ret DCE2_SmbFileAPIProcess(DCE2_SmbSsnData* ssd, Profile profile(dce2_smb_pstat_smb_file_api); Packet* p = DetectionEngine::get_current_packet(); - DetectionEngine::get_current_packet(); FileFlows* file_flows = FileFlows::get_file_flows(p->flow); if (!file_flows->file_process(p, data_ptr, (int)data_len, position, upload, DCE2_SmbIsVerdictSuspend(upload, position)))