MON-147783 check_exec doesn't lock on recursion backport 24.04(#1688)

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; }
 

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();
 }

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());

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);
+}

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));
 }