Pull request #4194: memory: prevent data race between main and packet threads

Merge in SNORT/snort3 from ~YCHALOV/snort3:memory_cap_data_race_fix to master

Squashed commit of the following:

commit ef724cb45bb450574339403684605444afa2e61b
Author: Yurii Chalov <ychalov@cisco.com>
Date:   Thu Feb 1 23:15:02 2024 +0100

    memory: prevent data race between main and packet threads

diff --git a/src/main/analyzer_command.cc b/src/main/analyzer_command.cc
index 7a8389ed14ee53fdcd95e1af4adc5e56e6566260..0c594b39d5bd9edced3e52a54e807cd87e3c932e 100644 (file)
--- a/src/main/analyzer_command.cc
+++ b/src/main/analyzer_command.cc
@@ -119,7 +119,7 @@ bool ACGetStats::execute(Analyzer&, void**)
 
 ACGetStats::~ACGetStats()
 {
-
+    ModuleManager::accumulate_module("memory");
     // FIXIT-L This should track the owner so it can dump stats to the
     // shell instead of the logs when initiated by a shell command
     DropStats(ctrlcon);
@@ -137,6 +137,12 @@ bool ACResetStats::execute(Analyzer&, void**)
 ACResetStats::ACResetStats(clear_counter_type_t requested_type_l) : requested_type(
         requested_type_l) { }
 
+ACResetStats::~ACResetStats()
+{
+    if (requested_type == TYPE_MODULE or requested_type == TYPE_ALL)
+        ModuleManager::reset_module_stats("memory");
+}
+
 bool ACSwap::execute(Analyzer& analyzer, void** ac_state)
 {
     if (analyzer.get_state() != Analyzer::State::PAUSED and

diff --git a/src/main/analyzer_command.h b/src/main/analyzer_command.h
index a1226cd0c6df85eebaa6b2c1e32ff4b7220616ae..40f4317d6989ae4b7434863168111615239da23d 100644 (file)
--- a/src/main/analyzer_command.h
+++ b/src/main/analyzer_command.h
@@ -103,6 +103,7 @@ public:
     explicit ACResetStats(clear_counter_type_t requested_type);
     bool execute(Analyzer&, void**) override;
     const char* stringify() override { return "RESET_STATS"; }
+    ~ACResetStats() override;
 private:
     clear_counter_type_t requested_type;
 };

diff --git a/src/managers/module_manager.cc b/src/managers/module_manager.cc
index b6f44145154fa69a435f999a9e101bca3778895c..95127e5a33b0aec7587046f2811184f2e0618fee 100644 (file)
--- a/src/managers/module_manager.cc
+++ b/src/managers/module_manager.cc
@@ -1491,6 +1491,16 @@ void ModuleManager::reset_stats(SnortConfig*)
     }
 }
 
