refactor: build packaged_task in optional_task

src/pam/main.cc

@@ -49,7 +49,7 @@
 const auto DEFAULT_TIMEOUT =
     std::chrono::duration<int, std::chrono::milliseconds::period>(2500);
 
-#define S(msg) gettext (msg)
+#define S(msg) gettext(msg)
 
 /**
  * Inspect the status code returned by the compare process
@@ -112,7 +112,7 @@ auto howdy_error(int status,
  * @param  conv_function PAM conversation function
  * @return          Returns the conversation function return code
  */
- auto howdy_msg(char *username, int status, const INIReader &reader,
+auto howdy_msg(char *username, int status, const INIReader &reader,
                const std::function<int(int, const char *)> &conv_function)
     -> int {
   if (status != EXIT_SUCCESS) {
@@ -277,7 +277,7 @@ auto identify(pam_handle_t *pamh, int flags, int argc, const char **argv,
 
   // This task wait for the status of the python subprocess (we don't want a
   // zombie process)
-  optional_task<int> child_task(std::packaged_task<int()>([&] {
+  optional_task<int> child_task([&] {
     int status;
     wait(&status);
 
@@ -292,20 +292,17 @@ auto identify(pam_handle_t *pamh, int flags, int argc, const char **argv,
     cv.notify_one();
 
     return status;
-  }));
+  });
   child_task.activate();
 
   // This task waits for the password input (if the workaround wants it)
-  optional_task<std::tuple<int, char *>> pass_task(
+  optional_task<std::tuple<int, char *>> pass_task([&] {
-      std::packaged_task<std::tuple<int, char *>()>([&] {
     char *auth_tok_ptr = nullptr;
-        int pam_res =
+    int pam_res = pam_get_authtok(
-            pam_get_authtok(pamh, PAM_AUTHTOK,
+        pamh, PAM_AUTHTOK, const_cast<const char **>(&auth_tok_ptr), nullptr);
-                            const_cast<const char **>(&auth_tok_ptr), nullptr);
     {
       std::unique_lock<std::mutex> lk(m);
-          ConfirmationType type =
+      ConfirmationType type = confirmation_type.load(std::memory_order_relaxed);
-              confirmation_type.load(std::memory_order_relaxed);
       if (type == ConfirmationType::Unset) {
         confirmation_type.store(ConfirmationType::Pam,
                                 std::memory_order_relaxed);
@@ -314,7 +311,7 @@ auto identify(pam_handle_t *pamh, int flags, int argc, const char **argv,
     cv.notify_one();
 
     return std::tuple<int, char *>(pam_res, auth_tok_ptr);
-      }));
+  });
 
   if (auth_tok) {
     pass_task.activate();
@@ -331,8 +328,8 @@ auto identify(pam_handle_t *pamh, int flags, int argc, const char **argv,
 
     // If the workaround is native
     if (auth_tok) {
-      // We cancel the thread using pthread, pam_get_authtok seems to be a
+      // UNSAFE: We cancel the thread using pthread, pam_get_authtok seems to be
-      // cancellation point
+      // a cancellation point
       if (pass_task.is_active()) {
         pass_task.stop(true);
       }

src/pam/optional_task.hh

@@ -6,18 +6,19 @@
 #include <future>
 #include <thread>
 
+// A task executed only if activated.
 template <typename T> class optional_task {
   std::thread _thread;
   std::packaged_task<T()> _task;
   std::future<T> _future;
-  std::atomic<bool> _spawned;
+  bool _spawned;
-  std::atomic<bool> _is_active;
+  bool _is_active;
 
 public:
-  explicit optional_task(std::packaged_task<T()> task);
+  explicit optional_task(std::function<T()> fn);
   void activate();
-  template <typename Dur, typename Rat>
+  template <typename R, typename P>
-  auto wait(std::chrono::duration<Dur, Rat> dur) -> std::future_status;
+  auto wait(std::chrono::duration<R, P> dur) -> std::future_status;
   auto get() -> T;
   auto is_active() -> bool;
   void stop(bool force);
@@ -25,22 +26,28 @@ public:
 };
 
 template <typename T>
-optional_task<T>::optional_task(std::packaged_task<T()> t)
+optional_task<T>::optional_task(std::function<T()> fn)
-    : _task(std::move(t)), _future(_task.get_future()) {}
+    : _task(std::packaged_task<T()>(std::move(fn))),
+      _future(_task.get_future()) {}
 
+// Create a new thread and launch the task on it.
 template <typename T> void optional_task<T>::activate() {
   _thread = std::thread(std::move(_task));
   _spawned = true;
   _is_active = true;
 }
 
+// Wait for `dur` time and return a `future` status.
 template <typename T>
-template <typename Dur, typename Rat>
+template <typename R, typename P>
-auto optional_task<T>::wait(std::chrono::duration<Dur, Rat> dur)
+auto optional_task<T>::wait(std::chrono::duration<R, P> dur)
     -> std::future_status {
   return _future.wait_for(dur);
 }
 
+// Get the value.
+// WARNING: The function hould be run only if the task has successfully been
+// stopped.
 template <typename T> auto optional_task<T>::get() -> T {
   assert(!_is_active && _spawned);
   return _future.get();
@@ -50,6 +57,11 @@ template <typename T> auto optional_task<T>::is_active() -> bool {
   return _is_active;
 }
 
+// Stop the thread:
+// - if `force` is `false`, by joining the thread.
+// - if `force` is `true`, by cancelling the thread using `pthread_cancel`.
+// WARNING: This function should be used with extreme caution when `force` is
+// set to `true`.
 template <typename T> void optional_task<T>::stop(bool force) {
   if (!(_is_active && _thread.joinable()) && _spawned) {
     _is_active = false;