[systemd-devel] [PATCH] localed: validate set-x11-keymap input
Jan Synacek
jsynacek at redhat.com
Tue Nov 11 01:03:36 PST 2014
Lennart Poettering <lennart at poettering.net> writes:
> 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.
The errno part of the error is logged and the "digestible by the user"
part is returned in the error message. I did this in exactly the same
way as it was already there a few lines below (x11_write_data()
call). Are you sure I should change it? If yes, both log_error()s should
probably be changed.
> 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.
Oops, old habit, sorry, will fix.
> I figure strv_split() does exactly the same thing, please use that.
That's what I had originally used. The problem is that it throws away
the empty parts, which is what I don't want. I need it to return the
empty strings after splitting as well. That's why I wrote my own that
uses strsep(). Since I figured it's probably not needed anywhere else, I
wrote it locally instead of introducing a new shared strv_<something>
function.
>> +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.
Again, are you sure? The logged error is very "local" to what the code is
trying to do.
>> +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?
That sounds ok, I'll see what I can do. I wanted to preserve as much of
the original code as I could, but maybe it wasn't the right decision.
> Lennart
Thanks for the review,
--
Jan Synacek
Software Engineer, Red Hat
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 818 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/systemd-devel/attachments/20141111/254ac2c6/attachment.sig>
More information about the systemd-devel
mailing list