[v2] maintainer omnibus: testing, fuzz, plugins, documentation

[PATCH v2 06/26] qemu-keymap: properly check return from xkb_keymap_mod_get_index

Posted by Alex Bennée 2 years, 7 months ago

We can return XKB_MOD_INVALID which rightly gets flagged by sanitisers
as an overly wide shift attempt.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 qemu-keymap.c | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

diff --git a/qemu-keymap.c b/qemu-keymap.c
index 229866e004..8c80f7a4ed 100644
--- a/qemu-keymap.c
+++ b/qemu-keymap.c
@@ -140,6 +140,18 @@ static void usage(FILE *out)
             names.options ?: "-");
 }
 
+static xkb_mod_mask_t get_mod(struct xkb_keymap *map, const char *name)
+{
+    xkb_mod_index_t mod;
+    xkb_mod_mask_t mask = 0;
+
+    mod = xkb_keymap_mod_get_index(map, name);
+    if (mod != XKB_MOD_INVALID) {
+        mask = (1 << mod);
+    }
+    return mask;
+}
+
 int main(int argc, char *argv[])
 {
     struct xkb_context *ctx;
@@ -215,14 +227,10 @@ int main(int argc, char *argv[])
                 mod, xkb_keymap_mod_get_name(map, mod));
     }
 
-    mod = xkb_keymap_mod_get_index(map, "Shift");
-    shift = (1 << mod);
-    mod = xkb_keymap_mod_get_index(map, "Control");
-    ctrl = (1 << mod);
-    mod = xkb_keymap_mod_get_index(map, "AltGr");
-    altgr = (1 << mod);
-    mod = xkb_keymap_mod_get_index(map, "NumLock");
-    numlock = (1 << mod);
+    shift = get_mod(map, "Shift");
+    ctrl = get_mod(map, "Control");
+    altgr = get_mod(map, "AltGr");
+    numlock = get_mod(map, "NumLock");
 
     state = xkb_state_new(map);
     xkb_keymap_key_for_each(map, walk_map, state);
-- 
2.39.2

Re: [PATCH v2 06/26] qemu-keymap: properly check return from xkb_keymap_mod_get_index

Posted by Richard Henderson 2 years, 7 months ago

On 6/26/23 23:59, Alex Bennée wrote:
> We can return XKB_MOD_INVALID which rightly gets flagged by sanitisers
> as an overly wide shift attempt.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>   qemu-keymap.c | 24 ++++++++++++++++--------
>   1 file changed, 16 insertions(+), 8 deletions(-)
> 
> diff --git a/qemu-keymap.c b/qemu-keymap.c
> index 229866e004..8c80f7a4ed 100644
> --- a/qemu-keymap.c
> +++ b/qemu-keymap.c
> @@ -140,6 +140,18 @@ static void usage(FILE *out)
>               names.options ?: "-");
>   }
>   
> +static xkb_mod_mask_t get_mod(struct xkb_keymap *map, const char *name)
> +{
> +    xkb_mod_index_t mod;
> +    xkb_mod_mask_t mask = 0;
> +
> +    mod = xkb_keymap_mod_get_index(map, name);
> +    if (mod != XKB_MOD_INVALID) {
> +        mask = (1 << mod);
> +    }
> +    return mask;
> +}

You have yet to answer Peter's question -- asked twice -- about what changes in the 
keymaps with this. If nothing, should it in fact be an assert instead?


r~

Re: [PATCH v2 06/26] qemu-keymap: properly check return from xkb_keymap_mod_get_index

Posted by Alex Bennée 2 years, 7 months ago

Richard Henderson <richard.henderson@linaro.org> writes:

> On 6/26/23 23:59, Alex Bennée wrote:
>> We can return XKB_MOD_INVALID which rightly gets flagged by sanitisers
>> as an overly wide shift attempt.
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> ---
>>   qemu-keymap.c | 24 ++++++++++++++++--------
>>   1 file changed, 16 insertions(+), 8 deletions(-)
>> diff --git a/qemu-keymap.c b/qemu-keymap.c
>> index 229866e004..8c80f7a4ed 100644
>> --- a/qemu-keymap.c
>> +++ b/qemu-keymap.c
>> @@ -140,6 +140,18 @@ static void usage(FILE *out)
>>               names.options ?: "-");
>>   }
>>   +static xkb_mod_mask_t get_mod(struct xkb_keymap *map, const char
>> *name)
>> +{
>> +    xkb_mod_index_t mod;
>> +    xkb_mod_mask_t mask = 0;
>> +
>> +    mod = xkb_keymap_mod_get_index(map, name);
>> +    if (mod != XKB_MOD_INVALID) {
>> +        mask = (1 << mod);
>> +    }
>> +    return mask;
>> +}
>
> You have yet to answer Peter's question -- asked twice -- about what
> changes in the keymaps with this. If nothing, should it in fact be an
> assert instead?

Ahh in the other thread. No change, it looks like AltGr just doesn't
exist for some keymaps:

  🕙21:20:36 alex@zen:qemu.git/builds/all  (399fc0c) (REBASING 2/22) [$!?] 
  ➜  diff -ub gb.before gb.after
  🕙21:20:43 alex@zen:qemu.git/builds/all  (399fc0c) (REBASING 2/22) [$!?] 
  ➜  diff -ub ara.before ara.after
  🕙21:20:50 alex@zen:qemu.git/builds/all  (399fc0c) (REBASING 2/22) [$!?] 
  ➜  ag "Alt" ara.after 
  21:#     9: Alt
  23:#    11: LAlt
  24:#    12: RAlt
  29:#    17: AltGr
  294:Alt_L 0x38
  1711:Alt_R 0xb8
  🕙21:22:14 alex@zen:qemu.git/builds/all  (399fc0c) (REBASING 2/22) [$!?] 
  ➜  ag "Alt" gb.after 
  21:#     9: Alt
  23:#    11: LAlt
  24:#    12: RAlt
  29:#    17: AltGr
  338:Alt_L 0x38
  1757:Alt_R 0xb8


-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH v2 06/26] qemu-keymap: properly check return from xkb_keymap_mod_get_index

Posted by Peter Maydell 2 years, 7 months ago

On Tue, 27 Jun 2023 at 10:57, Alex Bennée <alex.bennee@linaro.org> wrote:
> Ahh in the other thread. No change, it looks like AltGr just doesn't
> exist for some keymaps:
>
>   🕙21:20:36 alex@zen:qemu.git/builds/all  (399fc0c) (REBASING 2/22) [$!?]
>   ➜  diff -ub gb.before gb.after
>   🕙21:20:43 alex@zen:qemu.git/builds/all  (399fc0c) (REBASING 2/22) [$!?]
>   ➜  diff -ub ara.before ara.after
>   🕙21:20:50 alex@zen:qemu.git/builds/all  (399fc0c) (REBASING 2/22) [$!?]
>   ➜  ag "Alt" ara.after
>   21:#     9: Alt
>   23:#    11: LAlt
>   24:#    12: RAlt
>   29:#    17: AltGr
>   294:Alt_L 0x38
>   1711:Alt_R 0xb8
>   🕙21:22:14 alex@zen:qemu.git/builds/all  (399fc0c) (REBASING 2/22) [$!?]
>   ➜  ag "Alt" gb.after
>   21:#     9: Alt
>   23:#    11: LAlt
>   24:#    12: RAlt
>   29:#    17: AltGr
>   338:Alt_L 0x38
>   1757:Alt_R 0xb8

