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 8e0abfa7c6781ccda72110195be5965e5487dfee..0f7a86a9cf8d5d3d9f13ecbf4cbc2233e7ab672f 100644 (file)
--- 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 15e514aaafc55497d47ce4f91d7254e1819c5933..fd82e35234df0c0c5def49484eb3dbbdd00cdf8d 100644 (file)
--- 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 9a554f51db9f8f4dc25b8a04957e1735b2244e2b..b70a30cf9a1da41714bdfa7a5abb60a7cc6c9601 100644 (file)
--- 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 59c44ca17266da70615f44ff6ca9e031542ee1dd..607168fe2a7d3b2c396095c823365a60cc2f225d 100644 (file)
--- 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 60b081e5e6dc61e6b13d9ce0bc1709fa0d4d86cc..119472b60d603efef5a7ef618be8547472992c7c 100644 (file)
--- 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 fdda2da4b4c9b20181ac0df46d09e84f71f81545..e58441df586923d2ab082c22aa474e010a0dcce1 100644 (file)
--- 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();