Pull request #5178: file_api: fix tsan data races in circular buffer, file cache...

Merge in SNORT/snort3 from ~LBEVINAM/snort3:tsan/file-api to master

Squashed commit of the following:

commit d473dcabf7c244f34a2c667027038f815f2170f4
Author: Lokesh Bevinamarad <lbevinam@cisco.com>
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 64572dfdf07ff49253bb7e7ed5a3603fa95da279..015eb7423b52d838a3cc98bbc14948956e28b726 100644 (file)
--- a/src/file_api/circular_buffer.cc
+++ b/src/file_api/circular_buffer.cc
@@ -36,15 +36,17 @@
 
 #include "circular_buffer.h"
 
+#include <atomic>
+
 #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<uint64_t> start;   /* index of oldest element, reader update only */
+    std::atomic<uint64_t> 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 f7a861c88b4bb3e3c8645b000aedd56eabbfeff9..a4f5d620e09f7c4a7bcedc08d673121548b50099 100644 (file)
--- 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<std::mutex> 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<std::mutex> 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<time_t>(lookup_timeout), 0 };
+                add_time = { static_cast<time_t>(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 fce6104546a445cf520ed13e38a7d0603b564703..b6fd504f0903254048b01e4d5f32cb31cef3ca43 100644 (file)
--- 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<int64_t> block_timeout{DEFAULT_FILE_BLOCK_TIMEOUT};
+    std::atomic<int64_t> lookup_timeout{DEFAULT_FILE_LOOKUP_TIMEOUT};
     int64_t max_files = DEFAULT_MAX_FILES_CACHED;
-    std::atomic<int64_t> 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 f495c664bae314163d2f8a93c6be3568ed87d8ea..f3ffb5152f1f93cafe2aa8a569aa336707184b51 100644 (file)
--- 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 4397495d0a604a6e8823051755ba3717c1f9adcd..8d0409e950a16c711c9a31ad001c99f1c5ef5228 100644 (file)
--- 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<FileRule> file_rules;
     std::map<std::string, FileVerdict> file_shas;
+    FileRule default_rule;
     bool type_enabled = false;
     bool signature_enabled = false;
     bool capture_enabled = false;