refactor: apply suggestions from clang-tidy

src/pam/main.cc

@@ -1,3 +1,4 @@
+#include <boost/locale/message.hpp>
 #include <cerrno>
 #include <csignal>
 #include <cstdlib>
@@ -41,8 +42,10 @@
 #include "main.hh"
 #include "optional_task.hh"
 
-using namespace std;
+const auto DEFAULT_TIMEOUT =
-using namespace boost::locale;
+    std::chrono::duration<int, std::chrono::milliseconds::period>(2500);
+
+#define S(msg) boost::locale::dgettext("pam", msg)
 
 /**
  * Inspect the status code returned by the compare process
@@ -50,7 +53,9 @@ using namespace boost::locale;
  * @param  conv_function The PAM conversation function
  * @return               A PAM return code
  */
-int howdy_error(int status, function<int(int, const char *)> conv_function) {
+auto howdy_error(int status,
+                 const std::function<int(int, const char *)> &conv_function)
+    -> int {
   // If the process has exited
   if (WIFEXITED(status)) {
     // Get the status code returned
@@ -59,8 +64,7 @@ int howdy_error(int status, function<int(int, const char *)> conv_function) {
     switch (status) {
     // Status 10 means we couldn't find any face models
     case 10:
-      conv_function(PAM_ERROR_MSG,
+      conv_function(PAM_ERROR_MSG, S("There is no face model known").c_str());
-                    dgettext("pam", "There is no face model known"));
       syslog(LOG_NOTICE, "Failure, no face model known");
       break;
     // Status 11 means we exceded the maximum retry count
@@ -73,15 +77,14 @@ int howdy_error(int status, function<int(int, const char *)> conv_function) {
       break;
     // Status 13 means the image was too dark
     case 13:
-      conv_function(PAM_ERROR_MSG,
+      conv_function(PAM_ERROR_MSG, S("Face detection image too dark").c_str());
-                    dgettext("pam", "Face detection image too dark"));
       syslog(LOG_ERR, "Failure, image too dark");
       break;
     // Otherwise, we can't describe what happened but it wasn't successful
     default:
       conv_function(
           PAM_ERROR_MSG,
-          string(dgettext("pam", "Unknown error: ") + status).c_str());
+          S("Unknown error: ").append(std::to_string(status)).c_str());
       syslog(LOG_ERR, "Failure, unknown error %d", status);
     }
   } else {
@@ -106,17 +109,18 @@ int howdy_error(int status, function<int(int, const char *)> conv_function) {
  * @param  conv_function PAM conversation function
  * @return          Returns the conversation function return code
  */
-int howdy_msg(char *username, int status, INIReader &reader,
+auto howdy_msg(char *username, int status, INIReader &reader,
-              function<int(int, const char *)> conv_function) {
+               const std::function<int(int, const char *)> &conv_function)
+    -> int {
   if (status != EXIT_SUCCESS) {
     return howdy_error(status, conv_function);
   }
 
   if (!reader.GetBoolean("core", "no_confirmation", true)) {
     // Construct confirmation text from i18n string
-    string confirm_text = dgettext("pam", "Identified face as {}");
+    std::string confirm_text = S("Identified face as {}");
-    string identify_msg =
+    std::string identify_msg =
-        confirm_text.replace(confirm_text.find("{}"), 2, string(username));
+        confirm_text.replace(confirm_text.find("{}"), 2, std::string(username));
     conv_function(PAM_TEXT_INFO, identify_msg.c_str());
   }
 
@@ -132,7 +136,7 @@ int howdy_msg(char *username, int status, INIReader &reader,
  * @param  message String to show the user
  * @return         Returns the conversation function return code
  */
-int send_message(struct pam_conv *conv, int type, const char *message) {
+auto send_message(struct pam_conv *conv, int type, const char *message) -> int {
   // No need to free this, it's allocated on the stack
   const struct pam_message msg = {.msg_style = type, .msg = message};
   const struct pam_message *msgp = &msg;
@@ -153,8 +157,8 @@ int send_message(struct pam_conv *conv, int type, const char *message) {
  * @param  auth_tok True if we should ask for a password too
  * @return          Returns a PAM return code
  */
-int identify(pam_handle_t *pamh, int flags, int argc, const char **argv,
+auto identify(pam_handle_t *pamh, int flags, int argc, const char **argv,
-             bool auth_tok) {
+              bool auth_tok) -> int {
   INIReader reader("/lib/security/howdy/config.ini");
   // Open the system log so we can write to it
   openlog("pam_howdy", 0, LOG_AUTHPRIV);
@@ -169,19 +173,22 @@ int identify(pam_handle_t *pamh, int flags, int argc, const char **argv,
 
   // Will contain PAM conversation structure
   struct pam_conv *conv = nullptr;
+  const void **conv_ptr =
+      const_cast<const void **>(reinterpret_cast<void **>(&conv));
   // Will contain the responses from PAM functions
   int pam_res = PAM_IGNORE;
 
   // Try to get the conversation function and error out if we can't
-  if ((pam_res = pam_get_item(pamh, PAM_CONV, (const void **)&conv)) !=
+  if ((pam_res = pam_get_item(pamh, PAM_CONV, conv_ptr)) != PAM_SUCCESS) {
-      PAM_SUCCESS) {
     syslog(LOG_ERR, "Failed to acquire conversation");
     return pam_res;
   }
 
   // Wrap the PAM conversation function in our own, easier function
-  auto conv_function =
+  auto conv_function = [conv](int msg_type, const char *msg) {
-      bind(send_message, conv, placeholders::_1, placeholders::_2);
+    return send_message(conv, std::forward<decltype(msg_type)>(msg_type),
+                        std::forward<decltype(msg)>(msg));
+  };
 
   // Error out if we could not ready the config file
   if (reader.ParseError() < 0) {
@@ -212,15 +219,18 @@ int identify(pam_handle_t *pamh, int flags, int argc, const char **argv,
     int return_value =
         glob("/proc/acpi/button/lid/*/state", 0, nullptr, &glob_result);
 
-    // TODO: We ignore the result
     if (return_value != 0) {
+      syslog(LOG_ERR, "Failed to read files from glob: %d", return_value);
+      if (errno != 0) {
+        syslog(LOG_ERR, "Underlying error: %s (%d)", strerror(errno), errno);
+      }
       globfree(&glob_result);
     }
 
     for (size_t i = 0; i < glob_result.gl_pathc; i++) {
-      ifstream file(string(glob_result.gl_pathv[i]));
+      std::ifstream file(std::string(glob_result.gl_pathv[i]));
-      string lid_state;
+      std::string lid_state;
-      getline(file, lid_state, (char)file.eof());
+      std::getline(file, lid_state, static_cast<char>(file.eof()));
 
       if (lid_state.find("closed") != std::string::npos) {
         globfree(&glob_result);
@@ -235,9 +245,8 @@ int identify(pam_handle_t *pamh, int flags, int argc, const char **argv,
 
   // If enabled, send a notice to the user that facial login is being attempted
   if (reader.GetBoolean("core", "detection_notice", false)) {
-    if ((pam_res = conv_function(
+    if ((conv_function(PAM_TEXT_INFO,
-             PAM_TEXT_INFO,
+                       S("Attempting facial authentication").c_str())) !=
-             dgettext("pam", "Attempting facial authentication"))) !=
         PAM_SUCCESS) {
       syslog(LOG_ERR, "Failed to send detection notice");
     }
@@ -245,8 +254,8 @@ int identify(pam_handle_t *pamh, int flags, int argc, const char **argv,
 
   // Get the username from PAM, needed to match correct face model
   char *username = nullptr;
-  if ((pam_res = pam_get_user(pamh, (const char **)&username, nullptr)) !=
+  if ((pam_res = pam_get_user(pamh, const_cast<const char **>(&username),
-      PAM_SUCCESS) {
+                              nullptr)) != PAM_SUCCESS) {
     syslog(LOG_ERR, "Failed to get username");
     return pam_res;
   }
@@ -254,13 +263,14 @@ int identify(pam_handle_t *pamh, int flags, int argc, const char **argv,
   posix_spawn_file_actions_t file_actions;
   posix_spawn_file_actions_init(&file_actions);
 
-  const char *const args[] = {
+  const char *const args[] = {"/usr/bin/python3", // NOLINT
-      "/usr/bin/python3", "/lib/security/howdy/compare.py", username, nullptr};
+                              "/lib/security/howdy/compare.py", username,
+                              nullptr};
   pid_t child_pid;
 
   // Start the python subprocess
   if (posix_spawnp(&child_pid, "/usr/bin/python3", &file_actions, nullptr,
-                   (char *const *)args, nullptr) < 0) {
+                   const_cast<char *const *>(args), nullptr) > 0) {
     syslog(LOG_ERR, "Can't spawn the howdy process: %s (%d)", strerror(errno),
            errno);
     return PAM_SYSTEM_ERR;
@@ -268,21 +278,22 @@ int identify(pam_handle_t *pamh, int flags, int argc, const char **argv,
 
   // NOTE: We should replace mutex and condition_variable by atomic wait, but
   // it's too recent (C++20)
-  mutex m;
+  std::mutex m;
-  condition_variable cv;
+  std::condition_variable cv;
-  atomic<Type> confirmation_type(Type::Unset);
+  std::atomic<ConfirmationType> confirmation_type(ConfirmationType::Unset);
 
   // This task wait for the status of the python subprocess (we don't want a
   // zombie process)
-  optional_task<int> child_task(packaged_task<int()>([&] {
+  optional_task<int> child_task(std::packaged_task<int()>([&] {
     int status;
     wait(&status);
 
     {
-      unique_lock<mutex> lk(m);
+      std::unique_lock<std::mutex> lk(m);
-      Type type = confirmation_type.load(memory_order_relaxed);
+      ConfirmationType type = confirmation_type.load(std::memory_order_relaxed);
-      if (type == Type::Unset) {
+      if (type == ConfirmationType::Unset) {
-        confirmation_type.store(Type::Howdy, memory_order_relaxed);
+        confirmation_type.store(ConfirmationType::Howdy,
+                                std::memory_order_relaxed);
       }
     }
     cv.notify_one();
@@ -292,21 +303,24 @@ int identify(pam_handle_t *pamh, int flags, int argc, const char **argv,
   child_task.activate();
 
   // This task waits for the password input (if the workaround wants it)
-  optional_task<tuple<int, char *>> pass_task(
+  optional_task<std::tuple<int, char *>> pass_task(
-      packaged_task<tuple<int, char *>()>([&] {
+      std::packaged_task<std::tuple<int, char *>()>([&] {
         char *auth_tok_ptr = nullptr;
-        int pam_res = pam_get_authtok(pamh, PAM_AUTHTOK,
+        int pam_res =
-                                      (const char **)&auth_tok_ptr, nullptr);
+            pam_get_authtok(pamh, PAM_AUTHTOK,
+                            const_cast<const char **>(&auth_tok_ptr), nullptr);
         {
-          unique_lock<mutex> lk(m);
+          std::unique_lock<std::mutex> lk(m);
-          Type type = confirmation_type.load(memory_order_relaxed);
+          ConfirmationType type =
-          if (type == Type::Unset) {
+              confirmation_type.load(std::memory_order_relaxed);
-            confirmation_type.store(Type::Pam, memory_order_relaxed);
+          if (type == ConfirmationType::Unset) {
+            confirmation_type.store(ConfirmationType::Pam,
+                                    std::memory_order_relaxed);
           }
         }
         cv.notify_one();
 
-        return tuple<int, char *>(pam_res, auth_tok_ptr);
+        return std::tuple<int, char *>(pam_res, auth_tok_ptr);
       }));
 
   if (auth_tok) {
@@ -315,11 +329,11 @@ int identify(pam_handle_t *pamh, int flags, int argc, const char **argv,
 
   // Wait for the end either of the child or the password input
   {
-    unique_lock<mutex> lk(m);
+    std::unique_lock<std::mutex> lk(m);
-    cv.wait(lk, [&] { return confirmation_type != Type::Unset; });
+    cv.wait(lk, [&] { return confirmation_type != ConfirmationType::Unset; });
   }
 
-  if (confirmation_type == Type::Howdy) {
+  if (confirmation_type == ConfirmationType::Howdy) {
     child_task.stop(false);
 
     // If the workaround is native
@@ -333,66 +347,67 @@ int identify(pam_handle_t *pamh, int flags, int argc, const char **argv,
     int howdy_status = child_task.get();
 
     return howdy_msg(username, howdy_status, reader, conv_function);
-  } else {
-    // The password has been entered
-
-    // We need to be sure that we're not going to block forever if the
-    // child has a problem
-    if (child_task.wait(2.5s) == future_status::timeout) {
-      kill(child_pid, SIGTERM);
-    }
-    child_task.stop(false);
-
-    // We just wait for the thread to stop since it's this one which sent us the
-    // confirmation type
-    if (workaround == Workaround::Input && auth_tok) {
-      pass_task.stop(false);
-    }
-
-    char *password = nullptr;
-    tie(pam_res, password) = pass_task.get();
-
-    if (pam_res != PAM_SUCCESS)
-      return pam_res;
-
-    int howdy_status = child_task.get();
-    // If python process (or user) sent Enter key
-    if (strlen(password) == 0) {
-      return howdy_msg(username, howdy_status, reader, conv_function);
-    }
-
-    // The password has been entered, we are passing it to PAM stack
-    return PAM_IGNORE;
   }
+
+  // The password has been entered
+
+  // We need to be sure that we're not going to block forever if the
+  // child has a problem
+  if (child_task.wait(DEFAULT_TIMEOUT) == std::future_status::timeout) {
+    kill(child_pid, SIGTERM);
+  }
+  child_task.stop(false);
+
+  // We just wait for the thread to stop since it's this one which sent us the
+  // confirmation type
+  if (workaround == Workaround::Input && auth_tok) {
+    pass_task.stop(false);
+  }
+
+  char *password = nullptr;
+  std::tie(pam_res, password) = pass_task.get();
+
+  if (pam_res != PAM_SUCCESS) {
+    return pam_res;
+  }
+
+  int howdy_status = child_task.get();
+  // If python process (or user) sent Enter key
+  if (strlen(password) == 0) {
+    return howdy_msg(username, howdy_status, reader, conv_function);
+  }
+
+  // The password has been entered, we are passing it to PAM stack
+  return PAM_IGNORE;
 }
 
 // Called by PAM when a user needs to be authenticated, for example by running
 // the sudo command
-PAM_EXTERN int pam_sm_authenticate(pam_handle_t *pamh, int flags, int argc,
+PAM_EXTERN auto pam_sm_authenticate(pam_handle_t *pamh, int flags, int argc,
-                                   const char **argv) {
+                                    const char **argv) -> int {
   return identify(pamh, flags, argc, argv, true);
 }
 
 // Called by PAM when a session is started, such as by the su command
-PAM_EXTERN int pam_sm_open_session(pam_handle_t *pamh, int flags, int argc,
+PAM_EXTERN auto pam_sm_open_session(pam_handle_t *pamh, int flags, int argc,
-                                   const char **argv) {
+                                    const char **argv) -> int {
   return identify(pamh, flags, argc, argv, false);
 }
 
 // The functions below are required by PAM, but not needed in this module
-PAM_EXTERN int pam_sm_acct_mgmt(pam_handle_t *pamh, int flags, int argc,
+PAM_EXTERN auto pam_sm_acct_mgmt(pam_handle_t *pamh, int flags, int argc,
-                                const char **argv) {
+                                 const char **argv) -> int {
   return PAM_IGNORE;
 }
-PAM_EXTERN int pam_sm_close_session(pam_handle_t *pamh, int flags, int argc,
+PAM_EXTERN auto pam_sm_close_session(pam_handle_t *pamh, int flags, int argc,
-                                    const char **argv) {
+                                     const char **argv) -> int {
   return PAM_IGNORE;
 }
-PAM_EXTERN int pam_sm_chauthtok(pam_handle_t *pamh, int flags, int argc,
+PAM_EXTERN auto pam_sm_chauthtok(pam_handle_t *pamh, int flags, int argc,
-                                const char **argv) {
+                                 const char **argv) -> int {
   return PAM_IGNORE;
 }
-PAM_EXTERN int pam_sm_setcred(pam_handle_t *pamh, int flags, int argc,
+PAM_EXTERN auto pam_sm_setcred(pam_handle_t *pamh, int flags, int argc,
-                              const char **argv) {
+                               const char **argv) -> int {
   return PAM_IGNORE;
 }

src/pam/main.hh

@@ -4,16 +4,18 @@
 #include <cstdint>
 #include <string>
 
-enum class Type { Unset, Howdy, Pam };
+enum class ConfirmationType { Unset, Howdy, Pam };
 
 enum class Workaround { Off, Input, Native };
 
-inline Workaround get_workaround(std::string workaround) {
+inline auto get_workaround(const std::string &workaround) -> Workaround {
-  if (workaround == "input")
+  if (workaround == "input") {
     return Workaround::Input;
+  }
 
-  if (workaround == "native")
+  if (workaround == "native") {
     return Workaround::Native;
+  }
 
   return Workaround::Off;
 }

src/pam/optional_task.hh

@@ -14,12 +14,13 @@ template <typename T> class optional_task {
   std::atomic<bool> _is_active;
 
 public:
-  optional_task(std::packaged_task<T()>);
+  explicit optional_task(std::packaged_task<T()> task);
   void activate();
-  template <typename Dur> std::future_status wait(std::chrono::duration<Dur>);
+  template <typename Dur, typename Rat>
-  T get();
+  auto wait(std::chrono::duration<Dur, Rat> dur) -> std::future_status;
-  bool is_active();
+  auto get() -> T;
-  void stop(bool);
+  auto is_active() -> bool;
+  void stop(bool force);
   ~optional_task();
 };
 
@@ -34,17 +35,20 @@ template <typename T> void optional_task<T>::activate() {
 }
 
 template <typename T>
-template <typename Dur>
+template <typename Dur, typename Rat>
-std::future_status optional_task<T>::wait(std::chrono::duration<Dur> dur) {
+auto optional_task<T>::wait(std::chrono::duration<Dur, Rat> dur)
+    -> std::future_status {
   return _future.wait_for(dur);
 }
 
-template <typename T> T optional_task<T>::get() {
+template <typename T> auto optional_task<T>::get() -> T {
   assert(!_is_active && _spawned);
   return _future.get();
 }
 
-template <typename T> bool optional_task<T>::is_active() { return _is_active; }
+template <typename T> auto optional_task<T>::is_active() -> bool {
+  return _is_active;
+}
 
 template <typename T> void optional_task<T>::stop(bool force) {
   if (!(_is_active && _thread.joinable()) && _spawned) {
@@ -62,8 +66,9 @@ template <typename T> void optional_task<T>::stop(bool force) {
 }
 
 template <typename T> optional_task<T>::~optional_task<T>() {
-  if (_is_active && _spawned)
+  if (_is_active && _spawned) {
     stop(false);
+  }
 }
 
 #endif // OPTIONAL_TASK_H_