Merge pull request #3134 in SNORT/snort3 from ~VHORBATO/snort3:uni_list_fix to master

Squashed commit of the following:

commit 207aca5fe21b8c09ce9d0f5c0dfca3b571356e69
Author: Vitalii <vhorbato@cisco.com>
Date:   Tue Oct 26 09:37:55 2021 +0300

    flow: use the same pkt_type to link and unlink unidirectional flows

    Use Flow::key::pkt_type instead of Flow::pkt_type, which is set later and
    might not be available at link_uni time.

    Traces enabled for the 'stream' module.

diff --git a/src/flow/flow_cache.cc b/src/flow/flow_cache.cc
index 3d62ad426e64590d3b465a5cac835452336d85fd..ab159ddf8b91bf119bf5b961fa7cb10526a964fe 100644 (file)
--- a/src/flow/flow_cache.cc
+++ b/src/flow/flow_cache.cc
@@ -29,9 +29,11 @@
 #include "hash/zhash.h"
 #include "helpers/flag_context.h"
 #include "ips_options/ips_flowbits.h"
+#include "main/snort_debug.h"
 #include "memory/memory_cap.h"
 #include "packet_io/active.h"
 #include "packet_tracer/packet_tracer.h"
+#include "stream/base/stream_module.h"
 #include "time/packet_time.h"
 #include "utils/stats.h"
 
@@ -54,6 +56,7 @@ static const unsigned ALL_FLOWS = 3;
 //-------------------------------------------------------------------------
 
 THREAD_LOCAL bool FlowCache::pruning_in_progress = false;
