From: Steve Chew (stechew) Date: Fri, 25 Jun 2021 17:33:45 +0000 (+0000) Subject: Merge pull request #2952 in SNORT/snort3 from ~SBAIGAL/snort3:better to master X-Git-Tag: 3.1.7.0~5 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=1fbb8fa8e6f6e51d3ac987bade23973760036314;p=thirdparty%2Fsnort3.git Merge pull request #2952 in SNORT/snort3 from ~SBAIGAL/snort3:better to master Squashed commit of the following: commit bf82e65e3296202f7d81e1bd14d7447d1baa25c3 Author: Steven Baigal (sbaigal) 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 2a074eebe..e706247c7 100644 --- 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 29333df2d..446cd193e 100644 --- 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 eb29f1696..76d1d6ee0 100644 --- 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 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::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 e4cb34628..7b61026eb 100644 --- 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(); + } }