From: Ron Dempster (rdempste) <rdempste@cisco.com>
Date: Wed, 9 Dec 2020 17:30:29 +0000 (+0000)
Subject: Merge pull request #2647 in SNORT/snort3 from ~RDEMPSTE/snort3:removed_inspectors... 
X-Git-Tag: 3.0.3-6~17
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=311c2607cb3ac46abe4cc242d5fb2599e8846732;p=thirdparty%2Fsnort3.git

Merge pull request #2647 in SNORT/snort3 from ~RDEMPSTE/snort3:removed_inspectors to master

Squashed commit of the following:

commit 7225fb279cd1e10e52599be338717df86035b943
Author: Ron Dempster (rdempste) <rdempste@cisco.com>
Date:   Tue Dec 8 08:00:54 2020 -0500

    packet_tracer: Fix the debug session information for non-ip packets

commit d9a1d78c903830f71fbe33dc834912204e7f6579
Author: Ron Dempster (rdempste) <rdempste@cisco.com>
Date:   Wed Dec 2 11:52:54 2020 -0500

    stream: fix stream clean up when going from enabled to disabled

commit 5e6d47c4f4b8370769bb30a88e706ceccb5899ba
Author: Ron Dempster (rdempste) <rdempste@cisco.com>
Date:   Wed Nov 25 13:51:08 2020 -0500

    managers: don't allow a referenced inspector to stall emptying the trash

commit 1843e30d47f5083a2d84f0061ba56d97dd2b0fe7
Author: Ron Dempster (rdempste) <rdempste@cisco.com>
Date:   Sat Dec 5 08:02:49 2020 -0500

    managers: track removed inspectors during reload and call tear_down and tterm to release resources
---

diff --git a/src/framework/inspector.h b/src/framework/inspector.h
index 9641c3806..641778531 100644
--- a/src/framework/inspector.h
+++ b/src/framework/inspector.h
@@ -72,6 +72,12 @@ public:
     // return verification status
     virtual bool configure(SnortConfig*) { return true; }
 
+    // cleanup for inspector instance removal from the running configuration
+    // this is only called for inspectors in the default inspection policy that
+    // were present in the prior snort configuration and were removed in the snort
+    // configuration that is being loaded during a reload_config command
+    virtual void tear_down(SnortConfig*) { }
+
     // called on controls after everything is configured
     // return true if there is nothing to do ever based on config
     virtual bool disable(SnortConfig*) { return false; }
diff --git a/src/main.cc b/src/main.cc
index 12506f745..9b044399a 100644
--- a/src/main.cc
+++ b/src/main.cc
@@ -353,7 +353,7 @@ int main_reload_config(lua_State* L)
 
     current_request->respond(".. reloading configuration\n");
     const SnortConfig* old = SnortConfig::get_conf();
-    SnortConfig* sc = Snort::get_reload_config(fname, plugin_path);
+    SnortConfig* sc = Snort::get_reload_config(fname, plugin_path, old);
 
     if ( !sc )
     {
diff --git a/src/main/analyzer.cc b/src/main/analyzer.cc
index b90d418dc..fc7a8de59 100644
--- a/src/main/analyzer.cc
+++ b/src/main/analyzer.cc
@@ -597,6 +597,11 @@ void Analyzer::reinit(const SnortConfig* sc)
     TraceApi::thread_reinit(sc->trace_config);
 }
 