+extern THREAD_LOCAL const snort::Trace* stream_trace;
 
 FlowCache::FlowCache(const FlowCacheConfig& cfg) : config(cfg)
 {
@@ -110,19 +113,39 @@ Flow* FlowCache::find(const FlowKey* key)
 // always prepend
 void FlowCache::link_uni(Flow* flow)
 {
-    if ( flow->pkt_type == PktType::IP )
+    if ( flow->key->pkt_type == PktType::IP )
+    {
+        debug_logf(stream_trace, TRACE_FLOW, nullptr, 
+            "linking unidirectional flow (IP) to list of size: %u\n", 
+            uni_ip_flows->get_count());
         uni_ip_flows->link_uni(flow);
+    }
     else
+    {
+        debug_logf(stream_trace, TRACE_FLOW, nullptr, 
+            "linking unidirectional flow (non-IP) to list of size: %u\n", 
+            uni_flows->get_count());
         uni_flows->link_uni(flow);
+    }
 }
 
 // but remove from any point
 void FlowCache::unlink_uni(Flow* flow)
 {
-    if ( flow->pkt_type == PktType::IP )
-        uni_ip_flows->unlink_uni(flow);
+    if ( flow->key->pkt_type == PktType::IP )
+    {
+        if ( uni_ip_flows->unlink_uni(flow) )
+            debug_logf(stream_trace, TRACE_FLOW, nullptr, 
+                "unlinked unidirectional flow (IP) from list, size: %u\n", 
+                uni_ip_flows->get_count());
+    }
     else
-        uni_flows->unlink_uni(flow);
+    {
+        if ( uni_flows->unlink_uni(flow) )
+            debug_logf(stream_trace, TRACE_FLOW, nullptr, 
+                "unlinked unidirectional flow (non-IP) from list, size: %u\n",
+                uni_flows->get_count());
+    }
 }
 
 Flow* FlowCache::allocate(const FlowKey* key)
@@ -164,8 +187,7 @@ Flow* FlowCache::allocate(const FlowKey* key)
 
 void FlowCache::remove(Flow* flow)
 {
-    if ( flow->next )
-        unlink_uni(flow);
+    unlink_uni(flow);
 
     // FIXIT-M This check is added for offload case where both Flow::reset
     // and Flow::retire try remove the flow from hash. Flow::reset should
@@ -423,8 +445,7 @@ unsigned FlowCache::delete_active_flows(unsigned mode, unsigned num_to_delete, u
         }
 
         // we have a winner...
-        if ( flow->next )
-            unlink_uni(flow);
+        unlink_uni(flow);
 
         if ( flow->was_blocked() )
             delete_stats.update(FlowDeleteState::BLOCKED);

diff --git a/src/flow/flow_control.cc b/src/flow/flow_control.cc
index 7abd95ae559da9a151aeec6b79697651e97c5589..f51858faf88e35c6c094857c0180d72eb689be8b 100644 (file)
--- a/src/flow/flow_control.cc
+++ b/src/flow/flow_control.cc
@@ -414,7 +414,7 @@ bool FlowControl::process(PktType type, Packet* p, bool* new_flow)
 
     // FIXIT-M refactor to unlink_uni immediately after session
     // is processed by inspector manager (all flows)
-    if ( flow->next && is_bidirectional(flow) )
+    if ( is_bidirectional(flow) )
         cache->unlink_uni(flow);
 
     return true;

diff --git a/src/flow/flow_uni_list.h b/src/flow/flow_uni_list.h
index 1c29e02f8a255f3720bf55cce77937e4d72b705d..a566e5305a87f847cea27d5023c3bd89eee3b371 100644 (file)
--- a/src/flow/flow_uni_list.h
+++ b/src/flow/flow_uni_list.h
@@ -52,15 +52,16 @@ public:
         ++count;
     }
 
-    void unlink_uni(snort::Flow* flow)
+    bool unlink_uni(snort::Flow* flow)
     {
         if ( !flow->next )
-            return;
+            return false;
 
         flow->next->prev = flow->prev;
         flow->prev->next = flow->next;
         flow->next = flow->prev = nullptr;
         --count;
+        return true;
     }
 
     snort::Flow* get_oldest_uni()

diff --git a/src/flow/test/flow_cache_test.cc b/src/flow/test/flow_cache_test.cc
index 9a74618ca0b6d328a2290c154716071af1c051be..080faaa5b1d99ef5d0dad966b735eb1397d5da8a 100644 (file)
--- a/src/flow/test/flow_cache_test.cc
+++ b/src/flow/test/flow_cache_test.cc
@@ -28,6 +28,7 @@
 
 #include "detection/detection_engine.h"
 #include "main/snort_config.h"
+#include "main/snort_debug.h"
 #include "managers/inspector_manager.h"
 #include "memory/memory_cap.h"
 #include "packet_io/active.h"
@@ -53,6 +54,7 @@ THREAD_LOCAL bool Active::s_suspend = false;
 THREAD_LOCAL Active::ActiveSuspendReason Active::s_suspend_reason = Active::ASP_NONE;
 
 THREAD_LOCAL PacketTracer* snort::s_pkt_trace = nullptr;
+THREAD_LOCAL const Trace* stream_trace = nullptr;
 
 void Active::drop_packet(snort::Packet const*, bool) { }
 PacketTracer::~PacketTracer() = default;
@@ -93,6 +95,9 @@ bool ExpectCache::is_expected(Packet*) { return true; }
 Flow* HighAvailabilityManager::import(Packet&, FlowKey&) { return nullptr; }
 bool HighAvailabilityManager::in_standby(Flow*) { return true; }
 SfIpRet SfIp::set(void const*, int) { return SFIP_SUCCESS; }
+void snort::trace_vprintf(const char*, TraceLevel, const char*, const Packet*, const char*, va_list) {}
+uint8_t snort::TraceApi::get_constraints_generation() { return 0; }
+void snort::TraceApi::filter(const Packet&) {}
 namespace memory
 {
 void MemoryCap::update_allocations(size_t) { }

diff --git a/src/stream/base/stream_module.cc b/src/stream/base/stream_module.cc
index 7895e88658ea03c762e3009027c65a75b8501497..72da087b949180151d6f205cf37dbc52b2c296d9 100644 (file)
--- a/src/stream/base/stream_module.cc
+++ b/src/stream/base/stream_module.cc
@@ -36,6 +36,15 @@
 using namespace snort;
 using namespace std;
 
+#ifdef DEBUG_MSGS
+static const TraceOption stream_trace_options[] =
+{
+    { "base", TRACE_BASE, "enable base stream trace logging" },
+    { "flow", TRACE_FLOW, "enable flow trace logging" },
+    { nullptr, 0, nullptr }
+};
+#endif
+
 THREAD_LOCAL const Trace* stream_trace = nullptr;
 static THREAD_LOCAL timeval reload_time { };
 
@@ -119,8 +128,7 @@ const TraceOption* StreamModule::get_trace_options() const
 #ifndef DEBUG_MSGS
     return nullptr;
 #else
-    static const TraceOption stream_trace_options(nullptr, 0, nullptr);
-    return &stream_trace_options;
+    return stream_trace_options;
 #endif
 }
 

diff --git a/src/stream/base/stream_module.h b/src/stream/base/stream_module.h
index 4f4c1de4b5e4dff925d9dc8971b9b1e9daff2c04..5b5c66031e723325040e207738afcaaa61fae67a 100644 (file)
--- a/src/stream/base/stream_module.h
+++ b/src/stream/base/stream_module.h
@@ -37,6 +37,14 @@ extern THREAD_LOCAL snort::ProfileStats s5PerfStats;
 extern THREAD_LOCAL class FlowControl* flow_con;
 extern THREAD_LOCAL const snort::Trace* stream_trace;
 
+#ifdef DEBUG_MSGS
+enum
+{
+    TRACE_BASE = 0,
+    TRACE_FLOW
+};
+#endif
+
 //-------------------------------------------------------------------------
 // stream module
 //-------------------------------------------------------------------------

diff --git a/src/stream/stream.cc b/src/stream/stream.cc
index db0f0633b7c2f61ab1da4462a8895423afa1f7b8..70e715500b3b5b0cc760bf9195f22a95d126b3bc 100644 (file)
--- a/src/stream/stream.cc
+++ b/src/stream/stream.cc
@@ -252,7 +252,7 @@ void Stream::stop_inspection(
 {
     assert(flow && flow->session);
 
-    debug_logf(stream_trace, p, "stop inspection on flow, dir %s \n",
+    debug_logf(stream_trace, TRACE_BASE, p, "stop inspection on flow, dir %s \n",
         dir == SSN_DIR_BOTH ? "BOTH" :
         ((dir == SSN_DIR_FROM_CLIENT) ? "FROM_CLIENT" : "FROM_SERVER"));