I'm having some difficulty interpreting this output. It
seems to show that there is an AltGr modifier in both
mappings (that's why it appears in the modifier listing).
And for me (xkeyboard-config 2.33) in the gb mapping it's
used too:

# evdev 2 (0x2), QKeyCode "1", number 0x2
1 0x02
exclam 0x02 shift
onesuperior 0x02 altgr
exclamdown 0x02 shift altgr

(i.e. the '1' key is 1 with no modifiers, ! with shift,
superscript-1 with altgr, and inverted exclamation mark
with shift-altgr).

The 'ara' keymap likewise has and uses altgr:
# evdev 2 (0x2), QKeyCode "1", number 0x2
1 0x02
exclam 0x02 shift
Arabic_1 0x02 altgr

So on the machines where we were running into this,
what's the version of xkeyboard-config and do we
output the same mapping as we do on machines with
the older xkeyboard-config ?

thanks
-- PMM

Re: [PATCH v2 06/26] qemu-keymap: properly check return from xkb_keymap_mod_get_index

Posted by Alex Bennée 2 years, 7 months ago

Peter Maydell <peter.maydell@linaro.org> writes:

> On Tue, 27 Jun 2023 at 10:57, Alex Bennée <alex.bennee@linaro.org> wrote:
>> Ahh in the other thread. No change, it looks like AltGr just doesn't
>> exist for some keymaps:
>>
>>   🕙21:20:36 alex@zen:qemu.git/builds/all  (399fc0c) (REBASING 2/22) [$!?]
>>   ➜  diff -ub gb.before gb.after
>>   🕙21:20:43 alex@zen:qemu.git/builds/all  (399fc0c) (REBASING 2/22) [$!?]
>>   ➜  diff -ub ara.before ara.after
>>   🕙21:20:50 alex@zen:qemu.git/builds/all  (399fc0c) (REBASING 2/22) [$!?]
>>   ➜  ag "Alt" ara.after
>>   21:#     9: Alt
>>   23:#    11: LAlt
>>   24:#    12: RAlt
>>   29:#    17: AltGr
>>   294:Alt_L 0x38
>>   1711:Alt_R 0xb8
>>   🕙21:22:14 alex@zen:qemu.git/builds/all  (399fc0c) (REBASING 2/22) [$!?]
>>   ➜  ag "Alt" gb.after
>>   21:#     9: Alt
>>   23:#    11: LAlt
>>   24:#    12: RAlt
>>   29:#    17: AltGr
>>   338:Alt_L 0x38
>>   1757:Alt_R 0xb8
>
> I'm having some difficulty interpreting this output. It
> seems to show that there is an AltGr modifier in both
> mappings (that's why it appears in the modifier listing).
> And for me (xkeyboard-config 2.33) in the gb mapping it's
> used too:
>
> # evdev 2 (0x2), QKeyCode "1", number 0x2
> 1 0x02
> exclam 0x02 shift
> onesuperior 0x02 altgr
> exclamdown 0x02 shift altgr
>
> (i.e. the '1' key is 1 with no modifiers, ! with shift,
> superscript-1 with altgr, and inverted exclamation mark
> with shift-altgr).
>
> The 'ara' keymap likewise has and uses altgr:
> # evdev 2 (0x2), QKeyCode "1", number 0x2
> 1 0x02
> exclam 0x02 shift
> Arabic_1 0x02 altgr

Ahh right I see those too.

>
> So on the machines where we were running into this,
> what's the version of xkeyboard-config and do we
> output the same mapping as we do on machines with
> the older xkeyboard-config ?

2.35 I think:

ii  xkb-data                       2.35.1-1                    all          X Keyboard Extension (XKB) configuration data

>
> thanks
> -- PMM


-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH v2 06/26] qemu-keymap: properly check return from xkb_keymap_mod_get_index

Posted by Thomas Huth 2 years, 7 months ago

On 26/06/2023 23.59, Alex Bennée wrote:
> We can return XKB_MOD_INVALID which rightly gets flagged by sanitisers
> as an overly wide shift attempt.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>   qemu-keymap.c | 24 ++++++++++++++++--------
>   1 file changed, 16 insertions(+), 8 deletions(-)

Reviewed-by: Thomas Huth <thuth@redhat.com>