+void Analyzer::stop_removed(const SnortConfig* sc)
+{
+    InspectorManager::thread_stop_removed(sc);
+}
+
 void Analyzer::term()
 {
     const SnortConfig* sc = SnortConfig::get_conf();
diff --git a/src/main/analyzer.h b/src/main/analyzer.h
index 01f4111f4..bb2f1340a 100644
--- a/src/main/analyzer.h
+++ b/src/main/analyzer.h
@@ -105,6 +105,7 @@ public:
     void resume(uint64_t msg_cnt);
     void reload_daq();
     void reinit(const snort::SnortConfig*);
+    void stop_removed(const snort::SnortConfig*);
     void rotate();
     snort::SFDAQInstance* get_daq_instance() { return daq_instance; }
 
diff --git a/src/main/analyzer_command.cc b/src/main/analyzer_command.cc
index ea2a50980..64cfe2438 100644
--- a/src/main/analyzer_command.cc
+++ b/src/main/analyzer_command.cc
@@ -146,6 +146,7 @@ bool ACSwap::execute(Analyzer& analyzer, void** ac_state)
             if ( reload_tuners->empty() )
             {
                 delete reload_tuners;
+                ps->finish(analyzer);
                 return true;
             }
 
diff --git a/src/main/snort.cc b/src/main/snort.cc
index f5c3da1fa..30647d189 100644
--- a/src/main/snort.cc
+++ b/src/main/snort.cc
@@ -447,7 +447,8 @@ void Snort::reload_failure_cleanup(SnortConfig* sc)
 
 // FIXIT-M refactor this so startup and reload call the same core function to
 // instantiate things that can be reloaded
-SnortConfig* Snort::get_reload_config(const char* fname, const char* plugin_path)
+SnortConfig* Snort::get_reload_config(const char* fname, const char* plugin_path,
+    const SnortConfig* old)
 {
     reloading = true;
     ModuleManager::reset_errors();
@@ -477,6 +478,7 @@ SnortConfig* Snort::get_reload_config(const char* fname, const char* plugin_path
         return nullptr;
     }
 
+    InspectorManager::tear_down_removed_inspectors(old, sc);
     InspectorManager::prepare_controls(sc);
 
     FileService::verify_reload(sc);
diff --git a/src/main/snort.h b/src/main/snort.h
index 82c930e99..c6eee51fd 100644
--- a/src/main/snort.h
+++ b/src/main/snort.h
@@ -38,7 +38,8 @@ struct SnortConfig;
 class Snort
 {
 public:
-    static SnortConfig* get_reload_config(const char* fname, const char* plugin_path = nullptr);
+    static SnortConfig* get_reload_config(const char* fname, const char* plugin_path,
+        const SnortConfig* old);
     static SnortConfig* get_updated_policy(SnortConfig*, const char* fname, const char* iname);
     static SnortConfig* get_updated_module(SnortConfig*, const char* name);
     static void setup(int argc, char* argv[]);
diff --git a/src/main/swapper.cc b/src/main/swapper.cc
index 39f6a09a7..68777ede5 100644
--- a/src/main/swapper.cc
+++ b/src/main/swapper.cc
@@ -23,6 +23,8 @@
 
 #include "swapper.h"
 
+#include "managers/inspector_manager.h"
+
 #include "analyzer.h"
 #include "snort.h"
 #include "snort_config.h"
@@ -51,6 +53,8 @@ Swapper::Swapper()
 
 Swapper::~Swapper()
 {
+    if ( new_conf )
+        InspectorManager::clear_removed_inspectors(new_conf);
     if ( old_conf )
         delete old_conf;
 }
@@ -66,3 +70,9 @@ void Swapper::apply(Analyzer& analyzer)
             analyzer.reinit(new_conf);
     }
 }
+
+void Swapper::finish(Analyzer& analyzer)
+{
+    if ( new_conf )
+        analyzer.stop_removed(new_conf);
+}
diff --git a/src/main/swapper.h b/src/main/swapper.h
index 4defaaf8e..78ce49b90 100644
--- a/src/main/swapper.h
+++ b/src/main/swapper.h
@@ -40,6 +40,7 @@ public:
     ~Swapper();
 
     void apply(Analyzer&);
+    void finish(Analyzer&);
     snort::SnortConfig* get_new_conf() { return new_conf; }
 
     static bool get_reload_in_progress() { return reload_in_progress; }
diff --git a/src/main/test/stubs.h b/src/main/test/stubs.h
index 5405b56ba..703f4e6e6 100644
--- a/src/main/test/stubs.h
+++ b/src/main/test/stubs.h
@@ -200,6 +200,7 @@ void InspectorManager::thread_init(const SnortConfig*) { }
 void InspectorManager::thread_term() { }
 void InspectorManager::thread_stop(const SnortConfig*) { }
 void InspectorManager::thread_reinit(const SnortConfig*) { }
+void InspectorManager::thread_stop_removed(const SnortConfig*) { }
 void ModuleManager::accumulate() { }
 void Stream::handle_timeouts(bool) { }
 void Stream::purge_flows() { }
diff --git a/src/managers/inspector_manager.cc b/src/managers/inspector_manager.cc
index bcb164d92..a17c76810 100644
--- a/src/managers/inspector_manager.cc
+++ b/src/managers/inspector_manager.cc
@@ -218,6 +218,7 @@ void PHVector::add_control(PHInstance* p)
 struct FrameworkPolicy
 {
     PHInstanceList ilist;   // List of inspector module instances
+    PHInstanceList removed_ilist;   // List of removed inspector module instances
 
     PHVector passive;
     PHVector packet;
@@ -392,12 +393,15 @@ static void empty_trash(PHList& trash)
     while ( !trash.empty() )
     {
         auto* p = trash.front();
+        trash.pop_front();
 
         if ( !p->is_inactive() )
+        {
+            trash.emplace_back(p);
             return;
+        }
 
         InspectorManager::free_inspector(p);
-        trash.pop_front();
     }
 }
 
@@ -532,6 +536,31 @@ Binder* InspectorManager::get_binder()
     return (Binder*)pi->framework_policy->binder;
 }
 
