Xlib contract for XEvent up/down-casting
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*)&fakeEvent);
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)?
Xlib.h says that the XEvent union "is defined so Xlib can always use the same
sized event structure *internally* (emphasis added), to avoid memory
fragmentation." This makes me think that clients should not use XEvent (and
perhaps XEvent itself should not be API beyond being an opaque pointer).
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?
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?
--Jeremy
Re: Xlib contract for XEvent up/down-casting
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*)&fakeEvent);
>
> 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.
Re: Xlib contract for XEvent up/down-casting
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, &wire);
+ if (ret)
+ {
+ ret = (*dpy->event_vec[type])(dpy, &lib, &wire);
+ if (ret)
+ ret = _XPutBackEvent(dpy, &lib);
+ }
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
