Bug 17923

Summary: xcb backend: XNextEvent() segfault when XlibDisplayIOError is true
Product: xorg Reporter: Fumihito YOSHIDA <fumihito.yoshida>
Component: Lib/XlibAssignee: Jamey Sharp <jamey>
Status: RESOLVED NOTOURBUG QA Contact: Xorg Project Team <xorg-team>
Severity: critical    
Priority: high CC: ThJaeger
Version: git   
Hardware: All   
OS: All   
Whiteboard: 2011BRB_Reviewed
i915 platform: i915 features:

Description Fumihito YOSHIDA 2008-10-06 04:08:24 UTC
XNextEvent() segfault when XlibDisplayIOError is true
It cause by xcb_io.c::_XReadEvents() and xcb_io.c::_XSend()'s some code.
This problem affected to libX11 1.1.2 -1.1.5 and git (Release: R7.2 - R7.4) .

When dpy->head is null, the XNextEvent() called _XReadEvents(),
but xcb_io.c::_XReadEvents() and xcb_io.c::_XSend() has dpy->flags check,
when XlibDiskpayIOError bit is set, their procedure return without any work.

       if(dpy->flags & XlibDisplayIOError)
               return;

So, XNextEvent()'s dpy is still null, and crash.

This problem will crash IIIMF, Compiz and so on.


in Classical X event loop:
 // It will cause segfault in some conditions....

Display *pdisplay;
XEvent event;
pdisplay = XOpenDisplay(NULL);

for (; ;){
       XNextEvent(pdisplay, &event);
       /* X Event Handler */
}

This code causes segfault at XNextEvent(). If you trace it, you will find below crash point.

--- XNextEvent() ----------------------------------
int
XNextEvent (
       register Display *dpy,
       register XEvent *event)
{
       register _XQEvent *qelt;

       LockDisplay(dpy);

       if (dpy->head == NULL)
           _XReadEvents(dpy);  // <- it hope _XReadEvents return valid
                                      //  handler, it believev
dpy->head != null...
       qelt = dpy->head;
       *event = qelt->event; // <- but, dpy->head->event is null, CRASH.
                                     // Because dpy->head is null!
       _XDeq(dpy, NULL, qelt);
       UnlockDisplay(dpy);
       return 0;
}
--------------------------------------------------

If you replace "if(dpy->head == null)" to "while(dpy->head == null)",
the problem solved.
// Notice: if => while replacement cause busy loop and possible infinite loop.

This segfaults conditions are:
 a) X Event queue is empty.
 b) XDisplayIOError is true.
You call XNextEvent() when (a && b), your program will segfault.


Case2: Modern X event loop style is below:
 // It wont cause segfault, and lockless.

Display *pdisplay;
XEvent event;
pdisplay = XOpenDisplay(NULL);

for (; ;){
       if (XPending()){
               XNextEvent(pdisplay, &event);
               /* X Event Handler */
       }
       /* other event looping and some usleeps. */
}

Because this implement want evading XNextEvent()'s blocking.
XNextEvent() always return least one events, but when X event
queue is empty, cause blocking-wait for next new events.

But, man XNextEvent(3) says:
|  The XNextEvent function copies the first event from the event queue
|  into the specified XEvent structure and then removes it from the queue.
|  If the event queue is empty, XNextEvent flushes the output buffer and
|  blocks until an event is received.

Current implement is not "blocks", "sometimes blocks, sometimes crash".

And we have big issue, the classical X event loop(Case1) are not failure
in old-days, their coding were(are?) right. (But, their old style coding are not
efficient, we need modern style.).

We can find their classical event loop in many code asset, they will crash
by this problem.....

I suggest dirty hack.
 XlibDisplayIOError flag can be cleard with some wait times.

--- NextEvent.c.orig
+++ NextEvent.c
@@ -45,9 +45,14 @@
       register _XQEvent *qelt;

       LockDisplay(dpy);

-       if (dpy->head == NULL)
+        while(dpy->head == NULL){
           _XReadEvents(dpy);
+           usleep(400);
+       }
       qelt = dpy->head;
       *event = qelt->event;
       _XDeq(dpy, NULL, qelt);
Comment 1 Tom Jaeger 2009-01-09 06:53:41 UTC
Does your application use XTest?  If so, can you check if this patch fixes the issue?

http://cgit.freedesktop.org/xorg/xserver/commit/?id=b2756a71a432f7cf7c870a48676c98625512558d
Comment 2 Jeremy Huddleston Sequoia 2011-10-03 16:57:16 UTC
This is a bit old.  Is it still an issue?
Comment 3 Jamey Sharp 2011-10-03 17:15:08 UTC
I'm not sure how this bug ever could have triggered. It requires that you enter XNextEvent after _XIOError was called, but Xlib calls exit() on IO error. So either you longjmp'd out of the IO error handler, in which case you're not allowed to touch the Display further, or you're trying to clean up the Display in an atexit handler, where it seems like a bad plan to process events.

I'm also not seeing anything sane that Xlib can do if this happens. The caller has done something wrong. We could replace the check with an assertion, since the program is going to crash one way or another.

I'd happily take a patch that replaces the early-return with an assert, but barring that or evidence that this is actually a libX11 bug, I'm closing this.

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.