Re: [PATCH] xkb: Do not use base group as an array index.
On Thu, Dec 20, 2012 at 11:47:32AM +0100, Andreas Wettstein wrote:
> > is there some sort of test-case that triggers this issue reliable?
>
> I found it when I tried:
>
> key {
> type= "ONE_LEVEL",
> symbols[Group1]= [ NoSymbol ],
> actions[Group1]= [ LatchGroup(group=-1, clearLocks) ]
> };
>
> and then hit F7. Using SetGroup(group=-1) should "work" equally well.
thanks, verified (didn't crash, but out of bounds for sure)
> > also, why do we ignore locked groups here? It'd be great to have some
> > explanation, this is code that hardly anyone knows inside-out, so having
> > this in the commit message would help a lot.
>
> Actually, at first I fixed the crash using the effective group. But
> then I found in section 2.3.1 of the X Keyboard Extension Protocol
> Specification:
>
> If the IgnoreGroupLock control is set, the locked state of the
> keyboard group is not considered when activating passive grabs.
>
> I believe that this is what this piece of code is about, and I think my
> choice is as close as possible to this description.
>
> And what is the idea behind the specification? Probably, the idea is
> that users switching between two distinct two-level layouts (such as us
> and ru) using a lock can have that lock partially ignored, while users
> using a single three level layout implemented using two groups (for core
> protocol compatibility) will not get Mode_switch ignored, which makes
> sense, as it actually selects symbols of the same layout.
I suspect the reason is to avoid triggering passive grabs when caps lock is
on. If you have a grab that should trigger on, say, shift W said grab should
probably not trigger if you have caps lock on and hit W.
merged the patch, thanks.
Cheers,
Peter
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH] xkb: Do not use base group as an array index.
> is there some sort of test-case that triggers this issue reliable?
I found it when I tried:
key {
type= "ONE_LEVEL",
symbols[Group1]= [ NoSymbol ],
actions[Group1]= [ LatchGroup(group=-1, clearLocks) ]
};
and then hit F7. Using SetGroup(group=-1) should "work" equally well.
> also, why do we ignore locked groups here? It'd be great to have some
> explanation, this is code that hardly anyone knows inside-out, so having
> this in the commit message would help a lot.
Actually, at first I fixed the crash using the effective group. But
then I found in section 2.3.1 of the X Keyboard Extension Protocol
Specification:
If the IgnoreGroupLock control is set, the locked state of the
keyboard group is not considered when activating passive grabs.
I believe that this is what this piece of code is about, and I think my
choice is as close as possible to this description.
And what is the idea behind the specification? Probably, the idea is
that users switching between two distinct two-level layouts (such as us
and ru) using a lock can have that lock partially ignored, while users
using a single three level layout implemented using two groups (for core
protocol compatibility) will not get Mode_switch ignored, which makes
sense, as it actually selects symbols of the same layout.
Regards,
Andreas
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH] xkb: Do not use base group as an array index.
On Wed, Dec 19, 2012 at 06:13:21PM +0100, Andreas Wettstein wrote:
> The base group is not brought into range and, therefore, using it as an array
> index crashed the X server. Also, at this place, we should ignore locked
> groups, but not latched groups. Therefore, use sum of base and latched
> groups,
> brought into range.
is there some sort of test-case that triggers this issue reliable?
also, why do we ignore locked groups here? It'd be great to have some
explanation, this is code that hardly anyone knows inside-out, so having
this in the commit message would help a lot.
patch itself looks good though, but I'm lacking context here so I don't know
if there could be any side-effects.
Cheers,
Peter
>
> Signed-off-by: Andreas Wettstein
> ---
> xkb/xkbUtils.c | 10 --
> 1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/xkb/xkbUtils.c b/xkb/xkbUtils.c
> index c23cd77..6c6af60 100644
> --- a/xkb/xkbUtils.c
> +++ b/xkb/xkbUtils.c
> @@ -642,6 +642,7 @@ XkbComputeCompatState(XkbSrvInfoPtr xkbi)
> CARD16 grp_mask;
> XkbStatePtr state = &xkbi->state;
> XkbCompatMapPtr map;
> +XkbControlsPtr ctrls;
>
> if (!state || !xkbi->desc || !xkbi->desc->ctrls || !xkbi->desc->compat)
> return;
> @@ -650,9 +651,14 @@ XkbComputeCompatState(XkbSrvInfoPtr xkbi)
> grp_mask = map->groups[state->group].mask;
> state->compat_state = state->mods | grp_mask;
> state->compat_lookup_mods = state->lookup_mods | grp_mask;
> +ctrls= xkbi->desc->ctrls;
>
> -if (xkbi->desc->ctrls->enabled_ctrls & XkbIgnoreGroupLockMask)
> -grp_mask = map->groups[state->base_group].mask;
> +if (ctrls->enabled_ctrls & XkbIgnoreGroupLockMask) {
> + unsigned char grp = state->base_group+state->latched_group;
> + if (grp >= ctrls->num_groups)
> + grp = XkbAdjustGroup(XkbCharToInt(grp), ctrls);
> +grp_mask = map->groups[grp].mask;
> +}
> state->compat_grab_mods = state->grab_mods | grp_mask;
> return;
> }
> --
> 1.8.0
>
> ___
> xorg-devel@lists.x.org: X.Org development
> Archives: http://lists.x.org/archives/xorg-devel
> Info: http://lists.x.org/mailman/listinfo/xorg-devel
>
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel
[PATCH] xkb: Do not use base group as an array index.
The base group is not brought into range and, therefore, using it as an array
index crashed the X server. Also, at this place, we should ignore locked
groups, but not latched groups. Therefore, use sum of base and latched groups,
brought into range.
Signed-off-by: Andreas Wettstein
---
xkb/xkbUtils.c | 10 --
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/xkb/xkbUtils.c b/xkb/xkbUtils.c
index c23cd77..6c6af60 100644
--- a/xkb/xkbUtils.c
+++ b/xkb/xkbUtils.c
@@ -642,6 +642,7 @@ XkbComputeCompatState(XkbSrvInfoPtr xkbi)
CARD16 grp_mask;
XkbStatePtr state = &xkbi->state;
XkbCompatMapPtr map;
+XkbControlsPtr ctrls;
if (!state || !xkbi->desc || !xkbi->desc->ctrls || !xkbi->desc->compat)
return;
@@ -650,9 +651,14 @@ XkbComputeCompatState(XkbSrvInfoPtr xkbi)
grp_mask = map->groups[state->group].mask;
state->compat_state = state->mods | grp_mask;
state->compat_lookup_mods = state->lookup_mods | grp_mask;
+ctrls= xkbi->desc->ctrls;
-if (xkbi->desc->ctrls->enabled_ctrls & XkbIgnoreGroupLockMask)
-grp_mask = map->groups[state->base_group].mask;
+if (ctrls->enabled_ctrls & XkbIgnoreGroupLockMask) {
+ unsigned char grp = state->base_group+state->latched_group;
+ if (grp >= ctrls->num_groups)
+ grp = XkbAdjustGroup(XkbCharToInt(grp), ctrls);
+grp_mask = map->groups[grp].mask;
+}
state->compat_grab_mods = state->grab_mods | grp_mask;
return;
}
--
1.8.0
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel
