[PATCH] Fix handling of SendEvent requests which set the SendEvent bit before sending the event down the wire

[Sam Spilsbury]

On Tue, 2011-09-13 at 10:38 -0500, Jamey Sharp wrote:
> On Tue, Sep 13, 2011 at 07:31:12PM +0800, Sam Spilsbury wrote:
> > I noticed that the server is not removing the SendEvent (0x80) bit in
> > ProcSendEvent in dix/events.c before doing range checks on the event
> > type. This would cause these range checks to return an invalid BadValue
> > error.
> > 
> > I am not sure if this is *really* a bug in the server or a bug in
> > extension libraries (like libXext) who set the SendEvent bit before
> > converting the event to wire format, since ProcSendEvent sets the
> > SendEvent bit anyways just before it writes the event to the client.
> 
> To decide whose bug this is, I checked the X protocol specification. The
> only relevant bit I found was this:
> 
> 	"The event code must be one of the core events or one of the
> 	events defined by an extension ... The contents of the event are
> 	otherwise unaltered and unchecked by the server except to force
> 	on the most significant bit of the event code."
> 
> The first sentence suggests to me that this is a client bug, but the use
> of "force on" in the second suggests that it's supposed to be OK if the
> bit is on already.

Yeah. I'm also thinking of client libraries that we don't know about
which may set the SendEvent bit before transferring to wire format and
as such we should just mask it out in the server before doing a range
check (since we force it back on anyways).

> 
> Since it's clear what SendEvent should mean with the SendEvent bit set,
> I'd be inclined to merge this patch. However, the client libraries
> should also be fixed, for compatibility with existing X servers. Could
> you put together a patch for any client libraries you've spotted with
> this issue?
> 

Sure.

> > +    if (stuff->event.u.u.type & 0x80)
> > +	stuff->event.u.u.type &= ~(0x80);
> 
> I think this would be more clear if you delete the "if" statement. Just
> unconditionally mask off the SendEvent bit; if it already wasn't set,
> the statement will just have no effect.
> 

Sure. I think I only had the if check there to ensure that we weren't
masking off bits that we shouldn't have been masking off (bitwise is not
really my area of expertise) so this can be removed.

Thanks,

Sam

> Still, either way:
> 
> Reviewed-by: Jamey Sharp <jamey at minilop.net>
> 
> Jamey