+void InspectorManager::clear_removed_inspectors(SnortConfig* sc)
+{
+    FrameworkPolicy* fp = sc->policy_map->get_inspection_policy()->framework_policy;
+    for ( auto* p : fp->removed_ilist )
+        p->handler->rem_ref();
+    fp->removed_ilist.clear();
+}
+
+void InspectorManager::tear_down_removed_inspectors(const SnortConfig* old, SnortConfig* sc)
+{
+    FrameworkPolicy* fp = get_default_inspection_policy(sc)->framework_policy;
+    InspectionPolicy* old_p = get_default_inspection_policy(old);
+    FrameworkPolicy* old_fp = old_p->framework_policy;
+    for (auto it = old_fp->ilist.begin(); it != old_fp->ilist.end(); ++it)
+    {
+        PHInstance* instance = get_instance(fp, (*it)->name.c_str());
+        if (!instance)
+        {
+            fp->removed_ilist.emplace_back(*it);
+            (*it)->handler->add_ref();
+            (*it)->handler->tear_down(sc);
+        }
+    }
+}
+
 // FIXIT-P cache get_inspector() returns or provide indexed lookup
 Inspector* InspectorManager::get_inspector(const char* key, bool dflt_only, const SnortConfig* sc)
 {
@@ -728,6 +757,26 @@ void InspectorManager::thread_reinit(const SnortConfig* sc)
     }
 }
 
