Merge pull request #2952 in SNORT/snort3 from ~SBAIGAL/snort3:better to master

Squashed commit of the following:

commit bf82e65e3296202f7d81e1bd14d7447d1baa25c3
Author: Steven Baigal (sbaigal) <sbaigal@cisco.com>
Date:   Wed Jun 23 11:39:45 2021 -0400

    control: resolve socket issues due to race conditions

diff --git a/src/control/control.cc b/src/control/control.cc
index 2a074eebef66feb07a4fc41d69eb0a94a3aeabb1..e706247c7ef470bbd79a9054f29c93314ef0a2ce 100644 (file)
--- a/src/control/control.cc
+++ b/src/control/control.cc
@@ -38,7 +38,7 @@ using namespace snort;
 ControlConn* ControlConn::query_from_lua(const lua_State* L)
 {
 #ifdef SHELL
-    return ControlMgmt::find_control(L);
+    return ( L ? ControlMgmt::find_control(L) : nullptr );
 #else
     UNUSED(L);
     return nullptr;
@@ -64,6 +64,8 @@ ControlConn::~ControlConn()
 
 void ControlConn::shutdown()
 {
+    if (is_closed())
+        return;
     if (!local)
         close(fd);
     fd = -1;
@@ -140,6 +142,11 @@ void ControlConn::block()
     blocked = true;
 }
 
+void ControlConn::remove()
+{
+    removed = true;
+}
+
 void ControlConn::unblock()
 {
     if (blocked)
@@ -185,7 +192,7 @@ bool ControlConn::respond(const char* format, va_list& ap)
 
 bool ControlConn::respond(const char* format, ...)
 {
-    if (is_closed())
+    if (is_closed() or is_removed())
         return false;
 
     va_list ap;

diff --git a/src/control/control.h b/src/control/control.h
index 29333df2d8566fd5db7c1e145ae8e8c11caec33e..446cd193e2729a60433b0c44ea17b03017e1c485 100644 (file)
--- a/src/control/control.h
+++ b/src/control/control.h
@@ -45,9 +45,11 @@ public:
 
     void block();
     void unblock();
+    void remove();
 
     bool is_blocked() const { return blocked; }
     bool is_closed() const { return (fd == -1); }
+    bool is_removed() const { return removed; }
     bool has_pending_command() const { return !pending_commands.empty(); }
 
     void configure() const;
@@ -56,11 +58,11 @@ public:
     void shutdown();
 
     SO_PUBLIC bool is_local() const { return local; }
-    SO_PUBLIC bool respond(const char* format, va_list& ap);
     SO_PUBLIC bool respond(const char* format, ...) __attribute__((format (printf, 2, 3)));
     SO_PUBLIC static ControlConn* query_from_lua(const lua_State*);
 
 private:
+    bool respond(const char* format, va_list& ap);
     bool show_prompt();
 
 private:
@@ -70,6 +72,7 @@ private:
     int fd;
     bool local = false;
     bool blocked = false;
+    bool removed = false;
 };
 
 #endif

diff --git a/src/control/control_mgmt.cc b/src/control/control_mgmt.cc
index eb29f1696d8a79498d9c290307f85c22db9d302d..76d1d6ee03d0318e9d9e5465a0c9a07e8279e7ef 100644 (file)
--- a/src/control/control_mgmt.cc
+++ b/src/control/control_mgmt.cc
@@ -53,6 +53,13 @@ static struct sockaddr_in in_addr;
 static struct sockaddr_un unix_addr;
 static std::unordered_map<int, ControlConn*> controls;
 
+#define READY 1
+#define DEAD  2
+struct FdEvents{
+    int fd;
+    unsigned flag;
+};
+
 #ifdef __linux__
 
 //-------------------------------------------------------------------------
@@ -105,7 +112,7 @@ static void unregister_control_fd(const int, const int curr_fd)
     nfds--;
 }
 
-static bool poll_control_fds(int ready[MAX_CONTROL_FDS], unsigned& nready, int dead[MAX_CONTROL_FDS], unsigned& ndead)
+static bool poll_control_fds(FdEvents ready[MAX_CONTROL_FDS], unsigned& nready)
 {
     if (epoll_fd == -1 || nfds == 0)
         return false;
@@ -118,18 +125,19 @@ static bool poll_control_fds(int ready[MAX_CONTROL_FDS], unsigned& nready, int d
             ErrorMessage("Failed to poll control descriptors: %s\n", get_error(errno));
         return false;
     }
-    nready = ndead = 0;
+    nready = ret;
     for (int i = 0; i < ret; i++)
     {
         struct epoll_event* ev = &events[i];
-        int fd = ev->data.fd;
+        ready[i].fd = ev->data.fd;
+        ready[i].flag = 0;
         if (ev->events & POLLIN)
-            ready[nready++] = fd;
+            ready[i].flag |= READY;
         if (ev->events & (POLLHUP | POLLERR))
         {
             if (ev->events & POLLERR)
-                ErrorMessage("Failed to poll control descriptor %d!\n", fd);
-            dead[ndead++] = fd;
+                ErrorMessage("Failed to poll control descriptor %d!\n", ev->data.fd);
+            ready[i].flag |= DEAD;
         }
     }
 
@@ -188,7 +196,7 @@ static void unregister_control_fd(const int orig_fd, const int)
     }
 }
 