+void ModuleManager::reset_module_stats(const char* name)
+{
+    ModHook* mh = get_hook(name);
+    if ( mh )
+    {
+        lock_guard<mutex> lock(stats_mutex);
+        mh->mod->reset_stats();
+    }
+}
+
 void ModuleManager::clear_global_active_counters()
 {
     auto mod_hooks = get_all_modhooks();

diff --git a/src/managers/module_manager.h b/src/managers/module_manager.h
index 9b2fb9be2ae1492b9ba9a3c5377fe12170fcb3f6..5be30b73ceadc92d055925ca293367e93b093620 100644 (file)
--- a/src/managers/module_manager.h
+++ b/src/managers/module_manager.h
@@ -93,6 +93,7 @@ public:
 
     static void reset_stats(SnortConfig*);
     static void reset_stats(clear_counter_type_t);
+    static void reset_module_stats(const char* name);
 
     static void clear_global_active_counters();
     static bool is_parallel_cmd(std::string control_cmd);

diff --git a/src/memory/memory_cap.cc b/src/memory/memory_cap.cc
index 85b8ab06558eb5c6450283434619728687342231..2254999941fc2379c40fc18a57b04899cf577516 100644 (file)
--- a/src/memory/memory_cap.cc
+++ b/src/memory/memory_cap.cc
@@ -34,6 +34,7 @@
 #include "main/snort_config.h"
 #include "main/snort_types.h"
 #include "main/thread.h"
+#include "managers/module_manager.h"
 #include "time/periodic.h"
 #include "trace/trace_api.h"
 #include "utils/stats.h"
@@ -81,7 +82,18 @@ static void epoch_check(void*)
     else
         current_epoch = 0;
 
+#ifndef REG_TEST
     MemoryCounts& mc = MemoryCap::get_mem_stats();
+#else
+    auto orig_type = get_thread_type();
+
+    // Due to call of epoch_check in first pthread
+    // for build with REG_TEST, we need make that
+    // pthread think that we're main thread
+    set_thread_type(STHREAD_TYPE_MAIN);
+    MemoryCounts& mc = MemoryCap::get_mem_stats();
+    set_thread_type(orig_type);
+#endif
 
     if ( total > mc.max_in_use )
         mc.max_in_use = total;
@@ -143,7 +155,7 @@ void MemoryCap::test_main_check()
 void MemoryCap::init(unsigned n)
 {
     assert(in_main_thread());
-    pkt_mem_stats.resize(n);
+    pkt_mem_stats.resize(n + 1);
 
 #ifdef UNIT_TEST
     pkt_mem_stats[0] = { };
@@ -216,7 +228,7 @@ MemoryCounts& MemoryCap::get_mem_stats()
     if ( in_main_thread() )
         return pkt_mem_stats[0];
 
-    auto id = get_instance_id();
+    auto id = get_instance_id() + 1;
     return pkt_mem_stats[id];
 }
 
@@ -302,21 +314,10 @@ void MemoryCap::free_space()
 
 // required to capture any update in final epoch
 // which happens after packet threads have stopped
-void MemoryCap::update_pegs(PegCount* pc)
+void MemoryCap::update_pegs()
 {
-    MemoryCounts* mp = (MemoryCounts*)pc;
-    const MemoryCounts& mc = get_mem_stats();
-
     if ( config.enabled )
-    {
-        mp->epochs = mc.epochs;
-        mp->cur_in_use = mc.cur_in_use;
-    }
-    else
-    {
-        mp->allocated = 0;
-        mp->deallocated = 0;
-    }
+        ModuleManager::accumulate_module("memory");
 }
 
 // called at startup and shutdown

diff --git a/src/memory/memory_cap.h b/src/memory/memory_cap.h
index a511cafb33f63ca84acedb94d64f88c377772f73..590267e88d0359979d4f774ff589fe3f627da221 100644 (file)
--- a/src/memory/memory_cap.h
+++ b/src/memory/memory_cap.h
@@ -82,7 +82,7 @@ public:
     static MemoryCounts& get_mem_stats();
 
     // main thread - shutdown
-    static void update_pegs(PegCount*);
+    static void update_pegs();
 
     static void dump_mem_stats(ControlConn*);
 #if defined(REG_TEST) || defined(UNIT_TEST)

diff --git a/src/memory/test/memory_cap_test.cc b/src/memory/test/memory_cap_test.cc
index 436349b8eed2f9de86440d3ff1bf2808338281b7..403cb39ea3472c22fd88ff527f1e176bce370f7c 100644 (file)
--- a/src/memory/test/memory_cap_test.cc
+++ b/src/memory/test/memory_cap_test.cc
@@ -26,6 +26,7 @@
 
 #include "log/messages.h"
 #include "main/thread.h"
+#include "managers/module_manager.h"
 #include "profiler/profiler.h"
 #include "time/periodic.h"
 #include "trace/trace_api.h"
@@ -67,6 +68,8 @@ void Periodic::register_handler(PeriodicHook, void*, uint16_t, uint32_t) { }
 
 void Profiler::register_module(const char*, const char*, snort::Module*) { }
 
+void ModuleManager::accumulate_module(const char*) { }
+
 //--------------------------------------------------------------------------
 // mocks
 //--------------------------------------------------------------------------
@@ -122,6 +125,9 @@ SThreadType get_thread_type()
 
 }
 
