Re: Xlib contract for XEvent up/down-casting

[Keith Packard]

Jeremy Huddleston Sequoia  writes:

> So the problem is clear, but I'm not sure which side needs to change.

Probably both?

How about a simple hack in XPutBackEvent that uses _XEventToWire
followed by _XWireToEvent? That would also clear out any unused bits
from the event before inserting it into the queue and check to make sure
the event type was valid.

diff --git a/src/PutBEvent.c b/src/PutBEvent.c
index 0f9df342..f7b74b31 100644
--- a/src/PutBEvent.c
+++ b/src/PutBEvent.c
@@ -79,9 +79,22 @@ XPutBackEvent (
 register XEvent *event)
{
int ret;
+   xEvent wire = {0};
+   XEvent lib = {0};
+   Status (*fp)(Display *, XEvent *, xEvent *);
+   int type = event->type & 0177;
 
LockDisplay(dpy);
-   ret = _XPutBackEvent(dpy, event);
+   fp = dpy->wire_vec[type];
+   if (fp == NULL)
+   fp = _XEventToWire;
+   ret = (*fp)(dpy, event, );
+   if (ret)
+   {
+   ret = (*dpy->event_vec[type])(dpy, , );
+   if (ret)
+   ret = _XPutBackEvent(dpy, );
+   }
UnlockDisplay(dpy);
return ret;
}

> With a quick grep through our source code, I see some (XEvent *)
> casting in many other locations (xterm, cairo, libXt, ...).  So is the
> burden on the caller to ensure that their events are properly padded
> into an XEvent, or is the burden on the recipient of an XEvent to
> ensure that they determine the event type before reading the event?

Yeah, that's problematic behavior for sure; one should never cast to a
pointer of a type which is larger. But, we can't exactly fix the Xlib
API to make that unnecessary.

I guess the question is whether there are other places in Xlib which
take an XEvent * and access fields beyond those defined by the related
union member?

> Or is this unique to XPutBackEvent(), and freeglut is abusing
> XPutBackEvent() since XPutBackEvent() should be use to "push an event
> back onto the head of the display's event queue" (meaning the event
> should have previously been popped from the queue and thus be
> appropriately sized, not something created on the stack of arbitrary
> size?

The XPutBackEvent docs don't explicitly require that the event come from
the queue, it seems to use that only as an example.

-- 
-keith


signature.asc
Description: PGP signature

Re: Xlib contract for XEvent up/down-casting

[Po Lu]

Jeremy Huddleston Sequoia  writes:

> I've been running XQuartz with ASan+UBSan to try to catch some issues
> some users have reported, and I stumbled across something below GLUT
> (specifically, freeglut 2.8.1), which does:
>
> XConfigureEvent fakeEvent = {0};
> ...
> XPutBackEvent(fgDisplay.Display, (XEvent*));
>
> and XPutBackEvent eventually does:
>
> XEvent store = *event;
>
> which overflows the stack on read because:
>
> sizeof(XConfigureEvent) == 88
> sizeof(XEvent) == 192
>
> So the problem is clear, but I'm not sure which side needs to change.
>
> What is the contract for Xlib's APIs that take XEvent *?  Is Xlib
> expected to handle any XEvent "subtype", or does it need to be exactly
> an XEvent (ie: is it the client's responsibility to pad it)?

It needs to be an XEvent, since the event ends up back on the event
queue.  The client is supposed to pad it.