Incorrect handling of group indicators

[mahkoh]

Consider the following code:

        if (led->which_groups != 0 && led->groups != 0) {
            if (led->which_groups & XKB_STATE_LAYOUT_EFFECTIVE)
                group_mask |= (1u << state->components.group);
            if (led->which_groups & XKB_STATE_LAYOUT_DEPRESSED)
                group_mask |= (1u << state->components.base_group);
            if (led->which_groups & XKB_STATE_LAYOUT_LATCHED)
                group_mask |= (1u << state->components.latched_group);
            if (led->which_groups & XKB_STATE_LAYOUT_LOCKED)
                group_mask |= (1u << state->components.locked_group);

            if (led->groups & group_mask) {
                state->components.leds |= (1u << idx);
                continue;
            }
        }

And the specification

The which_groups and the groups fields of an indicator map determine how the keyboard group state affects the corresponding indicator. The which_groups field controls the interpretation of groups and may contain any one of the following values:

Value	Interpretation of the Groups Field
IM_UseNone	The groups field and the current keyboard group state are ignored.
IM_UseBase	If groups is non-zero, the indicator is lit whenever the base keyboard group is non-zero. If groups is zero, the indicator is lit whenever the base keyboard group is zero.
IM_UseLatched	If groups is non-zero, the indicator is lit whenever the latched keyboard group is non-zero. If groups is zero, the indicator is lit whenever the latched keyboard group is zero.
IM_UseLocked	The groups field is interpreted as a mask. The indicator is lit when the current locked keyboard group matches one of the bits that are set in groups .
IM_UseEffective	The groups field is interpreted as a mask. The indicator is lit when the current effective keyboard group matches one of the bits that are set in groups .

There are several problem with the code.

1. whichGroupState is not a bitfield.
2. When the base/latched group are selected, group should not be treated as a mask.
3. Shifting by base_group and latched_group is undefined behavior since they are arbitrary integers, not necessarily in the range [0, 32).

[wismill]

1. whichGroupState is not a bitfield.

Except it is in both xkbcomp and xserver, and has been for so so long that it is the spec that should be fixed:

The which_groups field controls the interpretation of groups and may contain any one of the following values

2. When the base/latched group are selected, group should not be treated as a mask.

This is not what is written in the spec. For theses states, group has just 2 cases: zero/non -zero. It seems that always treating group as a mask (as in all the reference implementations) is the sensible direction.

3. Shifting by base_group and latched_group is undefined behavior since they are arbitrary integers, not necessarily in the range [0, 32).

I agree. They are even signed integers. But getting a value e.g. > 32 is not easy: the parser limits the absolute value of the field group in group actions to the maximum number of groups, currently 4. So this is a practical issue only for negative values. This will get a realistic issue for positive values when we will raise the maximum groups count to 32 (see #515).

[mahkoh]

This is not what is written in the spec. For theses states, group has just 2 cases: zero/non -zero. It seems that always treating group as a mask (as in all the reference implementations) is the sensible direction.

The intersection might be empty even if both values are non-0.

[wismill]

I agree, the current code is incorrect for the base/latched cases.

[wismill]

@mahkoh I invite you to check the fix in #581.