+void set_thread_type(SThreadType)
+{ pkt_thread = false; }
+
 static void free_space()
 {
     pkt_thread = true;
@@ -129,6 +135,19 @@ static void free_space()
     pkt_thread = false;
 }
 
+static MemoryCounts& get_main_stats()
+{
+    return MemoryCap::get_mem_stats();
+}
+
+static MemoryCounts& get_pkt_stats()
+{
+    pkt_thread = true;
+    MemoryCounts& res = MemoryCap::get_mem_stats();
+    pkt_thread = false;
+    return res;
+}
+
 //--------------------------------------------------------------------------
 // tests
 //--------------------------------------------------------------------------
@@ -242,7 +261,8 @@ TEST(memory, prune1)
     MemoryCap::start(config, pruner);
     MemoryCap::thread_init();
 
-    const MemoryCounts& mc = MemoryCap::get_mem_stats();
+    const MemoryCounts& mc = get_main_stats();
+    const MemoryCounts& pc = get_pkt_stats();
     CHECK(mc.start_up_use == start);
 
     fd.flows = 3;
@@ -259,11 +279,11 @@ TEST(memory, prune1)
 
     free_space(); // finish pruning
     UNSIGNED_LONGS_EQUAL(1, fd.flows);
-    UNSIGNED_LONGS_EQUAL(2, mc.reap_decrease);
+    UNSIGNED_LONGS_EQUAL(2, pc.reap_decrease);
 
     free_space();
     UNSIGNED_LONGS_EQUAL(1, fd.flows);
-    UNSIGNED_LONGS_EQUAL(2, mc.reap_decrease);
+    UNSIGNED_LONGS_EQUAL(2, pc.reap_decrease);
 
     periodic_check(); // still over the limit
     CHECK(heap->epoch == 3);
@@ -277,7 +297,7 @@ TEST(memory, prune1)
 
     free_space(); // abort
     UNSIGNED_LONGS_EQUAL(0, fd.flows);
-    UNSIGNED_LONGS_EQUAL(3, mc.reap_decrease);
+    UNSIGNED_LONGS_EQUAL(3, pc.reap_decrease);
 
     heap->total = cap + 1; // over the limit
     periodic_check();
@@ -295,8 +315,8 @@ TEST(memory, prune1)
 
     free_space(); // abort, reap_increase update
     UNSIGNED_LONGS_EQUAL(0, fd.flows);
-    UNSIGNED_LONGS_EQUAL(3, mc.reap_decrease);
-    UNSIGNED_LONGS_EQUAL(4, mc.reap_increase);
+    UNSIGNED_LONGS_EQUAL(3, pc.reap_decrease);
+    UNSIGNED_LONGS_EQUAL(4, pc.reap_increase);
 
     heap->total = cap + 1; // over the limit
     periodic_check();
@@ -314,21 +334,21 @@ TEST(memory, prune1)
 
     free_space(); // abort, reap_increase update
     UNSIGNED_LONGS_EQUAL(0, fd.flows);
-    UNSIGNED_LONGS_EQUAL(3, mc.reap_decrease);
-    UNSIGNED_LONGS_EQUAL(6, mc.reap_increase);
+    UNSIGNED_LONGS_EQUAL(3, pc.reap_decrease);
+    UNSIGNED_LONGS_EQUAL(6, pc.reap_increase);
 
     UNSIGNED_LONGS_EQUAL(start, mc.start_up_use);
     UNSIGNED_LONGS_EQUAL(cap + 1, mc.max_in_use);
     UNSIGNED_LONGS_EQUAL(cap, mc.cur_in_use);
 
     UNSIGNED_LONGS_EQUAL(heap->epoch, mc.epochs);
