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.

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

Range of accepted screen values does not match #571

Open
wysiwys opened this issue Dec 22, 2024 · 4 comments
Open

Range of accepted screen values does not match #571

wysiwys opened this issue Dec 22, 2024 · 4 comments
Labels
needs info X11 legacy: compatibility Indicate a need to ensure compatibility with X11

Comments

@wysiwys
Copy link
Contributor

wysiwys commented Dec 22, 2024

In src/keymap.h, the type of the screen member in the xkb_switch_screen_action struct is int8_t. This also matches the type specified in the X Keyboard Extension Protocol Specification for SA_SwitchScreen. However, during the validation step in src/xkbcomp/action.c, in the function HandleSwitchScreen(), all values in the range [-255, +255] are accepted.

Should the validation step instead check that the value is in [-128, +127], in order to fit inside the range of int8_t?

@wismill wismill added X11 legacy: compatibility Indicate a need to ensure compatibility with X11 needs info labels Dec 28, 2024
@wismill
Copy link
Member

wismill commented Dec 28, 2024

I would say the range should be -128..+127 indeed, but we parse as X11 xkbcomp does.

@wysiwys
Copy link
Contributor Author

wysiwys commented Jan 7, 2025

Thanks! Is this a situation where libxkbcommon's xkbcomp tries to match the X11 version as closely as possible?

@wismill
Copy link
Member

wismill commented Jan 7, 2025

We try to follow X11 xkbcomp behavior as much as possible, but if it is buggy we may diverge. Here I have not analyzed much the impact, since libxkbcommon does not offer an API to handle this type of action, although we parse and serialize it.

@wysiwys
Copy link
Contributor Author

wysiwys commented Jan 7, 2025

As far as I can tell, the impact would be quite limited on modern systems. This is because there is generally only one screen index (0) available on modern systems that use the X server, with all monitors being combined into one conceptual 'screen'. However, there may be some other configurations I am not aware of, which could be relevant.

For the situation where the parsed screen index exceeds the range, would it make sense to add a log message? This could be a way to notify the user of how the screens are handled without changing the actual behavior of the parser.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs info X11 legacy: compatibility Indicate a need to ensure compatibility with X11
Projects
None yet
Development

No branches or pull requests

2 participants