| Message ID | 20230620150335.814005-1-alex.bennee@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [RFC] qemu-keymap: properly check return from xkb_keymap_mod_get_index | expand |
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
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
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
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~
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
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);
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(-)