+void InspectorManager::thread_stop_removed(const SnortConfig* sc)
+{
+    // pin->tinit() only called for default policy
+    InspectionPolicy* pi = get_default_inspection_policy(sc);
+
+    if ( pi && pi->framework_policy )
+    {
+        // Call pin->tterm() for anything that has been initialized and removed
+        for ( auto* p : pi->framework_policy->removed_ilist )
+        {
+            PHGlobal& phg = get_thread_local_plugin(p->pp_class.api);
+            if ( phg.instance_initialized )
+            {
+                p->handler->tterm();
+                phg.instance_initialized = false;
+            }
+        }
+    }
+}
+
 void InspectorManager::thread_stop(const SnortConfig* sc)
 {
     // If thread_init() was never called, we have nothing to do.
@@ -738,10 +787,6 @@ void InspectorManager::thread_stop(const SnortConfig* sc)
     set_default_policy(sc);
     InspectionPolicy* pi = get_inspection_policy();
 
-    // FIXIT-RC Any inspectors that were once configured/instantiated but
-    // no longer exist in the conf cannot have their instance tterm()
-    // called and will leak!
-
     if ( pi && pi->framework_policy )
     {
         for ( auto* p : pi->framework_policy->ilist )
diff --git a/src/managers/inspector_manager.h b/src/managers/inspector_manager.h
index 377dc1354..dcc84724c 100644
--- a/src/managers/inspector_manager.h
+++ b/src/managers/inspector_manager.h
@@ -77,6 +77,7 @@ public:
 
     static void thread_init(const SnortConfig*);
     static void thread_reinit(const SnortConfig*);
+    static void thread_stop_removed(const SnortConfig*);
 
     static void thread_stop(const SnortConfig*);
     static void thread_term();
@@ -88,6 +89,8 @@ public:
 
     static void clear(Packet*);
     static void empty_trash();
+    static void tear_down_removed_inspectors(const SnortConfig*, SnortConfig*);
+    static void clear_removed_inspectors(SnortConfig*);
 
 #ifdef PIGLET
     static Inspector* instantiate(const char*, Module*, SnortConfig*);
diff --git a/src/network_inspectors/packet_tracer/packet_tracer.cc b/src/network_inspectors/packet_tracer/packet_tracer.cc
index dd28e5a29..258ddaf45 100644
--- a/src/network_inspectors/packet_tracer/packet_tracer.cc
+++ b/src/network_inspectors/packet_tracer/packet_tracer.cc
@@ -346,15 +346,37 @@ void PacketTracer::add_packet_type_info(const Packet& p)
 void PacketTracer::add_eth_header_info(const Packet& p)
 {
     auto eh = layer::get_eth_layer(&p);
-    if (!(shell_enabled) && eh )
+    if (eh)
     {
-        // MAC layer
-        PacketTracer::log("%02X:%02X:%02X:%02X:%02X:%02X -> %02X:%02X:%02X:%02X:%02X:%02X %04X\n",
-            eh->ether_src[0], eh->ether_src[1], eh->ether_src[2],
-            eh->ether_src[3], eh->ether_src[4], eh->ether_src[5],
-            eh->ether_dst[0], eh->ether_dst[1], eh->ether_dst[2],
-            eh->ether_dst[3], eh->ether_dst[4], eh->ether_dst[5],
-            (uint16_t)eh->ethertype());
+        if (shell_enabled)
+        {
+            PacketTracer::log("\n");
+            char gr_buf[32] = { '\0' };
+            if (p.is_inter_group_flow())
+                snprintf(gr_buf, sizeof(gr_buf), " GR=%hd-%hd", p.pkth->ingress_group,
+                    p.pkth->egress_group);
+
+            snprintf(debug_session, sizeof(debug_session),
+                "%02X:%02X:%02X:%02X:%02X:%02X -> %02X:%02X:%02X:%02X:%02X:%02X %04X"
+                " AS=%hu ID=%u%s ",
+                eh->ether_src[0], eh->ether_src[1], eh->ether_src[2],
+                eh->ether_src[3], eh->ether_src[4], eh->ether_src[5],
+                eh->ether_dst[0], eh->ether_dst[1], eh->ether_dst[2],
+                eh->ether_dst[3], eh->ether_dst[4], eh->ether_dst[5],
+                (uint16_t)eh->ethertype(), p.pkth->address_space_id, get_instance_id(),
+                gr_buf);
+            s_pkt_trace->active = true;
+        }
+        else
+        {
+            // MAC layer
+            PacketTracer::log("%02X:%02X:%02X:%02X:%02X:%02X -> %02X:%02X:%02X:%02X:%02X:%02X %04X\n",
+                eh->ether_src[0], eh->ether_src[1], eh->ether_src[2],
+                eh->ether_src[3], eh->ether_src[4], eh->ether_src[5],
+                eh->ether_dst[0], eh->ether_dst[1], eh->ether_dst[2],
+                eh->ether_dst[3], eh->ether_dst[4], eh->ether_dst[5],
+                (uint16_t)eh->ethertype());
+        }
     }
 }
 
diff --git a/src/stream/base/stream_base.cc b/src/stream/base/stream_base.cc
index fe914be03..2de2a211b 100644
--- a/src/stream/base/stream_base.cc
+++ b/src/stream/base/stream_base.cc
@@ -156,6 +156,8 @@ public:
     StreamBase(const StreamModuleConfig*);
     void show(const SnortConfig*) const override;
 
+    void tear_down(SnortConfig*) override;
+
     void tinit() override;
     void tterm() override;
 
@@ -168,6 +170,9 @@ public:
 StreamBase::StreamBase(const StreamModuleConfig* c)
 { config = *c; }
 
+void StreamBase::tear_down(SnortConfig* sc)
+{ sc->register_reload_resource_tuner(new StreamUnloadReloadResourceManager); }
+
 void StreamBase::tinit()
 {
     assert(!flow_con && config.flow_cache_cfg.max_flows);
@@ -212,6 +217,9 @@ void StreamBase::tterm()
 {
     StreamHAManager::tterm();
     FlushBucket::clear();
+    base_prep();
+    delete flow_con;
+    flow_con = nullptr;
 }
 
 void StreamBase::show(const SnortConfig* sc) const
@@ -316,10 +324,6 @@ static void base_tterm()
 {
     StreamHAManager::tterm();
     FlushBucket::clear();
-
-    // this can't happen sooner because the counts haven't been harvested yet
-    delete flow_con;
-    flow_con = nullptr;
     TcpStreamTracker::thread_term();
 }
 
diff --git a/src/stream/base/stream_module.cc b/src/stream/base/stream_module.cc
index 3157338f8..0bdbbb369 100644
--- a/src/stream/base/stream_module.cc
+++ b/src/stream/base/stream_module.cc
@@ -292,6 +292,44 @@ bool StreamReloadResourceManager::tune_resources(unsigned work_limit)
     return ( flows_to_delete ) ? false : true;
 }
 
+bool StreamUnloadReloadResourceManager::tinit()
+{
+    unsigned max_flows = flow_con->get_flow_cache_config().max_flows;
+    if (max_flows)
+    {
+        stream_base_stats.reload_total_deletes += max_flows;
+        return true;
+    }
+    return false;
+}
+
+bool StreamUnloadReloadResourceManager::tune_packet_context()
+{
+    ++stream_base_stats.reload_tuning_packets;
+    return tune_resources(max_work);
+}
+
+bool StreamUnloadReloadResourceManager::tune_idle_context()
+{
+    ++stream_base_stats.reload_tuning_idle;
+    return tune_resources(max_work_idle);
+}
+
+bool StreamUnloadReloadResourceManager::tune_resources(unsigned work_limit)
+{
+    unsigned flows_to_delete = flow_con->get_flows_allocated();
+
+    if (!flows_to_delete)
+        return true;
+
+    if (flows_to_delete > work_limit)
+        flows_to_delete -= flow_con->delete_flows(work_limit);
+    else
+        flows_to_delete -= flow_con->delete_flows(flows_to_delete);
+
+    return (flows_to_delete) ? false : true;
+}
+
 void StreamModuleConfig::show() const
 {
     ConfigLogger::log_value("max_flows", flow_cache_cfg.max_flows);
diff --git a/src/stream/base/stream_module.h b/src/stream/base/stream_module.h
index fcf54c1a4..a789bc5a0 100644
--- a/src/stream/base/stream_module.h
+++ b/src/stream/base/stream_module.h
@@ -117,6 +117,19 @@ private:
     timeval reload_time{};
 };
 
+class StreamUnloadReloadResourceManager : public snort::ReloadResourceTuner
+{
+public:
+    StreamUnloadReloadResourceManager() { }
+
+    bool tinit() override;
+    bool tune_packet_context() override;
+    bool tune_idle_context() override;
+
+private:
+    bool tune_resources(unsigned work_limit);
+};
+
 class StreamModule : public snort::Module
 {
 public:
diff --git a/src/stream/tcp/tcp_segment_descriptor.cc b/src/stream/tcp/tcp_segment_descriptor.cc
index 8b0ee0b66..4dc188b24 100644
--- a/src/stream/tcp/tcp_segment_descriptor.cc
+++ b/src/stream/tcp/tcp_segment_descriptor.cc
@@ -97,7 +97,10 @@ void TcpSegmentDescriptor::setup()
 { ma_pseudo_packet = new Packet(false); }
 
 void TcpSegmentDescriptor::clear()
-{ delete ma_pseudo_packet; }
+{
+    delete ma_pseudo_packet;
+    ma_pseudo_packet = nullptr;
+}
 
 uint32_t TcpSegmentDescriptor::init_mss(uint16_t* value)
 {