From c86bca60148e0a19560d992269231c66e0f37273 Mon Sep 17 00:00:00 2001
From: jean-christophe81 <98889244+jean-christophe81@users.noreply.github.com>
Date: Fri, 20 Sep 2024 14:07:58 +0200
Subject: [PATCH] MON-147699 check_exec doesn't lock on recursion (#1688)
 (#1704)

* check_exec doesn't lock on recursion

* implement process class with a template parameter
---
 agent/inc/com/centreon/agent/check_exec.hh    |  8 +-
 agent/src/check_exec.cc                       | 10 +-
 agent/src/main.cc                             |  2 +
 agent/test/check_exec_test.cc                 | 23 +++++
 .../com/centreon/common/process/process.hh    | 94 ++++++++++++-------
 common/process/src/process.cc                 | 71 +++++++++-----
 common/tests/process_test.cc                  |  2 +-
 7 files changed, 144 insertions(+), 66 deletions(-)

diff --git a/agent/inc/com/centreon/agent/check_exec.hh b/agent/inc/com/centreon/agent/check_exec.hh
index 8adb1a35134..c458194bb18 100644
--- a/agent/inc/com/centreon/agent/check_exec.hh
+++ b/agent/inc/com/centreon/agent/check_exec.hh
@@ -37,7 +37,7 @@ namespace detail {
  * ensure that completion is called for the right process and not for the
  * previous one
  */
-class process : public common::process {
+class process : public common::process<false> {
   bool _process_ended;
   bool _stdout_eof;
   std::string _stdout;
@@ -54,9 +54,11 @@ class process : public common::process {
 
   void start(unsigned running_index);
 
-  void kill() { common::process::kill(); }
+  void kill() { common::process<false>::kill(); }
 
-  int get_exit_status() const { return common::process::get_exit_status(); }
+  int get_exit_status() const {
+    return common::process<false>::get_exit_status();
+  }
 
   const std::string& get_stdout() const { return _stdout; }
 
diff --git a/agent/src/check_exec.cc b/agent/src/check_exec.cc
index b26c07ab36b..bd475ef5d08 100644
--- a/agent/src/check_exec.cc
+++ b/agent/src/check_exec.cc
@@ -32,7 +32,7 @@ detail::process::process(const std::shared_ptr<asio::io_context>& io_context,
                          const std::shared_ptr<spdlog::logger>& logger,
                          const std::string& cmd_line,
                          const std::shared_ptr<check_exec>& parent)
-    : common::process(io_context, logger, cmd_line), _parent(parent) {}
+    : common::process<false>(io_context, logger, cmd_line), _parent(parent) {}
 
 /**
  * @brief start a new process, if a previous one is already running, it's killed
@@ -44,7 +44,7 @@ void detail::process::start(unsigned running_index) {
   _stdout_eof = false;
   _running_index = running_index;
   _stdout.clear();
-  common::process::start_process(false);
+  common::process<false>::start_process(false);
 }
 
 /**
@@ -61,7 +61,7 @@ void detail::process::on_stdout_read(const boost::system::error_code& err,
     _stdout_eof = true;
     _on_completion();
   }
-  common::process::on_stdout_read(err, nb_read);
+  common::process<false>::on_stdout_read(err, nb_read);
 }
 
 /**
@@ -76,7 +76,7 @@ void detail::process::on_stderr_read(const boost::system::error_code& err,
     SPDLOG_LOGGER_ERROR(_logger, "process error: {}",
                         std::string_view(_stderr_read_buffer, nb_read));
   }
-  common::process::on_stderr_read(err, nb_read);
+  common::process<false>::on_stderr_read(err, nb_read);
 }
 
 /**
@@ -91,7 +91,7 @@ void detail::process::on_process_end(const boost::system::error_code& err,
     _stdout += fmt::format("fail to execute process {} : {}", get_exe_path(),
                            err.message());
   }
-  common::process::on_process_end(err, raw_exit_status);
+  common::process<false>::on_process_end(err, raw_exit_status);
   _process_ended = true;
   _on_completion();
 }
diff --git a/agent/src/main.cc b/agent/src/main.cc
index e613d749d2f..34d11ab1874 100644
--- a/agent/src/main.cc
+++ b/agent/src/main.cc
@@ -81,6 +81,8 @@ static std::string read_file(const std::string& file_path) {
       ss << file.rdbuf();
       file.close();
       return ss.str();
+    } else {
+      SPDLOG_LOGGER_ERROR(g_logger, "fail to open {}", file_path);
     }
   } catch (const std::exception& e) {
     SPDLOG_LOGGER_ERROR(g_logger, "fail to read {}: {}", file_path, e.what());
diff --git a/agent/test/check_exec_test.cc b/agent/test/check_exec_test.cc
index c5cf3b15bbc..b3b547cfd13 100644
--- a/agent/test/check_exec_test.cc
+++ b/agent/test/check_exec_test.cc
@@ -131,3 +131,26 @@ TEST(check_exec_test, bad_command) {
             "or directory");
 #endif
 }
+
+TEST(check_exec_test, recurse_not_lock) {
+  command_line = ECHO_PATH " hello toto";
+  std::condition_variable cond;
+  unsigned cpt = 0;
+  std::shared_ptr<check_exec> check = check_exec::load(
+      g_io_context, spdlog::default_logger(), time_point(), serv, cmd_name,
+      command_line, engine_to_agent_request_ptr(),
+      [&](const std::shared_ptr<com::centreon::agent::check>& caller, int,
+          const std::list<com::centreon::common::perfdata>& perfdata,
+          const std::list<std::string>& output) {
+        if (!cpt) {
+          ++cpt;
+          caller->start_check(std::chrono::seconds(1));
+        } else
+          cond.notify_one();
+      });
+  check->start_check(std::chrono::seconds(1));
+
+  std::mutex mut;
+  std::unique_lock l(mut);
+  cond.wait(l);
+}
diff --git a/common/process/inc/com/centreon/common/process/process.hh b/common/process/inc/com/centreon/common/process/process.hh
index ea097b44c01..06a6799bd3b 100644
--- a/common/process/inc/com/centreon/common/process/process.hh
+++ b/common/process/inc/com/centreon/common/process/process.hh
@@ -26,6 +26,33 @@ namespace detail {
 struct boost_process;
 }  // namespace detail
 
+namespace detail {
+template <bool use_mutex>
+class mutex;
+
+template <bool use_mutex>
+class lock;
+
+template <>
+class mutex<true> : public absl::Mutex {};
+
+template <>
+class lock<true> : public absl::MutexLock {
+ public:
+  lock(absl::Mutex* mut) : absl::MutexLock(mut) {}
+};
+
+template <>
+class mutex<false> {};
+
+template <>
+class lock<false> {
+ public:
+  lock(mutex<false>* dummy_mut) {}
+};
+
+}  // namespace detail
+
 /**
  * @brief This class allow to exec a process asynchronously.
  * It's a base class. If you want to get stdin and stdout returned data, you
@@ -37,22 +64,23 @@ struct boost_process;
  * When completion methods like on_stdout_read are called, _protect is already
  * locked
  */
-class process : public std::enable_shared_from_this<process> {
+
+template <bool use_mutex = true>
+class process : public std::enable_shared_from_this<process<use_mutex>> {
+  using std::enable_shared_from_this<process<use_mutex>>::shared_from_this;
   std::string _exe_path;
   std::vector<std::string> _args;
 
-  std::deque<std::shared_ptr<std::string>> _stdin_write_queue
-      ABSL_GUARDED_BY(_protect);
-  bool _write_pending ABSL_GUARDED_BY(_protect) = false;
+  std::deque<std::shared_ptr<std::string>> _stdin_write_queue;
+  bool _write_pending;
 
-  std::shared_ptr<detail::boost_process> _proc ABSL_GUARDED_BY(_protect);
+  std::shared_ptr<detail::boost_process> _proc;
 
   int _exit_status = 0;
 
-  absl::Mutex _protect;
+  detail::mutex<use_mutex> _protect;
 
-  void stdin_write_no_lock(const std::shared_ptr<std::string>& data)
-      ABSL_EXCLUSIVE_LOCKS_REQUIRED(_protect);
+  void stdin_write_no_lock(const std::shared_ptr<std::string>& data);
   void stdin_write(const std::shared_ptr<std::string>& data);
 
   void stdout_read();
@@ -62,22 +90,18 @@ class process : public std::enable_shared_from_this<process> {
   std::shared_ptr<asio::io_context> _io_context;
   std::shared_ptr<spdlog::logger> _logger;
 
-  char _stdout_read_buffer[0x1000] ABSL_GUARDED_BY(_protect);
-  char _stderr_read_buffer[0x1000] ABSL_GUARDED_BY(_protect);
+  char _stdout_read_buffer[0x1000];
+  char _stderr_read_buffer[0x1000];
 
   virtual void on_stdout_read(const boost::system::error_code& err,
-                              size_t nb_read)
-      ABSL_EXCLUSIVE_LOCKS_REQUIRED(_protect);
+                              size_t nb_read);
   virtual void on_stderr_read(const boost::system::error_code& err,
-                              size_t nb_read)
-      ABSL_EXCLUSIVE_LOCKS_REQUIRED(_protect);
+                              size_t nb_read);
 
   virtual void on_process_end(const boost::system::error_code& err,
-                              int raw_exit_status)
-      ABSL_EXCLUSIVE_LOCKS_REQUIRED(_protect);
+                              int raw_exit_status);
 
-  virtual void on_stdin_write(const boost::system::error_code& err)
-      ABSL_EXCLUSIVE_LOCKS_REQUIRED(_protect);
+  virtual void on_stdin_write(const boost::system::error_code& err);
 
  public:
   template <typename string_iterator>
@@ -126,12 +150,13 @@ class process : public std::enable_shared_from_this<process> {
  * @param arg_begin iterator to first argument
  * @param arg_end iterator after the last argument
  */
+template <bool use_mutex>
 template <typename string_iterator>
-process::process(const std::shared_ptr<asio::io_context>& io_context,
-                 const std::shared_ptr<spdlog::logger>& logger,
-                 const std::string_view& exe_path,
-                 string_iterator arg_begin,
-                 string_iterator arg_end)
+process<use_mutex>::process(const std::shared_ptr<asio::io_context>& io_context,
+                            const std::shared_ptr<spdlog::logger>& logger,
+                            const std::string_view& exe_path,
+                            string_iterator arg_begin,
+                            string_iterator arg_end)
     : _exe_path(exe_path),
       _args(arg_begin, arg_end),
       _io_context(io_context),
@@ -146,11 +171,13 @@ process::process(const std::shared_ptr<asio::io_context>& io_context,
  * @param exe_path path of executable without argument
  * @param args container of arguments
  */
+template <bool use_mutex>
 template <typename args_container>
-process::process(const std::shared_ptr<boost::asio::io_context>& io_context,
-                 const std::shared_ptr<spdlog::logger>& logger,
-                 const std::string_view& exe_path,
-                 const args_container& args)
+process<use_mutex>::process(
+    const std::shared_ptr<boost::asio::io_context>& io_context,
+    const std::shared_ptr<spdlog::logger>& logger,
+    const std::string_view& exe_path,
+    const args_container& args)
     : _exe_path(exe_path),
       _args(args),
       _io_context(io_context),
@@ -166,11 +193,13 @@ process::process(const std::shared_ptr<boost::asio::io_context>& io_context,
  * @param exe_path path of executable without argument
  * @param args brace of arguments {"--flag1", "arg1", "-c", "arg2"}
  */
+template <bool use_mutex>
 template <typename string_type>
-process::process(const std::shared_ptr<boost::asio::io_context>& io_context,
-                 const std::shared_ptr<spdlog::logger>& logger,
-                 const std::string_view& exe_path,
-                 const std::initializer_list<string_type>& args)
+process<use_mutex>::process(
+    const std::shared_ptr<boost::asio::io_context>& io_context,
+    const std::shared_ptr<spdlog::logger>& logger,
+    const std::string_view& exe_path,
+    const std::initializer_list<string_type>& args)
     : _exe_path(exe_path), _io_context(io_context), _logger(logger) {
   _args.reserve(args.size());
   for (const auto& str : args) {
@@ -185,8 +214,9 @@ process::process(const std::shared_ptr<boost::asio::io_context>& io_context,
  * can be used to construct a std::string
  * @param content
  */
+template <bool use_mutex>
 template <typename string_class>
-void process::write_to_stdin(const string_class& content) {
+void process<use_mutex>::write_to_stdin(const string_class& content) {
   stdin_write(std::make_shared<std::string>(content));
 }
 
diff --git a/common/process/src/process.cc b/common/process/src/process.cc
index f8e982c9a71..6036a0fca19 100644
--- a/common/process/src/process.cc
+++ b/common/process/src/process.cc
@@ -144,9 +144,11 @@ using namespace com::centreon::common;
  * @param cmd_line cmd line split (the first element is the path of the
  * executable)
  */
-process::process(const std::shared_ptr<boost::asio::io_context>& io_context,
-                 const std::shared_ptr<spdlog::logger>& logger,
-                 const std::string_view& cmd_line)
+template <bool use_mutex>
+process<use_mutex>::process(
+    const std::shared_ptr<boost::asio::io_context>& io_context,
+    const std::shared_ptr<spdlog::logger>& logger,
+    const std::string_view& cmd_line)
     : _io_context(io_context), _logger(logger) {
 #ifdef _WINDOWS
   auto split_res = boost::program_options::split_winmain(std::string(cmd_line));
@@ -174,9 +176,10 @@ process::process(const std::shared_ptr<boost::asio::io_context>& io_context,
  * @param enable_stdin On Windows set it to false if you doesn't want to write
  * on child stdin
  */
-void process::start_process(bool enable_stdin) {
+template <bool use_mutex>
+void process<use_mutex>::start_process(bool enable_stdin) {
   SPDLOG_LOGGER_DEBUG(_logger, "start process: {}", _exe_path);
-  absl::MutexLock l(&_protect);
+  detail::lock<use_mutex> l(&_protect);
   _stdin_write_queue.clear();
   _write_pending = false;
 
@@ -190,7 +193,7 @@ void process::start_process(bool enable_stdin) {
     _proc->proc.async_wait(
         [me = shared_from_this(), current = _proc](
             const boost::system::error_code& err, int raw_exit_status) {
-          absl::MutexLock l(&me->_protect);
+          detail::lock<use_mutex> l(&me->_protect);
           if (current != me->_proc) {
             return;
           }
@@ -210,8 +213,9 @@ void process::start_process(bool enable_stdin) {
  * @param err
  * @param raw_exit_status end status of the process
  */
-void process::on_process_end(const boost::system::error_code& err,
-                             int raw_exit_status) {
+template <bool use_mutex>
+void process<use_mutex>::on_process_end(const boost::system::error_code& err,
+                                        int raw_exit_status) {
   if (err) {
     SPDLOG_LOGGER_ERROR(_logger, "fail async_wait of {}: {}", _exe_path,
                         err.message());
@@ -227,8 +231,9 @@ void process::on_process_end(const boost::system::error_code& err,
  * @brief kill child process
  *
  */
-void process::kill() {
-  absl::MutexLock l(&_protect);
+template <bool use_mutex>
+void process<use_mutex>::kill() {
+  detail::lock<use_mutex> l(&_protect);
   if (_proc) {
     SPDLOG_LOGGER_INFO(_logger, "kill process");
     boost::system::error_code err;
@@ -246,8 +251,9 @@ void process::kill() {
  *
  * @param data
  */
-void process::stdin_write(const std::shared_ptr<std::string>& data) {
-  absl::MutexLock l(&_protect);
+template <bool use_mutex>
+void process<use_mutex>::stdin_write(const std::shared_ptr<std::string>& data) {
+  detail::lock<use_mutex> l(&_protect);
   stdin_write_no_lock(data);
 }
 
@@ -257,7 +263,9 @@ void process::stdin_write(const std::shared_ptr<std::string>& data) {
  *
  * @param data
  */
-void process::stdin_write_no_lock(const std::shared_ptr<std::string>& data) {
+template <bool use_mutex>
+void process<use_mutex>::stdin_write_no_lock(
+    const std::shared_ptr<std::string>& data) {
   if (!_proc) {
     SPDLOG_LOGGER_ERROR(_logger, "stdin_write process {} not started",
                         _exe_path);
@@ -272,7 +280,7 @@ void process::stdin_write_no_lock(const std::shared_ptr<std::string>& data) {
           asio::buffer(*data),
           [me = shared_from_this(), caller = _proc, data](
               const boost::system::error_code& err, size_t nb_written) {
-            absl::MutexLock l(&me->_protect);
+            detail::lock<use_mutex> l(&me->_protect);
             if (caller != me->_proc) {
               return;
             }
@@ -294,7 +302,8 @@ void process::stdin_write_no_lock(const std::shared_ptr<std::string>& data) {
  *
  * @param err
  */
-void process::on_stdin_write(const boost::system::error_code& err) {
+template <bool use_mutex>
+void process<use_mutex>::on_stdin_write(const boost::system::error_code& err) {
   _write_pending = false;
 
   if (err) {
@@ -321,14 +330,15 @@ void process::on_stdin_write(const boost::system::error_code& err) {
  * @brief asynchronous read from child process stdout
  *
  */
-void process::stdout_read() {
+template <bool use_mutex>
+void process<use_mutex>::stdout_read() {
   if (_proc) {
     try {
       _proc->stdout_pipe.async_read_some(
           asio::buffer(_stdout_read_buffer),
           [me = shared_from_this(), caller = _proc](
               const boost::system::error_code& err, size_t nb_read) {
-            absl::MutexLock l(&me->_protect);
+            detail::lock<use_mutex> l(&me->_protect);
             if (caller != me->_proc) {
               return;
             }
@@ -336,7 +346,7 @@ void process::stdout_read() {
           });
     } catch (const std::exception& e) {
       _io_context->post([me = shared_from_this(), caller = _proc]() {
-        absl::MutexLock l(&me->_protect);
+        detail::lock<use_mutex> l(&me->_protect);
         me->on_stdout_read(std::make_error_code(std::errc::broken_pipe), 0);
       });
     }
@@ -351,8 +361,9 @@ void process::stdout_read() {
  * @param err
  * @param nb_read
  */
-void process::on_stdout_read(const boost::system::error_code& err,
-                             size_t nb_read) {
+template <bool use_mutex>
+void process<use_mutex>::on_stdout_read(const boost::system::error_code& err,
+                                        size_t nb_read) {
   if (err) {
     if (err == asio::error::eof || err == asio::error::broken_pipe) {
       SPDLOG_LOGGER_DEBUG(_logger, "fail read from stdout of process {}: {}",
@@ -372,14 +383,15 @@ void process::on_stdout_read(const boost::system::error_code& err,
  * @brief asynchronous read from child process stderr
  *
  */
-void process::stderr_read() {
+template <bool use_mutex>
+void process<use_mutex>::stderr_read() {
   if (_proc) {
     try {
       _proc->stderr_pipe.async_read_some(
           asio::buffer(_stderr_read_buffer),
           [me = shared_from_this(), caller = _proc](
               const boost::system::error_code& err, size_t nb_read) {
-            absl::MutexLock l(&me->_protect);
+            detail::lock<use_mutex> l(&me->_protect);
             if (caller != me->_proc) {
               return;
             }
@@ -387,7 +399,7 @@ void process::stderr_read() {
           });
     } catch (const std::exception& e) {
       _io_context->post([me = shared_from_this(), caller = _proc]() {
-        absl::MutexLock l(&me->_protect);
+        detail::lock<use_mutex> l(&me->_protect);
         me->on_stderr_read(std::make_error_code(std::errc::broken_pipe), 0);
       });
     }
@@ -402,8 +414,9 @@ void process::stderr_read() {
  * @param err
  * @param nb_read
  */
-void process::on_stderr_read(const boost::system::error_code& err,
-                             size_t nb_read) {
+template <bool use_mutex>
+void process<use_mutex>::on_stderr_read(const boost::system::error_code& err,
+                                        size_t nb_read) {
   if (err) {
     if (err == asio::error::eof || err == asio::error::broken_pipe) {
       SPDLOG_LOGGER_DEBUG(_logger, "fail read from stderr of process {}: {}",
@@ -418,3 +431,11 @@ void process::on_stderr_read(const boost::system::error_code& err,
     stderr_read();
   }
 }
+
+namespace com::centreon::common {
+
+template class process<true>;
+
+template class process<false>;
+
+}  // namespace com::centreon::common
\ No newline at end of file
diff --git a/common/tests/process_test.cc b/common/tests/process_test.cc
index 81186e0318b..325524a406a 100644
--- a/common/tests/process_test.cc
+++ b/common/tests/process_test.cc
@@ -44,7 +44,7 @@ class process_test : public ::testing::Test {
   }
 };
 
-class process_wait : public process {
+class process_wait : public process<> {
   std::mutex _cond_m;
   std::condition_variable _cond;
   std::string _stdout;