fix(userspace/falco): make signal handlers safe with multi-threading

Signed-off-by: Jason Dellaluce <jasondellaluce@gmail.com>

userspace/falco/app_actions/create_signal_handlers.cpp

@@ -30,26 +30,24 @@ using namespace falco::app;
 // provided application, and in unregister_signal_handlers it will be
 // rebound back to the dummy application.
 
-static application dummy;
-static std::reference_wrapper<application> s_app = dummy;
 static int inot_fd;
 
-static void signal_callback(int signal)
+static void terminate_signal_handler(int signal)
 {
-	falco_logger::log(LOG_INFO, "SIGINT received, exiting...\n");
+	ASSERT(falco::app::g_terminate.is_lock_free());
-	s_app.get().terminate();
+	falco::app::g_terminate.store(APP_SIGNAL_SET, std::memory_order_seq_cst);
 }
 
-static void reopen_outputs(int signal)
+static void reopen_outputs_signal_handler(int signal)
 {
-	falco_logger::log(LOG_INFO, "SIGUSR1 received, reopening outputs...\n");
+	ASSERT(falco::app::g_reopen_outputs.is_lock_free());
-	s_app.get().reopen_outputs();
+	falco::app::g_reopen_outputs.store(APP_SIGNAL_SET, std::memory_order_seq_cst);
 }
 
-static void restart_falco(int signal)
+static void restart_signal_handler(int signal)
 {
-	falco_logger::log(LOG_INFO, "SIGHUP received, restarting...\n");
+	ASSERT(falco::app::g_restart.is_lock_free());
-	s_app.get().restart();
+	falco::app::g_restart.store(APP_SIGNAL_SET, std::memory_order_seq_cst);
 }
 
 bool application::create_handler(int sig, void (*func)(int), run_result &ret)
@@ -74,15 +72,23 @@ bool application::create_handler(int sig, void (*func)(int), run_result &ret)
 
 application::run_result application::create_signal_handlers()
 {
-	run_result ret;
+	falco::app::g_terminate.store(APP_SIGNAL_NOT_SET, std::memory_order_seq_cst);
-	s_app = *this;
+	falco::app::g_restart.store(APP_SIGNAL_NOT_SET, std::memory_order_seq_cst);
-	if(! create_handler(SIGINT, ::signal_callback, ret) ||
+	falco::app::g_reopen_outputs.store(APP_SIGNAL_NOT_SET, std::memory_order_seq_cst);
-	   ! create_handler(SIGTERM, ::signal_callback, ret) ||
+	
-	   ! create_handler(SIGUSR1, ::reopen_outputs, ret) ||
+	if (!g_terminate.is_lock_free()
-	   ! create_handler(SIGHUP, ::restart_falco, ret))
+		|| !g_restart.is_lock_free()
+		|| !g_reopen_outputs.is_lock_free())
 	{
-		s_app = dummy;
+		falco_logger::log(LOG_WARNING, "Bundled atomics implementation is not lock-free, signal handlers may be unstable\n");
 	}
+
+	// we use the if just to make sure we return at the first failed statement
+	run_result ret;
+	if(! create_handler(SIGINT, ::terminate_signal_handler, ret) ||
+	   ! create_handler(SIGTERM, ::terminate_signal_handler, ret) ||
+	   ! create_handler(SIGUSR1, ::reopen_outputs_signal_handler, ret) ||
+	   ! create_handler(SIGHUP, ::restart_signal_handler, ret));
 	return ret;
 }
 
@@ -99,7 +105,7 @@ application::run_result application::attach_inotify_signals()
 		struct sigaction sa;
 		sigemptyset(&sa.sa_mask);
 		sa.sa_flags = SA_RESTART;
-		sa.sa_handler = restart_falco;
+		sa.sa_handler = restart_signal_handler;
 		if (sigaction(SIGIO, &sa, NULL) == -1)
 		{
 			return run_result::fatal("Failed to link SIGIO to inotify handler");
@@ -169,7 +175,5 @@ bool application::unregister_signal_handlers(std::string &errstr)
 		errstr = ret.errstr;
 		return false;
 	}
-
-	s_app = dummy;
 	return true;
 }

userspace/falco/app_actions/process_events.cpp

