From 59cbabf7342884732fd79e4dfe77d2766b03ac96 Mon Sep 17 00:00:00 2001 From: MusiKid Date: Fri, 21 Jan 2022 20:50:27 +0100 Subject: [PATCH] refactor: apply suggestions from clang-tidy --- src/pam/main.cc | 203 +++++++++++++++++++++------------------ src/pam/main.hh | 10 +- src/pam/optional_task.hh | 25 +++-- 3 files changed, 130 insertions(+), 108 deletions(-) diff --git a/src/pam/main.cc b/src/pam/main.cc index 067ae56..e0eccce 100644 --- a/src/pam/main.cc +++ b/src/pam/main.cc @@ -1,3 +1,4 @@ +#include #include #include #include @@ -41,8 +42,10 @@ #include "main.hh" #include "optional_task.hh" -using namespace std; -using namespace boost::locale; +const auto DEFAULT_TIMEOUT = + std::chrono::duration(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 conv_function) { +auto howdy_error(int status, + const std::function &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 conv_function) { switch (status) { // Status 10 means we couldn't find any face models case 10: - conv_function(PAM_ERROR_MSG, - dgettext("pam", "There is no face model known")); + conv_function(PAM_ERROR_MSG, S("There is no face model known").c_str()); 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 conv_function) { break; // Status 13 means the image was too dark case 13: - conv_function(PAM_ERROR_MSG, - dgettext("pam", "Face detection image too dark")); + conv_function(PAM_ERROR_MSG, S("Face detection image too dark").c_str()); 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 conv_function) { * @param conv_function PAM conversation function * @return Returns the conversation function return code */ -int howdy_msg(char *username, int status, INIReader &reader, - function conv_function) { +auto howdy_msg(char *username, int status, INIReader &reader, + const std::function &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 {}"); - string identify_msg = - confirm_text.replace(confirm_text.find("{}"), 2, string(username)); + std::string confirm_text = S("Identified face as {}"); + std::string identify_msg = + 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, - bool auth_tok) { +auto identify(pam_handle_t *pamh, int flags, int argc, const char **argv, + 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(reinterpret_cast(&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)) != - PAM_SUCCESS) { + if ((pam_res = pam_get_item(pamh, PAM_CONV, conv_ptr)) != 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 = - bind(send_message, conv, placeholders::_1, placeholders::_2); + auto conv_function = [conv](int msg_type, const char *msg) { + return send_message(conv, std::forward(msg_type), + std::forward(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])); - string lid_state; - getline(file, lid_state, (char)file.eof()); + std::ifstream file(std::string(glob_result.gl_pathv[i])); + std::string lid_state; + std::getline(file, lid_state, static_cast(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( - PAM_TEXT_INFO, - dgettext("pam", "Attempting facial authentication"))) != + if ((conv_function(PAM_TEXT_INFO, + S("Attempting facial authentication").c_str())) != 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)) != - PAM_SUCCESS) { + if ((pam_res = pam_get_user(pamh, const_cast(&username), + 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[] = { - "/usr/bin/python3", "/lib/security/howdy/compare.py", username, nullptr}; + const char *const args[] = {"/usr/bin/python3", // NOLINT + "/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(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; - condition_variable cv; - atomic confirmation_type(Type::Unset); + std::mutex m; + std::condition_variable cv; + std::atomic confirmation_type(ConfirmationType::Unset); // This task wait for the status of the python subprocess (we don't want a // zombie process) - optional_task child_task(packaged_task([&] { + optional_task child_task(std::packaged_task([&] { int status; wait(&status); { - unique_lock lk(m); - Type type = confirmation_type.load(memory_order_relaxed); - if (type == Type::Unset) { - confirmation_type.store(Type::Howdy, memory_order_relaxed); + std::unique_lock lk(m); + ConfirmationType type = confirmation_type.load(std::memory_order_relaxed); + if (type == ConfirmationType::Unset) { + 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> pass_task( - packaged_task()>([&] { + optional_task> pass_task( + std::packaged_task()>([&] { char *auth_tok_ptr = nullptr; - int pam_res = pam_get_authtok(pamh, PAM_AUTHTOK, - (const char **)&auth_tok_ptr, nullptr); + int pam_res = + pam_get_authtok(pamh, PAM_AUTHTOK, + const_cast(&auth_tok_ptr), nullptr); { - unique_lock lk(m); - Type type = confirmation_type.load(memory_order_relaxed); - if (type == Type::Unset) { - confirmation_type.store(Type::Pam, memory_order_relaxed); + std::unique_lock lk(m); + ConfirmationType type = + confirmation_type.load(std::memory_order_relaxed); + if (type == ConfirmationType::Unset) { + confirmation_type.store(ConfirmationType::Pam, + std::memory_order_relaxed); } } cv.notify_one(); - return tuple(pam_res, auth_tok_ptr); + return std::tuple(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 lk(m); - cv.wait(lk, [&] { return confirmation_type != Type::Unset; }); + std::unique_lock lk(m); + 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, - const char **argv) { +PAM_EXTERN auto pam_sm_authenticate(pam_handle_t *pamh, int flags, int argc, + 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, - const char **argv) { +PAM_EXTERN auto pam_sm_open_session(pam_handle_t *pamh, int flags, int argc, + 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, - const char **argv) { +PAM_EXTERN auto pam_sm_acct_mgmt(pam_handle_t *pamh, int flags, int argc, + const char **argv) -> int { return PAM_IGNORE; } -PAM_EXTERN int pam_sm_close_session(pam_handle_t *pamh, int flags, int argc, - const char **argv) { +PAM_EXTERN auto pam_sm_close_session(pam_handle_t *pamh, int flags, int argc, + const char **argv) -> int { return PAM_IGNORE; } -PAM_EXTERN int pam_sm_chauthtok(pam_handle_t *pamh, int flags, int argc, - const char **argv) { +PAM_EXTERN auto pam_sm_chauthtok(pam_handle_t *pamh, int flags, int argc, + const char **argv) -> int { return PAM_IGNORE; } -PAM_EXTERN int pam_sm_setcred(pam_handle_t *pamh, int flags, int argc, - const char **argv) { +PAM_EXTERN auto pam_sm_setcred(pam_handle_t *pamh, int flags, int argc, + const char **argv) -> int { return PAM_IGNORE; } diff --git a/src/pam/main.hh b/src/pam/main.hh index df05f9a..17a6626 100644 --- a/src/pam/main.hh +++ b/src/pam/main.hh @@ -4,16 +4,18 @@ #include #include -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) { - if (workaround == "input") +inline auto get_workaround(const std::string &workaround) -> Workaround { + if (workaround == "input") { return Workaround::Input; + } - if (workaround == "native") + if (workaround == "native") { return Workaround::Native; + } return Workaround::Off; } diff --git a/src/pam/optional_task.hh b/src/pam/optional_task.hh index 827c535..892836c 100644 --- a/src/pam/optional_task.hh +++ b/src/pam/optional_task.hh @@ -14,12 +14,13 @@ template class optional_task { std::atomic _is_active; public: - optional_task(std::packaged_task); + explicit optional_task(std::packaged_task task); void activate(); - template std::future_status wait(std::chrono::duration); - T get(); - bool is_active(); - void stop(bool); + template + auto wait(std::chrono::duration dur) -> std::future_status; + auto get() -> T; + auto is_active() -> bool; + void stop(bool force); ~optional_task(); }; @@ -34,17 +35,20 @@ template void optional_task::activate() { } template -template -std::future_status optional_task::wait(std::chrono::duration dur) { +template +auto optional_task::wait(std::chrono::duration dur) + -> std::future_status { return _future.wait_for(dur); } -template T optional_task::get() { +template auto optional_task::get() -> T { assert(!_is_active && _spawned); return _future.get(); } -template bool optional_task::is_active() { return _is_active; } +template auto optional_task::is_active() -> bool { + return _is_active; +} template void optional_task::stop(bool force) { if (!(_is_active && _thread.joinable()) && _spawned) { @@ -62,8 +66,9 @@ template void optional_task::stop(bool force) { } template optional_task::~optional_task() { - if (_is_active && _spawned) + if (_is_active && _spawned) { stop(false); + } } #endif // OPTIONAL_TASK_H_