From: Mike Stepanek (mstepane) <mstepane@cisco.com>
Date: Wed, 1 Apr 2020 16:52:05 +0000 (+0000)
Subject: Merge pull request #2116 in SNORT/snort3 from ~MASHASAN/snort3:request_race to master
X-Git-Tag: 3.0.1-2~46
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=ddc86696ae1ff72e0b074b678759894e5122282c;p=thirdparty%2Fsnort3.git

Merge pull request #2116 in SNORT/snort3 from ~MASHASAN/snort3:request_race to master

Squashed commit of the following:

commit 3b0c7cc38f58f0f8fcbf864b5690cc4eb4cf6019
Author: Masud Hasan <mashasan@cisco.com>
Date:   Fri Mar 27 20:16:40 2020 -0400

    control: Fixing data races in request read and response
---

diff --git a/src/host_tracker/host_cache.h b/src/host_tracker/host_cache.h
index 8e0abfa7c..0f7a86a9c 100644
--- a/src/host_tracker/host_cache.h
+++ b/src/host_tracker/host_cache.h
@@ -117,6 +117,8 @@ public:
 
             if ( !list.empty() )
             {
+                // A data race when reload changes max_size while other threads read this may
+                // delay pruning by one round. Yet, we are avoiding mutex for better performance.
                 max_size = current_size;
                 if ( max_size > new_size )
                 {
diff --git a/src/main/control.cc b/src/main/control.cc
index 15e514aaa..fd82e3523 100644
--- a/src/main/control.cc
+++ b/src/main/control.cc
@@ -62,7 +62,7 @@ void ControlConn::configure() const
 
 int ControlConn::shell_execute(int& current_fd, Request*& current_request)
 {
-    if ( !request->read(fd) )
+    if ( !request->read() )
         return -1;
 
     current_fd = fd;
diff --git a/src/main/request.cc b/src/main/request.cc
index 9a554f51d..b70a30cf9 100644
--- a/src/main/request.cc
+++ b/src/main/request.cc
@@ -27,18 +27,18 @@
 #include "utils/util.h"
 
 using namespace snort;
+using namespace std;
 
 //-------------------------------------------------------------------------
 // request foo
 //-------------------------------------------------------------------------
 
-bool Request::read(const int& f)
+bool Request::read()
 {
     bool newline_found = false;
     char buf;
     ssize_t n = 0;
 
-    fd = f;
     while ( (bytes_read < sizeof(read_buf)) and ((n = ::read(fd, &buf, 1)) > 0) )
     {
         read_buf[bytes_read++] = buf;
@@ -91,19 +91,24 @@ void Request::respond(const char* s, bool queue_response, bool remote_only)
 
     if ( queue_response )
     {
-        queued_response = s;
+        lock_guard<mutex> lock(queued_response_mutex);
+        queued_response.emplace(s);
         return;
     }
     write_response(s);
 }
 
 #ifdef SHELL
-void Request::send_queued_response()
+bool Request::send_queued_response()
 {
-    if ( queued_response )
+    const char* qr;
     {
-        write_response(queued_response);
-        queued_response = nullptr;
+        lock_guard<mutex> lock(queued_response_mutex);
+        if ( queued_response.empty() )
+            return false;
+        qr = queued_response.front();
+        queued_response.pop();
     }
+    return write_response(qr);
 }
 #endif
diff --git a/src/main/request.h b/src/main/request.h
index 59c44ca17..607168fe2 100644
--- a/src/main/request.h
+++ b/src/main/request.h
@@ -22,25 +22,29 @@
 #ifndef REQUEST_H
 #define REQUEST_H
 
+#include <mutex>
+#include <queue>
+
 #include "main/snort_types.h"
 
 class Request
 {
 public:
-    Request(int f = -1) : fd(f), bytes_read(0), queued_response(nullptr) { }
+    Request(int f = -1) : fd(f), bytes_read(0) { }
 
-    bool read(const int&);
+    bool read();
     const char* get() { return read_buf; }
     bool write_response(const char* s) const;
     void respond(const char* s, bool queue_response = false, bool remote_only = false);
 #ifdef SHELL
-    void send_queued_response();
+    bool send_queued_response();
 #endif
 
 private:
     int fd;
     char read_buf[1024];
     size_t bytes_read;
-    const char* queued_response;
+    std::queue<const char*> queued_response;
+    std::mutex queued_response_mutex;
 };
 #endif
diff --git a/src/main/test/request_test.cc b/src/main/test/request_test.cc
index 60b081e5e..119472b60 100644
--- a/src/main/test/request_test.cc
+++ b/src/main/test/request_test.cc
@@ -21,6 +21,8 @@
 #include "config.h"
 #endif
 
+#include <unistd.h>
+
 #include "main/request.h"
 
 #include <CppUTest/CommandLineTestRunner.h>
@@ -41,19 +43,18 @@ TEST_GROUP(request_tests)
 {};
 
 //--------------------------------------------------------------------------
-// Make sure request->read does not modify value of passed-in fd
+// Make sure multiple responses are queued
 //--------------------------------------------------------------------------
-TEST(request_tests, request_read_fail_test)
+TEST(request_tests, queued_response_test)
 {
-    const int fd_orig_val = 10;
-    int current_fd = fd_orig_val;
-
-    Request *request = new Request(current_fd);
-
-    CHECK(request->read(current_fd) == false);
-    CHECK(current_fd == fd_orig_val);
+    Request request(STDOUT_FILENO);
 
-    delete request;
+    CHECK(request.send_queued_response() == false); // empty queue
+    request.respond("reloading", true);
+    request.respond("swapping", true);
+    CHECK(request.send_queued_response() == true);
+    CHECK(request.send_queued_response() == true);
+    CHECK(request.send_queued_response() == false); // empty queue after being written
 }
 
 //-------------------------------------------------------------------------
diff --git a/src/main/thread_config.cc b/src/main/thread_config.cc
index fdda2da4b..e58441df5 100644
--- a/src/main/thread_config.cc
+++ b/src/main/thread_config.cc
@@ -155,6 +155,9 @@ void ThreadConfig::implement_thread_affinity(SThreadType type, unsigned id)
         desired_cpuset = iter->second->cpuset;
     else
         desired_cpuset = process_cpuset;
+
+    // A data race in this library function ensues from the usage of a static variable
+    // to dump chars when calculating string length. This does not affect functionality.
     hwloc_bitmap_list_asprintf(&s, desired_cpuset);
 
     current_cpuset = hwloc_bitmap_alloc();