From 315edc2c115a28e9a9463ad5e914610ce893e170 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Thu, 4 Mar 2021 09:39:29 +0100 Subject: [PATCH 1/5] Move basic/kbd-util to shared/ It is (or should be used) in localectl, localed, and a few other places, no reason to keep it in basic/. --- src/basic/meson.build | 2 -- src/{basic => shared}/kbd-util.c | 1 - src/{basic => shared}/kbd-util.h | 0 src/shared/meson.build | 2 ++ 4 files changed, 2 insertions(+), 3 deletions(-) rename src/{basic => shared}/kbd-util.c (99%) rename src/{basic => shared}/kbd-util.h (100%) diff --git a/src/basic/meson.build b/src/basic/meson.build index 282118bb1a..37ef434542 100644 --- a/src/basic/meson.build +++ b/src/basic/meson.build @@ -83,8 +83,6 @@ basic_sources = files(''' io-util.c io-util.h ioprio.h - kbd-util.c - kbd-util.h khash.c khash.h label.c diff --git a/src/basic/kbd-util.c b/src/shared/kbd-util.c similarity index 99% rename from src/basic/kbd-util.c rename to src/shared/kbd-util.c index 267803ec2c..79ae962530 100644 --- a/src/basic/kbd-util.c +++ b/src/shared/kbd-util.c @@ -89,7 +89,6 @@ int get_keymaps(char ***ret) { } bool keymap_is_valid(const char *name) { - if (isempty(name)) return false; diff --git a/src/basic/kbd-util.h b/src/shared/kbd-util.h similarity index 100% rename from src/basic/kbd-util.h rename to src/shared/kbd-util.h diff --git a/src/shared/meson.build b/src/shared/meson.build index 4b27a95cc2..940345c495 100644 --- a/src/shared/meson.build +++ b/src/shared/meson.build @@ -149,6 +149,8 @@ shared_sources = files(''' json-internal.h json.c json.h + kbd-util.c + kbd-util.h killall.c killall.h libcrypt-util.c From 3864b4b038b4c8213cbe0ed60281217c37e0a426 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Thu, 4 Mar 2021 10:42:53 +0100 Subject: [PATCH 2/5] shared/kbd-util: fix return value confusion with nftw() We would return a real error sometimes from the callback, and FTW_STOP other times. Because of FTW_ACTIONRETVAL, everything except FTW_STOP would be ignored. I don't think using FTW_ACTIONRETVAL is useful. nftw() can only be used meaningfully with errno. Even if we return a proper value ourselves from the callback, it will be propagated as a return value from nftw(), but there is no way to distinguish this from a value generated by nftw() itself, which is -1/-EPERM on error. So let's set errno ourselves so the caller can at least look at that. The code still ignores all errors. --- TODO | 2 ++ src/shared/kbd-util.c | 42 +++++++++++++++++++----------------------- 2 files changed, 21 insertions(+), 23 deletions(-) diff --git a/TODO b/TODO index 87a92af806..2e8f24750b 100644 --- a/TODO +++ b/TODO @@ -7,6 +7,8 @@ Bugfixes: * userdbctl: "Password OK: yes" is shown even when there are no passwords or the password is locked. +* Get rid of nftw(). We should refuse to use such useless APIs on principle. + External: * Fedora: add an rpmlint check that verifies that all unit files in the RPM are listed in %systemd_post macros. diff --git a/src/shared/kbd-util.c b/src/shared/kbd-util.c index 79ae962530..fb98dd262e 100644 --- a/src/shared/kbd-util.c +++ b/src/shared/kbd-util.c @@ -31,14 +31,14 @@ static int nftw_cb( return 0; p = strdup(basename(fpath)); - if (!p) - return FTW_STOP; + if (!p) { + errno = ENOMEM; + return -1; + } e = endswith(p, ".map"); - if (e) - *e = 0; - - e = endswith(p, ".map.gz"); + if (!e) + e = endswith(p, ".map.gz"); if (e) *e = 0; @@ -46,37 +46,33 @@ static int nftw_cb( return 0; r = set_consume(keymaps, TAKE_PTR(p)); - if (r < 0 && r != -EEXIST) - return r; + if (r < 0 && r != -EEXIST) { + errno = -r; + return -1; + } return 0; } int get_keymaps(char ***ret) { - _cleanup_strv_free_ char **l = NULL; - const char *dir; - int r; - keymaps = set_new(&string_hash_ops); if (!keymaps) return -ENOMEM; - NULSTR_FOREACH(dir, KBD_KEYMAP_DIRS) { - r = nftw(dir, nftw_cb, 20, FTW_PHYS|FTW_ACTIONRETVAL); + const char *dir; + NULSTR_FOREACH(dir, KBD_KEYMAP_DIRS) + if (nftw(dir, nftw_cb, 20, FTW_PHYS) < 0) { + if (errno != ENOENT) + log_debug_errno(errno, "Failed to read keymap list from %s, ignoring: %m", dir); + } - if (r == FTW_STOP) - log_debug("Directory not found %s", dir); - else if (r < 0) - log_debug_errno(r, "Can't add keymap: %m"); - } - - l = set_get_strv(keymaps); + _cleanup_strv_free_ char **l = set_get_strv(keymaps); if (!l) { - set_free_free(keymaps); + keymaps = set_free_free(keymaps); return -ENOMEM; } - set_free(keymaps); + keymaps = set_free(keymaps); if (strv_isempty(l)) return -ENOENT; From 1d230090c3545cfbc40f2377a22c11c383d56d45 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Thu, 4 Mar 2021 10:50:32 +0100 Subject: [PATCH 3/5] shared/kbd-util: return error on resource errors I guess we should still not fail on failure to access a directory and such. --- src/shared/kbd-util.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/shared/kbd-util.c b/src/shared/kbd-util.c index fb98dd262e..f5669f7c03 100644 --- a/src/shared/kbd-util.c +++ b/src/shared/kbd-util.c @@ -2,6 +2,7 @@ #include +#include "errno-util.h" #include "kbd-util.h" #include "log.h" #include "nulstr-util.h" @@ -62,8 +63,13 @@ int get_keymaps(char ***ret) { const char *dir; NULSTR_FOREACH(dir, KBD_KEYMAP_DIRS) if (nftw(dir, nftw_cb, 20, FTW_PHYS) < 0) { - if (errno != ENOENT) - log_debug_errno(errno, "Failed to read keymap list from %s, ignoring: %m", dir); + if (errno == ENOENT) + continue; + if (ERRNO_IS_RESOURCE(errno)) { + keymaps = set_free_free(keymaps); + return log_warning_errno(errno, "Failed to read keymap list from %s: %m", dir); + } + log_debug_errno(errno, "Failed to read keymap list from %s, ignoring: %m", dir); } _cleanup_strv_free_ char **l = set_get_strv(keymaps); From 9ef70c06c663a05f7646d690e00e4b4a73af8c61 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Thu, 4 Mar 2021 11:27:05 +0100 Subject: [PATCH 4/5] localed: refuse to set a keymap which is not installed In https://bugzilla.redhat.com/show_bug.cgi?id=1933873 a keymap was set without the package that provides it being installed (it2 is in kbd-legacy, which is not installed by default). Setting a non-installed keymap is problematic, because it results in nasty failures afterward (*). So let's to the same as e.g. for locale data, and refuse a setting if the definition doesn't exists in the filesystem. The implementation using nftw() is not the most efficient, but I think it's OK in this case. This is definitely not in any kind of hot path, and I prefer not to duplicate the filename manipulation logic in a second function. (*) If the keymap is not found, vconsole-setup.service will fail. dracut-cmdline-ask.service has Requires=vconsole-setup.service, so it will also fail, and this breaks boot. dracut-cmdline-ask.service having a hard dependency is appropriate though: we sadly don't display what the keymap is, and with a wrong keymap, any attempts to enter a password are likely to fail. --- src/locale/localed.c | 19 +++++++++++++------ src/shared/kbd-util.c | 29 +++++++++++++++++++++++++++++ src/shared/kbd-util.h | 1 + 3 files changed, 43 insertions(+), 6 deletions(-) diff --git a/src/locale/localed.c b/src/locale/localed.c index 9c97313edc..f44c0bab2a 100644 --- a/src/locale/localed.c +++ b/src/locale/localed.c @@ -19,6 +19,7 @@ #include "bus-polkit.h" #include "def.h" #include "dlfcn-util.h" +#include "kbd-util.h" #include "keymap-util.h" #include "locale-util.h" #include "macro.h" @@ -474,7 +475,7 @@ static int method_set_locale(sd_bus_message *m, void *userdata, sd_bus_error *er static int method_set_vc_keyboard(sd_bus_message *m, void *userdata, sd_bus_error *error) { Context *c = userdata; - const char *keymap, *keymap_toggle; + const char *name, *keymap, *keymap_toggle; int convert, interactive, r; assert(m); @@ -490,17 +491,23 @@ static int method_set_vc_keyboard(sd_bus_message *m, void *userdata, sd_bus_erro r = vconsole_read_data(c, m); if (r < 0) { log_error_errno(r, "Failed to read virtual console keymap data: %m"); - return sd_bus_error_setf(error, SD_BUS_ERROR_FAILED, "Failed to read virtual console keymap data"); + return sd_bus_error_set_errnof(error, r, "Failed to read virtual console keymap data: %m"); + } + + FOREACH_STRING(name, keymap ?: keymap_toggle, keymap ? keymap_toggle : NULL) { + r = keymap_exists(name); /* This also verifies that the keymap name is kosher. */ + if (r < 0) { + log_error_errno(r, "Failed to check keymap %s: %m", name); + return sd_bus_error_set_errnof(error, r, "Failed to check keymap %s: %m", name); + } + if (r == 0) + return sd_bus_error_setf(error, SD_BUS_ERROR_FAILED, "Keymap %s is not installed.", name); } if (streq_ptr(keymap, c->vc_keymap) && streq_ptr(keymap_toggle, c->vc_keymap_toggle)) return sd_bus_reply_method_return(m, NULL); - if ((keymap && (!filename_is_valid(keymap) || !string_is_safe(keymap))) || - (keymap_toggle && (!filename_is_valid(keymap_toggle) || !string_is_safe(keymap_toggle)))) - return sd_bus_error_setf(error, SD_BUS_ERROR_INVALID_ARGS, "Received invalid keymap data"); - r = bus_verify_polkit_async( m, CAP_SYS_ADMIN, diff --git a/src/shared/kbd-util.c b/src/shared/kbd-util.c index f5669f7c03..430ae7c881 100644 --- a/src/shared/kbd-util.c +++ b/src/shared/kbd-util.c @@ -12,6 +12,7 @@ #include "strv.h" #include "utf8.h" +static thread_local const char *keymap_name = NULL; static thread_local Set *keymaps = NULL; static int nftw_cb( @@ -24,6 +25,9 @@ static int nftw_cb( char *e; int r; + /* If keymap_name is non-null, return true if keymap keymap_name is found. + * Otherwise, add all keymaps to keymaps. */ + if (tflag != FTW_F) return 0; @@ -43,6 +47,9 @@ static int nftw_cb( if (e) *e = 0; + if (keymap_name) + return streq(p, keymap_name); + if (!keymap_is_valid(p)) return 0; @@ -108,3 +115,25 @@ bool keymap_is_valid(const char *name) { return true; } + +int keymap_exists(const char *name) { + int r = 0; + + if (!keymap_is_valid(name)) + return -EINVAL; + + keymap_name = name; + + const char *dir; + NULSTR_FOREACH(dir, KBD_KEYMAP_DIRS) { + r = nftw(dir, nftw_cb, 20, FTW_PHYS); + if (r > 0) + break; + if (r < 0 && errno != ENOENT) + log_debug_errno(errno, "Failed to read keymap list from %s, ignoring: %m", dir); + } + + keymap_name = NULL; + + return r > 0; +} diff --git a/src/shared/kbd-util.h b/src/shared/kbd-util.h index 6714aeb9e0..a2fc2e6a3e 100644 --- a/src/shared/kbd-util.h +++ b/src/shared/kbd-util.h @@ -18,3 +18,4 @@ int get_keymaps(char ***l); bool keymap_is_valid(const char *name); +int keymap_exists(const char *name); From 73d0806abb3217dd4f3c0f9859bef4813cb96c99 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Thu, 4 Mar 2021 11:52:25 +0100 Subject: [PATCH 5/5] shared/kbd-util: simplify suffix stripping It only came to me now that this can be prettified. --- src/shared/kbd-util.c | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-) diff --git a/src/shared/kbd-util.c b/src/shared/kbd-util.c index 430ae7c881..92abaea65e 100644 --- a/src/shared/kbd-util.c +++ b/src/shared/kbd-util.c @@ -22,7 +22,6 @@ static int nftw_cb( struct FTW *ftwbuf) { _cleanup_free_ char *p = NULL; - char *e; int r; /* If keymap_name is non-null, return true if keymap keymap_name is found. @@ -31,22 +30,18 @@ static int nftw_cb( if (tflag != FTW_F) return 0; - if (!endswith(fpath, ".map") && - !endswith(fpath, ".map.gz")) + fpath = basename(fpath); + + const char *e = endswith(fpath, ".map") ?: endswith(fpath, ".map.gz"); + if (!e) return 0; - p = strdup(basename(fpath)); + p = strndup(fpath, e - fpath); if (!p) { errno = ENOMEM; return -1; } - e = endswith(p, ".map"); - if (!e) - e = endswith(p, ".map.gz"); - if (e) - *e = 0; - if (keymap_name) return streq(p, keymap_name);