[systemd-devel] [PATCH v2] localed: validate set-x11-keymap input
Lennart Poettering
lennart at poettering.net
Fri Nov 14 08:31:40 PST 2014
On Fri, 14.11.14 12:42, Jan Synacek (jsynacek at redhat.com) wrote:
>
> 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;
As mentioned earlier, we usually use "typedef enum" for internal
enums.
> +
> +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;
I'd really prefer if we could update strv_split() with a new flag or
so that doesn't collapse multiple separator fields into one.
> +}
> +
> +int xkb_keymap_get_components(X11Keymap *keymap) {
> + _cleanup_strv_free_ char **models = NULL, **options = NULL;
> + _cleanup_fclose_ FILE *f;
Needs to be initialized to NULL. Otherwise we might call fclose() on
an uninitialized pointer of hashmap_new() fails.
> + char line[LINE_MAX];
> + enum KeymapComponent state = NONE;
> + size_t m = 0, o = 0, allocm = 0, alloco = 0;
> +
Spurious newline?
> + Hashmap *x11_layouts;
> + int r;
> +
> + 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;
> + }
x11_layouts is not freed here...
> +
> + 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();
> + }
It's nicer to just invoke hashmap_put() right-away, and figure out
by the return value if the thing worked...
> +
> + 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;
> + }
Instead of returning a newly allocated string here, can't we just pass
in the sd_bus_error object and make the function fill it in?
> + 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) {
Hmm, wouldn't this call be better in localectl.c? I mean, we don't use
it anywhere else, do we?
> + 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();
There's hashmap_get_strv() for cases like this.
Also, please neer invoke strv_extend() when appending to unbounded
arrays. It's slow!
> +
> + 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);
> +
> + } else
> + assert_not_reached("Wrong parameter.");
> +}
> +
> +void xkb_keymap_free_components(X11Keymap *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);
> + }
Humm, when clearing up hashmaps please just write a loop that steals
the first entry, free it and then repeat. Iterating through a hashmap
while deleting its entries is unnecessary...
Lennart
--
Lennart Poettering, Red Hat
More information about the systemd-devel
mailing list