Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

keymap: fix modular arithmetic #671

Merged
merged 1 commit into from
Mar 8, 2025
Merged

keymap: fix modular arithmetic #671

merged 1 commit into from
Mar 8, 2025

Conversation

mahkoh
Copy link
Contributor

@mahkoh mahkoh commented Mar 7, 2025

Consider num_groups = 1.

Fixes: 67b03ce ("state: correctly wrap state->locked_group and ->group")

This should be backported.

@mahkoh mahkoh force-pushed the jorth/modular-arithmetic branch from 5b54b39 to 37b415d Compare March 7, 2025 17:15
@wismill wismill force-pushed the jorth/modular-arithmetic branch from 37b415d to 29d22b3 Compare March 8, 2025 15:21
@wismill
Copy link
Member

wismill commented Mar 8, 2025

Good catch! I fixed the compilation error, added a test and further comments.

@wismill wismill requested a review from bluetech March 8, 2025 15:22
@wismill wismill added bug Indicates an unexpected problem or unintended behavior state Indicates a need for improvements or additions to the xkb_state API labels Mar 8, 2025
@wismill
Copy link
Member

wismill commented Mar 8, 2025

Note that this is unlikely to trigger:

  • when there is only one group, no group switch keysym is expected;
  • when there are multiple groups, it triggers only for negative multiples of the group count, e.g. -2 and -4 for 2 groups. This is difficult to achieve with the current xkeyboard-config.

So this explains it was undetected for so long.

@mahkoh
Copy link
Contributor Author

mahkoh commented Mar 8, 2025 •
edited
Loading

This causes a segfault in SDL because it initializes its group to -1 and then calls xkb_keymap_num_levels_for_key (in some situations) before the group gets set to something sensible.

@wismill
Copy link
Member

wismill commented Mar 8, 2025

What is SDL? Do you have a link to the corresponding bug report?

@mahkoh
Copy link
Contributor Author

mahkoh commented Mar 8, 2025 •
edited
Loading

https://github.com/libsdl-org/SDL

There is no bug report. I encountered this when running https://gitlab.com/amini-allight/radv-bug-demo/. It only triggers if the first group does not have a name, i.e., xkb_keymap_layout_get_name returns NULL.

@wismill
Copy link
Member

wismill commented Mar 8, 2025

While we will try to release soon with the fix, it seems still quite unlikely to trigger the bug in usual situations, e.g. when taking the state from the compositor (in the context of my previous comment).

@wismill wismill force-pushed the jorth/modular-arithmetic branch from 29d22b3 to 5d88c43 Compare March 8, 2025 19:14
@wismill
Copy link
Member

wismill commented Mar 8, 2025

Added changelog entry and more tests, especially every function in the API taking a xkb_layout_index_t parameter.

bluetech
bluetech approved these changes Mar 8, 2025
View reviewed changes
Copy link
Member

@bluetech bluetech left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @mahkoh and @wismill. LGTM.

@mahkoh @wismill
keymap: Fix segfault due to invalid group wrapping
d397668 
The modular arithmetic is incorrect for negative values, e.g. for
num_groups = 1. It triggers a segfault for the following settings:

- layouts count (per key or total) N: `N > 0`, and
- layout index n: `n = - k * N` (`k > 0`)

% returns the *remainder* of the division, not the modulus (see C11
standard 6.5.5 “Multiplicative operators”: a % b = a - (a/b)*b. While
both operators return the same result for positive operands, they do
not for e.g. a negative dividend: remainder may be negative (in the
open interval ]-num_groups, num_groups[) while the modulus is always
positive. So if the remainder is negative, we must add `num_groups` to
get the modulus.

Fixes: 67b03ce ("state: correctly wrap state->locked_group and ->group")
Signed-off-by: Julian Orth <[email protected]>
Co-authored-by: Julian Orth <[email protected]>
Co-authored-by: Pierre Le Marre <[email protected]>
@wismill wismill force-pushed the jorth/modular-arithmetic branch from 5d88c43 to d397668 Compare March 8, 2025 23:30
@wismill wismill merged commit cb3565b into xkbcommon:master Mar 8, 2025
5 checks passed
@wismill wismill mentioned this pull request Mar 8, 2025
Open
19 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wismill wismill wismill left review comments

@bluetech bluetech bluetech approved these changes

Assignees
No one assigned
Labels
bug Indicates an unexpected problem or unintended behavior state Indicates a need for improvements or additions to the xkb_state API
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mahkoh @wismill @bluetech