-    UNSIGNED_LONGS_EQUAL(heap->alloc, mc.allocated);
-    UNSIGNED_LONGS_EQUAL(heap->dealloc, mc.deallocated);
+    UNSIGNED_LONGS_EQUAL(heap->alloc, pc.allocated);
+    UNSIGNED_LONGS_EQUAL(heap->dealloc, pc.deallocated);
 
-    UNSIGNED_LONGS_EQUAL(4, mc.reap_cycles);
-    UNSIGNED_LONGS_EQUAL(5, mc.reap_attempts);
-    UNSIGNED_LONGS_EQUAL(0, mc.reap_failures);
-    UNSIGNED_LONGS_EQUAL(3, mc.reap_aborts);
+    UNSIGNED_LONGS_EQUAL(4, pc.reap_cycles);
+    UNSIGNED_LONGS_EQUAL(5, pc.reap_attempts);
+    UNSIGNED_LONGS_EQUAL(0, pc.reap_failures);
+    UNSIGNED_LONGS_EQUAL(3, pc.reap_aborts);
 
     heap->total = start;
     MemoryCap::stop();
@@ -368,12 +388,12 @@ TEST(memory, prune3)
     free_space();
     CHECK(fd.flows == 0);
 
-    const MemoryCounts& mc = MemoryCap::get_mem_stats();
-    UNSIGNED_LONGS_EQUAL(1, mc.reap_cycles);
-    UNSIGNED_LONGS_EQUAL(3, mc.reap_attempts);
-    UNSIGNED_LONGS_EQUAL(0, mc.reap_failures);
-    UNSIGNED_LONGS_EQUAL(1, mc.reap_decrease);
-    UNSIGNED_LONGS_EQUAL(0, mc.reap_increase);
+    const MemoryCounts& pc = get_pkt_stats();
+    UNSIGNED_LONGS_EQUAL(1, pc.reap_cycles);
+    UNSIGNED_LONGS_EQUAL(3, pc.reap_attempts);
+    UNSIGNED_LONGS_EQUAL(0, pc.reap_failures);
+    UNSIGNED_LONGS_EQUAL(1, pc.reap_decrease);
+    UNSIGNED_LONGS_EQUAL(0, pc.reap_increase);
 
     MemoryCap::stop();
 }
@@ -410,12 +430,12 @@ TEST(memory, two_cycles)
     free_space();
     CHECK(fd.flows == 1);
 
-    const MemoryCounts& mc = MemoryCap::get_mem_stats();
-    UNSIGNED_LONGS_EQUAL(2, mc.reap_cycles);
-    UNSIGNED_LONGS_EQUAL(2, mc.reap_attempts);
-    UNSIGNED_LONGS_EQUAL(0, mc.reap_failures);
-    UNSIGNED_LONGS_EQUAL(11, mc.reap_decrease);
-    UNSIGNED_LONGS_EQUAL(0, mc.reap_increase);
+    const MemoryCounts& pc = get_pkt_stats();
+    UNSIGNED_LONGS_EQUAL(2, pc.reap_cycles);
+    UNSIGNED_LONGS_EQUAL(2, pc.reap_attempts);
+    UNSIGNED_LONGS_EQUAL(0, pc.reap_failures);
+    UNSIGNED_LONGS_EQUAL(11, pc.reap_decrease);
+    UNSIGNED_LONGS_EQUAL(0, pc.reap_increase);
 
     MemoryCap::stop();
 }
@@ -430,7 +450,7 @@ TEST(memory, reap_failure)
     MemoryCap::start(config, pruner);
     MemoryCap::thread_init();
 
-    const MemoryCounts& mc = MemoryCap::get_mem_stats();
+    const MemoryCounts& pc = get_pkt_stats();
 
     fd.flows = 1;
     heap->total = cap + 1;
