[RFC PATCH] qemu-keymap: properly check return from xkb_keymap_mod_get_index

[RFC PATCH] qemu-keymap: properly check return from xkb_keymap_mod_get_index

[Alex Bennée]

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: [RFC PATCH] qemu-keymap: properly check return from xkb_keymap_mod_get_index

[Peter Maydell]

On Tue, 20 Jun 2023 at 16:04, Alex Bennée <alex.bennee@linaro.org> 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(-)

Looking at the code that works with the mask values
we are getting here, I think this ought to work
(if there's no AltGr modifier then the 0 mask means
the key state will be the same in normal and with the
altgr mask supplied, which we already check for).
Did you eyeball the output, though?

Also, which keymap runs into this? Is it every keymap
for some new version of the xkb data (which would imply
that the problem is that the AltGr modifier has changed
name), or is it only one specific keymap that happens
to have no AltGr key?

thanks
-- PMM

Re: [RFC PATCH] qemu-keymap: properly check return from xkb_keymap_mod_get_index

[Alex Bennée]

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

> On Tue, 20 Jun 2023 at 16:04, Alex Bennée <alex.bennee@linaro.org> 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(-)
>
> Looking at the code that works with the mask values
> we are getting here, I think this ought to work
> (if there's no AltGr modifier then the 0 mask means
> the key state will be the same in normal and with the
> altgr mask supplied, which we already check for).
> Did you eyeball the output, though?
>
> Also, which keymap runs into this? Is it every keymap
> for some new version of the xkb data (which would imply
> that the problem is that the AltGr modifier has changed
> name), or is it only one specific keymap that happens
> to have no AltGr key?

ar maybe? it only got flagged in clang-system once fedora was updated (I
assume with better sanitizers):

  [2773/3696] Generating pc-bios/keymaps/ar with a custom command
  FAILED: pc-bios/keymaps/ar 
  /builds/stsquad/qemu/build/qemu-keymap -f pc-bios/keymaps/ar -l ar
  ../qemu-keymap.c:223:16: runtime error: shift exponent 4294967295 is too large for 32-bit type 'int'
  SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior ../qemu-keymap.c:223:16 in 
  [2774/3696] Generating pc-bios/edk2-x86_64-code.fd with a custom command (wrapped by meson to capture output)
  [2775/3696] Generating pc-bios/edk2-x86_64-secure-code.fd with a custom command (wrapped by meson to capture output)
  ninja: build stopped: subcommand failed.
  make: *** [Makefile:153: run-ninja] Error 1

https://gitlab.com/stsquad/qemu/-/jobs/4500683186#L3957

>
> thanks
> -- PMM


-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [RFC PATCH] qemu-keymap: properly check return from xkb_keymap_mod_get_index

[Peter Maydell]

On Tue, 20 Jun 2023 at 16:38, Alex Bennée <alex.bennee@linaro.org> wrote:
>
>
> Peter Maydell <peter.maydell@linaro.org> writes:
>
> > On Tue, 20 Jun 2023 at 16:04, Alex Bennée <alex.bennee@linaro.org> 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(-)
> >
> > Looking at the code that works with the mask values
> > we are getting here, I think this ought to work
> > (if there's no AltGr modifier then the 0 mask means
> > the key state will be the same in normal and with the
> > altgr mask supplied, which we already check for).
> > Did you eyeball the output, though?
> >
> > Also, which keymap runs into this? Is it every keymap
> > for some new version of the xkb data (which would imply
> > that the problem is that the AltGr modifier has changed
> > name), or is it only one specific keymap that happens
> > to have no AltGr key?
>
> ar maybe? it only got flagged in clang-system once fedora was updated (I
> assume with better sanitizers):
>
>   [2773/3696] Generating pc-bios/keymaps/ar with a custom command
>   FAILED: pc-bios/keymaps/ar
>   /builds/stsquad/qemu/build/qemu-keymap -f pc-bios/keymaps/ar -l ar
>   ../qemu-keymap.c:223:16: runtime error: shift exponent 4294967295 is too large for 32-bit type 'int'
>   SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior ../qemu-keymap.c:223:16 in
>   [2774/3696] Generating pc-bios/edk2-x86_64-code.fd with a custom command (wrapped by meson to capture output)
>   [2775/3696] Generating pc-bios/edk2-x86_64-secure-code.fd with a custom command (wrapped by meson to capture output)
>   ninja: build stopped: subcommand failed.
>   make: *** [Makefile:153: run-ninja] Error 1