@@ -97,11 +97,20 @@ application::run_result application::do_inspect(
 	{
 		rc = inspector->next(&ev);
 
-		if(m_state->terminate.load(std::memory_order_seq_cst)
+		if(should_terminate())
-			|| m_state->restart.load(std::memory_order_seq_cst))
 		{
+			terminate();
 			break;
 		}
+		else if(should_restart())
+		{
+			restart();
+			break;
+		}
+		else if (should_reopen_outputs())
+		{
+			reopen_outputs();
+		}
 		else if(rc == SCAP_TIMEOUT)
 		{
 			if(unlikely(ev == nullptr))

userspace/falco/application.cpp

@@ -26,9 +26,41 @@ limitations under the License.
 
 using namespace std::placeholders;
 
+static inline bool should_take_action_to_signal(std::atomic<int>& v)
+{
+	// we expected the signal to be received, and we try to set action-taken flag
+	int value = APP_SIGNAL_SET;
+	while (!v.compare_exchange_weak(
+			value,
+			APP_SIGNAL_ACTION_TAKEN,
+			std::memory_order_seq_cst,
+			std::memory_order_seq_cst))
+	{
+		// application already took action, there's no need to do it twice
+		if (value == APP_SIGNAL_ACTION_TAKEN)
+		{
+			return false;
+		}
+
+		// signal did was not really received, so we "fake" receiving it
+		if (value == APP_SIGNAL_NOT_SET)
+		{
+			v.store(APP_SIGNAL_SET, std::memory_order_seq_cst);
+		}
+
+		// reset "expected" CAS variable and keep looping until we succeed
+		value = APP_SIGNAL_SET;
+	}
+	return true;
+}
+
 namespace falco {
 namespace app {
 
+std::atomic<int> g_terminate(APP_SIGNAL_NOT_SET);
+std::atomic<int> g_restart(APP_SIGNAL_NOT_SET);
+std::atomic<int> g_reopen_outputs(APP_SIGNAL_NOT_SET);
+
 application::run_result::run_result()
 	: success(true), errstr(""), proceed(true)
 {
@@ -39,9 +71,7 @@ application::run_result::~run_result()
 }
 
 application::state::state()
-	: restart(false),
+	: loaded_sources(),
-	  terminate(false),
-	  loaded_sources(),
 	  enabled_sources(),
 	  source_infos(),
 	  plugin_configs(),
@@ -70,27 +100,29 @@ application::~application()
 
 void application::terminate()
 {
-	if(m_state != nullptr)
+	if (should_take_action_to_signal(falco::app::g_terminate))
 	{
-		m_state->terminate.store(true, std::memory_order_seq_cst);
+		falco_logger::log(LOG_INFO, "SIGINT received, exiting...\n");
 	}
 }
 
 void application::reopen_outputs()
 {
+	if (should_take_action_to_signal(falco::app::g_reopen_outputs))
+	{
+		falco_logger::log(LOG_INFO, "SIGUSR1 received, reopening outputs...\n");
 		if(m_state != nullptr && m_state->outputs != nullptr)
 		{
-		// note: it is ok to do this inside the signal handler because
-		// in the current falco_outputs implementation this is non-blocking
 			m_state->outputs->reopen_outputs();
 		}
+	}
 }
 
 void application::restart()
 {
-	if(m_state != nullptr)
+	if (should_take_action_to_signal(falco::app::g_restart))
 	{
-		m_state->restart.store(true, std::memory_order_seq_cst);
+		falco_logger::log(LOG_INFO, "SIGHUP received, restarting...\n");
 	}
 }
 
@@ -196,7 +228,7 @@ bool application::run(std::string &errstr, bool &restart)
 		errstr = res.errstr;
 	}
 
-	restart = m_state->restart;
+	restart = should_restart();
 
 	return res.success;
 }

userspace/falco/application.h

@@ -30,9 +30,19 @@ limitations under the License.
 #include <atomic>
 #include <unordered_set>
 
+#define APP_SIGNAL_NOT_SET          0   // The signal flag is not set
+#define APP_SIGNAL_SET              1   // The signal flag has been set
+#define APP_SIGNAL_ACTION_TAKEN     2   // The signal flag has been set and the application took action
+
 namespace falco {
 namespace app {
 
+// these are used to control the lifecycle of the application
+// through signal handlers or internal calls
+extern std::atomic<int> g_terminate;
+extern std::atomic<int> g_restart;
+extern std::atomic<int> g_reopen_outputs;
+
 class application {
 public:
 	application();
@@ -42,13 +52,6 @@ public:
 	application(const application&) = delete;
 	application& operator = (const application&) = delete;
 
-	// These are only used in signal handlers. Other than there,
-	// the control flow of the application should not be changed
-	// from the outside.
-	void terminate();
-	void reopen_outputs();
-	void restart();
-
 	bool init(int argc, char **argv, std::string &errstr);
 
 	// Returns whether the application completed with errors or
@@ -86,9 +89,6 @@ private:
 		state();
 		virtual ~state();
 
-		std::atomic<bool> restart;
-		std::atomic<bool> terminate;
-
 		std::shared_ptr<falco_configuration> config;
 		std::shared_ptr<falco_outputs> outputs;
 		std::shared_ptr<falco_engine> engine;
@@ -317,6 +317,23 @@ private:
 		return !m_options.gvisor_config.empty();
 	}
 
+	// used in signal handlers to control the flow of the application
+	void terminate();
+	void restart();
+	void reopen_outputs();
+	inline bool should_terminate()
+	{
+		return g_terminate.load(std::memory_order_seq_cst) != APP_SIGNAL_NOT_SET;
+	}
+	inline bool should_restart()
+	{
+		return g_restart.load(std::memory_order_seq_cst) != APP_SIGNAL_NOT_SET;
+	}
+	inline bool should_reopen_outputs()
+	{
+		return g_reopen_outputs.load(std::memory_order_seq_cst) != APP_SIGNAL_NOT_SET;
+	}
+
 	std::unique_ptr<state> m_state;
 	cmdline_options m_options;
 	bool m_initialized;