-static bool poll_control_fds(int ready[MAX_CONTROL_FDS], unsigned& nready, int dead[MAX_CONTROL_FDS], unsigned& ndead)
+static bool poll_control_fds(FdEvents ready[MAX_CONTROL_FDS], unsigned& nready)
 {
     if (npfds == 0)
         return false;
@@ -200,19 +208,21 @@ static bool poll_control_fds(int ready[MAX_CONTROL_FDS], unsigned& nready, int d
             ErrorMessage("Failed to poll control descriptors: %s\n", get_error(errno));
         return false;
     }
-    nready = ndead = 0;
-    for (unsigned i = 0; i < MAX_CONTROL_FDS; i++)
+    nready = ret;
+    for (unsigned i = 0; i < ret; i++)
     {
         struct pollfd* pfd = &pfds[i];
         int fd = pfd->fd;
+        ready[i].fd = fd;
+        ready[i].flag = 0;
         if (pfd->revents & (POLLHUP | POLLERR | POLLNVAL))
         {
             if (pfd->revents & (POLLERR | POLLNVAL))
                 ErrorMessage("Failed to poll control descriptor %d!\n", fd);
-            dead[ndead++] = fd;
+            ready[i].flag |= DEAD;
         }
         if (pfd->revents & POLLIN)
-            ready[nready++] = fd;
+            ready[i].flag |= READY;
     }
     return true;
 }
@@ -295,13 +305,17 @@ static bool accept_conn()
 static void delete_control(const std::unordered_map<int, ControlConn*>::const_iterator& iter)
 {
     ControlConn* ctrlcon = iter->second;
+    unregister_control_fd(iter->first, ctrlcon->get_fd());
 
-    // FIXIT-L hacky way to keep the control around until it's no longer being referenced
     if (ctrlcon->is_blocked())
-        return;
+    {
+        ctrlcon->remove();
+    }
+    else
+    {
+        delete ctrlcon;
+    }
 
-    unregister_control_fd(iter->first, ctrlcon->get_fd());
-    delete ctrlcon;
     controls.erase(iter);
 }
 
@@ -445,30 +459,35 @@ void ControlMgmt::socket_term()
 
 bool ControlMgmt::service_users()
 {
-    static int ready[MAX_CONTROL_FDS], dead[MAX_CONTROL_FDS];
-    unsigned nready, ndead;
+    static FdEvents event[MAX_CONTROL_FDS];
+    unsigned nevent;
 
-    if (!poll_control_fds(ready, nready, dead, ndead))
+    if (!poll_control_fds(event, nevent))
         return false;
 
-    // Process ready descriptors first, even if they're dead, to honor their last request
     unsigned serviced = 0;
-    for (unsigned i = 0; i < nready; i++)
+    for (unsigned i = 0; i < nevent; i++)
     {
-        int fd = ready[i];
-        if (fd == listener)
+        int fd = event[i].fd;
+        if (event[i].flag & READY)
         {
-            // Got a new connection request, attempt to accept it and store it in controls
-            if (accept_conn())
+            // Process ready descriptors first, even if they're dead, to honor their last request
+            if (fd == listener)
+            {
+                // Got a new connection request, attempt to accept it and store it in controls
+                if (accept_conn())
+                    serviced++;
+            }
+            else if (process_control_commands(fd))
                 serviced++;
         }
-        else if (process_control_commands(fd))
+        if (event[i].flag & DEAD)
+        {
+            delete_control(fd);
             serviced++;
+        }
     }
 
-    for (unsigned i = 0; i < ndead; i++)
-        delete_control(dead[i]);
-
     return (serviced > 0);
 }
 

diff --git a/src/main/ac_shell_cmd.cc b/src/main/ac_shell_cmd.cc
index e4cb346288452723138743cbb55f4e2752d95405..7b61026ebec75d6c65d643d06aa1e36354ce210c 100644 (file)
--- a/src/main/ac_shell_cmd.cc
+++ b/src/main/ac_shell_cmd.cc
@@ -45,5 +45,10 @@ ACShellCmd::~ACShellCmd()
     delete ac;
 
     if (ctrlcon)
-        ctrlcon->unblock();
+    {
+        if (ctrlcon->is_removed())
+            delete ctrlcon;
+        else
+            ctrlcon->unblock();
+    }
 }