From: Yurii Chalov -X (ychalov - SOFTSERVE INC at Cisco) Date: Fri, 16 Feb 2024 16:35:23 +0000 (+0000) Subject: Pull request #4194: memory: prevent data race between main and packet threads X-Git-Tag: 3.1.81.0~3 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=37bcc63e957bff0ef7103363126a4df8e3259626;p=thirdparty%2Fsnort3.git 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 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 7a8389ed1..0c594b39d 100644 --- 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 a1226cd0c..40f4317d6 100644 --- 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 b6f441451..95127e5a3 100644 --- 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 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 9b2fb9be2..5be30b73c 100644 --- 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 85b8ab065..225499994 100644 --- 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 a511cafb3..590267e88 100644 --- 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 436349b8e..403cb39ea 100644 --- 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 3a915ae05..b2f139d6a 100644 --- 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();