From: Lokesh Bevinamarad (lbevinam) Date: Mon, 16 Mar 2026 13:14:07 +0000 (+0000) Subject: Pull request #5178: file_api: fix tsan data races in circular buffer, file cache... X-Git-Tag: 3.12.1.0~15 X-Git-Url: http://git.ipfire.org/gitweb/index.cgi?a=commitdiff_plain;h=5b5f57cc7e9af7ff0c3544d7e907ebd0166cb027;p=thirdparty%2Fsnort3.git Pull request #5178: file_api: fix tsan data races in circular buffer, file cache, and file policy Merge in SNORT/snort3 from ~LBEVINAM/snort3:tsan/file-api to master Squashed commit of the following: commit d473dcabf7c244f34a2c667027038f815f2170f4 Author: Lokesh Bevinamarad Date: Thu Feb 26 05:53:49 2026 -0500 file_api: fix tsan datarace in circular buffer, file cache and file policy --- diff --git a/src/file_api/circular_buffer.cc b/src/file_api/circular_buffer.cc index 64572dfdf..015eb7423 100644 --- a/src/file_api/circular_buffer.cc +++ b/src/file_api/circular_buffer.cc @@ -36,15 +36,17 @@ #include "circular_buffer.h" +#include + #include "utils/util.h" /* Circular buffer object */ struct _CircularBuffer { - uint64_t size; /* maximum number of elements */ - uint64_t start; /* index of oldest element, reader update only */ - uint64_t end; /* index to write new element, writer update only*/ - ElemType* elems; /* vector of elements */ + uint64_t size; /* maximum number of elements */ + std::atomic start; /* index of oldest element, reader update only */ + std::atomic end; /* index to write new element, writer update only*/ + ElemType* elems; /* vector of elements */ }; /* This approach adds one byte to end and start pointers */ @@ -54,6 +56,8 @@ CircularBuffer* cbuffer_init(uint64_t size) CircularBuffer* cb = (CircularBuffer*)snort_calloc(sizeof(*cb)); cb->size = size + 1; + cb->start.store(0, std::memory_order_relaxed); + cb->end.store(0, std::memory_order_relaxed); cb->elems = (ElemType*)snort_calloc(cb->size, sizeof(ElemType)); if (!cb->elems) @@ -78,32 +82,30 @@ void cbuffer_free(CircularBuffer* cb) int cbuffer_is_full(CircularBuffer* cb) { - uint64_t next = cb->end + 1; + uint64_t next = cb->end.load(std::memory_order_relaxed) + 1; if ( next == cb->size ) next = 0; - return (next == cb->start); + return (next == cb->start.load(std::memory_order_acquire)); } int cbuffer_is_empty(CircularBuffer* cb) { - return (cb->end == cb->start); + return (cb->end.load(std::memory_order_acquire) == cb->start.load(std::memory_order_relaxed)); } /* Returns number of elements in use*/ uint64_t cbuffer_used(CircularBuffer* cb) { - /* cb->end < cb->start means passing the end of buffer */ - if (cb->end < cb->start) - { - return (cb->size + cb->end - cb->start); - } + uint64_t end = cb->end.load(std::memory_order_acquire); + uint64_t start = cb->start.load(std::memory_order_acquire); + + if (end < start) + return (cb->size + end - start); else - { - return (cb->end - cb->start); - } + return (end - start); } /* Returns total number of elements*/ @@ -124,7 +126,7 @@ uint64_t cbuffer_size(CircularBuffer* cb) */ int cbuffer_write(CircularBuffer* cb, const ElemType elem) { - uint64_t w = cb->end; + uint64_t w = cb->end.load(std::memory_order_relaxed); if ( cbuffer_is_full (cb)) /* full, return error */ { @@ -135,7 +137,7 @@ int cbuffer_write(CircularBuffer* cb, const ElemType elem) if ( w == cb->size ) w = 0; - cb->end = w; + cb->end.store(w, std::memory_order_release); return CB_SUCCESS; } @@ -152,7 +154,7 @@ int cbuffer_write(CircularBuffer* cb, const ElemType elem) */ int cbuffer_read(CircularBuffer* cb, ElemType* elem) { - uint64_t r = cb->start; + uint64_t r = cb->start.load(std::memory_order_relaxed); if (cbuffer_is_empty(cb)) /* Empty, return error */ { @@ -163,7 +165,7 @@ int cbuffer_read(CircularBuffer* cb, ElemType* elem) if ( r == cb->size ) r = 0; - cb->start = r; + cb->start.store(r, std::memory_order_release); return CB_SUCCESS; } diff --git a/src/file_api/file_cache.cc b/src/file_api/file_cache.cc index f7a861c88..a4f5d620e 100644 --- a/src/file_api/file_cache.cc +++ b/src/file_api/file_cache.cc @@ -102,14 +102,12 @@ FileCache::~FileCache() void FileCache::set_block_timeout(int64_t timeout) { - std::lock_guard lock(cache_mutex); - block_timeout = timeout; + block_timeout.store(timeout, std::memory_order_relaxed); } void FileCache::set_lookup_timeout(int64_t timeout) { - std::lock_guard lock(cache_mutex); - lookup_timeout = timeout; + lookup_timeout.store(timeout, std::memory_order_relaxed); } void FileCache::set_max_files(int64_t max) @@ -295,7 +293,8 @@ FileContext* FileCache::get_file(Flow* flow, uint64_t file_id, bool to_create, b { bool cache_full = false; int64_t cache_expire = 0; - return get_file(flow, file_id, to_create, lookup_timeout, using_cache_entry, cache_full, cache_expire); + return get_file(flow, file_id, to_create, lookup_timeout.load(std::memory_order_relaxed), + using_cache_entry, cache_full, cache_expire); } FileVerdict FileCache::check_verdict(Packet* p, FileInfo* file, @@ -454,6 +453,8 @@ bool FileCache::apply_verdict(Packet* p, FileContext* file_ctx, FileVerdict verd file_ctx->stop_file_capture(); return false; case FILE_VERDICT_PENDING: + { + int64_t cur_lookup_timeout = lookup_timeout.load(std::memory_order_relaxed); packet_gettimeofday(&now); if (timerisset(&file_ctx->pending_expire_time) and @@ -486,11 +487,11 @@ bool FileCache::apply_verdict(Packet* p, FileContext* file_ctx, FileVerdict verd FILE_DEBUG(file_trace, DEFAULT_TRACE_OPTION_ID, TRACE_INFO_LEVEL, p, "File signature lookup: timed out after %" PRIi64 "ms.\n", - time_elapsed_ms(&now, &file_ctx->pending_expire_time, lookup_timeout)); + time_elapsed_ms(&now, &file_ctx->pending_expire_time, cur_lookup_timeout)); if (PacketTracer::is_active()) { - PacketTracer::log("File signature lookup: timed out after %" PRIi64 "ms.\n", time_elapsed_ms(&now, &file_ctx->pending_expire_time, lookup_timeout)); + PacketTracer::log("File signature lookup: timed out after %" PRIi64 "ms.\n", time_elapsed_ms(&now, &file_ctx->pending_expire_time, cur_lookup_timeout)); } } else @@ -499,7 +500,7 @@ bool FileCache::apply_verdict(Packet* p, FileContext* file_ctx, FileVerdict verd if (!timerisset(&file_ctx->pending_expire_time)) { - add_time = { static_cast(lookup_timeout), 0 }; + add_time = { static_cast(cur_lookup_timeout), 0 }; timeradd(&now, &add_time, &file_ctx->pending_expire_time); FILE_DEBUG(file_trace, DEFAULT_TRACE_OPTION_ID, TRACE_INFO_LEVEL, p, @@ -515,12 +516,12 @@ bool FileCache::apply_verdict(Packet* p, FileContext* file_ctx, FileVerdict verd // be there. if (!(p->packet_flags & PKT_RETRANSMIT) or p->is_retry()) { - PacketTracer::log("File signature lookup: adding packet to retry queue. Resume=%d, Waited %" PRIi64 "ms.\n", resume, time_elapsed_ms(&now, &file_ctx->pending_expire_time, lookup_timeout)); + PacketTracer::log("File signature lookup: adding packet to retry queue. Resume=%d, Waited %" PRIi64 "ms.\n", resume, time_elapsed_ms(&now, &file_ctx->pending_expire_time, cur_lookup_timeout)); FILE_DEBUG(file_trace, DEFAULT_TRACE_OPTION_ID, TRACE_INFO_LEVEL, p, "File signature lookup adding packet to retry queue" "Resume=%d, Waited %" PRIi64 "ms.\n", resume, - time_elapsed_ms(&now, &file_ctx->pending_expire_time, lookup_timeout)); + time_elapsed_ms(&now, &file_ctx->pending_expire_time, cur_lookup_timeout)); } } @@ -535,7 +536,7 @@ bool FileCache::apply_verdict(Packet* p, FileContext* file_ctx, FileVerdict verd if (resume) policy->log_file_action(flow, file_ctx, FILE_RESUME_BLOCK); - else if (store_verdict(flow, file_ctx, lookup_timeout, cache_full, file_ctx->is_cacheable()) != 0) + else if (store_verdict(flow, file_ctx, cur_lookup_timeout, cache_full, file_ctx->is_cacheable()) != 0) { if (cache_full) { @@ -559,6 +560,7 @@ bool FileCache::apply_verdict(Packet* p, FileContext* file_ctx, FileVerdict verd } } return true; + } default: return false; } @@ -572,7 +574,7 @@ bool FileCache::apply_verdict(Packet* p, FileContext* file_ctx, FileVerdict verd } else if (bool is_cacheable = file_ctx->is_cacheable()) { - if (store_verdict(flow, file_ctx, block_timeout, cache_full, is_cacheable) != 0) + if (store_verdict(flow, file_ctx, block_timeout.load(std::memory_order_relaxed), cache_full, is_cacheable) != 0) { if (PacketTracer::is_active()) { diff --git a/src/file_api/file_cache.h b/src/file_api/file_cache.h index fce610454..b6fd504f0 100644 --- a/src/file_api/file_cache.h +++ b/src/file_api/file_cache.h @@ -78,9 +78,9 @@ private: /* The hash table of expected files */ ExpectedFileCache* fileHash = nullptr; - int64_t block_timeout = DEFAULT_FILE_BLOCK_TIMEOUT; + std::atomic block_timeout{DEFAULT_FILE_BLOCK_TIMEOUT}; + std::atomic lookup_timeout{DEFAULT_FILE_LOOKUP_TIMEOUT}; int64_t max_files = DEFAULT_MAX_FILES_CACHED; - std::atomic lookup_timeout = DEFAULT_FILE_LOOKUP_TIMEOUT; std::mutex cache_mutex; }; diff --git a/src/file_api/file_policy.cc b/src/file_api/file_policy.cc index f495c664b..f3ffb5152 100644 --- a/src/file_api/file_policy.cc +++ b/src/file_api/file_policy.cc @@ -31,8 +31,6 @@ using namespace snort; -static FileRule emptyRule; - void FileRule::clear() { when.type_id = 0; @@ -124,10 +122,9 @@ void FilePolicy::load() if (capture_enabled) FileService::enable_file_capture(); - // Use default global setting - emptyRule.use.type_enabled = type_enabled; - emptyRule.use.signature_enabled = signature_enabled; - emptyRule.use.capture_enabled = capture_enabled; + default_rule.use.type_enabled = type_enabled; + default_rule.use.signature_enabled = signature_enabled; + default_rule.use.capture_enabled = capture_enabled; } FileRule& FilePolicy::match_file_rule(Flow*, FileInfo* file) @@ -142,7 +139,7 @@ FileRule& FilePolicy::match_file_rule(Flow*, FileInfo* file) return file_rules[i]; } - return emptyRule; + return default_rule; } FileVerdict FilePolicy::match_file_signature(Flow*, FileInfo* file) diff --git a/src/file_api/file_policy.h b/src/file_api/file_policy.h index 4397495d0..8d0409e95 100644 --- a/src/file_api/file_policy.h +++ b/src/file_api/file_policy.h @@ -84,6 +84,7 @@ private: FileVerdict match_file_signature(snort::Flow*, snort::FileInfo*); std::vector file_rules; std::map file_shas; + FileRule default_rule; bool type_enabled = false; bool signature_enabled = false; bool capture_enabled = false;