@@ -441,7 +461,7 @@ TEST(memory, reap_failure)
 
     free_space(); // reap failure
     CHECK(fd.flows == -1);
-    UNSIGNED_LONGS_EQUAL(1, mc.reap_decrease);
+    UNSIGNED_LONGS_EQUAL(1, pc.reap_decrease);
 
     fd.flows = 1;
     heap->total = cap + 1;
@@ -454,8 +474,8 @@ TEST(memory, reap_failure)
 
     free_space(); // reap failure, reap_increase update
     CHECK(fd.flows == -1);
-    UNSIGNED_LONGS_EQUAL(1, mc.reap_decrease);
-    UNSIGNED_LONGS_EQUAL(2, mc.reap_increase);
+    UNSIGNED_LONGS_EQUAL(1, pc.reap_decrease);
+    UNSIGNED_LONGS_EQUAL(2, pc.reap_increase);
 
     fd.flows = 1;
     heap->total = cap + 1;
@@ -468,12 +488,12 @@ TEST(memory, reap_failure)
 
     free_space(); // reap failure, reap_increase update
     CHECK(fd.flows == -1);
-    UNSIGNED_LONGS_EQUAL(1, mc.reap_decrease);
-    UNSIGNED_LONGS_EQUAL(3, mc.reap_increase);
+    UNSIGNED_LONGS_EQUAL(1, pc.reap_decrease);
+    UNSIGNED_LONGS_EQUAL(3, pc.reap_increase);
 
-    UNSIGNED_LONGS_EQUAL(3, mc.reap_cycles);
-    UNSIGNED_LONGS_EQUAL(6, mc.reap_attempts);
-    UNSIGNED_LONGS_EQUAL(3, mc.reap_failures);
+    UNSIGNED_LONGS_EQUAL(3, pc.reap_cycles);
+    UNSIGNED_LONGS_EQUAL(6, pc.reap_attempts);
+    UNSIGNED_LONGS_EQUAL(3, pc.reap_failures);
 
     MemoryCap::stop();
 }
@@ -507,12 +527,12 @@ TEST(memory, reap_freed_outside_of_pruning)
     free_space();
     UNSIGNED_LONGS_EQUAL(0, fd.flows);
 
-    const MemoryCounts& mc = MemoryCap::get_mem_stats();
-    UNSIGNED_LONGS_EQUAL(1, mc.reap_cycles);
-    UNSIGNED_LONGS_EQUAL(2, mc.reap_attempts);
-    UNSIGNED_LONGS_EQUAL(0, mc.reap_failures);
-    UNSIGNED_LONGS_EQUAL(2, mc.reap_decrease);
-    UNSIGNED_LONGS_EQUAL(0, mc.reap_increase);
+    const MemoryCounts& pc = get_pkt_stats();
+    UNSIGNED_LONGS_EQUAL(1, pc.reap_cycles);
+    UNSIGNED_LONGS_EQUAL(2, pc.reap_attempts);
+    UNSIGNED_LONGS_EQUAL(0, pc.reap_failures);
+    UNSIGNED_LONGS_EQUAL(2, pc.reap_decrease);
+    UNSIGNED_LONGS_EQUAL(0, pc.reap_increase);
 
     MemoryCap::stop();
 }

diff --git a/src/utils/stats.cc b/src/utils/stats.cc
index 3a915ae05fca836655a53ae857fdeda62fd1f59d..b2f139d6a83ad8d22a071ee300fe9a8f0d6c349b 100644 (file)
--- a/src/utils/stats.cc
+++ b/src/utils/stats.cc
@@ -284,8 +284,8 @@ void DropStats(ControlConn* ctrlcon)
 
 void PrintStatistics()
 {
-    if ( PegCount* pc = ModuleManager::get_stats("memory") )
-        memory::MemoryCap::update_pegs(pc);
+    if ( ModuleManager::get_stats("memory") )
+        memory::MemoryCap::update_pegs();
 
     DropStats();
     timing_stats();