From b0fcb7c045c2b82ee804c6e6b67543cbe56c4cbc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 4 Oct 2022 12:37:49 +0200 Subject: [PATCH 01/27] sd-bus: decrease indentation --- src/libsystemd/sd-bus/bus-error.c | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/src/libsystemd/sd-bus/bus-error.c b/src/libsystemd/sd-bus/bus-error.c index 218677eac4..aca3ac36d2 100644 --- a/src/libsystemd/sd-bus/bus-error.c +++ b/src/libsystemd/sd-bus/bus-error.c @@ -587,16 +587,13 @@ _public_ int sd_bus_error_set_errnof(sd_bus_error *e, int error, const char *for } const char *bus_error_message(const sd_bus_error *e, int error) { + /* Sometimes, the D-Bus server is a little bit too verbose with + * its error messages, so let's override them here */ + if (sd_bus_error_has_name(e, SD_BUS_ERROR_ACCESS_DENIED)) + return "Access denied"; - if (e) { - /* Sometimes, the D-Bus server is a little bit too verbose with - * its error messages, so let's override them here */ - if (sd_bus_error_has_name(e, SD_BUS_ERROR_ACCESS_DENIED)) - return "Access denied"; - - if (e->message) - return e->message; - } + if (e && e->message) + return e->message; return strerror_safe(error); } From 2b9e90554087154126fedb5f8a70abaefececb0c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 4 Oct 2022 14:14:13 +0200 Subject: [PATCH 02/27] pam_systemd_home: inline loop variable declaration --- src/home/pam_systemd_home.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/home/pam_systemd_home.c b/src/home/pam_systemd_home.c index 35195c2ef0..cddb7b3a2c 100644 --- a/src/home/pam_systemd_home.c +++ b/src/home/pam_systemd_home.c @@ -25,12 +25,10 @@ static int parse_argv( bool *please_suspend, bool *debug) { - int i; - assert(argc >= 0); assert(argc == 0 || argv); - for (i = 0; i < argc; i++) { + for (int i = 0; i < argc; i++) { const char *v; if ((v = startswith(argv[i], "suspend="))) { From 6b7834fe5de3de690e6efb9467c61691c4b2f30f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Thu, 6 Oct 2022 13:07:49 +0200 Subject: [PATCH 03/27] basic/log: include the log syntax callback in the errno protection block In general, log_syntax_internal() must keep errno unchanged. But the call to log_syntax_callback() was added outside of the block protected by PROTECT_ERRNO. --- src/basic/log.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/src/basic/log.c b/src/basic/log.c index c5e16d669b..2f5353c739 100644 --- a/src/basic/log.c +++ b/src/basic/log.c @@ -1341,18 +1341,19 @@ int log_syntax_internal( const char *func, const char *format, ...) { + PROTECT_ERRNO; + if (log_syntax_callback) log_syntax_callback(unit, level, log_syntax_callback_userdata); - PROTECT_ERRNO; - char buffer[LINE_MAX]; - va_list ap; - const char *unit_fmt = NULL; - if (_likely_(LOG_PRI(level) > log_max_level) || log_target == LOG_TARGET_NULL) return -ERRNO_VALUE(error); + char buffer[LINE_MAX]; + va_list ap; + const char *unit_fmt = NULL; + errno = ERRNO_VALUE(error); va_start(ap, format); From 50c5b991df302e7d143e8a859dd9d95c419d60fc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Thu, 6 Oct 2022 14:41:50 +0200 Subject: [PATCH 04/27] basic/errno-util: add helper to protect and set errno in one step This pattern is used in a few places. Those are changed in this patch. Subsequent patches will add more. --- src/basic/errno-util.h | 4 ++++ src/basic/log.c | 9 +++------ 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/src/basic/errno-util.h b/src/basic/errno-util.h index 648de50eb4..a71864ca60 100644 --- a/src/basic/errno-util.h +++ b/src/basic/errno-util.h @@ -22,6 +22,10 @@ static inline void _reset_errno_(int *saved_errno) { _saved_errno_ = -1; \ } while (false) +#define LOCAL_ERRNO(value) \ + PROTECT_ERRNO; \ + errno = abs(value) + static inline int negative_errno(void) { /* This helper should be used to shut up gcc if you know 'errno' is * negative. Instead of "return -errno;", use "return negative_errno();" diff --git a/src/basic/log.c b/src/basic/log.c index 2f5353c739..39d08b0928 100644 --- a/src/basic/log.c +++ b/src/basic/log.c @@ -732,14 +732,12 @@ int log_internalv( const char *format, va_list ap) { - char buffer[LINE_MAX]; - PROTECT_ERRNO; - if (_likely_(LOG_PRI(level) > log_max_level)) return -ERRNO_VALUE(error); /* Make sure that %m maps to the specified error (or "Success"). */ - errno = ERRNO_VALUE(error); + char buffer[LINE_MAX]; + LOCAL_ERRNO(ERRNO_VALUE(error)); (void) vsnprintf(buffer, sizeof buffer, format, ap); @@ -777,14 +775,13 @@ int log_object_internalv( const char *format, va_list ap) { - PROTECT_ERRNO; char *buffer, *b; if (_likely_(LOG_PRI(level) > log_max_level)) return -ERRNO_VALUE(error); /* Make sure that %m maps to the specified error (or "Success"). */ - errno = ERRNO_VALUE(error); + LOCAL_ERRNO(ERRNO_VALUE(error)); /* Prepend the object name before the message */ if (object) { From b1eff892bb7c4ba2fac2b1fdf54d86534aaf2d08 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 4 Oct 2022 14:19:12 +0200 Subject: [PATCH 05/27] shared/pam-util: add pam_syslog_errno() wrapper that sets errno So far our pam code was using strerror_safe(). But that's not a good approach, because strerror_safe() is not thread-safe, and the pam code is "library code" that should be thread-safe. In fact, the whole effort to use strerror() is unnecessary, because pam_syslog() is documented to support %m. The implementation in linux-pam simply uses vasprintf(). If we use %m too, we get rid of the issue. The wrapper sets errno temporarily from the argument. Apparently some PAM consumers run multiple PAM stacks in threads, so we should avoid non-thread-safe code. The new helper returns PAM_BUF_ERR for ENOMEM, and PAM_SERVICE_ERR in other cases. This may change the returned code in some cases, but I think a) it doesn't matter much, b) it's probably for the better. E.g. we might now return PAM_SERVICE_ERR if the dbus message is borked, and PAM_SERVICE_ERR seems appropriate. --- src/shared/pam-util.c | 28 ++++++++++------------------ src/shared/pam-util.h | 19 ++++++++++++++++--- 2 files changed, 26 insertions(+), 21 deletions(-) diff --git a/src/shared/pam-util.c b/src/shared/pam-util.c index 621e7fe802..d9fee22cc6 100644 --- a/src/shared/pam-util.c +++ b/src/shared/pam-util.c @@ -9,22 +9,16 @@ #include "macro.h" #include "pam-util.h" -int pam_log_oom(pam_handle_t *handle) { - /* This is like log_oom(), but uses PAM logging */ - pam_syslog(handle, LOG_ERR, "Out of memory."); - return PAM_BUF_ERR; -} +int pam_syslog_errno(pam_handle_t *handle, int level, int error, const char *format, ...) { + va_list ap; -int pam_bus_log_create_error(pam_handle_t *handle, int r) { - /* This is like bus_log_create_error(), but uses PAM logging */ - pam_syslog(handle, LOG_ERR, "Failed to create bus message: %s", strerror_safe(r)); - return PAM_BUF_ERR; -} + LOCAL_ERRNO(error); -int pam_bus_log_parse_error(pam_handle_t *handle, int r) { - /* This is like bus_log_parse_error(), but uses PAM logging */ - pam_syslog(handle, LOG_ERR, "Failed to parse bus message: %s", strerror_safe(r)); - return PAM_BUF_ERR; + va_start(ap, format); + pam_vsyslog(handle, LOG_ERR, format, ap); + va_end(ap); + + return error == -ENOMEM ? PAM_BUF_ERR : PAM_SERVICE_ERR; } static void cleanup_system_bus(pam_handle_t *handle, void *data, int error_status) { @@ -50,10 +44,8 @@ int pam_acquire_bus_connection(pam_handle_t *handle, sd_bus **ret) { } r = sd_bus_open_system(&bus); - if (r < 0) { - pam_syslog(handle, LOG_ERR, "Failed to connect to system bus: %s", strerror_safe(r)); - return PAM_SERVICE_ERR; - } + if (r < 0) + return pam_syslog_errno(handle, LOG_ERR, r, "Failed to connect to system bus: %m"); r = pam_set_data(handle, "systemd-system-bus", bus, cleanup_system_bus); if (r != PAM_SUCCESS) { diff --git a/src/shared/pam-util.h b/src/shared/pam-util.h index 41f1835b3c..4a97d2d132 100644 --- a/src/shared/pam-util.h +++ b/src/shared/pam-util.h @@ -5,9 +5,22 @@ #include "sd-bus.h" -int pam_log_oom(pam_handle_t *handle); -int pam_bus_log_create_error(pam_handle_t *handle, int r); -int pam_bus_log_parse_error(pam_handle_t *handle, int r); +int pam_syslog_errno(pam_handle_t *handle, int level, int error, const char *format, ...) _printf_(4,5); + +static inline int pam_log_oom(pam_handle_t *handle) { + /* This is like log_oom(), but uses PAM logging */ + return pam_syslog_errno(handle, LOG_ERR, ENOMEM, "Out of memory."); +} + +static inline int pam_bus_log_create_error(pam_handle_t *handle, int r) { + /* This is like bus_log_create_error(), but uses PAM logging */ + return pam_syslog_errno(handle, LOG_ERR, r, "Failed to create bus message: %m"); +} + +static inline int pam_bus_log_parse_error(pam_handle_t *handle, int r) { + /* This is like bus_log_parse_error(), but uses PAM logging */ + return pam_syslog_errno(handle, LOG_ERR, r, "Failed to parse bus message: %m"); +} int pam_acquire_bus_connection(pam_handle_t *handle, sd_bus **ret); int pam_release_bus_connection(pam_handle_t *handle); From 544ec3c0abd15eabb86ad2da7cea698ba736d59d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 4 Oct 2022 14:25:02 +0200 Subject: [PATCH 06/27] pam_systemd_home: use pam_syslog_errno() Like in the previous commit, the return code on error might be changed. --- src/home/pam_systemd_home.c | 101 +++++++++++++----------------------- 1 file changed, 36 insertions(+), 65 deletions(-) diff --git a/src/home/pam_systemd_home.c b/src/home/pam_systemd_home.c index cddb7b3a2c..7f613c16d7 100644 --- a/src/home/pam_systemd_home.c +++ b/src/home/pam_systemd_home.c @@ -205,20 +205,16 @@ static int acquire_user_record( } r = json_parse(json, JSON_PARSE_SENSITIVE, &v, NULL, NULL); - if (r < 0) { - pam_syslog(handle, LOG_ERR, "Failed to parse JSON user record: %s", strerror_safe(r)); - return PAM_SERVICE_ERR; - } + if (r < 0) + return pam_syslog_errno(handle, LOG_ERR, r, "Failed to parse JSON user record: %m"); ur = user_record_new(); if (!ur) return pam_log_oom(handle); r = user_record_load(ur, v, USER_RECORD_LOAD_REFUSE_SECRET|USER_RECORD_PERMISSIVE); - if (r < 0) { - pam_syslog(handle, LOG_ERR, "Failed to load user record: %s", strerror_safe(r)); - return PAM_SERVICE_ERR; - } + if (r < 0) + return pam_syslog_errno(handle, LOG_ERR, r, "Failed to load user record: %m"); /* Safety check if cached record actually matches what we are looking for */ if (!streq_ptr(username, ur->user_name)) { @@ -317,10 +313,8 @@ static int handle_generic_user_record_error( } r = user_record_set_password(secret, STRV_MAKE(newp), true); - if (r < 0) { - pam_syslog(handle, LOG_ERR, "Failed to store password: %s", strerror_safe(r)); - return PAM_SERVICE_ERR; - } + if (r < 0) + return pam_syslog_errno(handle, LOG_ERR, r, "Failed to store password: %m"); } else if (sd_bus_error_has_name(error, BUS_ERROR_BAD_RECOVERY_KEY)) { _cleanup_(erase_and_freep) char *newp = NULL; @@ -344,10 +338,8 @@ static int handle_generic_user_record_error( } r = user_record_set_password(secret, STRV_MAKE(newp), true); - if (r < 0) { - pam_syslog(handle, LOG_ERR, "Failed to store recovery key: %s", strerror_safe(r)); - return PAM_SERVICE_ERR; - } + if (r < 0) + return pam_syslog_errno(handle, LOG_ERR, r, "Failed to store recovery key: %m"); } else if (sd_bus_error_has_name(error, BUS_ERROR_BAD_PASSWORD_AND_NO_TOKEN)) { _cleanup_(erase_and_freep) char *newp = NULL; @@ -370,10 +362,8 @@ static int handle_generic_user_record_error( } r = user_record_set_password(secret, STRV_MAKE(newp), true); - if (r < 0) { - pam_syslog(handle, LOG_ERR, "Failed to store password: %s", strerror_safe(r)); - return PAM_SERVICE_ERR; - } + if (r < 0) + return pam_syslog_errno(handle, LOG_ERR, r, "Failed to store password: %m"); } else if (sd_bus_error_has_name(error, BUS_ERROR_TOKEN_PIN_NEEDED)) { _cleanup_(erase_and_freep) char *newp = NULL; @@ -390,10 +380,8 @@ static int handle_generic_user_record_error( } r = user_record_set_token_pin(secret, STRV_MAKE(newp), false); - if (r < 0) { - pam_syslog(handle, LOG_ERR, "Failed to store PIN: %s", strerror_safe(r)); - return PAM_SERVICE_ERR; - } + if (r < 0) + return pam_syslog_errno(handle, LOG_ERR, r, "Failed to store PIN: %m"); } else if (sd_bus_error_has_name(error, BUS_ERROR_TOKEN_PROTECTED_AUTHENTICATION_PATH_NEEDED)) { @@ -402,10 +390,9 @@ static int handle_generic_user_record_error( (void) pam_prompt(handle, PAM_ERROR_MSG, NULL, "Please authenticate physically on security token of user %s.", user_name); r = user_record_set_pkcs11_protected_authentication_path_permitted(secret, true); - if (r < 0) { - pam_syslog(handle, LOG_ERR, "Failed to set PKCS#11 protected authentication path permitted flag: %s", strerror_safe(r)); - return PAM_SERVICE_ERR; - } + if (r < 0) + return pam_syslog_errno(handle, LOG_ERR, r, + "Failed to set PKCS#11 protected authentication path permitted flag: %m"); } else if (sd_bus_error_has_name(error, BUS_ERROR_TOKEN_USER_PRESENCE_NEEDED)) { @@ -414,10 +401,9 @@ static int handle_generic_user_record_error( (void) pam_prompt(handle, PAM_ERROR_MSG, NULL, "Please confirm presence on security token of user %s.", user_name); r = user_record_set_fido2_user_presence_permitted(secret, true); - if (r < 0) { - pam_syslog(handle, LOG_ERR, "Failed to set FIDO2 user presence permitted flag: %s", strerror_safe(r)); - return PAM_SERVICE_ERR; - } + if (r < 0) + return pam_syslog_errno(handle, LOG_ERR, r, + "Failed to set FIDO2 user presence permitted flag: %m"); } else if (sd_bus_error_has_name(error, BUS_ERROR_TOKEN_USER_VERIFICATION_NEEDED)) { @@ -426,10 +412,9 @@ static int handle_generic_user_record_error( (void) pam_prompt(handle, PAM_ERROR_MSG, NULL, "Please verify user on security token of user %s.", user_name); r = user_record_set_fido2_user_verification_permitted(secret, true); - if (r < 0) { - pam_syslog(handle, LOG_ERR, "Failed to set FIDO2 user verification permitted flag: %s", strerror_safe(r)); - return PAM_SERVICE_ERR; - } + if (r < 0) + return pam_syslog_errno(handle, LOG_ERR, r, + "Failed to set FIDO2 user verification permitted flag: %m"); } else if (sd_bus_error_has_name(error, BUS_ERROR_TOKEN_PIN_LOCKED)) { @@ -452,10 +437,8 @@ static int handle_generic_user_record_error( } r = user_record_set_token_pin(secret, STRV_MAKE(newp), false); - if (r < 0) { - pam_syslog(handle, LOG_ERR, "Failed to store PIN: %s", strerror_safe(r)); - return PAM_SERVICE_ERR; - } + if (r < 0) + return pam_syslog_errno(handle, LOG_ERR, r, "Failed to store PIN: %m"); } else if (sd_bus_error_has_name(error, BUS_ERROR_TOKEN_BAD_PIN_FEW_TRIES_LEFT)) { _cleanup_(erase_and_freep) char *newp = NULL; @@ -473,10 +456,8 @@ static int handle_generic_user_record_error( } r = user_record_set_token_pin(secret, STRV_MAKE(newp), false); - if (r < 0) { - pam_syslog(handle, LOG_ERR, "Failed to store PIN: %s", strerror_safe(r)); - return PAM_SERVICE_ERR; - } + if (r < 0) + return pam_syslog_errno(handle, LOG_ERR, r, "Failed to store PIN: %m"); } else if (sd_bus_error_has_name(error, BUS_ERROR_TOKEN_BAD_PIN_ONE_TRY_LEFT)) { _cleanup_(erase_and_freep) char *newp = NULL; @@ -494,10 +475,8 @@ static int handle_generic_user_record_error( } r = user_record_set_token_pin(secret, STRV_MAKE(newp), false); - if (r < 0) { - pam_syslog(handle, LOG_ERR, "Failed to store PIN: %s", strerror_safe(r)); - return PAM_SERVICE_ERR; - } + if (r < 0) + return pam_syslog_errno(handle, LOG_ERR, r, "Failed to store PIN: %m"); } else { pam_syslog(handle, LOG_ERR, "Failed to acquire home for user %s: %s", user_name, bus_error_message(error, ret)); @@ -595,10 +574,8 @@ static int acquire_home( if (!isempty(cached_password)) { r = user_record_set_password(secret, STRV_MAKE(cached_password), true); - if (r < 0) { - pam_syslog(handle, LOG_ERR, "Failed to store password: %s", strerror_safe(r)); - return PAM_SERVICE_ERR; - } + if (r < 0) + return pam_syslog_errno(handle, LOG_ERR, r, "Failed to store password: %m"); } } @@ -659,11 +636,9 @@ static int acquire_home( return pam_bus_log_parse_error(handle, r); acquired_fd = fcntl(fd, F_DUPFD_CLOEXEC, 3); - if (acquired_fd < 0) { - pam_syslog(handle, LOG_ERR, "Failed to duplicate acquired fd: %s", bus_error_message(&error, r)); - return PAM_SERVICE_ERR; - } - + if (acquired_fd < 0) + return pam_syslog_errno(handle, LOG_ERR, errno, + "Failed to duplicate acquired fd: %m"); break; } @@ -1060,10 +1035,8 @@ _public_ PAM_EXTERN int pam_sm_chauthtok( if (!isempty(old_password)) { r = user_record_set_password(old_secret, STRV_MAKE(old_password), true); - if (r < 0) { - pam_syslog(handle, LOG_ERR, "Failed to store old password: %s", strerror_safe(r)); - return PAM_SERVICE_ERR; - } + if (r < 0) + return pam_syslog_errno(handle, LOG_ERR, r, "Failed to store old password: %m"); } new_secret = user_record_new(); @@ -1071,10 +1044,8 @@ _public_ PAM_EXTERN int pam_sm_chauthtok( return pam_log_oom(handle); r = user_record_set_password(new_secret, STRV_MAKE(new_password), true); - if (r < 0) { - pam_syslog(handle, LOG_ERR, "Failed to store new password: %s", strerror_safe(r)); - return PAM_SERVICE_ERR; - } + if (r < 0) + return pam_syslog_errno(handle, LOG_ERR, r, "Failed to store new password: %m"); for (;;) { _cleanup_(sd_bus_error_free) sd_bus_error error = SD_BUS_ERROR_NULL; From 7e7b53b4a7dec7dd4ecb7188d4ab890c2fad85bd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 4 Oct 2022 14:29:53 +0200 Subject: [PATCH 07/27] pam_systemd: use pam_syslog_errno() The debug message for "nice" is tweaked to show the level that was set. Like in the two previous commits, the return code on error might be changed. --- src/login/pam_systemd.c | 40 ++++++++++++++++++---------------------- 1 file changed, 18 insertions(+), 22 deletions(-) diff --git a/src/login/pam_systemd.c b/src/login/pam_systemd.c index faaf6c9a69..7a624d2bd7 100644 --- a/src/login/pam_systemd.c +++ b/src/login/pam_systemd.c @@ -133,20 +133,16 @@ static int acquire_user_record( /* Parse cached record */ r = json_parse(json, JSON_PARSE_SENSITIVE, &v, NULL, NULL); - if (r < 0) { - pam_syslog(handle, LOG_ERR, "Failed to parse JSON user record: %s", strerror_safe(r)); - return PAM_SERVICE_ERR; - } + if (r < 0) + return pam_syslog_errno(handle, LOG_ERR, r, "Failed to parse JSON user record: %m"); ur = user_record_new(); if (!ur) return pam_log_oom(handle); r = user_record_load(ur, v, USER_RECORD_LOAD_REFUSE_SECRET|USER_RECORD_PERMISSIVE); - if (r < 0) { - pam_syslog(handle, LOG_ERR, "Failed to load user record: %s", strerror_safe(r)); - return PAM_SERVICE_ERR; - } + if (r < 0) + return pam_syslog_errno(handle, LOG_ERR, r, "Failed to load user record: %m"); /* Safety check if cached record actually matches what we are looking for */ if (!streq_ptr(username, ur->user_name)) { @@ -159,15 +155,13 @@ static int acquire_user_record( /* Request the record ourselves */ r = userdb_by_name(username, 0, &ur); if (r < 0) { - pam_syslog(handle, LOG_ERR, "Failed to get user record: %s", strerror_safe(r)); + pam_syslog_errno(handle, LOG_ERR, r, "Failed to get user record: %m"); return PAM_USER_UNKNOWN; } r = json_variant_format(ur->json, 0, &formatted); - if (r < 0) { - pam_syslog(handle, LOG_ERR, "Failed to format user JSON: %s", strerror_safe(r)); - return PAM_SERVICE_ERR; - } + if (r < 0) + return pam_syslog_errno(handle, LOG_ERR, r, "Failed to format user JSON: %m"); /* And cache it for everyone else */ r = pam_set_data(handle, field, formatted, pam_cleanup_free); @@ -495,7 +489,7 @@ static bool validate_runtime_directory(pam_handle_t *handle, const char *path, u } if (lstat(path, &st) < 0) { - pam_syslog(handle, LOG_ERR, "Failed to stat() runtime directory '%s': %s", path, strerror_safe(errno)); + pam_syslog_errno(handle, LOG_ERR, errno, "Failed to stat() runtime directory '%s': %m", path); goto fail; } @@ -627,9 +621,11 @@ static int apply_user_record_settings(pam_handle_t *handle, UserRecord *ur, bool if (nice_is_valid(ur->nice_level)) { if (nice(ur->nice_level) < 0) - pam_syslog(handle, LOG_ERR, "Failed to set nice level to %i, ignoring: %s", ur->nice_level, strerror_safe(errno)); + pam_syslog_errno(handle, LOG_ERR, errno, + "Failed to set nice level to %i, ignoring: %m", ur->nice_level); else if (debug) - pam_syslog(handle, LOG_DEBUG, "Nice level set, based on user record."); + pam_syslog(handle, LOG_DEBUG, + "Nice level set to %i, based on user record.", ur->nice_level); } for (int rl = 0; rl < _RLIMIT_MAX; rl++) { @@ -639,9 +635,11 @@ static int apply_user_record_settings(pam_handle_t *handle, UserRecord *ur, bool r = setrlimit_closest(rl, ur->rlimits[rl]); if (r < 0) - pam_syslog(handle, LOG_ERR, "Failed to set resource limit %s, ignoring: %s", rlimit_to_string(rl), strerror_safe(r)); + pam_syslog_errno(handle, LOG_ERR, r, + "Failed to set resource limit %s, ignoring: %m", rlimit_to_string(rl)); else if (debug) - pam_syslog(handle, LOG_DEBUG, "Resource limit %s set, based on user record.", rlimit_to_string(rl)); + pam_syslog(handle, LOG_DEBUG, + "Resource limit %s set, based on user record.", rlimit_to_string(rl)); } return PAM_SUCCESS; @@ -1001,10 +999,8 @@ _public_ PAM_EXTERN int pam_sm_open_session( if (session_fd >= 0) { session_fd = fcntl(session_fd, F_DUPFD_CLOEXEC, 3); - if (session_fd < 0) { - pam_syslog(handle, LOG_ERR, "Failed to dup session fd: %m"); - return PAM_SESSION_ERR; - } + if (session_fd < 0) + return pam_syslog_errno(handle, LOG_ERR, errno, "Failed to dup session fd: %m"); r = pam_set_data(handle, "systemd.session-fd", FD_TO_PTR(session_fd), NULL); if (r != PAM_SUCCESS) { From 4ac6ac9f09be05f17e454a3abc5f388f9ac51916 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 4 Oct 2022 15:47:12 +0200 Subject: [PATCH 08/27] shared/pam-util: add pam_syslog_pam_error() wrapper This is a primitive helper that wraps calls to pam_syslog() replacing @PAMERR@ with pam_strerror() output in the format string. This allows for a bunch of boilerplate to be removed. @PAMERR@ is only supported at the end of the string. Similarly to %m, realistically that's the only place where it is useful. Note that unlike in logging functions in log.[ch], here the error value is only used for the message and is not saved anywhere, so we don't need to care about SYNTHETIC_ERRNO. --- src/shared/pam-util.c | 44 +++++++++++++++++++++++++++++++++---------- src/shared/pam-util.h | 2 ++ 2 files changed, 36 insertions(+), 10 deletions(-) diff --git a/src/shared/pam-util.c b/src/shared/pam-util.c index d9fee22cc6..9d74e08a2b 100644 --- a/src/shared/pam-util.c +++ b/src/shared/pam-util.c @@ -8,6 +8,8 @@ #include "errno-util.h" #include "macro.h" #include "pam-util.h" +#include "stdio-util.h" +#include "string-util.h" int pam_syslog_errno(pam_handle_t *handle, int level, int error, const char *format, ...) { va_list ap; @@ -21,6 +23,32 @@ int pam_syslog_errno(pam_handle_t *handle, int level, int error, const char *for return error == -ENOMEM ? PAM_BUF_ERR : PAM_SERVICE_ERR; } +int pam_syslog_pam_error(pam_handle_t *handle, int level, int error, const char *format, ...) { + /* This wraps pam_syslog() but will replace @PAMERR@ with a string from pam_strerror(). + * @PAMERR@ must be at the very end. */ + + va_list ap; + va_start(ap, format); + + const char *p = endswith(format, "@PAMERR@"); + if (p) { + const char *pamerr = pam_strerror(handle, error); + if (strchr(pamerr, '%')) + pamerr = "n/a"; /* We cannot have any formatting chars */ + + char buf[p - format + strlen(pamerr) + 1]; + xsprintf(buf, "%*s%s", (int)(p - format), format, pamerr); + DISABLE_WARNING_FORMAT_NONLITERAL; + pam_vsyslog(handle, level, buf, ap); + REENABLE_WARNING; + } else + pam_vsyslog(handle, level, format, ap); + + va_end(ap); + + return error; +} + static void cleanup_system_bus(pam_handle_t *handle, void *data, int error_status) { sd_bus_flush_close_unref(data); } @@ -38,20 +66,16 @@ int pam_acquire_bus_connection(pam_handle_t *handle, sd_bus **ret) { *ret = sd_bus_ref(TAKE_PTR(bus)); /* Increase the reference counter, so that the PAM data stays valid */ return PAM_SUCCESS; } - if (!IN_SET(r, PAM_SUCCESS, PAM_NO_MODULE_DATA)) { - pam_syslog(handle, LOG_ERR, "Failed to get bus connection: %s", pam_strerror(handle, r)); - return r; - } + if (!IN_SET(r, PAM_SUCCESS, PAM_NO_MODULE_DATA)) + return pam_syslog_pam_error(handle, LOG_ERR, r, "Failed to get bus connection: @PAMERR@"); r = sd_bus_open_system(&bus); if (r < 0) return pam_syslog_errno(handle, LOG_ERR, r, "Failed to connect to system bus: %m"); r = pam_set_data(handle, "systemd-system-bus", bus, cleanup_system_bus); - if (r != PAM_SUCCESS) { - pam_syslog(handle, LOG_ERR, "Failed to set PAM bus data: %s", pam_strerror(handle, r)); - return r; - } + if (r != PAM_SUCCESS) + return pam_syslog_pam_error(handle, LOG_ERR, r, "Failed to set PAM bus data: @PAMERR@"); sd_bus_ref(bus); *ret = TAKE_PTR(bus); @@ -64,9 +88,9 @@ int pam_release_bus_connection(pam_handle_t *handle) { r = pam_set_data(handle, "systemd-system-bus", NULL, NULL); if (r != PAM_SUCCESS) - pam_syslog(handle, LOG_ERR, "Failed to release PAM user record data: %s", pam_strerror(handle, r)); + return pam_syslog_pam_error(handle, LOG_ERR, r, "Failed to release PAM user record data: @PAMERR@"); - return r; + return PAM_SUCCESS; } void pam_cleanup_free(pam_handle_t *handle, void *data, int error_status) { diff --git a/src/shared/pam-util.h b/src/shared/pam-util.h index 4a97d2d132..1a17ea18c5 100644 --- a/src/shared/pam-util.h +++ b/src/shared/pam-util.h @@ -7,6 +7,8 @@ int pam_syslog_errno(pam_handle_t *handle, int level, int error, const char *format, ...) _printf_(4,5); +int pam_syslog_pam_error(pam_handle_t *handle, int level, int error, const char *format, ...) _printf_(4,5); + static inline int pam_log_oom(pam_handle_t *handle) { /* This is like log_oom(), but uses PAM logging */ return pam_syslog_errno(handle, LOG_ERR, ENOMEM, "Out of memory."); From e91b05f418b08c1e9c134c87b3e63264b18e348c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 11 Oct 2022 14:51:47 +0200 Subject: [PATCH 09/27] pam_systemd_home: use pam_syslog_pam_error() The message in acquire_home() was looking at the wrong variable ('r' instead of 'acquired_fd'). Apart from that, no change in behaviour is intended. --- src/home/pam_systemd_home.c | 255 +++++++++++++++--------------------- 1 file changed, 104 insertions(+), 151 deletions(-) diff --git a/src/home/pam_systemd_home.c b/src/home/pam_systemd_home.c index 7f613c16d7..8b41416578 100644 --- a/src/home/pam_systemd_home.c +++ b/src/home/pam_systemd_home.c @@ -105,15 +105,11 @@ static int acquire_user_record( if (!username) { r = pam_get_user(handle, &username, NULL); - if (r != PAM_SUCCESS) { - pam_syslog(handle, LOG_ERR, "Failed to get user name: %s", pam_strerror(handle, r)); - return r; - } + if (r != PAM_SUCCESS) + return pam_syslog_pam_error(handle, LOG_ERR, r, "Failed to get user name: @PAMERR@"); - if (isempty(username)) { - pam_syslog(handle, LOG_ERR, "User name not set."); - return PAM_SERVICE_ERR; - } + if (isempty(username)) + return pam_syslog_pam_error(handle, LOG_ERR, PAM_SERVICE_ERR, "User name not set."); } /* Let's bypass all IPC complexity for the two user names we know for sure we don't manage, and for @@ -133,10 +129,8 @@ static int acquire_user_record( /* Let's use the cache, so that we can share it between the session and the authentication hooks */ r = pam_get_data(handle, homed_field, (const void**) &json); - if (!IN_SET(r, PAM_SUCCESS, PAM_NO_MODULE_DATA)) { - pam_syslog(handle, LOG_ERR, "Failed to get PAM user record data: %s", pam_strerror(handle, r)); - return r; - } + if (!IN_SET(r, PAM_SUCCESS, PAM_NO_MODULE_DATA)) + return pam_syslog_pam_error(handle, LOG_ERR, r, "Failed to get PAM user record data: @PAMERR@"); if (r == PAM_SUCCESS && json) { /* We determined earlier that this is not a homed user? Then exit early. (We use -1 as * negative cache indicator) */ @@ -177,11 +171,9 @@ static int acquire_user_record( return pam_log_oom(handle); r = pam_set_data(handle, homed_field, json_copy, pam_cleanup_free); - if (r != PAM_SUCCESS) { - pam_syslog(handle, LOG_ERR, "Failed to set PAM user record data '%s': %s", - homed_field, pam_strerror(handle, r)); - return r; - } + if (r != PAM_SUCCESS) + return pam_syslog_pam_error(handle, LOG_ERR, r, + "Failed to set PAM user record data '%s': @PAMERR@", homed_field); /* Take a second copy: for the generic data field, the one which we share with * pam_systemd. While we insist on only reusing homed records, pam_systemd is fine with homed @@ -195,11 +187,9 @@ static int acquire_user_record( return pam_log_oom(handle); r = pam_set_data(handle, generic_field, json_copy, pam_cleanup_free); - if (r != PAM_SUCCESS) { - pam_syslog(handle, LOG_ERR, "Failed to set PAM user record data '%s': %s", - homed_field, pam_strerror(handle, r)); - return r; - } + if (r != PAM_SUCCESS) + return pam_syslog_pam_error(handle, LOG_ERR, r, + "Failed to set PAM user record data '%s': @PAMERR@", homed_field); TAKE_PTR(json_copy); } @@ -217,10 +207,9 @@ static int acquire_user_record( return pam_syslog_errno(handle, LOG_ERR, r, "Failed to load user record: %m"); /* Safety check if cached record actually matches what we are looking for */ - if (!streq_ptr(username, ur->user_name)) { - pam_syslog(handle, LOG_ERR, "Acquired user record does not match user name."); - return PAM_SERVICE_ERR; - } + if (!streq_ptr(username, ur->user_name)) + return pam_syslog_pam_error(handle, LOG_ERR, PAM_SERVICE_ERR, + "Acquired user record does not match user name."); if (ret_record) *ret_record = TAKE_PTR(ur); @@ -231,8 +220,9 @@ user_unknown: /* Cache this, so that we don't check again */ r = pam_set_data(handle, homed_field, POINTER_MAX, NULL); if (r != PAM_SUCCESS) - pam_syslog(handle, LOG_ERR, "Failed to set PAM user record data '%s' to invalid, ignoring: %s", - homed_field, pam_strerror(handle, r)); + pam_syslog_pam_error(handle, LOG_ERR, r, + "Failed to set PAM user record data '%s' to invalid, ignoring: @PAMERR@", + homed_field); return PAM_USER_UNKNOWN; } @@ -250,7 +240,8 @@ static int release_user_record(pam_handle_t *handle, const char *username) { r = pam_set_data(handle, homed_field, NULL, NULL); if (r != PAM_SUCCESS) - pam_syslog(handle, LOG_ERR, "Failed to release PAM user record data '%s': %s", homed_field, pam_strerror(handle, r)); + pam_syslog_pam_error(handle, LOG_ERR, r, + "Failed to release PAM user record data '%s': @PAMERR@", homed_field); generic_field = strjoin("systemd-user-record-", username); if (!generic_field) @@ -258,7 +249,8 @@ static int release_user_record(pam_handle_t *handle, const char *username) { k = pam_set_data(handle, generic_field, NULL, NULL); if (k != PAM_SUCCESS) - pam_syslog(handle, LOG_ERR, "Failed to release PAM user record data '%s': %s", generic_field, pam_strerror(handle, k)); + pam_syslog_pam_error(handle, LOG_ERR, k, + "Failed to release PAM user record data '%s': @PAMERR@", generic_field); return IN_SET(r, PAM_SUCCESS, PAM_NO_MODULE_DATA) ? k : r; } @@ -282,14 +274,15 @@ static int handle_generic_user_record_error( /* Logs about all errors, except for PAM_CONV_ERR, i.e. when requesting more info failed. */ if (sd_bus_error_has_name(error, BUS_ERROR_HOME_ABSENT)) { - (void) pam_prompt(handle, PAM_ERROR_MSG, NULL, "Home of user %s is currently absent, please plug in the necessary storage device or backing file system.", user_name); - pam_syslog(handle, LOG_ERR, "Failed to acquire home for user %s: %s", user_name, bus_error_message(error, ret)); - return PAM_PERM_DENIED; + (void) pam_prompt(handle, PAM_ERROR_MSG, NULL, + "Home of user %s is currently absent, please plug in the necessary storage device or backing file system.", user_name); + return pam_syslog_pam_error(handle, LOG_ERR, PAM_PERM_DENIED, + "Failed to acquire home for user %s: %s", user_name, bus_error_message(error, ret)); } else if (sd_bus_error_has_name(error, BUS_ERROR_AUTHENTICATION_LIMIT_HIT)) { (void) pam_prompt(handle, PAM_ERROR_MSG, NULL, "Too frequent login attempts for user %s, try again later.", user_name); - pam_syslog(handle, LOG_ERR, "Failed to acquire home for user %s: %s", user_name, bus_error_message(error, ret)); - return PAM_MAXTRIES; + return pam_syslog_pam_error(handle, LOG_ERR, PAM_MAXTRIES, + "Failed to acquire home for user %s: %s", user_name, bus_error_message(error, ret)); } else if (sd_bus_error_has_name(error, BUS_ERROR_BAD_PASSWORD)) { _cleanup_(erase_and_freep) char *newp = NULL; @@ -307,10 +300,9 @@ static int handle_generic_user_record_error( if (r != PAM_SUCCESS) return PAM_CONV_ERR; /* no logging here */ - if (isempty(newp)) { - pam_syslog(handle, LOG_DEBUG, "Password request aborted."); - return PAM_AUTHTOK_ERR; - } + if (isempty(newp)) + return pam_syslog_pam_error(handle, LOG_DEBUG, PAM_AUTHTOK_ERR, + "Password request aborted."); r = user_record_set_password(secret, STRV_MAKE(newp), true); if (r < 0) @@ -332,10 +324,9 @@ static int handle_generic_user_record_error( if (r != PAM_SUCCESS) return PAM_CONV_ERR; /* no logging here */ - if (isempty(newp)) { - pam_syslog(handle, LOG_DEBUG, "Recovery key request aborted."); - return PAM_AUTHTOK_ERR; - } + if (isempty(newp)) + return pam_syslog_pam_error(handle, LOG_DEBUG, PAM_AUTHTOK_ERR, + "Recovery key request aborted."); r = user_record_set_password(secret, STRV_MAKE(newp), true); if (r < 0) @@ -356,10 +347,10 @@ static int handle_generic_user_record_error( if (r != PAM_SUCCESS) return PAM_CONV_ERR; /* no logging here */ - if (isempty(newp)) { - pam_syslog(handle, LOG_DEBUG, "Password request aborted."); - return PAM_AUTHTOK_ERR; - } + if (isempty(newp)) + return pam_syslog_pam_error(handle, LOG_DEBUG, PAM_AUTHTOK_ERR, + "Password request aborted."); + r = user_record_set_password(secret, STRV_MAKE(newp), true); if (r < 0) @@ -374,10 +365,8 @@ static int handle_generic_user_record_error( if (r != PAM_SUCCESS) return PAM_CONV_ERR; /* no logging here */ - if (isempty(newp)) { - pam_syslog(handle, LOG_DEBUG, "PIN request aborted."); - return PAM_AUTHTOK_ERR; - } + if (isempty(newp)) + return pam_syslog_pam_error(handle, LOG_DEBUG, PAM_AUTHTOK_ERR, "PIN request aborted."); r = user_record_set_token_pin(secret, STRV_MAKE(newp), false); if (r < 0) @@ -431,10 +420,8 @@ static int handle_generic_user_record_error( if (r != PAM_SUCCESS) return PAM_CONV_ERR; /* no logging here */ - if (isempty(newp)) { - pam_syslog(handle, LOG_DEBUG, "PIN request aborted."); - return PAM_AUTHTOK_ERR; - } + if (isempty(newp)) + return pam_syslog_pam_error(handle, LOG_DEBUG, PAM_AUTHTOK_ERR, "PIN request aborted."); r = user_record_set_token_pin(secret, STRV_MAKE(newp), false); if (r < 0) @@ -450,10 +437,8 @@ static int handle_generic_user_record_error( if (r != PAM_SUCCESS) return PAM_CONV_ERR; /* no logging here */ - if (isempty(newp)) { - pam_syslog(handle, LOG_DEBUG, "PIN request aborted."); - return PAM_AUTHTOK_ERR; - } + if (isempty(newp)) + return pam_syslog_pam_error(handle, LOG_DEBUG, PAM_AUTHTOK_ERR, "PIN request aborted."); r = user_record_set_token_pin(secret, STRV_MAKE(newp), false); if (r < 0) @@ -469,19 +454,16 @@ static int handle_generic_user_record_error( if (r != PAM_SUCCESS) return PAM_CONV_ERR; /* no logging here */ - if (isempty(newp)) { - pam_syslog(handle, LOG_DEBUG, "PIN request aborted."); - return PAM_AUTHTOK_ERR; - } + if (isempty(newp)) + return pam_syslog_pam_error(handle, LOG_DEBUG, PAM_AUTHTOK_ERR, "PIN request aborted."); r = user_record_set_token_pin(secret, STRV_MAKE(newp), false); if (r < 0) return pam_syslog_errno(handle, LOG_ERR, r, "Failed to store PIN: %m"); - } else { - pam_syslog(handle, LOG_ERR, "Failed to acquire home for user %s: %s", user_name, bus_error_message(error, ret)); - return PAM_SERVICE_ERR; - } + } else + return pam_syslog_pam_error(handle, LOG_ERR, PAM_SERVICE_ERR, + "Failed to acquire home for user %s: %s", user_name, bus_error_message(error, ret)); return PAM_SUCCESS; } @@ -513,15 +495,11 @@ static int acquire_home( * authentication if possible, but with authentication if necessary. */ r = pam_get_user(handle, &username, NULL); - if (r != PAM_SUCCESS) { - pam_syslog(handle, LOG_ERR, "Failed to get user name: %s", pam_strerror(handle, r)); - return r; - } + if (r != PAM_SUCCESS) + return pam_syslog_pam_error(handle, LOG_ERR, r, "Failed to get user name: @PAMERR@"); - if (isempty(username)) { - pam_syslog(handle, LOG_ERR, "User name not set."); - return PAM_SERVICE_ERR; - } + if (isempty(username)) + return pam_syslog_pam_error(handle, LOG_ERR, PAM_SERVICE_ERR, "User name not set."); /* If we already have acquired the fd, let's shortcut this */ fd_field = strjoin("systemd-home-fd-", username); @@ -529,10 +507,9 @@ static int acquire_home( return pam_log_oom(handle); r = pam_get_data(handle, fd_field, &home_fd_ptr); - if (!IN_SET(r, PAM_SUCCESS, PAM_NO_MODULE_DATA)) { - pam_syslog(handle, LOG_ERR, "Failed to retrieve PAM home reference fd: %s", pam_strerror(handle, r)); - return r; - } + if (!IN_SET(r, PAM_SUCCESS, PAM_NO_MODULE_DATA)) + return pam_syslog_pam_error(handle, LOG_ERR, r, + "Failed to retrieve PAM home reference fd: @PAMERR@"); if (r == PAM_SUCCESS && PTR_TO_FD(home_fd_ptr) >= 0) return PAM_SUCCESS; @@ -567,10 +544,9 @@ static int acquire_home( * without anything, maybe some other authentication mechanism systemd-homed * implements (such as PKCS#11) allows us to authenticate without anything else. */ r = pam_get_item(handle, PAM_AUTHTOK, (const void**) &cached_password); - if (!IN_SET(r, PAM_BAD_ITEM, PAM_SUCCESS)) { - pam_syslog(handle, LOG_ERR, "Failed to get cached password: %s", pam_strerror(handle, r)); - return r; - } + if (!IN_SET(r, PAM_BAD_ITEM, PAM_SUCCESS)) + return pam_syslog_pam_error(handle, LOG_ERR, r, + "Failed to get cached password: @PAMERR@"); if (!isempty(cached_password)) { r = user_record_set_password(secret, STRV_MAKE(cached_password), true); @@ -643,9 +619,10 @@ static int acquire_home( } if (++n_attempts >= 5) { - (void) pam_prompt(handle, PAM_ERROR_MSG, NULL, "Too many unsuccessful login attempts for user %s, refusing.", ur->user_name); - pam_syslog(handle, LOG_ERR, "Failed to acquire home for user %s: %s", ur->user_name, bus_error_message(&error, r)); - return PAM_MAXTRIES; + (void) pam_prompt(handle, PAM_ERROR_MSG, NULL, + "Too many unsuccessful login attempts for user %s, refusing.", ur->user_name); + return pam_syslog_pam_error(handle, LOG_ERR, PAM_MAXTRIES, + "Failed to acquire home for user %s: %s", ur->user_name, bus_error_message(&error, r)); } /* Try again, this time with authentication if we didn't do that before. */ @@ -655,17 +632,13 @@ static int acquire_home( /* Later PAM modules may need the auth token, but only during pam_authenticate. */ if (please_authenticate && !strv_isempty(secret->password)) { r = pam_set_item(handle, PAM_AUTHTOK, *secret->password); - if (r < 0) { - pam_syslog(handle, LOG_ERR, "Failed to set PAM auth token: %s", pam_strerror(handle, r)); - return r; - } + if (r != PAM_SUCCESS) + return pam_syslog_pam_error(handle, LOG_ERR, r, "Failed to set PAM auth token: @PAMERR@"); } r = pam_set_data(handle, fd_field, FD_TO_PTR(acquired_fd), cleanup_home_fd); - if (r < 0) { - pam_syslog(handle, LOG_ERR, "Failed to set PAM bus data: %s", pam_strerror(handle, r)); - return r; - } + if (r != PAM_SUCCESS) + return pam_syslog_pam_error(handle, LOG_ERR, r, "Failed to set PAM bus data: @PAMERR@"); TAKE_FD(acquired_fd); if (do_auth) { @@ -678,7 +651,6 @@ static int acquire_home( } pam_syslog(handle, LOG_NOTICE, "Home for user %s successfully acquired.", ur->user_name); - return PAM_SUCCESS; } @@ -697,16 +669,14 @@ static int release_home_fd(pam_handle_t *handle, const char *username) { r = pam_get_data(handle, fd_field, &home_fd_ptr); if (r == PAM_NO_MODULE_DATA || (r == PAM_SUCCESS && PTR_TO_FD(home_fd_ptr) < 0)) return PAM_NO_MODULE_DATA; - if (r != PAM_SUCCESS) { - pam_syslog(handle, LOG_ERR, "Failed to retrieve PAM home reference fd: %s", pam_strerror(handle, r)); - return r; - } + if (r != PAM_SUCCESS) + return pam_syslog_pam_error(handle, LOG_ERR, r, "Failed to retrieve PAM home reference fd: @PAMERR@"); r = pam_set_data(handle, fd_field, NULL, NULL); if (r != PAM_SUCCESS) - pam_syslog(handle, LOG_ERR, "Failed to release PAM home reference fd: %s", pam_strerror(handle, r)); + return pam_syslog_pam_error(handle, LOG_ERR, r, "Failed to release PAM home reference fd: @PAMERR@"); - return r; + return PAM_SUCCESS; } _public_ PAM_EXTERN int pam_sm_authenticate( @@ -762,16 +732,14 @@ _public_ PAM_EXTERN int pam_sm_open_session( return r; r = pam_putenv(handle, "SYSTEMD_HOME=1"); - if (r != PAM_SUCCESS) { - pam_syslog(handle, LOG_ERR, "Failed to set PAM environment variable $SYSTEMD_HOME: %s", pam_strerror(handle, r)); - return r; - } + if (r != PAM_SUCCESS) + return pam_syslog_pam_error(handle, LOG_ERR, r, + "Failed to set PAM environment variable $SYSTEMD_HOME: @PAMERR@"); r = pam_putenv(handle, suspend_please ? "SYSTEMD_HOME_SUSPEND=1" : "SYSTEMD_HOME_SUSPEND=0"); - if (r != PAM_SUCCESS) { - pam_syslog(handle, LOG_ERR, "Failed to set PAM environment variable $SYSTEMD_HOME_SUSPEND: %s", pam_strerror(handle, r)); - return r; - } + if (r != PAM_SUCCESS) + return pam_syslog_pam_error(handle, LOG_ERR, r, + "Failed to set PAM environment variable $SYSTEMD_HOME_SUSPEND: @PAMERR@"); /* Let's release the D-Bus connection, after all the session might live quite a long time, and we are * not going to process the bus connection in that time, so let's better close before the daemon @@ -802,15 +770,11 @@ _public_ PAM_EXTERN int pam_sm_close_session( pam_syslog(handle, LOG_DEBUG, "pam-systemd-homed session end"); r = pam_get_user(handle, &username, NULL); - if (r != PAM_SUCCESS) { - pam_syslog(handle, LOG_ERR, "Failed to get user name: %s", pam_strerror(handle, r)); - return r; - } + if (r != PAM_SUCCESS) + return pam_syslog_pam_error(handle, LOG_ERR, r, "Failed to get user name: @PAMERR@"); - if (isempty(username)) { - pam_syslog(handle, LOG_ERR, "User name not set."); - return PAM_SERVICE_ERR; - } + if (isempty(username)) + return pam_syslog_pam_error(handle, LOG_ERR, PAM_SERVICE_ERR, "User name not set."); /* Let's explicitly drop the reference to the homed session, so that the subsequent ReleaseHome() * call will be able to do its thing. */ @@ -836,10 +800,9 @@ _public_ PAM_EXTERN int pam_sm_close_session( if (r < 0) { if (sd_bus_error_has_name(&error, BUS_ERROR_HOME_BUSY)) pam_syslog(handle, LOG_NOTICE, "Not deactivating home directory of %s, as it is still used.", username); - else { - pam_syslog(handle, LOG_ERR, "Failed to release user home: %s", bus_error_message(&error, r)); - return PAM_SESSION_ERR; - } + else + return pam_syslog_pam_error(handle, LOG_ERR, PAM_SESSION_ERR, + "Failed to release user home: %s", bus_error_message(&error, r)); } return PAM_SUCCESS; @@ -990,35 +953,27 @@ _public_ PAM_EXTERN int pam_sm_chauthtok( /* Start with cached credentials */ r = pam_get_item(handle, PAM_OLDAUTHTOK, (const void**) &old_password); - if (!IN_SET(r, PAM_BAD_ITEM, PAM_SUCCESS)) { - pam_syslog(handle, LOG_ERR, "Failed to get old password: %s", pam_strerror(handle, r)); - return r; - } + if (!IN_SET(r, PAM_BAD_ITEM, PAM_SUCCESS)) + return pam_syslog_pam_error(handle, LOG_ERR, r, "Failed to get old password: @PAMERR@"); + r = pam_get_item(handle, PAM_AUTHTOK, (const void**) &new_password); - if (!IN_SET(r, PAM_BAD_ITEM, PAM_SUCCESS)) { - pam_syslog(handle, LOG_ERR, "Failed to get cached password: %s", pam_strerror(handle, r)); - return r; - } + if (!IN_SET(r, PAM_BAD_ITEM, PAM_SUCCESS)) + return pam_syslog_pam_error(handle, LOG_ERR, r, "Failed to get cached password: @PAMERR@"); if (isempty(new_password)) { /* No, it's not cached, then let's ask for the password and its verification, and cache * it. */ r = pam_get_authtok_noverify(handle, &new_password, "New password: "); - if (r != PAM_SUCCESS) { - pam_syslog(handle, LOG_ERR, "Failed to get new password: %s", pam_strerror(handle, r)); - return r; - } - if (isempty(new_password)) { - pam_syslog(handle, LOG_DEBUG, "Password request aborted."); - return PAM_AUTHTOK_ERR; - } + if (r != PAM_SUCCESS) + return pam_syslog_pam_error(handle, LOG_ERR, r, "Failed to get new password: @PAMERR@"); + + if (isempty(new_password)) + return pam_syslog_pam_error(handle, LOG_DEBUG, PAM_AUTHTOK_ERR, "Password request aborted."); r = pam_get_authtok_verify(handle, &new_password, "new password: "); /* Lower case, since PAM prefixes 'Repeat' */ - if (r != PAM_SUCCESS) { - pam_syslog(handle, LOG_ERR, "Failed to get password again: %s", pam_strerror(handle, r)); - return r; - } + if (r != PAM_SUCCESS) + return pam_syslog_pam_error(handle, LOG_ERR, r, "Failed to get password again: @PAMERR@"); // FIXME: pam_pwquality will ask for the password a third time. It really shouldn't do // that, and instead assume the password was already verified once when it is found to be @@ -1070,16 +1025,14 @@ _public_ PAM_EXTERN int pam_sm_chauthtok( r = sd_bus_call(bus, m, HOME_SLOW_BUS_CALL_TIMEOUT_USEC, &error, NULL); if (r < 0) { r = handle_generic_user_record_error(handle, ur->user_name, old_secret, r, &error); - if (r == PAM_CONV_ERR) { - pam_syslog(handle, LOG_ERR, "Failed to prompt for password/prompt."); - return PAM_CONV_ERR; - } + if (r == PAM_CONV_ERR) + return pam_syslog_pam_error(handle, LOG_ERR, r, + "Failed to prompt for password/prompt."); if (r != PAM_SUCCESS) return r; - } else { - pam_syslog(handle, LOG_NOTICE, "Successfully changed password for user %s.", ur->user_name); - return PAM_SUCCESS; - } + } else + return pam_syslog_pam_error(handle, LOG_NOTICE, PAM_SUCCESS, + "Successfully changed password for user %s.", ur->user_name); if (++n_attempts >= 5) break; @@ -1087,6 +1040,6 @@ _public_ PAM_EXTERN int pam_sm_chauthtok( /* Try again */ }; - pam_syslog(handle, LOG_NOTICE, "Failed to change password for user %s: %m", ur->user_name); - return PAM_MAXTRIES; + return pam_syslog_pam_error(handle, LOG_NOTICE, PAM_MAXTRIES, + "Failed to change password for user %s: @PAMERR@", ur->user_name); } From f48e93764fecae3e6acb7d248522878686377fde Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Mon, 10 Oct 2022 14:59:50 +0200 Subject: [PATCH 10/27] pam_systemd: use pam_syslog_pam_error() Error handling in acquire_user_record() was checking the wrong condition (PAM errors are always >= 0, so r < 0 cannot match). Apart from the fix for error handling, no change in behaviour is intended. I did some minor adjustements to formatting and added _cleanup_ in one more place. --- src/login/pam_systemd.c | 159 +++++++++++++++------------------------- 1 file changed, 59 insertions(+), 100 deletions(-) diff --git a/src/login/pam_systemd.c b/src/login/pam_systemd.c index 7a624d2bd7..a288b3602a 100644 --- a/src/login/pam_systemd.c +++ b/src/login/pam_systemd.c @@ -107,15 +107,11 @@ static int acquire_user_record( assert(handle); r = pam_get_user(handle, &username, NULL); - if (r != PAM_SUCCESS) { - pam_syslog(handle, LOG_ERR, "Failed to get user name: %s", pam_strerror(handle, r)); - return r; - } + if (r != PAM_SUCCESS) + return pam_syslog_pam_error(handle, LOG_ERR, r, "Failed to get user name: @PAMERR@"); - if (isempty(username)) { - pam_syslog(handle, LOG_ERR, "User name not valid."); - return PAM_SERVICE_ERR; - } + if (isempty(username)) + return pam_syslog_pam_error(handle, LOG_ERR, PAM_SERVICE_ERR, "User name not valid."); /* If pam_systemd_homed (or some other module) already acquired the user record we can reuse it * here. */ @@ -124,10 +120,8 @@ static int acquire_user_record( return pam_log_oom(handle); r = pam_get_data(handle, field, (const void**) &json); - if (!IN_SET(r, PAM_SUCCESS, PAM_NO_MODULE_DATA)) { - pam_syslog(handle, LOG_ERR, "Failed to get PAM user record data: %s", pam_strerror(handle, r)); - return r; - } + if (!IN_SET(r, PAM_SUCCESS, PAM_NO_MODULE_DATA)) + return pam_syslog_pam_error(handle, LOG_ERR, r, "Failed to get PAM user record data: @PAMERR@"); if (r == PAM_SUCCESS && json) { _cleanup_(json_variant_unrefp) JsonVariant *v = NULL; @@ -145,10 +139,9 @@ static int acquire_user_record( return pam_syslog_errno(handle, LOG_ERR, r, "Failed to load user record: %m"); /* Safety check if cached record actually matches what we are looking for */ - if (!streq_ptr(username, ur->user_name)) { - pam_syslog(handle, LOG_ERR, "Acquired user record does not match user name."); - return PAM_SERVICE_ERR; - } + if (!streq_ptr(username, ur->user_name)) + return pam_syslog_pam_error(handle, LOG_ERR, PAM_SERVICE_ERR, + "Acquired user record does not match user name."); } else { _cleanup_free_ char *formatted = NULL; @@ -165,19 +158,15 @@ static int acquire_user_record( /* And cache it for everyone else */ r = pam_set_data(handle, field, formatted, pam_cleanup_free); - if (r < 0) { - pam_syslog(handle, LOG_ERR, "Failed to set PAM user record data '%s': %s", - field, pam_strerror(handle, r)); - return r; - } - + if (r != PAM_SUCCESS) + return pam_syslog_pam_error(handle, LOG_ERR, r, + "Failed to set PAM user record data '%s': @PAMERR@", field); TAKE_PTR(formatted); } - if (!uid_is_valid(ur->uid)) { - pam_syslog(handle, LOG_ERR, "Acquired user record does not have a UID."); - return PAM_SERVICE_ERR; - } + if (!uid_is_valid(ur->uid)) + return pam_syslog_pam_error(handle, LOG_ERR, PAM_SERVICE_ERR, + "Acquired user record does not have a UID."); if (ret_record) *ret_record = TAKE_PTR(ur); @@ -320,10 +309,8 @@ static int export_legacy_dbus_address( return pam_log_oom(handle); r = pam_misc_setenv(handle, "DBUS_SESSION_BUS_ADDRESS", t, 0); - if (r != PAM_SUCCESS) { - pam_syslog(handle, LOG_ERR, "Failed to set bus variable: %s", pam_strerror(handle, r)); - return r; - } + if (r != PAM_SUCCESS) + return pam_syslog_pam_error(handle, LOG_ERR, r, "Failed to set bus variable: @PAMERR@"); return PAM_SUCCESS; } @@ -465,9 +452,10 @@ static int update_environment(pam_handle_t *handle, const char *key, const char r = pam_misc_setenv(handle, key, value, 0); if (r != PAM_SUCCESS) - pam_syslog(handle, LOG_ERR, "Failed to set environment variable %s: %s", key, pam_strerror(handle, r)); + return pam_syslog_pam_error(handle, LOG_ERR, r, + "Failed to set environment variable %s: @PAMERR@", key); - return r; + return PAM_SUCCESS; } static bool validate_runtime_directory(pam_handle_t *handle, const char *path, uid_t uid) { @@ -517,10 +505,9 @@ static int pam_putenv_and_log(pam_handle_t *handle, const char *e, bool debug) { assert(e); r = pam_putenv(handle, e); - if (r != PAM_SUCCESS) { - pam_syslog(handle, LOG_ERR, "Failed to set PAM environment variable %s: %s", e, pam_strerror(handle, r)); - return r; - } + if (r != PAM_SUCCESS) + return pam_syslog_pam_error(handle, LOG_ERR, r, + "Failed to set PAM environment variable %s: @PAMERR@", e); if (debug) pam_syslog(handle, LOG_DEBUG, "PAM environment variable %s set based on user record.", e); @@ -660,10 +647,8 @@ static int configure_runtime_directory( return PAM_SUCCESS; r = pam_misc_setenv(handle, "XDG_RUNTIME_DIR", rt, 0); - if (r != PAM_SUCCESS) { - pam_syslog(handle, LOG_ERR, "Failed to set runtime dir: %s", pam_strerror(handle, r)); - return r; - } + if (r != PAM_SUCCESS) + return pam_syslog_pam_error(handle, LOG_ERR, r, "Failed to set runtime dir: @PAMERR@"); return export_legacy_dbus_address(handle, rt); } @@ -719,10 +704,8 @@ _public_ PAM_EXTERN int pam_sm_open_session( * leave. */ r = pam_get_item(handle, PAM_SERVICE, (const void**) &service); - if (!IN_SET(r, PAM_BAD_ITEM, PAM_SUCCESS)) { - pam_syslog(handle, LOG_ERR, "Failed to get PAM service: %s", pam_strerror(handle, r)); - return r; - } + if (!IN_SET(r, PAM_BAD_ITEM, PAM_SUCCESS)) + return pam_syslog_pam_error(handle, LOG_ERR, r, "Failed to get PAM service: @PAMERR@"); if (streq_ptr(service, "systemd-user")) { char rt[STRLEN("/run/user/") + DECIMAL_STR_MAX(uid_t)]; @@ -737,25 +720,17 @@ _public_ PAM_EXTERN int pam_sm_open_session( /* Otherwise, we ask logind to create a session for us */ r = pam_get_item(handle, PAM_XDISPLAY, (const void**) &display); - if (!IN_SET(r, PAM_BAD_ITEM, PAM_SUCCESS)) { - pam_syslog(handle, LOG_ERR, "Failed to get PAM XDISPLAY: %s", pam_strerror(handle, r)); - return r; - } + if (!IN_SET(r, PAM_BAD_ITEM, PAM_SUCCESS)) + return pam_syslog_pam_error(handle, LOG_ERR, r, "Failed to get PAM_XDISPLAY: @PAMERR@"); r = pam_get_item(handle, PAM_TTY, (const void**) &tty); - if (!IN_SET(r, PAM_BAD_ITEM, PAM_SUCCESS)) { - pam_syslog(handle, LOG_ERR, "Failed to get PAM TTY: %s", pam_strerror(handle, r)); - return r; - } + if (!IN_SET(r, PAM_BAD_ITEM, PAM_SUCCESS)) + return pam_syslog_pam_error(handle, LOG_ERR, r, "Failed to get PAM_TTY: @PAMERR@"); r = pam_get_item(handle, PAM_RUSER, (const void**) &remote_user); - if (!IN_SET(r, PAM_BAD_ITEM, PAM_SUCCESS)) { - pam_syslog(handle, LOG_ERR, "Failed to get PAM RUSER: %s", pam_strerror(handle, r)); - return r; - } + if (!IN_SET(r, PAM_BAD_ITEM, PAM_SUCCESS)) + return pam_syslog_pam_error(handle, LOG_ERR, r, "Failed to get PAM_RUSER: @PAMERR@"); r = pam_get_item(handle, PAM_RHOST, (const void**) &remote_host); - if (!IN_SET(r, PAM_BAD_ITEM, PAM_SUCCESS)) { - pam_syslog(handle, LOG_ERR, "Failed to get PAM RHOST: %s", pam_strerror(handle, r)); - return r; - } + if (!IN_SET(r, PAM_BAD_ITEM, PAM_SUCCESS)) + return pam_syslog_pam_error(handle, LOG_ERR, r, "Failed to get PAM_RHOST: @PAMERR@"); seat = getenv_harder(handle, "XDG_SEAT", NULL); cvtnr = getenv_harder(handle, "XDG_VTNR", NULL); @@ -823,30 +798,20 @@ _public_ PAM_EXTERN int pam_sm_open_session( remote = !isempty(remote_host) && !is_localhost(remote_host); r = pam_get_data(handle, "systemd.memory_max", (const void **)&memory_max); - if (!IN_SET(r, PAM_SUCCESS, PAM_NO_MODULE_DATA)) { - pam_syslog(handle, LOG_ERR, "Failed to get PAM systemd.memory_max data: %s", pam_strerror(handle, r)); - return r; - } + if (!IN_SET(r, PAM_SUCCESS, PAM_NO_MODULE_DATA)) + return pam_syslog_pam_error(handle, LOG_ERR, r, "Failed to get PAM systemd.memory_max data: @PAMERR@"); r = pam_get_data(handle, "systemd.tasks_max", (const void **)&tasks_max); - if (!IN_SET(r, PAM_SUCCESS, PAM_NO_MODULE_DATA)) { - pam_syslog(handle, LOG_ERR, "Failed to get PAM systemd.tasks_max data: %s", pam_strerror(handle, r)); - return r; - } + if (!IN_SET(r, PAM_SUCCESS, PAM_NO_MODULE_DATA)) + return pam_syslog_pam_error(handle, LOG_ERR, r, "Failed to get PAM systemd.tasks_max data: @PAMERR@"); r = pam_get_data(handle, "systemd.cpu_weight", (const void **)&cpu_weight); - if (!IN_SET(r, PAM_SUCCESS, PAM_NO_MODULE_DATA)) { - pam_syslog(handle, LOG_ERR, "Failed to get PAM systemd.cpu_weight data: %s", pam_strerror(handle, r)); - return r; - } + if (!IN_SET(r, PAM_SUCCESS, PAM_NO_MODULE_DATA)) + return pam_syslog_pam_error(handle, LOG_ERR, r, "Failed to get PAM systemd.cpu_weight data: @PAMERR@"); r = pam_get_data(handle, "systemd.io_weight", (const void **)&io_weight); - if (!IN_SET(r, PAM_SUCCESS, PAM_NO_MODULE_DATA)) { - pam_syslog(handle, LOG_ERR, "Failed to get PAM systemd.io_weight data: %s", pam_strerror(handle, r)); - return r; - } + if (!IN_SET(r, PAM_SUCCESS, PAM_NO_MODULE_DATA)) + return pam_syslog_pam_error(handle, LOG_ERR, r, "Failed to get PAM systemd.io_weight data: @PAMERR@"); r = pam_get_data(handle, "systemd.runtime_max_sec", (const void **)&runtime_max_sec); - if (!IN_SET(r, PAM_SUCCESS, PAM_NO_MODULE_DATA)) { - pam_syslog(handle, LOG_ERR, "Failed to get PAM systemd.runtime_max_sec data: %s", pam_strerror(handle, r)); - return r; - } + if (!IN_SET(r, PAM_SUCCESS, PAM_NO_MODULE_DATA)) + return pam_syslog_pam_error(handle, LOG_ERR, r, "Failed to get PAM systemd.runtime_max_sec data: @PAMERR@"); /* Talk to logind over the message bus */ @@ -992,22 +957,18 @@ _public_ PAM_EXTERN int pam_sm_open_session( } r = pam_set_data(handle, "systemd.existing", INT_TO_PTR(!!existing), NULL); - if (r != PAM_SUCCESS) { - pam_syslog(handle, LOG_ERR, "Failed to install existing flag: %s", pam_strerror(handle, r)); - return r; - } + if (r != PAM_SUCCESS) + return pam_syslog_pam_error(handle, LOG_ERR, r, "Failed to install existing flag: @PAMERR@"); if (session_fd >= 0) { - session_fd = fcntl(session_fd, F_DUPFD_CLOEXEC, 3); - if (session_fd < 0) + _cleanup_close_ int fd = fcntl(session_fd, F_DUPFD_CLOEXEC, 3); + if (fd < 0) return pam_syslog_errno(handle, LOG_ERR, errno, "Failed to dup session fd: %m"); - r = pam_set_data(handle, "systemd.session-fd", FD_TO_PTR(session_fd), NULL); - if (r != PAM_SUCCESS) { - pam_syslog(handle, LOG_ERR, "Failed to install session fd: %s", pam_strerror(handle, r)); - safe_close(session_fd); - return r; - } + r = pam_set_data(handle, "systemd.session-fd", FD_TO_PTR(fd), NULL); + if (r != PAM_SUCCESS) + return pam_syslog_pam_error(handle, LOG_ERR, r, "Failed to install session fd: @PAMERR@"); + TAKE_FD(fd); } success: @@ -1048,10 +1009,9 @@ _public_ PAM_EXTERN int pam_sm_close_session( /* Only release session if it wasn't pre-existing when we * tried to create it */ r = pam_get_data(handle, "systemd.existing", &existing); - if (!IN_SET(r, PAM_SUCCESS, PAM_NO_MODULE_DATA)) { - pam_syslog(handle, LOG_ERR, "Failed to get PAM systemd.existing data: %s", pam_strerror(handle, r)); - return r; - } + if (!IN_SET(r, PAM_SUCCESS, PAM_NO_MODULE_DATA)) + return pam_syslog_pam_error(handle, LOG_ERR, r, + "Failed to get PAM systemd.existing data: @PAMERR@"); id = pam_getenv(handle, "XDG_SESSION_ID"); if (id && !existing) { @@ -1066,10 +1026,9 @@ _public_ PAM_EXTERN int pam_sm_close_session( return r; r = bus_call_method(bus, bus_login_mgr, "ReleaseSession", &error, NULL, "s", id); - if (r < 0) { - pam_syslog(handle, LOG_ERR, "Failed to release session: %s", bus_error_message(&error, r)); - return PAM_SESSION_ERR; - } + if (r < 0) + return pam_syslog_pam_error(handle, LOG_ERR, PAM_SESSION_ERR, + "Failed to release session: %s", bus_error_message(&error, r)); } /* Note that we are knowingly leaking the FIFO fd here. This way, logind can watch us die. If we From 8ad572e60d97add640e1c5c0194d4c89dd1be3ae Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 4 Oct 2022 16:37:57 +0200 Subject: [PATCH 11/27] analyze: use the same error variable name as everywhere else --- src/analyze/analyze-verify-util.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/analyze/analyze-verify-util.c b/src/analyze/analyze-verify-util.c index bbadd1a9f4..a646e5807c 100644 --- a/src/analyze/analyze-verify-util.c +++ b/src/analyze/analyze-verify-util.c @@ -214,7 +214,7 @@ static int verify_documentation(Unit *u, bool check_man) { } static int verify_unit(Unit *u, bool check_man, const char *root) { - _cleanup_(sd_bus_error_free) sd_bus_error err = SD_BUS_ERROR_NULL; + _cleanup_(sd_bus_error_free) sd_bus_error error = SD_BUS_ERROR_NULL; int r, k; assert(u); @@ -223,9 +223,9 @@ static int verify_unit(Unit *u, bool check_man, const char *root) { unit_dump(u, stdout, "\t"); log_unit_debug(u, "Creating %s/start job", u->id); - r = manager_add_job(u->manager, JOB_START, u, JOB_REPLACE, NULL, &err, NULL); + r = manager_add_job(u->manager, JOB_START, u, JOB_REPLACE, NULL, &error, NULL); if (r < 0) - log_unit_error_errno(u, r, "Failed to create %s/start: %s", u->id, bus_error_message(&err, r)); + log_unit_error_errno(u, r, "Failed to create %s/start: %s", u->id, bus_error_message(&error, r)); k = verify_socket(u); if (k < 0 && r == 0) From 892dc967873cab3b8af158284a524d3148db674a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 7 Oct 2022 15:26:36 +0200 Subject: [PATCH 12/27] basic/stdio-util: allow xsprintf() to be used without any arguments MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit xsprintf(buf, "…: %m") would fail, now it works. --- src/basic/stdio-util.h | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/basic/stdio-util.h b/src/basic/stdio-util.h index 69d7062ec6..f647f125ac 100644 --- a/src/basic/stdio-util.h +++ b/src/basic/stdio-util.h @@ -9,16 +9,16 @@ #include "macro.h" #include "memory-util.h" -#define snprintf_ok(buf, len, fmt, ...) \ - ({ \ - char *_buf = (buf); \ - size_t _len = (len); \ - int _snpf = snprintf(_buf, _len, (fmt), __VA_ARGS__); \ - _snpf >= 0 && (size_t) _snpf < _len ? _buf : NULL; \ +#define snprintf_ok(buf, len, fmt, ...) \ + ({ \ + char *_buf = (buf); \ + size_t _len = (len); \ + int _snpf = snprintf(_buf, _len, (fmt), ##__VA_ARGS__); \ + _snpf >= 0 && (size_t) _snpf < _len ? _buf : NULL; \ }) #define xsprintf(buf, fmt, ...) \ - assert_message_se(snprintf_ok(buf, ELEMENTSOF(buf), fmt, __VA_ARGS__), "xsprintf: " #buf "[] must be big enough") + assert_message_se(snprintf_ok(buf, ELEMENTSOF(buf), fmt, ##__VA_ARGS__), "xsprintf: " #buf "[] must be big enough") #define VA_FORMAT_ADVANCE(format, ap) \ do { \ From 385530342869ce4c6ea7388262bec4a0398b9311 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 7 Oct 2022 09:15:44 +0200 Subject: [PATCH 13/27] core,logind,systemctl,journald: replace calls to strerror() with setting errno + %m strerror() is not thread safe and calling it just isn't worth the effort required to justify why it would be safe in those cases. It's easier to just use %m which is thread-safe out of the box. I don't think that any of the changes in the patch cause any functional difference. This is just about getting rid of calls to strerror() in general. When we print an error message and fail to format the string, using something like "(null)" is good enough. This is very very unlikely to happen anyway. --- src/core/cgroup.c | 16 ++++++---------- src/core/execute.c | 3 ++- src/core/selinux-access.c | 22 ++++++++++------------ src/core/socket.c | 12 +++++------- src/core/unit-serialize.c | 6 ++++-- src/journal/journald-server.c | 3 ++- src/systemctl/systemctl-show.c | 6 ++++-- 7 files changed, 33 insertions(+), 35 deletions(-) diff --git a/src/core/cgroup.c b/src/core/cgroup.c index 6d40faa48b..bbd107849c 100644 --- a/src/core/cgroup.c +++ b/src/core/cgroup.c @@ -398,17 +398,13 @@ static char *format_cgroup_memory_limit_comparison(char *buf, size_t l, Unit *u, * In the absence of reliably being able to detect whether memcg swap support is available or not, * only complain if the error is not ENOENT. */ if (r > 0 || IN_SET(r, -ENODATA, -EOWNERDEAD) || - (r == -ENOENT && streq(property_name, "MemorySwapMax"))) { + (r == -ENOENT && streq(property_name, "MemorySwapMax"))) buf[0] = 0; - return buf; - } - - if (r < 0) { - (void) snprintf(buf, l, " (error getting kernel value: %s)", strerror_safe(r)); - return buf; - } - - (void) snprintf(buf, l, " (different value in kernel: %" PRIu64 ")", kval); + else if (r < 0) { + errno = -r; + (void) snprintf(buf, l, " (error getting kernel value: %m)"); + } else + (void) snprintf(buf, l, " (different value in kernel: %" PRIu64 ")", kval); return buf; } diff --git a/src/core/execute.c b/src/core/execute.c index 6a4e1e0954..a2ed66f3ca 100644 --- a/src/core/execute.c +++ b/src/core/execute.c @@ -6428,9 +6428,10 @@ static void exec_command_dump(ExecCommand *c, FILE *f, const char *prefix) { prefix2 = strjoina(prefix, "\t"); cmd = quote_command_line(c->argv, SHELL_ESCAPE_EMPTY); + fprintf(f, "%sCommand Line: %s\n", - prefix, cmd ?: strerror_safe(ENOMEM)); + prefix, strnull(cmd)); exec_status_dump(&c->exec_status, f, prefix2); } diff --git a/src/core/selinux-access.c b/src/core/selinux-access.c index 848ae246a7..c69baa8a1a 100644 --- a/src/core/selinux-access.c +++ b/src/core/selinux-access.c @@ -137,6 +137,7 @@ _printf_(2, 3) static int log_callback(int type, const char *fmt, ...) { } static int access_init(sd_bus_error *error) { + int r; if (!mac_selinux_use()) return 0; @@ -145,23 +146,20 @@ static int access_init(sd_bus_error *error) { return 1; if (avc_open(NULL, 0) != 0) { - int saved_errno = errno; - bool enforce; + r = -errno; /* Save original errno for later */ - enforce = security_getenforce() != 0; - log_full_errno(enforce ? LOG_ERR : LOG_WARNING, saved_errno, "Failed to open the SELinux AVC: %m"); + bool enforce = security_getenforce() != 0; + log_full_errno(enforce ? LOG_ERR : LOG_WARNING, r, "Failed to open the SELinux AVC: %m"); - /* If enforcement isn't on, then let's suppress this - * error, and just don't do any AVC checks. The - * warning we printed is hence all the admin will - * see. */ + /* If enforcement isn't on, then let's suppress this error, and just don't do any AVC checks. + * The warning we printed is hence all the admin will see. */ if (!enforce) return 0; - /* Return an access denied error, if we couldn't load - * the AVC but enforcing mode was on, or we couldn't - * determine whether it is one. */ - return sd_bus_error_setf(error, SD_BUS_ERROR_ACCESS_DENIED, "Failed to open the SELinux AVC: %s", strerror_safe(saved_errno)); + /* Return an access denied error based on the original errno, if we couldn't load the AVC but + * enforcing mode was on, or we couldn't determine whether it is one. */ + errno = -r; + return sd_bus_error_setf(error, SD_BUS_ERROR_ACCESS_DENIED, "Failed to open the SELinux AVC: %m"); } selinux_set_callback(SELINUX_CB_AUDIT, (union selinux_callback) { .func_audit = audit_callback }); diff --git a/src/core/socket.c b/src/core/socket.c index 308f84898c..2ac041f9d7 100644 --- a/src/core/socket.c +++ b/src/core/socket.c @@ -739,16 +739,14 @@ static void socket_dump(Unit *u, FILE *f, const char *prefix) { switch (p->type) { case SOCKET_SOCKET: { _cleanup_free_ char *k = NULL; - const char *t; int r; r = socket_address_print(&p->address, &k); - if (r < 0) - t = strerror_safe(r); - else - t = k; - - fprintf(f, "%s%s: %s\n", prefix, listen_lookup(socket_address_family(&p->address), p->address.type), t); + if (r < 0) { + errno = -r; + fprintf(f, "%s%s: %m\n", prefix, listen_lookup(socket_address_family(&p->address), p->address.type)); + } else + fprintf(f, "%s%s: %s\n", prefix, listen_lookup(socket_address_family(&p->address), p->address.type), k); break; } case SOCKET_SPECIAL: diff --git a/src/core/unit-serialize.c b/src/core/unit-serialize.c index 797bf6c5ce..7fe9f3f231 100644 --- a/src/core/unit-serialize.c +++ b/src/core/unit-serialize.c @@ -849,8 +849,10 @@ void unit_dump(Unit *u, FILE *f, const char *prefix) { fprintf(f, "%s\tMerged into: %s\n", prefix, u->merged_into->id); - else if (u->load_state == UNIT_ERROR) - fprintf(f, "%s\tLoad Error Code: %s\n", prefix, strerror_safe(u->load_error)); + else if (u->load_state == UNIT_ERROR) { + errno = abs(u->load_error); + fprintf(f, "%s\tLoad Error Code: %m\n", prefix); + } for (const char *n = sd_bus_track_first(u->bus_track); n; n = sd_bus_track_next(u->bus_track)) fprintf(f, "%s\tBus Ref: %s\n", prefix, n); diff --git a/src/journal/journald-server.c b/src/journal/journald-server.c index bfa9f44a83..71d7a59bda 100644 --- a/src/journal/journald-server.c +++ b/src/journal/journald-server.c @@ -1094,7 +1094,8 @@ void server_driver_message(Server *s, pid_t object_pid, const char *message_id, /* We failed to format the message. Emit a warning instead. */ char buf[LINE_MAX]; - xsprintf(buf, "MESSAGE=Entry printing failed: %s", strerror_safe(r)); + errno = -r; + xsprintf(buf, "MESSAGE=Entry printing failed: %m"); n = 3; iovec[n++] = IOVEC_MAKE_STRING("PRIORITY=4"); diff --git a/src/systemctl/systemctl-show.c b/src/systemctl/systemctl-show.c index 4c7094678c..8d3db98c0a 100644 --- a/src/systemctl/systemctl-show.c +++ b/src/systemctl/systemctl-show.c @@ -670,8 +670,10 @@ static void print_status_info( if (i->status_text) printf(" Status: \"%s\"\n", i->status_text); - if (i->status_errno > 0) - printf(" Error: %i (%s)\n", i->status_errno, strerror_safe(i->status_errno)); + if (i->status_errno > 0) { + errno = i->status_errno; + printf(" Error: %i (%m)\n", i->status_errno); + } if (i->ip_ingress_bytes != UINT64_MAX && i->ip_egress_bytes != UINT64_MAX) printf(" IP: %s in, %s out\n", From 0cbefc7d4f3658f6ded60c5a98b0f938156ddc6b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 7 Oct 2022 09:18:26 +0200 Subject: [PATCH 14/27] shared/journal-importer: use %m instead of strerror() Here SYNTHETIC_ERRNO() was used based on the general rule that logging functions should do that when the error value is generated at the call site. But here we're really propagating a memory allocation error, which wasn't reported using errno, but the meaning is the same. And it's better to bend the rule a bit like this than to use strerror(). --- src/shared/journal-importer.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/shared/journal-importer.c b/src/shared/journal-importer.c index 66481da267..d9eabec886 100644 --- a/src/shared/journal-importer.c +++ b/src/shared/journal-importer.c @@ -426,11 +426,10 @@ int journal_importer_push_data(JournalImporter *imp, const char *data, size_t si assert(imp->state != IMPORTER_STATE_EOF); if (!realloc_buffer(imp, imp->filled + size)) - return log_error_errno(SYNTHETIC_ERRNO(ENOMEM), + return log_error_errno(ENOMEM, "Failed to store received data of size %zu " - "(in addition to existing %zu bytes with %zu filled): %s", - size, MALLOC_SIZEOF_SAFE(imp->buf), imp->filled, - strerror_safe(ENOMEM)); + "(in addition to existing %zu bytes with %zu filled): %m", + size, MALLOC_SIZEOF_SAFE(imp->buf), imp->filled); memcpy(imp->buf + imp->filled, data, size); imp->filled += size; From 2c5d05b3cd986568105d67891e4010b868dea24f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 7 Oct 2022 12:28:31 +0200 Subject: [PATCH 15/27] basic: add STRERROR() wrapper for strerror_r() --- src/basic/errno-util.h | 10 +++++++++ src/test/meson.build | 2 ++ src/test/test-errno-util.c | 44 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 56 insertions(+) create mode 100644 src/test/test-errno-util.c diff --git a/src/basic/errno-util.h b/src/basic/errno-util.h index a71864ca60..f0d24d95cb 100644 --- a/src/basic/errno-util.h +++ b/src/basic/errno-util.h @@ -6,6 +6,16 @@ #include "macro.h" +/* strerror(3) says that glibc uses a maximum length of 1024 bytes. */ +#define ERRNO_BUF_LEN 1024 + +/* Note: the lifetime of the compound literal is the immediately surrounding block, + * see C11 §6.5.2.5, and + * https://stackoverflow.com/questions/34880638/compound-literal-lifetime-and-if-blocks + * + * Note that we use the GNU variant of strerror_r() here. */ +#define STRERROR(errnum) strerror_r(abs(errnum), (char[ERRNO_BUF_LEN]){}, ERRNO_BUF_LEN) + static inline void _reset_errno_(int *saved_errno) { if (*saved_errno < 0) /* Invalidated by UNPROTECT_ERRNO? */ return; diff --git a/src/test/meson.build b/src/test/meson.build index 31ac149b96..86fc1d4fc0 100644 --- a/src/test/meson.build +++ b/src/test/meson.build @@ -615,6 +615,8 @@ tests += [ [files('test-errno-list.c') + generated_gperf_headers], + [files('test-errno-util.c')], + [files('test-ip-protocol-list.c') + shared_generated_gperf_headers], diff --git a/src/test/test-errno-util.c b/src/test/test-errno-util.c new file mode 100644 index 0000000000..284f451002 --- /dev/null +++ b/src/test/test-errno-util.c @@ -0,0 +1,44 @@ +/* SPDX-License-Identifier: LGPL-2.1-or-later */ + +#include "errno-util.h" +#include "stdio-util.h" +#include "string-util.h" +#include "tests.h" + +TEST(strerror_not_threadsafe) { + /* Just check that strerror really is not thread-safe. */ + log_info("strerror(%d) → %s", 200, strerror(200)); + log_info("strerror(%d) → %s", 201, strerror(201)); + log_info("strerror(%d) → %s", INT_MAX, strerror(INT_MAX)); + + log_info("strerror(%d), strerror(%d) → %p, %p", 200, 201, strerror(200), strerror(201)); + + /* This call is not allowed, because the first returned string becomes invalid when + * we call strerror the second time: + * + * log_info("strerror(%d), strerror(%d) → %s, %s", 200, 201, strerror(200), strerror(201)); + */ +} + +TEST(STRERROR) { + /* Just check that STRERROR really is thread-safe. */ + log_info("STRERROR(%d) → %s", 200, STRERROR(200)); + log_info("STRERROR(%d) → %s", 201, STRERROR(201)); + log_info("STRERROR(%d), STRERROR(%d) → %s, %s", 200, 201, STRERROR(200), STRERROR(201)); + + const char *a = STRERROR(200), *b = STRERROR(201); + assert_se(strstr(a, "200")); + assert_se(strstr(b, "201")); + + /* Check with negative values */ + assert_se(streq(a, STRERROR(-200))); + assert_se(streq(b, STRERROR(-201))); + + const char *c = STRERROR(INT_MAX); + char buf[DECIMAL_STR_MAX(int)]; + xsprintf(buf, "%d", INT_MAX); /* INT_MAX is hexadecimal, use printf to convert to decimal */ + log_info("STRERROR(%d) → %s", INT_MAX, c); + assert_se(strstr(c, buf)); +} + +DEFINE_TEST_MAIN(LOG_INFO); From a6e016af0106d4204ec4718ecfdc137355656241 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 7 Oct 2022 15:28:05 +0200 Subject: [PATCH 16/27] tree-wide: use STRERROR() --- src/journal/journalctl.c | 2 +- src/libsystemd/sd-bus/test-bus-error.c | 2 +- src/test/test-calendarspec.c | 8 ++++---- src/test/test-path-util.c | 16 ++++++++-------- src/test/test-sleep.c | 8 ++++---- src/test/test-tmpfile-util.c | 12 +++++++++--- 6 files changed, 27 insertions(+), 21 deletions(-) diff --git a/src/journal/journalctl.c b/src/journal/journalctl.c index f0d28fd48b..11de07fcfa 100644 --- a/src/journal/journalctl.c +++ b/src/journal/journalctl.c @@ -1460,7 +1460,7 @@ static int add_boot(sd_journal *j) { r = get_boots(j, NULL, &boot_id, arg_boot_offset); assert(r <= 1); if (r <= 0) { - const char *reason = (r == 0) ? "No such boot ID in journal" : strerror_safe(r); + const char *reason = (r == 0) ? "No such boot ID in journal" : STRERROR(r); if (sd_id128_is_null(arg_boot_id)) log_error("Data from the specified boot (%+i) is not available: %s", diff --git a/src/libsystemd/sd-bus/test-bus-error.c b/src/libsystemd/sd-bus/test-bus-error.c index cc59de12b4..a55f3f9856 100644 --- a/src/libsystemd/sd-bus/test-bus-error.c +++ b/src/libsystemd/sd-bus/test-bus-error.c @@ -99,7 +99,7 @@ TEST(error) { assert_se(!sd_bus_error_is_set(&error)); assert_se(sd_bus_error_set_errno(&error, EBUSY) == -EBUSY); assert_se(streq(error.name, "System.Error.EBUSY")); - assert_se(streq(error.message, strerror_safe(EBUSY))); + assert_se(streq(error.message, STRERROR(EBUSY))); assert_se(sd_bus_error_has_name(&error, "System.Error.EBUSY")); assert_se(sd_bus_error_get_errno(&error) == EBUSY); assert_se(sd_bus_error_is_set(&error)); diff --git a/src/test/test-calendarspec.c b/src/test/test-calendarspec.c index 8a0684ed66..564983b699 100644 --- a/src/test/test-calendarspec.c +++ b/src/test/test-calendarspec.c @@ -27,7 +27,7 @@ static void _test_one(int line, const char *input, const char *output) { u = now(CLOCK_REALTIME); r = calendar_spec_next_usec(c, u, &u); - log_info("Next: %s", r < 0 ? strerror_safe(r) : FORMAT_TIMESTAMP(u)); + log_info("Next: %s", r < 0 ? STRERROR(r) : FORMAT_TIMESTAMP(u)); calendar_spec_free(c); assert_se(calendar_spec_from_string(p, &c) >= 0); @@ -60,7 +60,7 @@ static void _test_next(int line, const char *input, const char *new_tz, usec_t a u = after; r = calendar_spec_next_usec(c, after, &u); - log_info("At: %s", r < 0 ? strerror_safe(r) : FORMAT_TIMESTAMP_STYLE(u, TIMESTAMP_US)); + log_info("At: %s", r < 0 ? STRERROR(r) : FORMAT_TIMESTAMP_STYLE(u, TIMESTAMP_US)); if (expect != USEC_INFINITY) assert_se(r >= 0 && u == expect); else @@ -104,10 +104,10 @@ TEST(hourly_bug_4031) { assert_se((r = calendar_spec_next_usec(c, n, &u)) >= 0); log_info("Now: %s (%"PRIu64")", FORMAT_TIMESTAMP_STYLE(n, TIMESTAMP_US), n); - log_info("Next hourly: %s (%"PRIu64")", r < 0 ? strerror_safe(r) : FORMAT_TIMESTAMP_STYLE(u, TIMESTAMP_US), u); + log_info("Next hourly: %s (%"PRIu64")", r < 0 ? STRERROR(r) : FORMAT_TIMESTAMP_STYLE(u, TIMESTAMP_US), u); assert_se((r = calendar_spec_next_usec(c, u, &w)) >= 0); - log_info("Next hourly: %s (%"PRIu64")", r < 0 ? strerror_safe(r) : FORMAT_TIMESTAMP_STYLE(w, TIMESTAMP_US), w); + log_info("Next hourly: %s (%"PRIu64")", r < 0 ? STRERROR(r) : FORMAT_TIMESTAMP_STYLE(w, TIMESTAMP_US), w); assert_se(n < u); assert_se(u <= n + USEC_PER_HOUR); diff --git a/src/test/test-path-util.c b/src/test/test-path-util.c index e3fcbd4513..f398761982 100644 --- a/src/test/test-path-util.c +++ b/src/test/test-path-util.c @@ -806,10 +806,10 @@ static void test_path_extract_filename_one(const char *input, const char *output int r; r = path_extract_filename(input, &k); - log_info_errno(r, "%s → %s/%m [expected: %s/%s]", - strnull(input), - strnull(k), /* strerror(r) is printed via %m, to avoid that the two strerror()'s overwrite each other's buffers */ - strnull(output), ret < 0 ? strerror_safe(ret) : "-"); + log_info("%s → %s/%s [expected: %s/%s]", + strnull(input), + strnull(k), r < 0 ? STRERROR(r) : "-", + strnull(output), ret < 0 ? STRERROR(ret) : "-"); assert_se(streq_ptr(k, output)); assert_se(r == ret); } @@ -850,10 +850,10 @@ static void test_path_extract_directory_one(const char *input, const char *outpu int r; r = path_extract_directory(input, &k); - log_info_errno(r, "%s → %s/%m [expected: %s/%s]", - strnull(input), - strnull(k), /* we output strerror_safe(r) via %m here, since otherwise the error buffer might be overwritten twice */ - strnull(output), strerror_safe(ret)); + log_info("%s → %s/%s [expected: %s/%s]", + strnull(input), + strnull(k), r < 0 ? STRERROR(r) : "-", + strnull(output), STRERROR(ret)); assert_se(streq_ptr(k, output)); assert_se(r == ret); diff --git a/src/test/test-sleep.c b/src/test/test-sleep.c index 6bac115f71..799e93dced 100644 --- a/src/test/test-sleep.c +++ b/src/test/test-sleep.c @@ -109,13 +109,13 @@ TEST(sleep) { log_info("/= high-level sleep verbs =/"); r = can_sleep(SLEEP_SUSPEND); - log_info("Suspend configured and possible: %s", r >= 0 ? yes_no(r) : strerror_safe(r)); + log_info("Suspend configured and possible: %s", r >= 0 ? yes_no(r) : STRERROR(r)); r = can_sleep(SLEEP_HIBERNATE); - log_info("Hibernation configured and possible: %s", r >= 0 ? yes_no(r) : strerror_safe(r)); + log_info("Hibernation configured and possible: %s", r >= 0 ? yes_no(r) : STRERROR(r)); r = can_sleep(SLEEP_HYBRID_SLEEP); - log_info("Hybrid-sleep configured and possible: %s", r >= 0 ? yes_no(r) : strerror_safe(r)); + log_info("Hybrid-sleep configured and possible: %s", r >= 0 ? yes_no(r) : STRERROR(r)); r = can_sleep(SLEEP_SUSPEND_THEN_HIBERNATE); - log_info("Suspend-then-Hibernate configured and possible: %s", r >= 0 ? yes_no(r) : strerror_safe(r)); + log_info("Suspend-then-Hibernate configured and possible: %s", r >= 0 ? yes_no(r) : STRERROR(r)); } static int intro(void) { diff --git a/src/test/test-tmpfile-util.c b/src/test/test-tmpfile-util.c index 002bd46162..415f185a81 100644 --- a/src/test/test-tmpfile-util.c +++ b/src/test/test-tmpfile-util.c @@ -13,7 +13,9 @@ static void test_tempfn_random_one(const char *p, const char *extra, const char int r; r = tempfn_random(p, extra, &s); - log_info_errno(r, "%s+%s → %s vs. %s (%i/%m vs. %i/%s)", p, strna(extra), strna(s), strna(expect), r, ret, strerror_safe(ret)); + log_info("%s+%s → %s vs. %s (%i/%s vs. %i/%s)", + p, strna(extra), strna(s), strna(expect), + r, STRERROR(r), ret, STRERROR(ret)); assert_se(!s == !expect); if (s) { @@ -89,7 +91,9 @@ static void test_tempfn_xxxxxx_one(const char *p, const char *extra, const char int r; r = tempfn_xxxxxx(p, extra, &s); - log_info_errno(r, "%s+%s → %s vs. %s (%i/%m vs. %i/%s)", p, strna(extra), strna(s), strna(expect), r, ret, strerror_safe(ret)); + log_info("%s+%s → %s vs. %s (%i/%s vs. %i/%s)", + p, strna(extra), strna(s), strna(expect), + r, STRERROR(r), ret, STRERROR(ret)); assert_se(!s == !expect); if (s) { @@ -164,7 +168,9 @@ static void test_tempfn_random_child_one(const char *p, const char *extra, const int r; r = tempfn_random_child(p, extra, &s); - log_info_errno(r, "%s+%s → %s vs. %s (%i/%m vs. %i/%s)", p, strna(extra), strna(s), strna(expect), r, ret, strerror_safe(ret)); + log_info_errno(r, "%s+%s → %s vs. %s (%i/%s vs. %i/%s)", + p, strna(extra), strna(s), strna(expect), + r, STRERROR(r), ret, STRERROR(ret)); assert_se(!s == !expect); if (s) { From f69ae8585f5ce6cd8d1e6f3ccd6c9c2cf153e846 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Mon, 10 Oct 2022 21:19:43 +0200 Subject: [PATCH 17/27] tree-wide: define and use STRERROR_OR_EOF() --- src/basic/errno-util.h | 5 +++++ src/journal-remote/journal-gatewayd.c | 4 ++-- src/libsystemd/sd-bus/test-bus-chat.c | 2 +- src/login/logind-seat.c | 8 ++++---- src/test/test-errno-util.c | 6 ++++++ 5 files changed, 18 insertions(+), 7 deletions(-) diff --git a/src/basic/errno-util.h b/src/basic/errno-util.h index f0d24d95cb..1e2e5b9f15 100644 --- a/src/basic/errno-util.h +++ b/src/basic/errno-util.h @@ -16,6 +16,11 @@ * Note that we use the GNU variant of strerror_r() here. */ #define STRERROR(errnum) strerror_r(abs(errnum), (char[ERRNO_BUF_LEN]){}, ERRNO_BUF_LEN) +/* A helper to print an error message or message for functions that return 0 on EOF. + * Note that we can't use ({ … }) to define a temporary variable, so errnum is + * evaluated twice. */ +#define STRERROR_OR_EOF(errnum) ((errnum) != 0 ? STRERROR(errnum) : "Unexpected EOF") + static inline void _reset_errno_(int *saved_errno) { if (*saved_errno < 0) /* Invalidated by UNPROTECT_ERRNO? */ return; diff --git a/src/journal-remote/journal-gatewayd.c b/src/journal-remote/journal-gatewayd.c index 3e2a85ce29..34def4670e 100644 --- a/src/journal-remote/journal-gatewayd.c +++ b/src/journal-remote/journal-gatewayd.c @@ -256,7 +256,7 @@ static ssize_t request_reader_entries( errno = 0; k = fread(buf, 1, n, m->tmp); if (k != n) { - log_error("Failed to read from file: %s", errno != 0 ? strerror_safe(errno) : "Premature EOF"); + log_error("Failed to read from file: %s", STRERROR_OR_EOF(errno)); return MHD_CONTENT_READER_END_WITH_ERROR; } @@ -600,7 +600,7 @@ static ssize_t request_reader_fields( errno = 0; k = fread(buf, 1, n, m->tmp); if (k != n) { - log_error("Failed to read from file: %s", errno != 0 ? strerror_safe(errno) : "Premature EOF"); + log_error("Failed to read from file: %s", STRERROR_OR_EOF(errno)); return MHD_CONTENT_READER_END_WITH_ERROR; } diff --git a/src/libsystemd/sd-bus/test-bus-chat.c b/src/libsystemd/sd-bus/test-bus-chat.c index df6dd62151..93e8ebfb1b 100644 --- a/src/libsystemd/sd-bus/test-bus-chat.c +++ b/src/libsystemd/sd-bus/test-bus-chat.c @@ -308,7 +308,7 @@ static void* client1(void *p) { errno = 0; if (read(pp[0], &x, 1) <= 0) { - log_error("Failed to read from pipe: %s", errno != 0 ? strerror_safe(errno) : "early read"); + log_error("Failed to read from pipe: %s", STRERROR_OR_EOF(errno)); goto finish; } diff --git a/src/login/logind-seat.c b/src/login/logind-seat.c index 43c72da11f..d8ad424bfe 100644 --- a/src/login/logind-seat.c +++ b/src/login/logind-seat.c @@ -389,11 +389,11 @@ int seat_read_active_vt(Seat *s) { if (lseek(s->manager->console_active_fd, SEEK_SET, 0) < 0) return log_error_errno(errno, "lseek on console_active_fd failed: %m"); + errno = 0; k = read(s->manager->console_active_fd, t, sizeof(t)-1); - if (k <= 0) { - log_error("Failed to read current console: %s", k < 0 ? strerror_safe(errno) : "EOF"); - return k < 0 ? -errno : -EIO; - } + if (k <= 0) + return log_error_errno(errno ?: EIO, + "Failed to read current console: %s", STRERROR_OR_EOF(errno)); t[k] = 0; truncate_nl(t); diff --git a/src/test/test-errno-util.c b/src/test/test-errno-util.c index 284f451002..f858927c92 100644 --- a/src/test/test-errno-util.c +++ b/src/test/test-errno-util.c @@ -41,4 +41,10 @@ TEST(STRERROR) { assert_se(strstr(c, buf)); } +TEST(STRERROR_OR_ELSE) { + log_info("STRERROR_OR_ELSE(0, \"EOF\") → %s", STRERROR_OR_EOF(0)); + log_info("STRERROR_OR_ELSE(EPERM, \"EOF\") → %s", STRERROR_OR_EOF(EPERM)); + log_info("STRERROR_OR_ELSE(-EPERM, \"EOF\") → %s", STRERROR_OR_EOF(-EPERM)); +} + DEFINE_TEST_MAIN(LOG_INFO); From f37508d506bc54bea16c638a6170870e1ee25a73 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Mon, 10 Oct 2022 09:31:58 +0200 Subject: [PATCH 18/27] tree-wide: get rid of lgtm annotations We now use CodeQL, and LGTM itself is gone. Those old annotations are not useful for anythign now. --- src/analyze/analyze-critical-chain.c | 6 +++--- src/basic/errno-util.h | 2 +- src/basic/in-addr-util.c | 6 +++--- src/libsystemd/sd-journal/journal-verify.c | 2 +- 4 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src/analyze/analyze-critical-chain.c b/src/analyze/analyze-critical-chain.c index bb077e2293..f782f95d5f 100644 --- a/src/analyze/analyze-critical-chain.c +++ b/src/analyze/analyze-critical-chain.c @@ -98,7 +98,7 @@ static int list_dependencies_one(sd_bus *bus, const char *name, unsigned level, return r; STRV_FOREACH(c, deps) { - times = hashmap_get(unit_times_hashmap, *c); /* lgtm [cpp/inconsistent-null-check] */ + times = hashmap_get(unit_times_hashmap, *c); if (times_in_range(times, boot) && times->activated >= service_longest) service_longest = times->activated; } @@ -107,7 +107,7 @@ static int list_dependencies_one(sd_bus *bus, const char *name, unsigned level, return r; STRV_FOREACH(c, deps) { - times = hashmap_get(unit_times_hashmap, *c); /* lgtm [cpp/inconsistent-null-check] */ + times = hashmap_get(unit_times_hashmap, *c); if (times_in_range(times, boot) && service_longest - times->activated <= arg_fuzz) to_print++; } @@ -116,7 +116,7 @@ static int list_dependencies_one(sd_bus *bus, const char *name, unsigned level, return r; STRV_FOREACH(c, deps) { - times = hashmap_get(unit_times_hashmap, *c); /* lgtm [cpp/inconsistent-null-check] */ + times = hashmap_get(unit_times_hashmap, *c); if (!times_in_range(times, boot) || service_longest - times->activated > arg_fuzz) continue; diff --git a/src/basic/errno-util.h b/src/basic/errno-util.h index 1e2e5b9f15..66b3e53ade 100644 --- a/src/basic/errno-util.h +++ b/src/basic/errno-util.h @@ -75,7 +75,7 @@ static inline int RET_NERRNO(int ret) { static inline const char *strerror_safe(int error) { /* 'safe' here does NOT mean thread safety. */ - return strerror(abs(error)); /* lgtm [cpp/potentially-dangerous-function] */ + return strerror(abs(error)); } static inline int errno_or_else(int fallback) { diff --git a/src/basic/in-addr-util.c b/src/basic/in-addr-util.c index cefe3f7661..7b05a40b08 100644 --- a/src/basic/in-addr-util.c +++ b/src/basic/in-addr-util.c @@ -66,7 +66,7 @@ bool in4_addr_is_link_local_dynamic(const struct in_addr *a) { bool in6_addr_is_link_local(const struct in6_addr *a) { assert(a); - return IN6_IS_ADDR_LINKLOCAL(a); /* lgtm [cpp/potentially-dangerous-function] */ + return IN6_IS_ADDR_LINKLOCAL(a); } int in_addr_is_link_local(int family, const union in_addr_union *u) { @@ -131,7 +131,7 @@ int in_addr_is_localhost(int family, const union in_addr_union *u) { return in4_addr_is_localhost(&u->in); if (family == AF_INET6) - return IN6_IS_ADDR_LOOPBACK(&u->in6); /* lgtm [cpp/potentially-dangerous-function] */ + return IN6_IS_ADDR_LOOPBACK(&u->in6); return -EAFNOSUPPORT; } @@ -144,7 +144,7 @@ int in_addr_is_localhost_one(int family, const union in_addr_union *u) { return be32toh(u->in.s_addr) == UINT32_C(0x7F000001); if (family == AF_INET6) - return IN6_IS_ADDR_LOOPBACK(&u->in6); /* lgtm [cpp/potentially-dangerous-function] */ + return IN6_IS_ADDR_LOOPBACK(&u->in6); return -EAFNOSUPPORT; } diff --git a/src/libsystemd/sd-journal/journal-verify.c b/src/libsystemd/sd-journal/journal-verify.c index b9f6a161ed..387963d6f3 100644 --- a/src/libsystemd/sd-journal/journal-verify.c +++ b/src/libsystemd/sd-journal/journal-verify.c @@ -67,7 +67,7 @@ static uint64_t scale_progress(uint64_t scale, uint64_t p, uint64_t m) { * Currently all callers use m >= 1, but we keep the check to be defensive. */ - if (p >= m || m == 0) // lgtm[cpp/constant-comparison] + if (p >= m || m == 0) return scale; return scale * p / m; From 5475e963c5e6ade35404384ba03caf79cb1bc2e5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 7 Oct 2022 15:52:33 +0200 Subject: [PATCH 19/27] analyze: add forgotten return statement We would fail with an assert in sd_bus_message_enter_container() afterwards. --- src/analyze/analyze-dot.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/analyze/analyze-dot.c b/src/analyze/analyze-dot.c index 13bea4598f..bf8aa8148f 100644 --- a/src/analyze/analyze-dot.c +++ b/src/analyze/analyze-dot.c @@ -149,7 +149,7 @@ int verb_dot(int argc, char *argv[], void *userdata) { r = bus_call_method(bus, bus_systemd_mgr, "ListUnits", &error, &reply, NULL); if (r < 0) - log_error_errno(r, "Failed to list units: %s", bus_error_message(&error, r)); + return log_error_errno(r, "Failed to list units: %s", bus_error_message(&error, r)); r = sd_bus_message_enter_container(reply, SD_BUS_TYPE_ARRAY, "(ssssssouso)"); if (r < 0) From a390b03089470582411ca2c36ea91ec503d2a024 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 7 Oct 2022 15:53:10 +0200 Subject: [PATCH 20/27] core: adjust log message "umount startup job" is rather confusing. Let's say "unmount job", which should be more meaningful for a user. --- src/core/automount.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/core/automount.c b/src/core/automount.c index 6c75d0e3ad..a44b8e878d 100644 --- a/src/core/automount.c +++ b/src/core/automount.c @@ -1041,7 +1041,7 @@ static int automount_dispatch_io(sd_event_source *s, int fd, uint32_t events, vo r = manager_add_job(UNIT(a)->manager, JOB_STOP, trigger, JOB_REPLACE, NULL, &error, NULL); if (r < 0) { - log_unit_warning(UNIT(a), "Failed to queue umount startup job: %s", bus_error_message(&error, r)); + log_unit_warning(UNIT(a), "Failed to queue unmount job: %s", bus_error_message(&error, r)); goto fail; } break; From eaaf7465ee6afd621df639f583b5affebd306e9c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 7 Oct 2022 15:56:25 +0200 Subject: [PATCH 21/27] sd-bus: make bus_error_message() a thread-safe macro strerror_r() is used instead of strerror(). The usual trick is employed: we allocate a buffer that lives until the end of the surrounding block to provide the scratch space. This change is particularly important forn sd-bus and the pam modules, which may be called from threaded code. I checked the codebase, and we only use bus_error_message() in log statements, so the returned pointer is not used beyond its valid lifetime. --- src/libsystemd/sd-bus/bus-error.c | 8 ++++---- src/libsystemd/sd-bus/bus-error.h | 8 +++++++- 2 files changed, 11 insertions(+), 5 deletions(-) diff --git a/src/libsystemd/sd-bus/bus-error.c b/src/libsystemd/sd-bus/bus-error.c index aca3ac36d2..4d687cfac9 100644 --- a/src/libsystemd/sd-bus/bus-error.c +++ b/src/libsystemd/sd-bus/bus-error.c @@ -494,7 +494,7 @@ _public_ int sd_bus_error_set_errno(sd_bus_error *e, int error) { *e = BUS_ERROR_FAILED; } - /* Now, fill in the message from strerror() if we can */ + /* Now, fill in the message from strerror_r() if we can */ bus_error_strerror(e, error); return -error; } @@ -555,7 +555,7 @@ _public_ int sd_bus_error_set_errnofv(sd_bus_error *e, int error, const char *fo } fail: - /* If that didn't work, use strerror() for the message */ + /* If that didn't work, use strerror_r() for the message */ bus_error_strerror(e, error); return -error; } @@ -586,7 +586,7 @@ _public_ int sd_bus_error_set_errnof(sd_bus_error *e, int error, const char *for return sd_bus_error_set_errno(e, error); } -const char *bus_error_message(const sd_bus_error *e, int error) { +const char* _bus_error_message(const sd_bus_error *e, int error, char buf[static ERRNO_BUF_LEN]) { /* Sometimes, the D-Bus server is a little bit too verbose with * its error messages, so let's override them here */ if (sd_bus_error_has_name(e, SD_BUS_ERROR_ACCESS_DENIED)) @@ -595,7 +595,7 @@ const char *bus_error_message(const sd_bus_error *e, int error) { if (e && e->message) return e->message; - return strerror_safe(error); + return strerror_r(abs(error), buf, ERRNO_BUF_LEN); } static bool map_ok(const sd_bus_error_map *map) { diff --git a/src/libsystemd/sd-bus/bus-error.h b/src/libsystemd/sd-bus/bus-error.h index 518493762c..c8768c9a7b 100644 --- a/src/libsystemd/sd-bus/bus-error.h +++ b/src/libsystemd/sd-bus/bus-error.h @@ -5,11 +5,17 @@ #include "sd-bus.h" +#include "errno-util.h" #include "macro.h" bool bus_error_is_dirty(sd_bus_error *e); -const char *bus_error_message(const sd_bus_error *e, int error); +const char* _bus_error_message(const sd_bus_error *e, int error, char buf[static ERRNO_BUF_LEN]); + +/* Note: the lifetime of the compound literal is the immediately surrounding block, + * see C11 §6.5.2.5, and + * https://stackoverflow.com/questions/34880638/compound-literal-lifetime-and-if-blocks */ +#define bus_error_message(e, error) _bus_error_message(e, error, (char[ERRNO_BUF_LEN]){}) #define BUS_ERROR_OOM SD_BUS_ERROR_MAKE_CONST(SD_BUS_ERROR_NO_MEMORY, "Out of memory") #define BUS_ERROR_FAILED SD_BUS_ERROR_MAKE_CONST(SD_BUS_ERROR_FAILED, "Operation failed") From 0cf1a4b3a7e0b870912ec0f986aa8107309a761e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 7 Oct 2022 15:59:34 +0200 Subject: [PATCH 22/27] Get rid of strerror_safe() --- .github/codeql-queries/PotentiallyDangerousFunction.ql | 2 +- src/basic/errno-util.h | 5 ----- 2 files changed, 1 insertion(+), 6 deletions(-) diff --git a/.github/codeql-queries/PotentiallyDangerousFunction.ql b/.github/codeql-queries/PotentiallyDangerousFunction.ql index 63fd14e75f..d5a5635c35 100644 --- a/.github/codeql-queries/PotentiallyDangerousFunction.ql +++ b/.github/codeql-queries/PotentiallyDangerousFunction.ql @@ -42,7 +42,7 @@ predicate potentiallyDangerousFunction(Function f, string message) { message = "Call to ntohs() is confusing. Use be16toh() instead." ) or ( f.getQualifiedName() = "strerror" and - message = "Call to strerror() is not thread-safe. Use strerror_r() or printf()'s %m format string instead." + message = "Call to strerror() is not thread-safe. Use printf()'s %m format string or STRERROR() instead." ) or ( f.getQualifiedName() = "accept" and message = "Call to accept() is not O_CLOEXEC-safe. Use accept4() instead." diff --git a/src/basic/errno-util.h b/src/basic/errno-util.h index 66b3e53ade..80b1edac6f 100644 --- a/src/basic/errno-util.h +++ b/src/basic/errno-util.h @@ -73,11 +73,6 @@ static inline int RET_NERRNO(int ret) { return ret; } -static inline const char *strerror_safe(int error) { - /* 'safe' here does NOT mean thread safety. */ - return strerror(abs(error)); -} - static inline int errno_or_else(int fallback) { /* To be used when invoking library calls where errno handling is not defined clearly: we return * errno if it is set, and the specified error otherwise. The idea is that the caller initializes From 29c45dc4348e7db61aa80ba1657cbc2d8b1a19ee Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Mon, 10 Oct 2022 09:18:26 +0200 Subject: [PATCH 23/27] man: use external .c files for three examples This way it's much easier to test that the code compiles without issues. It's also easier to edit the code. Indentation in one of the examples is reduced to two spaces. This is what we use in man pages to make them fit on screen better. --- man/journal-enumerate-fields.c | 21 +++++++++++++++++++++ man/journal-iterate-foreach.c | 29 +++++++++++++++++++++++++++++ man/journal-stream-fd.c | 28 ++++++++++++++++++++++++++++ man/sd_journal_enumerate_fields.xml | 21 +-------------------- man/sd_journal_next.xml | 29 +---------------------------- man/sd_journal_stream_fd.xml | 28 +--------------------------- 6 files changed, 81 insertions(+), 75 deletions(-) create mode 100644 man/journal-enumerate-fields.c create mode 100644 man/journal-iterate-foreach.c create mode 100644 man/journal-stream-fd.c diff --git a/man/journal-enumerate-fields.c b/man/journal-enumerate-fields.c new file mode 100644 index 0000000000..8a35785440 --- /dev/null +++ b/man/journal-enumerate-fields.c @@ -0,0 +1,21 @@ +/* SPDX-License-Identifier: CC0-1.0 */ + +#include +#include +#include + +int main(int argc, char *argv[]) { + sd_journal *j; + const char *field; + int r; + + r = sd_journal_open(&j, SD_JOURNAL_LOCAL_ONLY); + if (r < 0) { + fprintf(stderr, "Failed to open journal: %s\n", strerror(-r)); + return 1; + } + SD_JOURNAL_FOREACH_FIELD(j, field) + printf("%s\n", field); + sd_journal_close(j); + return 0; +} diff --git a/man/journal-iterate-foreach.c b/man/journal-iterate-foreach.c new file mode 100644 index 0000000000..ae53d467ed --- /dev/null +++ b/man/journal-iterate-foreach.c @@ -0,0 +1,29 @@ +/* SPDX-License-Identifier: CC0-1.0 */ + +#include +#include +#include + +int main(int argc, char *argv[]) { + int r; + sd_journal *j; + r = sd_journal_open(&j, SD_JOURNAL_LOCAL_ONLY); + if (r < 0) { + fprintf(stderr, "Failed to open journal: %s\n", strerror(-r)); + return 1; + } + SD_JOURNAL_FOREACH(j) { + const char *d; + size_t l; + + r = sd_journal_get_data(j, "MESSAGE", (const void **)&d, &l); + if (r < 0) { + fprintf(stderr, "Failed to read message field: %s\n", strerror(-r)); + continue; + } + + printf("%.*s\n", (int) l, d); + } + sd_journal_close(j); + return 0; +} diff --git a/man/journal-stream-fd.c b/man/journal-stream-fd.c new file mode 100644 index 0000000000..6bc5582189 --- /dev/null +++ b/man/journal-stream-fd.c @@ -0,0 +1,28 @@ +/* SPDX-License-Identifier: CC0-1.0 */ + +#include +#include +#include +#include +#include +#include + +int main(int argc, char *argv[]) { + int fd; + FILE *log; + fd = sd_journal_stream_fd("test", LOG_INFO, 1); + if (fd < 0) { + fprintf(stderr, "Failed to create stream fd: %s\n", strerror(-fd)); + return 1; + } + log = fdopen(fd, "w"); + if (!log) { + fprintf(stderr, "Failed to create file object: %m\n"); + close(fd); + return 1; + } + fprintf(log, "Hello World!\n"); + fprintf(log, SD_WARNING "This is a warning!\n"); + fclose(log); + return 0; +} diff --git a/man/sd_journal_enumerate_fields.xml b/man/sd_journal_enumerate_fields.xml index e074906980..2d8055ec89 100644 --- a/man/sd_journal_enumerate_fields.xml +++ b/man/sd_journal_enumerate_fields.xml @@ -94,26 +94,7 @@ Use the SD_JOURNAL_FOREACH_FIELD() macro to iterate through all field names in use in the current journal. - #include <stdio.h> -#include <string.h> -#include <systemd/sd-journal.h> - -int main(int argc, char *argv[]) { - sd_journal *j; - const char *field; - int r; - - r = sd_journal_open(&j, SD_JOURNAL_LOCAL_ONLY); - if (r < 0) { - fprintf(stderr, "Failed to open journal: %s\n", strerror(-r)); - return 1; - } - SD_JOURNAL_FOREACH_FIELD(j, field) - printf("%s\n", field); - sd_journal_close(j); - return 0; -} - + diff --git a/man/sd_journal_next.xml b/man/sd_journal_next.xml index e0f0a04974..628abb296c 100644 --- a/man/sd_journal_next.xml +++ b/man/sd_journal_next.xml @@ -129,34 +129,7 @@ Iterating through the journal: - #include <stdio.h> -#include <string.h> -#include <systemd/sd-journal.h> - -int main(int argc, char *argv[]) { - int r; - sd_journal *j; - r = sd_journal_open(&j, SD_JOURNAL_LOCAL_ONLY); - if (r < 0) { - fprintf(stderr, "Failed to open journal: %s\n", strerror(-r)); - return 1; - } - SD_JOURNAL_FOREACH(j) { - const char *d; - size_t l; - - r = sd_journal_get_data(j, "MESSAGE", (const void **)&d, &l); - if (r < 0) { - fprintf(stderr, "Failed to read message field: %s\n", strerror(-r)); - continue; - } - - printf("%.*s\n", (int) l, d); - } - sd_journal_close(j); - return 0; -} - + diff --git a/man/sd_journal_stream_fd.xml b/man/sd_journal_stream_fd.xml index af2234e77d..40d2804121 100644 --- a/man/sd_journal_stream_fd.xml +++ b/man/sd_journal_stream_fd.xml @@ -100,33 +100,7 @@ Creating a log stream suitable for fprintf3: - #include <syslog.h> -#include <stdio.h> -#include <string.h> -#include <unistd.h> -#include <systemd/sd-journal.h> -#include <systemd/sd-daemon.h> - -int main(int argc, char *argv[]) { - int fd; - FILE *log; - fd = sd_journal_stream_fd("test", LOG_INFO, 1); - if (fd < 0) { - fprintf(stderr, "Failed to create stream fd: %s\n", strerror(-fd)); - return 1; - } - log = fdopen(fd, "w"); - if (!log) { - fprintf(stderr, "Failed to create file object: %m\n"); - close(fd); - return 1; - } - fprintf(log, "Hello World!\n"); - fprintf(log, SD_WARNING "This is a warning!\n"); - fclose(log); - return 0; -} - + From b4096cecffd13491848b84bc27f00dbee06af52e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 7 Oct 2022 16:19:21 +0200 Subject: [PATCH 24/27] man: recommend %m over strerror() The need to set errno is very very ugly, but at least it is thread-safe and works correctly. Using strerror() is likely to be wrong, so let's not recommend that. People who do a lot of logging would provide use some wrapper that sets errno like we do, so nudge people towards %m. I tested that all the separate .c files compile cleanly. --- man/journal-enumerate-fields.c | 5 +++-- man/journal-iterate-foreach.c | 8 +++++--- man/journal-iterate-unique.c | 8 +++++--- man/journal-iterate-wait.c | 14 +++++++++----- man/journal-stream-fd.c | 5 +++-- man/print-unit-path.c | 5 +++-- man/sd_bus_new.xml | 6 ++++-- man/sd_device_ref.xml | 6 ++++-- man/sd_event_new.xml | 6 ++++-- man/sd_login_monitor_new.xml | 6 ++++-- 10 files changed, 44 insertions(+), 25 deletions(-) diff --git a/man/journal-enumerate-fields.c b/man/journal-enumerate-fields.c index 8a35785440..f4b6b7b078 100644 --- a/man/journal-enumerate-fields.c +++ b/man/journal-enumerate-fields.c @@ -1,7 +1,7 @@ /* SPDX-License-Identifier: CC0-1.0 */ +#include #include -#include #include int main(int argc, char *argv[]) { @@ -11,7 +11,8 @@ int main(int argc, char *argv[]) { r = sd_journal_open(&j, SD_JOURNAL_LOCAL_ONLY); if (r < 0) { - fprintf(stderr, "Failed to open journal: %s\n", strerror(-r)); + errno = -r; + fprintf(stderr, "Failed to open journal: %m\n"); return 1; } SD_JOURNAL_FOREACH_FIELD(j, field) diff --git a/man/journal-iterate-foreach.c b/man/journal-iterate-foreach.c index ae53d467ed..363101d5ba 100644 --- a/man/journal-iterate-foreach.c +++ b/man/journal-iterate-foreach.c @@ -1,7 +1,7 @@ /* SPDX-License-Identifier: CC0-1.0 */ +#include #include -#include #include int main(int argc, char *argv[]) { @@ -9,7 +9,8 @@ int main(int argc, char *argv[]) { sd_journal *j; r = sd_journal_open(&j, SD_JOURNAL_LOCAL_ONLY); if (r < 0) { - fprintf(stderr, "Failed to open journal: %s\n", strerror(-r)); + errno = -r; + fprintf(stderr, "Failed to open journal: %m\n"); return 1; } SD_JOURNAL_FOREACH(j) { @@ -18,7 +19,8 @@ int main(int argc, char *argv[]) { r = sd_journal_get_data(j, "MESSAGE", (const void **)&d, &l); if (r < 0) { - fprintf(stderr, "Failed to read message field: %s\n", strerror(-r)); + errno = -r; + fprintf(stderr, "Failed to read message field: %m\n"); continue; } diff --git a/man/journal-iterate-unique.c b/man/journal-iterate-unique.c index e9f273c6e5..ac0c638032 100644 --- a/man/journal-iterate-unique.c +++ b/man/journal-iterate-unique.c @@ -1,7 +1,7 @@ /* SPDX-License-Identifier: CC0-1.0 */ +#include #include -#include #include int main(int argc, char *argv[]) { @@ -12,12 +12,14 @@ int main(int argc, char *argv[]) { r = sd_journal_open(&j, SD_JOURNAL_LOCAL_ONLY); if (r < 0) { - fprintf(stderr, "Failed to open journal: %s\n", strerror(-r)); + errno = -r; + fprintf(stderr, "Failed to open journal: %m\n"); return 1; } r = sd_journal_query_unique(j, "_SYSTEMD_UNIT"); if (r < 0) { - fprintf(stderr, "Failed to query journal: %s\n", strerror(-r)); + errno = -r; + fprintf(stderr, "Failed to query journal: %m\n"); return 1; } SD_JOURNAL_FOREACH_UNIQUE(j, d, l) diff --git a/man/journal-iterate-wait.c b/man/journal-iterate-wait.c index 2b6d8a0fa8..60b3459a18 100644 --- a/man/journal-iterate-wait.c +++ b/man/journal-iterate-wait.c @@ -1,7 +1,7 @@ /* SPDX-License-Identifier: CC0-1.0 */ +#include #include -#include #include int main(int argc, char *argv[]) { @@ -9,7 +9,8 @@ int main(int argc, char *argv[]) { sd_journal *j; r = sd_journal_open(&j, SD_JOURNAL_LOCAL_ONLY); if (r < 0) { - fprintf(stderr, "Failed to open journal: %s\n", strerror(-r)); + errno = -r; + fprintf(stderr, "Failed to open journal: %m\n"); return 1; } for (;;) { @@ -17,21 +18,24 @@ int main(int argc, char *argv[]) { size_t l; r = sd_journal_next(j); if (r < 0) { - fprintf(stderr, "Failed to iterate to next entry: %s\n", strerror(-r)); + errno = -r; + fprintf(stderr, "Failed to iterate to next entry: %m\n"); break; } if (r == 0) { /* Reached the end, let's wait for changes, and try again */ r = sd_journal_wait(j, (uint64_t) -1); if (r < 0) { - fprintf(stderr, "Failed to wait for changes: %s\n", strerror(-r)); + errno = -r; + fprintf(stderr, "Failed to wait for changes: %m\n"); break; } continue; } r = sd_journal_get_data(j, "MESSAGE", &d, &l); if (r < 0) { - fprintf(stderr, "Failed to read message field: %s\n", strerror(-r)); + errno = -r; + fprintf(stderr, "Failed to read message field: %m\n"); continue; } printf("%.*s\n", (int) l, (const char*) d); diff --git a/man/journal-stream-fd.c b/man/journal-stream-fd.c index 6bc5582189..d59aa14c13 100644 --- a/man/journal-stream-fd.c +++ b/man/journal-stream-fd.c @@ -1,8 +1,8 @@ /* SPDX-License-Identifier: CC0-1.0 */ +#include #include #include -#include #include #include #include @@ -12,7 +12,8 @@ int main(int argc, char *argv[]) { FILE *log; fd = sd_journal_stream_fd("test", LOG_INFO, 1); if (fd < 0) { - fprintf(stderr, "Failed to create stream fd: %s\n", strerror(-fd)); + errno = -fd; + fprintf(stderr, "Failed to create stream fd: %m\n"); return 1; } log = fdopen(fd, "w"); diff --git a/man/print-unit-path.c b/man/print-unit-path.c index 41a21aab52..ff52fd28cd 100644 --- a/man/print-unit-path.c +++ b/man/print-unit-path.c @@ -1,7 +1,7 @@ /* SPDX-License-Identifier: CC0-1.0 */ +#include #include -#include #include #include @@ -21,7 +21,8 @@ #define MEMBER "GetUnitByPID" static int log_error(int error, const char *message) { - fprintf(stderr, "%s: %s\n", message, strerror(-error)); + errno = -error; + fprintf(stderr, "%s: %m\n", message); return error; } diff --git a/man/sd_bus_new.xml b/man/sd_bus_new.xml index 41964640b6..b45106168c 100644 --- a/man/sd_bus_new.xml +++ b/man/sd_bus_new.xml @@ -119,8 +119,10 @@ int r; … r = sd_bus_default(&bus); - if (r < 0) - fprintf(stderr, "Failed to allocate bus: %s\n", strerror(-r)); + if (r < 0) { + errno = -r; + fprintf(stderr, "Failed to allocate bus: %m\n"); + } … } diff --git a/man/sd_device_ref.xml b/man/sd_device_ref.xml index c2ba6b1571..2b8512a572 100644 --- a/man/sd_device_ref.xml +++ b/man/sd_device_ref.xml @@ -62,8 +62,10 @@ int r; … r = sd_device_new_from_syspath(&device, "…"); - if (r < 0) - fprintf(stderr, "Failed to allocate device: %s\n", strerror(-r)); + if (r < 0) { + errno = -r; + fprintf(stderr, "Failed to allocate device: %m\n"); + } … } diff --git a/man/sd_event_new.xml b/man/sd_event_new.xml index 352137ec09..8a105a764b 100644 --- a/man/sd_event_new.xml +++ b/man/sd_event_new.xml @@ -135,8 +135,10 @@ int r; … r = sd_event_default(&event); - if (r < 0) - fprintf(stderr, "Failed to allocate event loop: %s\n", strerror(-r)); + if (r < 0) { + errno = -r; + fprintf(stderr, "Failed to allocate event loop: %m\n"); + } … } diff --git a/man/sd_login_monitor_new.xml b/man/sd_login_monitor_new.xml index 081796249d..8118121281 100644 --- a/man/sd_login_monitor_new.xml +++ b/man/sd_login_monitor_new.xml @@ -116,8 +116,10 @@ int r; … r = sd_login_monitor_new(NULL, &m); - if (r < 0) - fprintf(stderr, "Failed to allocate login monitor object: %s\n", strerror(-r)); + if (r < 0) { + errno = -r; + fprintf(stderr, "Failed to allocate login monitor object: %m\n"); + } … } From 2fd922afed57c9a068c6d877b88d41391bea9721 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 7 Oct 2022 16:24:01 +0200 Subject: [PATCH 25/27] man: recommend strerror_r() over strerror() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Let's nudge people towards the use of an anonymous buffer like we do internally. "errno" → "errnum", to match the man page for strerror, and also to avoid confusion with the global variable. In general, I think that errno is a terrible interface and we shouldn't encourage people to use it. Those functions use errno-style error numbers, which are a different thing. --- man/sd-bus-errors.xml | 2 +- man/sd_notify.xml | 4 ++-- src/systemd/sd-daemon.h | 4 ++-- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/man/sd-bus-errors.xml b/man/sd-bus-errors.xml index a69efe03f0..4b9c237052 100644 --- a/man/sd-bus-errors.xml +++ b/man/sd-bus-errors.xml @@ -268,7 +268,7 @@ sd_bus_error3, sd_bus_message_set_allow_interactive_authorization3, errno3, - strerror3 + strerror_r3 diff --git a/man/sd_notify.xml b/man/sd_notify.xml index 31388b9c3d..de402950bb 100644 --- a/man/sd_notify.xml +++ b/man/sd_notify.xml @@ -424,8 +424,8 @@ sd_notifyf(0, "STATUS=Failed to start up: %s\n" "ERRNO=%i", - strerror(errno), - errno); + strerror_r(errnum, (char[1024]){}, 1024), + errnum); diff --git a/src/systemd/sd-daemon.h b/src/systemd/sd-daemon.h index 442b0ac604..53a1d7ccfe 100644 --- a/src/systemd/sd-daemon.h +++ b/src/systemd/sd-daemon.h @@ -260,8 +260,8 @@ int sd_notify(int unset_environment, const char *state); sd_notifyf(0, "STATUS=Failed to start up: %s\n" "ERRNO=%i", - strerror(errno), - errno); + strerror_r(errnum, (char[1024]){}, 1024), + errnum); See sd_notifyf(3) for more information. */ From b413d4fd3e8102a6e3785cd7e8e8a679a4473445 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Mon, 10 Oct 2022 11:00:57 +0200 Subject: [PATCH 26/27] man: use man7.org for strerror_r --- man/sd-bus-errors.xml | 2 +- man/sd_bus_error.xml | 2 +- man/sd_bus_error_add_map.xml | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/man/sd-bus-errors.xml b/man/sd-bus-errors.xml index 4b9c237052..f3b1515c78 100644 --- a/man/sd-bus-errors.xml +++ b/man/sd-bus-errors.xml @@ -268,7 +268,7 @@ sd_bus_error3, sd_bus_message_set_allow_interactive_authorization3, errno3, - strerror_r3 + strerror_r3 diff --git a/man/sd_bus_error.xml b/man/sd_bus_error.xml index 6f5e9e4b97..b2f62f2fa0 100644 --- a/man/sd_bus_error.xml +++ b/man/sd_bus_error.xml @@ -401,7 +401,7 @@ sd-bus-errors3, sd_bus_error_add_map3, errno3, - strerror_r3 + strerror_r3 diff --git a/man/sd_bus_error_add_map.xml b/man/sd_bus_error_add_map.xml index 2976233a7c..a49b554459 100644 --- a/man/sd_bus_error_add_map.xml +++ b/man/sd_bus_error_add_map.xml @@ -132,7 +132,7 @@ sd_bus_error3, sd-bus-errors3, errno3, - strerror_r3 + strerror_r3 From 0b6a47957bd4f1dd4801ee6d7eb337e09dcf7099 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Mon, 10 Oct 2022 14:50:35 +0200 Subject: [PATCH 27/27] tree-wide: drop () around the first argument of a ternary op https://github.com/systemd/systemd/pull/24933#discussion_r991242789 --- src/basic/in-addr-util.c | 5 ++--- src/basic/process-util.c | 2 +- src/boot/efi/boot.c | 4 ++-- src/core/job.c | 6 +++--- src/libsystemd-network/dhcp-packet.c | 2 +- src/login/logind-session-device.c | 2 +- src/network/networkd-state-file.c | 2 +- src/resolve/resolved-dns-transaction.h | 10 +++++----- 8 files changed, 16 insertions(+), 17 deletions(-) diff --git a/src/basic/in-addr-util.c b/src/basic/in-addr-util.c index 7b05a40b08..05c729d34d 100644 --- a/src/basic/in-addr-util.c +++ b/src/basic/in-addr-util.c @@ -194,8 +194,7 @@ int in_addr_prefix_intersect( assert(a); assert(b); - /* Checks whether there are any addresses that are in both - * networks */ + /* Checks whether there are any addresses that are in both networks */ m = MIN(aprefixlen, bprefixlen); @@ -203,7 +202,7 @@ int in_addr_prefix_intersect( uint32_t x, nm; x = be32toh(a->in.s_addr ^ b->in.s_addr); - nm = (m == 0) ? 0 : 0xFFFFFFFFUL << (32 - m); + nm = m == 0 ? 0 : 0xFFFFFFFFUL << (32 - m); return (x & nm) == 0; } diff --git a/src/basic/process-util.c b/src/basic/process-util.c index 3dd6f4df59..fb0b38fa49 100644 --- a/src/basic/process-util.c +++ b/src/basic/process-util.c @@ -815,7 +815,7 @@ int wait_for_terminate_with_timeout(pid_t pid, usec_t timeout) { if (status.si_pid == pid) { /* This is the correct child. */ if (status.si_code == CLD_EXITED) - return (status.si_status == 0) ? 0 : -EPROTO; + return status.si_status == 0 ? 0 : -EPROTO; else return -EPROTO; } diff --git a/src/boot/efi/boot.c b/src/boot/efi/boot.c index fdbc55b884..3e3268b4d4 100644 --- a/src/boot/efi/boot.c +++ b/src/boot/efi/boot.c @@ -734,12 +734,12 @@ static bool menu_run( if (refresh) { for (UINTN i = idx_first; i <= idx_last && i < config->entry_count; i++) { print_at(x_start, y_start + i - idx_first, - (i == idx_highlight) ? COLOR_HIGHLIGHT : COLOR_ENTRY, + i == idx_highlight ? COLOR_HIGHLIGHT : COLOR_ENTRY, lines[i]); if (i == config->idx_default_efivar) print_at(x_start, y_start + i - idx_first, - (i == idx_highlight) ? COLOR_HIGHLIGHT : COLOR_ENTRY, + i == idx_highlight ? COLOR_HIGHLIGHT : COLOR_ENTRY, unicode_supported() ? L" ►" : L"=>"); } refresh = false; diff --git a/src/core/job.c b/src/core/job.c index 419fe54bf0..032554a0ac 100644 --- a/src/core/job.c +++ b/src/core/job.c @@ -142,7 +142,7 @@ void job_uninstall(Job *j) { job_set_state(j, JOB_WAITING); - pj = (j->type == JOB_NOP) ? &j->unit->nop_job : &j->unit->job; + pj = j->type == JOB_NOP ? &j->unit->nop_job : &j->unit->job; assert(*pj == j); /* Detach from next 'bigger' objects */ @@ -203,7 +203,7 @@ Job* job_install(Job *j) { assert(j->type < _JOB_TYPE_MAX_IN_TRANSACTION); assert(j->state == JOB_WAITING); - pj = (j->type == JOB_NOP) ? &j->unit->nop_job : &j->unit->job; + pj = j->type == JOB_NOP ? &j->unit->nop_job : &j->unit->job; uj = *pj; if (uj) { @@ -264,7 +264,7 @@ int job_install_deserialized(Job *j) { "Invalid job type %s in deserialization.", strna(job_type_to_string(j->type))); - pj = (j->type == JOB_NOP) ? &j->unit->nop_job : &j->unit->job; + pj = j->type == JOB_NOP ? &j->unit->nop_job : &j->unit->job; if (*pj) return log_unit_debug_errno(j->unit, SYNTHETIC_ERRNO(EEXIST), "Unit already has a job installed. Not installing deserialized job."); diff --git a/src/libsystemd-network/dhcp-packet.c b/src/libsystemd-network/dhcp-packet.c index 9748f8dd40..b2efd5246d 100644 --- a/src/libsystemd-network/dhcp-packet.c +++ b/src/libsystemd-network/dhcp-packet.c @@ -45,7 +45,7 @@ int dhcp_message_init( "hlen" (hardware address length) MUST be 0. "chaddr" (client hardware address) field MUST be zeroed. */ - message->hlen = (arp_type == ARPHRD_INFINIBAND) ? 0 : hlen; + message->hlen = arp_type == ARPHRD_INFINIBAND ? 0 : hlen; memcpy_safe(message->chaddr, chaddr, message->hlen); message->xid = htobe32(xid); diff --git a/src/login/logind-session-device.c b/src/login/logind-session-device.c index acf0f20d85..4f934dd01d 100644 --- a/src/login/logind-session-device.c +++ b/src/login/logind-session-device.c @@ -49,7 +49,7 @@ static int session_device_notify(SessionDevice *sd, enum SessionDeviceNotificati sd->session->manager->bus, &m, path, "org.freedesktop.login1.Session", - (type == SESSION_DEVICE_RESUME) ? "ResumeDevice" : "PauseDevice"); + type == SESSION_DEVICE_RESUME ? "ResumeDevice" : "PauseDevice"); if (!m) return r; diff --git a/src/network/networkd-state-file.c b/src/network/networkd-state-file.c index 502da1afb2..d62cca6dd5 100644 --- a/src/network/networkd-state-file.c +++ b/src/network/networkd-state-file.c @@ -204,7 +204,7 @@ int manager_save(Manager *m) { const char *domainname; char **domains = NULL; - target_domains = (link->network->dhcp_use_domains == DHCP_USE_DOMAINS_YES) ? &search_domains : &route_domains; + target_domains = link->network->dhcp_use_domains == DHCP_USE_DOMAINS_YES ? &search_domains : &route_domains; r = sd_dhcp_lease_get_domainname(link->dhcp_lease, &domainname); if (r >= 0) { r = ordered_set_put_strdup(target_domains, domainname); diff --git a/src/resolve/resolved-dns-transaction.h b/src/resolve/resolved-dns-transaction.h index ab86f0f01f..5a0e35704c 100644 --- a/src/resolve/resolved-dns-transaction.h +++ b/src/resolve/resolved-dns-transaction.h @@ -213,8 +213,8 @@ DnsTransactionSource dns_transaction_source_from_string(const char *s) _pure_; /* Maximum attempts to send MDNS requests, see RFC 6762 Section 8.1 */ #define MDNS_TRANSACTION_ATTEMPTS_MAX 3 -#define TRANSACTION_ATTEMPTS_MAX(p) (((p) == DNS_PROTOCOL_LLMNR) ? \ - LLMNR_TRANSACTION_ATTEMPTS_MAX : \ - (((p) == DNS_PROTOCOL_MDNS) ? \ - MDNS_TRANSACTION_ATTEMPTS_MAX : \ - DNS_TRANSACTION_ATTEMPTS_MAX)) +#define TRANSACTION_ATTEMPTS_MAX(p) ((p) == DNS_PROTOCOL_LLMNR ? \ + LLMNR_TRANSACTION_ATTEMPTS_MAX : \ + (p) == DNS_PROTOCOL_MDNS ? \ + MDNS_TRANSACTION_ATTEMPTS_MAX : \ + DNS_TRANSACTION_ATTEMPTS_MAX)