[systemd-devel] [PATCH v2] localed: validate set-x11-keymap input
Ronny Chevalier
chevalier.ronny at gmail.com
Fri Nov 14 10:27:54 PST 2014
2014-11-14 12:42 GMT+01:00 Jan Synacek <jsynacek at redhat.com>:
> Try to validate the input similarly to how setxkbmap does it. Multiple
> layouts and variants can be specified, separated by a comma. Variants
> can also be left out, meaning that the user doesn't want any particular
> variant for the respective layout.
>
> Variants are validated respectively to their layouts. First variant
> validates against first layout, second variant to second layout, etc. If
> there are more entries of either layouts or variants, only their
> respective counterparts are validated, the rest is ignored.
>
> Examples:
> $ set-x11-keymap us,cz pc105 ,qwerty
> "us" is not validated, because no respective variant was specified. "cz"
> is checked for an existing "qwerty" variant, the check succeeds.
>
> $ set-x11-keymap us pc105 ,qwerty
> "us" is not validated as in the above example. The rest of the variants
> is ignored, because there are no respective layouts.
>
> $ set-x11-keymap us,cz pc105 qwerty
> "us" is validated against the "qwerty" variant, check fails, because
> there is no "qwerty" variant for the "us" layout.
>
> $ set-x11-keymap us,cz pc105 euro,qwerty
> Validation succeeds, there is the "euro" variant for the "us" layout,
> and "qwerty" variant for the "cz" layout.
>
> http://lists.freedesktop.org/archives/systemd-devel/2014-October/024411.html
> ---
> v2:
> - rewrite to use the X11Keymap struct
> - use greedy allocation where it makes sense
> - rename enum keymap_state to KeymapComponent
> - on the server side, propagate error to the client and don't log it
> - on the server side, change SD_BUS_ERROR_FAILED to SD_BUS_ERROR_INVALID_ARGS
>
> Makefile.am | 2 +
> src/locale/localectl.c | 85 ++-----------
> src/locale/localed.c | 6 +
> src/shared/xkb-util.c | 319 +++++++++++++++++++++++++++++++++++++++++++++++++
> src/shared/xkb-util.h | 50 ++++++++
> 5 files changed, 385 insertions(+), 77 deletions(-)
> create mode 100644 src/shared/xkb-util.c
> create mode 100644 src/shared/xkb-util.h
>
> diff --git a/Makefile.am b/Makefile.am
> index 701666c..224582c 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -772,6 +772,8 @@ libsystemd_shared_la_SOURCES = \
> src/shared/time-util.h \
> src/shared/locale-util.c \
> src/shared/locale-util.h \
> + src/shared/xkb-util.c \
> + src/shared/xkb-util.h \
> src/shared/mempool.c \
> src/shared/mempool.h \
> src/shared/hashmap.c \
> diff --git a/src/locale/localectl.c b/src/locale/localectl.c
> index d4a2d29..08a1011 100644
> --- a/src/locale/localectl.c
> +++ b/src/locale/localectl.c
> @@ -46,6 +46,7 @@
> #include "virt.h"
> #include "fileio.h"
> #include "locale-util.h"
> +#include "xkb-util.h"
>
> static bool arg_no_pager = false;
> static bool arg_ask_password = true;
> @@ -388,15 +389,8 @@ static int set_x11_keymap(sd_bus *bus, char **args, unsigned n) {
>
> static int list_x11_keymaps(sd_bus *bus, char **args, unsigned n) {
> _cleanup_fclose_ FILE *f = NULL;
> - _cleanup_strv_free_ char **list = NULL;
> - char line[LINE_MAX];
> - enum {
> - NONE,
> - MODELS,
> - LAYOUTS,
> - VARIANTS,
> - OPTIONS
> - } state = NONE, look_for;
> + _cleanup_(xkb_keymap_free_components) X11Keymap keymap = {};
> + enum KeymapComponent look_for;
> int r;
>
> if (n > 2) {
> @@ -404,12 +398,6 @@ static int list_x11_keymaps(sd_bus *bus, char **args, unsigned n) {
> return -EINVAL;
> }
>
> - f = fopen("/usr/share/X11/xkb/rules/base.lst", "re");
> - if (!f) {
> - log_error("Failed to open keyboard mapping list. %m");
> - return -errno;
> - }
> -
> if (streq(args[0], "list-x11-keymap-models"))
> look_for = MODELS;
> else if (streq(args[0], "list-x11-keymap-layouts"))
> @@ -421,71 +409,14 @@ static int list_x11_keymaps(sd_bus *bus, char **args, unsigned n) {
> else
> assert_not_reached("Wrong parameter");
>
> - FOREACH_LINE(line, f, break) {
> - char *l, *w;
> -
> - l = strstrip(line);
> -
> - if (isempty(l))
> - continue;
> -
> - if (l[0] == '!') {
> - if (startswith(l, "! model"))
> - state = MODELS;
> - else if (startswith(l, "! layout"))
> - state = LAYOUTS;
> - else if (startswith(l, "! variant"))
> - state = VARIANTS;
> - else if (startswith(l, "! option"))
> - state = OPTIONS;
> - else
> - state = NONE;
> -
> - continue;
> - }
> -
> - if (state != look_for)
> - continue;
> -
> - w = l + strcspn(l, WHITESPACE);
> -
> - if (n > 1) {
> - char *e;
> -
> - if (*w == 0)
> - continue;
> -
> - *w = 0;
> - w++;
> - w += strspn(w, WHITESPACE);
> -
> - e = strchr(w, ':');
> - if (!e)
> - continue;
> -
> - *e = 0;
> -
> - if (!streq(w, args[1]))
> - continue;
> - } else
> - *w = 0;
> -
> - r = strv_extend(&list, l);
> - if (r < 0)
> - return log_oom();
> - }
> -
> - if (strv_isempty(list)) {
> - log_error("Couldn't find any entries.");
> - return -ENOENT;
> - }
> -
> - strv_sort(list);
> - strv_uniq(list);
> + r = xkb_keymap_get_components(&keymap);
> + if (r < 0)
> + return r;
>
> pager_open_if_enabled();
>
> - strv_print(list);
> + xkb_keymap_print_components(&keymap, look_for, (n > 1) ? args[1] : NULL);
> +
> return 0;
> }
>
> diff --git a/src/locale/localed.c b/src/locale/localed.c
> index 9377ce5..e0f862c 100644
> --- a/src/locale/localed.c
> +++ b/src/locale/localed.c
> @@ -40,6 +40,7 @@
> #include "bus-message.h"
> #include "event-util.h"
> #include "locale-util.h"
> +#include "xkb-util.h"
>
> enum {
> /* We don't list LC_ALL here on purpose. People should be
> @@ -1031,6 +1032,7 @@ static int method_set_x11_keyboard(sd_bus *bus, sd_bus_message *m, void *userdat
> !streq_ptr(model, c->x11_model) ||
> !streq_ptr(variant, c->x11_variant) ||
> !streq_ptr(options, c->x11_options)) {
> + _cleanup_free_ char *msg = NULL;
>
> if ((layout && !string_is_safe(layout)) ||
> (model && !string_is_safe(model)) ||
> @@ -1050,6 +1052,10 @@ static int method_set_x11_keyboard(sd_bus *bus, sd_bus_message *m, void *userdat
> free_and_strdup(&c->x11_options, options) < 0)
> return -ENOMEM;
>
> + r = xkb_validate_keymaps(model, layout, variant, options, &msg);
> + if (r < 0)
> + return sd_bus_error_set(error, SD_BUS_ERROR_INVALID_ARGS, msg);
> +
> r = x11_write_data(c);
> if (r < 0) {
> log_error("Failed to set X11 keyboard layout: %s", strerror(-r));
> diff --git a/src/shared/xkb-util.c b/src/shared/xkb-util.c
> new file mode 100644
> index 0000000..89c9922
> --- /dev/null
> +++ b/src/shared/xkb-util.c
> @@ -0,0 +1,319 @@
> +/*-*- Mode: C; c-basic-offset: 8; indent-tabs-mode: nil -*-*/
> +
> +/***
> + This file is part of systemd.
> +
> + Copyright 2014 Lennart Poettering
> +
> + systemd is free software; you can redistribute it and/or modify it
> + under the terms of the GNU Lesser General Public License as published by
> + the Free Software Foundation; either version 2.1 of the License, or
> + (at your option) any later version.
> +
> + systemd is distributed in the hope that it will be useful, but
> + WITHOUT ANY WARRANTY; without even the implied warranty of
> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + Lesser General Public License for more details.
> +
> + You should have received a copy of the GNU Lesser General Public License
> + along with systemd; If not, see <http://www.gnu.org/licenses/>.
> +***/
> +
> +#include "xkb-util.h"
> +
> +static char **xkb_parse_argument(const char *arg) {
> + _cleanup_free_ char *input = NULL;
> + char *token;
> + char **result = NULL;
> + int r;
> +
> + if (!arg)
> + return NULL;
> +
> + input = strdup(arg);
> + if (!input)
> + return NULL;
> +
> + token = strsep(&input, ",");
> + while(token) {
> + r = strv_extend(&result, token);
> + if (r < 0)
> + return NULL;
> + token = strsep(&input, ",");
> + }
> +
> + return result;
> +}
> +
> +int xkb_keymap_get_components(X11Keymap *keymap) {
> + _cleanup_strv_free_ char **models = NULL, **options = NULL;
> + _cleanup_fclose_ FILE *f;
> + char line[LINE_MAX];
> + enum KeymapComponent state = NONE;
> + size_t m = 0, o = 0, allocm = 0, alloco = 0;
> +
> + Hashmap *x11_layouts;
> + int r;
> +
assert(keymap);
> + x11_layouts = hashmap_new(&string_hash_ops);
> + if (!x11_layouts)
> + return log_oom();
> +
> + f = fopen("/usr/share/X11/xkb/rules/base.lst", "re");
> + if (!f) {
> + log_error("Failed to open keyboard mapping list. %m");
> + return -errno;
> + }
> +
> + FOREACH_LINE(line, f, break) {
> + char *l, *w;
> + _cleanup_free_ char *layout = NULL;
> +
> + l = strstrip(line);
> +
> + if (isempty(l))
> + continue;
> +
> + if (l[0] == '!') {
> + if (startswith(l, "! model"))
> + state = MODELS;
> + else if (startswith(l, "! layout"))
> + state = LAYOUTS;
> + else if (startswith(l, "! variant"))
> + state = VARIANTS;
> + else if (startswith(l, "! option"))
> + state = OPTIONS;
> + else
> + state = NONE;
> +
> + continue;
> + }
> +
> + w = l + strcspn(l, WHITESPACE);
> +
> + if (state == VARIANTS && *w != 0) {
> + char *e;
> +
> + *w = 0;
> + w++;
> + w += strspn(w, WHITESPACE);
> +
> + e = strchr(w, ':');
> + if (!e)
> + continue;
> +
> + *e = 0;
> + layout = strdup(w);
> + } else {
> + *w = 0;
> + layout = strdup(l);
> + }
> +
> + if (!layout)
> + return log_oom();
> +
> + if (state == LAYOUTS) {
> + _cleanup_set_free_ Set *item = NULL;
> +
> + item = set_new(&string_hash_ops);
> + if (!item)
> + return log_oom();
> +
> + if (!hashmap_contains(x11_layouts, layout)) {
> + r = hashmap_put(x11_layouts, layout, item);
> + if (r < 0)
> + return log_oom();
> + }
> +
> + item = NULL;
> + layout = NULL;
> +
> + } else if (state == VARIANTS) {
> + _cleanup_set_free_ Set *item = NULL;
> +
> + if (!hashmap_contains(x11_layouts, layout)) {
> + item = set_new(&string_hash_ops);
> + if (!item)
> + return log_oom();
> +
> + } else {
> + item = hashmap_get(x11_layouts, layout);
> + assert(item);
> + }
> +
> + r = set_put_strdup(item, l);
> + if (r < 0)
> + return log_oom();
> +
> + r = hashmap_update(x11_layouts, layout, item);
> + if (r < 0)
> + return log_oom();
> +
> + item = NULL;
> +
> + } else if (state == MODELS) {
> + if (!GREEDY_REALLOC(models, allocm, m + 2))
> + return log_oom();
> +
> + models[m] = strdup(l);
> + if (!models[m])
> + return log_oom();
> +
> + m++;
> + } else if (state == OPTIONS) {
> + if (!GREEDY_REALLOC(options, alloco, o + 2))
> + return log_oom();
Why + 2 and not +1 (same above) ?
You are only storing one element after this, so you just need o+1.
> +
> + options[o] = strdup(l);
> + if (!options[o])
> + return log_oom();
> +
> + o++;
> + }
> + }
> +
> + if (m == 0)
> + log_warning("Couldn't find any xkb models.");
> + if (o == 0)
> + log_warning("Couldn't find any xkb options.");
> +
> + if (hashmap_isempty(x11_layouts))
> + log_warning("Couldn't find any xkb layouts or variants.");
> + else
> + keymap->x11_layouts = x11_layouts;
> +
> + models[m] = NULL;
> + keymap->models = models;
> + models = NULL;
> + options[o] = NULL;
> + keymap->options = options;
> + options = NULL;
> +
> + return 0;
> +}
> +
> +int xkb_validate_keymaps(const char *model,
> + const char *layouts_arg,
> + const char *variants_arg,
> + const char *options_arg,
> + char **error)
> +{
> + _cleanup_(xkb_keymap_free_components) X11Keymap keymap = {};
> + _cleanup_strv_free_ char **layouts = NULL, **variants = NULL;
> + char **it;
> + size_t i = 0, n;
> + int r;
> +
> + r = xkb_keymap_get_components(&keymap);
> + if (r < 0)
> + return r;
> +
> + layouts = xkb_parse_argument(layouts_arg);
> + variants = xkb_parse_argument(variants_arg);
> + n = strv_length(variants);
> +
> + STRV_FOREACH(it, layouts) {
> + Set *x11_layout;
> +
> + /* Empty layout, skip the variant check. */
> + if (streq(*it, ""))
> + goto next;
> +
> + x11_layout = hashmap_get(keymap.x11_layouts, *it);
> + if (!x11_layout) {
> + r = asprintf(error, "Requested layout '%s' not available.\n", *it);
> + if (r < 0)
> + return log_oom();
> + return -EINVAL;
> + }
> +
> + /* No variants, nothing to check. */
> + if (n == 0)
> + goto next;
> +
> + /* More layouts than variants, no need to look at variants anymore. */
> + if (i >= n)
> + goto next;
> +
> + /* Variant left out, skip the check. */
> + if (streq(variants[i], ""))
> + goto next;
> +
> + if (!set_contains(x11_layout, variants[i])) {
> + r = asprintf(error, "Requested variant '%s' for layout '%s' not available.\n", variants[i], *it);
> + if (r < 0)
> + return log_oom();
> + return -EINVAL;
> + }
> + next:
> + i++;
> + }
> +
> + /* Since setxkbmap doesn't validate model and options, we
> + don't either. It might be good to add the check, though. */
> + return 0;
> +}
> +
> +void xkb_keymap_print_components(X11Keymap *keymap, enum KeymapComponent look_for, const char *layout_prefix) {
assert(keymap);
> + if (look_for == LAYOUTS) {
> + Set *s;
> + char *k;
> + Iterator i;
> + /* XXX: Is there a better way to sort Hashmap keys? */
> + _cleanup_strv_free_ char **tmp = NULL;
> +
> + HASHMAP_FOREACH_KEY(s, k, keymap->x11_layouts, i)
> + if (strv_extend(&tmp, k) < 0)
> + (void) log_oom();
> +
> + strv_sort(tmp);
> + strv_print(tmp);
> +
> + } else if (look_for == VARIANTS) {
> + Set *s;
> + char *k;
> + Iterator i, j;
> + /* XXX: Is there a better way to sort Set keys? */
> + _cleanup_strv_free_ char **tmp = NULL;
> +
> + if (layout_prefix) {
> + s = hashmap_get(keymap->x11_layouts, layout_prefix);
> + SET_FOREACH(k, s, j)
> + if (strv_extend(&tmp, k) < 0)
> + (void) log_oom();
> + } else {
> + HASHMAP_FOREACH(s, keymap->x11_layouts, i)
> + SET_FOREACH(k, s, j)
> + strv_extend(&tmp, k);
> + }
> +
> + strv_sort(tmp);
> + strv_print(tmp);
> +
> + } else if (look_for == MODELS) {
> + strv_sort(keymap->models);
> + strv_print(keymap->models);
> +
> + } else if (look_for == OPTIONS) {
> + strv_sort(keymap->options);
> + strv_print(keymap->options);
> +
Useless line (same above)
> + } else
> + assert_not_reached("Wrong parameter.");
> +}
> +
> +void xkb_keymap_free_components(X11Keymap *keymap) {
assert(keymap);
> + if (keymap->x11_layouts) {
> + Set *s;
> + char *k;
> + Iterator i;
> +
> + HASHMAP_FOREACH_KEY(s, k, keymap->x11_layouts, i) {
> + free(k);
> + set_free_free(s);
> + }
> + hashmap_free(keymap->x11_layouts);
> + }
> + strv_free(keymap->models);
> + strv_free(keymap->options);
> +}
> diff --git a/src/shared/xkb-util.h b/src/shared/xkb-util.h
> new file mode 100644
> index 0000000..548dd29
> --- /dev/null
> +++ b/src/shared/xkb-util.h
> @@ -0,0 +1,50 @@
> +/*-*- Mode: C; c-basic-offset: 8; indent-tabs-mode: nil -*-*/
> +
> +#pragma once
> +
> +/***
> + This file is part of systemd.
> +
> + Copyright 2014 Lennart Poettering
> +
> + systemd is free software; you can redistribute it and/or modify it
> + under the terms of the GNU Lesser General Public License as published by
> + the Free Software Foundation; either version 2.1 of the License, or
> + (at your option) any later version.
> +
> + systemd is distributed in the hope that it will be useful, but
> + WITHOUT ANY WARRANTY; without even the implied warranty of
> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + Lesser General Public License for more details.
> +
> + You should have received a copy of the GNU Lesser General Public License
> + along with systemd; If not, see <http://www.gnu.org/licenses/>.
> +***/
> +
> +#include <stdbool.h>
Unused in this header.
> +
> +#include "macro.h"
Unused in this header.
> +#include "strv.h"
Unused in this header.
> +#include "util.h"
Unused in this header.
> +#include "set.h"
Unused, you meant hashmap.h ? (or just typedef struct Hashmap Hashmap;)
These includes needs to be in xkb-util.c, where they are really
needed, not here.
> +
> +enum KeymapComponent {
> + NONE,
> + MODELS,
> + LAYOUTS,
> + VARIANTS,
> + OPTIONS
> +};
> +
> +typedef struct {
> + Hashmap *x11_layouts; /* char* -> Set* */
> + char **models;
> + char **options;
> +} X11Keymap;
> +
> +
> +int xkb_keymap_get_components(X11Keymap *keymap);
> +int xkb_validate_keymaps(const char *model, const char *layouts_arg, const char *variants_arg, const char *options_arg, char **error);
> +
> +void xkb_keymap_print_components(X11Keymap *keymap, enum KeymapComponent, const char *layout_prefix);
> +void xkb_keymap_free_components(X11Keymap *keymap);
> --
> 1.9.3
>
> _______________________________________________
> systemd-devel mailing list
> systemd-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/systemd-devel
More information about the systemd-devel
mailing list