Changes to XInput Proto Number of Events Cause Xlib WireToEvent Vector Mismatch

[Peter Hutterer]

On Thu, Nov 26, 2009 at 01:43:25PM -0500, Nathan Kidd wrote:
> Peter Hutterer wrote:
> >On Wed, Nov 25, 2009 at 04:42:59PM -0500, Nathan Kidd wrote:
> >>In the last few years inputproto's number of events (IEVENTS) has jumped
> >>around quite a bit between 15 and 19, which has resulted in the
> >>following issue I've recently became aware of:
> >>...
> >AIUI, other extensions may be affected as well, not just
> >Xi. Any newer library extension run against an older server (where the
> >extension differs in the number of events) would suffer from this issue.
> 
> Correct; XInput just got noticed because it's had more development of
> late.  (The unfair rewards of all your work on Xi :) )
> 
> >Attached is an attempt of a fix to libXext. There's not a lot of
> >wriggle-room, the APIs are quite restrictive and we can't pass a great deal
> >of information around. Additionally, the library has no way of knowing how
> >many events a given extension has, it's pure guesswork (or compiled into the
> >library).
> 
> The reason not to just use the display's event_vec (attached patches,
> your log message reused) is to avoid adding a libX11/libXext version
> dependency?

yes. the patch I sent was actually the third version I implemented, the
former two I dropped for compatibility reasons. Having the solution in one
component only is preferabl. This bug was hard enough to triage as-is, I
don't think we'd want to make that even more complicated.
Especially if it also introduces a new symbol.


> Either approach fixes the WireToEvent case when considering events being
> generated by the server, but I don't think there's anything that can be
> done about the EventToWire (SendEvent request) case.  Your choice is to
> use the wrong EventToWire function for either the overlapping extension
> events, or the following extension events.  The former is preferred I
> think since it will only break very new code (using new events).

I think EvToWire should match WireToEv. If a client is trying to send an
event that the server doesn't know about then that's a bug in the client
anyway. I don't really see a way out to prevent a client from doing this in
a safe manner.

> Currently two unpatched clients could SendEvent new events (that xserver
> didn't know about) successfully if they query extensions in the same
> order.  The patch will break them, but that's the only fallout I can
> think of, and it seems necessary.

Right, but trying to send an XI 1.5 event through an XI 1.4 server should
fail anyhow, so...

> >From 9a723019b4cef32ac5a2f3c2e3aaadef5447f783 Mon Sep 17 00:00:00 2001
> From: Nathan Kidd <nkidd at opentext.com>
> Date: Thu, 26 Nov 2009 12:29:10 -0500
> Subject: [PATCH] Add XEGetWireToEvent()
> 
> Used by libXext to tell if an extension lib and xserver
> were built with a difference in the number of extension events,
> and avoid resulting protocol decoding errors.
> 
> Signed-off-by: Nathan Kidd <nkidd at opentext.com>
> ---
>  include/X11/Xlibint.h |    7 +++++++
>  src/InitExt.c         |   11 +++++++++++
>  2 files changed, 18 insertions(+), 0 deletions(-)
> 
> diff --git a/include/X11/Xlibint.h b/include/X11/Xlibint.h
> index 2acfc76..010e12e 100644
> --- a/include/X11/Xlibint.h
> +++ b/include/X11/Xlibint.h
> @@ -1184,6 +1184,13 @@ extern Bool (*XESetWireToEvent(
>      Display*, XEvent*, xEvent*
>  );
>  
> +extern Bool (*XEGetWireToEvent(
> +    Display*		/* display */,
> +    int			/* event_number */
> +))(
> +    Display*, XEvent*, xEvent*
> +);
> +
>  extern Bool (*XESetWireToEventCookie(
>      Display*		/* display */,
>      int			/* extension */,
> diff --git a/src/InitExt.c b/src/InitExt.c
> index 0e6c94e..3068159 100644
> --- a/src/InitExt.c
> +++ b/src/InitExt.c
> @@ -253,6 +253,17 @@ WireToEventType XESetWireToEvent(
>  	return (WireToEventType)oldproc;
>  }
>  
> +WireToEventType XEGetWireToEvent(
> +	Display *dpy,		/* display */
> +	int event_number)	/* event routine to get */
> +{
> +	register WireToEventType proc;
> +	LockDisplay (dpy);
> +	proc = dpy->event_vec[event_number];
> +	UnlockDisplay (dpy);
> +	return (WireToEventType)proc;
> +}
> +
>  typedef Bool (*WireToEventCookieType) (
>      Display*	/* display */,
>      XGenericEventCookie*	/* re */,
> -- 
> 1.6.0.4
> 
> 
> 

> >From f0bf7e955509afe8124925e9f930522df3babc5e Mon Sep 17 00:00:00 2001
> From: Nathan Kidd <nkidd at opentext.com>
> Date: Thu, 26 Nov 2009 11:54:36 -0500
> Subject: [PATCH] Don't smash the event_vec if num_events differs between lib and server.
> 
> If the library extension thinks there's more events to an extension than the
> server actually has, the event_vec for the overlapping range can get
> overwritten. This depends on the initialization order of the libraries.
> 
> Signed-off-by: Nathan Kidd <nkidd at opentext.com>
> ---
>  src/extutil.c |   20 ++++++++++++++++++++
>  1 files changed, 20 insertions(+), 0 deletions(-)
> 
> diff --git a/src/extutil.c b/src/extutil.c
> index ac861ea..a1a42f6 100644
> --- a/src/extutil.c
> +++ b/src/extutil.c
> @@ -119,6 +119,26 @@ XExtDisplayInfo *XextAddDisplay (
>  	int i, j;
>  
>  	for (i = 0, j = dpyinfo->codes->first_event; i < nevents; i++, j++) {
> +            /*
> +             * Xlib initializes all event vectors to unknown. If the
> +             * vector is not unknown it means we would overwrite another
> +             * extension's vector.  This can happen when the extension lib
> +             * is built with more events than the xserver was built with,
> +             * so the xserver's space between event_bases is smaller than
> +             * the lib expects.
> +             *
> +             * For receiving events it is safe to simply not set these extra
> +             * event vectors since the xserver doesn't know about these
> +             * events and thus will never send them.
> +             *
> +             * For sending events, we can't avoid using the wrong EventToWire
> +             * function; either for the (typically) few overlapping events in
> +             * this extension or else for the first overlapped events in the
> +             * next (event-base-wise) extension.  Prefer the former.
> +             */
> +	    if (XEGetWireToEvent (dpy, j) != _XUnknownWireEvent)
> +                break;
> +
>  	    XESetWireToEvent (dpy, j, hooks->wire_to_event);
>  	    XESetEventToWire (dpy, j, hooks->event_to_wire);
>  	}
> -- 
> 1.6.0.4

This only works in one case - if the higher one is initialized first. It
fails the other case. Given two extensions - base 80 + 20 and base 90 + 5.
If extension 1 registers first, range [80,100] would be occupied and cannot
be taken by the second extension. The server would still send events for the
second extension and they'd be processed by the wrong library extension.

That's why the trickery with (idx - 1) is necessary. If event_vec for the
event _before_ an extensions base has the same handler as the base, then you
know the previous one has overrun its limits and you can safely proceed.

For example, if extension 2 finds that event_vec[89] == event_vec[90], then
the first one is wrong. Extension 2 can safely overwrite them, as the server
will never send those event codes for the other extension anyway.

Cheers,
  Peter