[systemd-devel] [PATCH] localed: validate set-x11-keymap input

[Lennart Poettering]

On Tue, 04.11.14 12:05, Jan Synacek (jsynacek at redhat.com) wrote:

> diff --git a/src/locale/localectl.c b/src/locale/localectl.c
> index d4a2d29..8f9e4da 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;
> @@ -389,14 +390,7 @@ 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;
> +        enum keymap_state look_for;

While we don#t follow this rule too strictly we usually define typdefs
for enums and name them in CamelCase. Hence, please rename this type
"KeymapState" instead of "enum keymap_state".

That said, is "state" really the right name here? Shouldn't it be
"field" or "component" or "item" or so?

>              !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,12 @@ 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) {
> +                        log_error("Failed to validate X11 keyboard layout: %s", strerror(-r));
> +                        return sd_bus_error_set(error, SD_BUS_ERROR_FAILED, msg);
> +                }
> +

Please return the error back to the client instead of logging it
away. Use sd_bus_error_setf() for that.

Use SD_BUS_ERROR_INVALID_ARGS as error id.

> +
> +static char **xkb_parse_argument(const char *arg)
> +{
> +        _cleanup_free_ char *input;
> +        char *token;
> +        char **result = NULL;
> +        int r;
> +
> +        assert(arg);
> +
> +        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;
> +}

Please place the opening { of a function body on the same line as the
function declaration.

I figure strv_split() does exactly the same thing, please use that.

> +
> +int xkb_get_keymaps(char ***list, enum keymap_state look_for, const char *layout_prefix)
> +{
> +        _cleanup_fclose_ FILE *f;
> +        char line[LINE_MAX];
> +        enum keymap_state state = NONE;
> +        int r;
> +
> +        f = fopen("/usr/share/X11/xkb/rules/base.lst", "re");
> +        if (!f) {
> +                log_error("Failed to open keyboard mapping list. %m");
> +                return -errno;
> +        }

This should probably become a function call that returns errors but
doesn't log about them, leaving that to the caller.

> +int xkb_validate_keymaps(const char *model,
> +                         const char *layouts_arg,
> +                         const char *variants_arg,
> +                         const char *options_arg,
> +                         char **error)
> +{
> +        int r;
> +
> +        if (layouts_arg) {
> +                _cleanup_strv_free_ char **layouts_list = NULL;
> +                char **it, **layouts;
> +                int i = 0;
> +
> +                r = xkb_get_keymaps(&layouts_list, LAYOUTS, NULL);
> +                if (r < 0)
> +                        return r;
> +
> +                layouts = strv_split(layouts_arg, ",");
> +                if (!layouts)
> +                        return log_oom();
> +
> +                STRV_FOREACH(it, layouts) {
> +                        _cleanup_strv_free_ char **variants_list = NULL;
> +                        bool variants_left = true;
> +                        int n;
> +
> +                        if (!strv_find(layouts_list, *it)) {
> +                                r = asprintf(error, "Requested layout '%s' not available.\n", *it);
> +                                if (r < 0)
> +                                        return log_oom();
> +                                return -EINVAL;
> +                        }
> +
> +                        if (variants_left && variants_arg) {
> +                                _cleanup_strv_free_ char **variants;
> +
> +                                r = xkb_get_keymaps(&variants_list, VARIANTS, *it);
> +                                if (r < 0)
> +                                        return r;

Hmm, reading the file over and over and over again sounds less than
ideal. Maybe we should intrdouce a struct here and then make
xkb_get_keymaps() return an array of structs really?

Lennart

-- 
Lennart Poettering, Red Hat