OK; how does the output look from the new qemu-keymap on
that system ?

thanks
-- PMM

Re: [RFC PATCH] qemu-keymap: properly check return from xkb_keymap_mod_get_index

[Richard Henderson]

On 6/20/23 17:37, Alex Bennée wrote:
> ar maybe? it only got flagged in clang-system once fedora was updated (I
> assume with better sanitizers):
> 
>    [2773/3696] Generating pc-bios/keymaps/ar with a custom command
>    FAILED: pc-bios/keymaps/ar
>    /builds/stsquad/qemu/build/qemu-keymap -f pc-bios/keymaps/ar -l ar
>    ../qemu-keymap.c:223:16: runtime error: shift exponent 4294967295 is too large for 32-bit type 'int'
>    SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior ../qemu-keymap.c:223:16 in
>    [2774/3696] Generating pc-bios/edk2-x86_64-code.fd with a custom command (wrapped by meson to capture output)
>    [2775/3696] Generating pc-bios/edk2-x86_64-secure-code.fd with a custom command (wrapped by meson to capture output)
>    ninja: build stopped: subcommand failed.
>    make: *** [Makefile:153: run-ninja] Error 1
> 
> https://gitlab.com/stsquad/qemu/-/jobs/4500683186#L3957

Related:

https://gitlab.com/qemu-project/qemu/-/issues/1709
https://gitlab.com/qemu-project/qemu/-/issues/1712

which note that keymaps/ar has changed to keymaps/ara in xkeyboard-config from 2.38 to 2.39.


r~

Re: [RFC PATCH] qemu-keymap: properly check return from xkb_keymap_mod_get_index

[Peter Maydell]

On Tue, 20 Jun 2023 at 16:55, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 6/20/23 17:37, Alex Bennée wrote:
> > ar maybe? it only got flagged in clang-system once fedora was updated (I
> > assume with better sanitizers):
> >
> >    [2773/3696] Generating pc-bios/keymaps/ar with a custom command
> >    FAILED: pc-bios/keymaps/ar
> >    /builds/stsquad/qemu/build/qemu-keymap -f pc-bios/keymaps/ar -l ar
> >    ../qemu-keymap.c:223:16: runtime error: shift exponent 4294967295 is too large for 32-bit type 'int'
> >    SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior ../qemu-keymap.c:223:16 in
> >    [2774/3696] Generating pc-bios/edk2-x86_64-code.fd with a custom command (wrapped by meson to capture output)
> >    [2775/3696] Generating pc-bios/edk2-x86_64-secure-code.fd with a custom command (wrapped by meson to capture output)
> >    ninja: build stopped: subcommand failed.
> >    make: *** [Makefile:153: run-ninja] Error 1
> >
> > https://gitlab.com/stsquad/qemu/-/jobs/4500683186#L3957
>
> Related:
>
> https://gitlab.com/qemu-project/qemu/-/issues/1709
> https://gitlab.com/qemu-project/qemu/-/issues/1712
>
> which note that keymaps/ar has changed to keymaps/ara in xkeyboard-config from 2.38 to 2.39.

On Ubuntu I have xkb-data 2.33.1, but that already has
/usr/share/X11/xkb/symbols/ara, not ar.   Asking qemu-keymap
for either 'ar' or 'ara' works. So this is not so much
that the filename has changed in 2.39, but merely that
xkb has stopped accepting a legacy compatibility synonym.
The upstream change is:
https://gitlab.freedesktop.org/xkeyboard-config/xkeyboard-config/-/commit/470ad2cd8fea84d7210377161d86b31999bb5ea6

So the easy fix I think is to change the line in
pc-bios/keymaps/meson.build from
  'ar': '-l ar',
to
  'ar': '-l ara',

As the commit message notes, 'ara' has been the standard
xkb name upstream for over 15 years, so this won't break
our build on older versions of the xkb data.

(In theory we could also rename the pcbios keymap name,
but that seems like unnecessary effort.)

thanks
-- PMM