From 9c95c2491cafb85de6ce09c653a5754f0d4cb2f3 Mon Sep 17 00:00:00 2001 From: Cameron Gutman Date: Mon, 8 Nov 2021 20:01:56 -0600 Subject: [PATCH] x11: Use XCheckIfEvent() instead of XNextEvent() for thread-safety A racing reader could read from our fd between SDL_IOReady()/X11_Pending() and our call to XNextEvent() which will cause XNextEvent() to block for more data. Avoid this by using XCheckIfEvent() which will never block. This also fixes a bug where we could poll() for data, even when events were already read and pending in the queue. Unlike the Wayland implementation, this isn't totally thread-safe because nothing prevents a racing reader from reading events into the queue between our XCheckIfEvent() and SDL_IOReady() calls, but I think this is the best we can do with Xlib. --- src/video/x11/SDL_x11events.c | 47 ++++++++++++++++++----------------- src/video/x11/SDL_x11sym.h | 1 - 2 files changed, 24 insertions(+), 24 deletions(-) diff --git a/src/video/x11/SDL_x11events.c b/src/video/x11/SDL_x11events.c index 135d22dd3..57c498d3f 100644 --- a/src/video/x11/SDL_x11events.c +++ b/src/video/x11/SDL_x11events.c @@ -1557,23 +1557,21 @@ X11_HandleFocusChanges(_THIS) } } } -/* Ack! X11_XPending() actually performs a blocking read if no events available */ -static int -X11_Pending(Display * display) + +static Bool +isAnyEvent(Display *display, XEvent *ev, XPointer arg) { - /* Flush the display connection and look to see if events are queued */ - X11_XFlush(display); - if (X11_XEventsQueued(display, QueuedAlready)) { - return (1); + return True; +} + +static SDL_bool +X11_PollEvent(Display *display, XEvent *event) +{ + if (!X11_XCheckIfEvent(display, event, isAnyEvent, NULL)) { + return SDL_FALSE; } - /* More drastic measures are required -- see if X is ready to talk */ - if (SDL_IOReady(ConnectionNumber(display), SDL_IOR_READ, 0)) { - return (X11_XPending(display)); - } - - /* Oh well, nothing is ready .. */ - return (0); + return SDL_TRUE; } void @@ -1611,17 +1609,21 @@ X11_WaitEventTimeout(_THIS, int timeout) SDL_zero(xevent); - if (timeout == 0) { - if (X11_Pending(display)) { - X11_XNextEvent(display, &xevent); - } else { - return 0; - } + /* Flush and poll to grab any events already read and queued */ + X11_XFlush(display); + if (X11_PollEvent(display, &xevent)) { + /* Fall through */ + } else if (timeout == 0) { + return 0; } else { /* Use SDL_IOR_NO_RETRY to ensure SIGINT will break us out of our wait */ int err = SDL_IOReady(ConnectionNumber(display), SDL_IOR_READ | SDL_IOR_NO_RETRY, timeout); if (err > 0) { - X11_XNextEvent(display, &xevent); + if (!X11_PollEvent(display, &xevent)) { + /* Someone may have beat us to reading the fd. Return 1 here to + * trigger the normal spurious wakeup logic in the event core. */ + return 1; + } } else if (err == 0) { /* Timeout */ return 0; @@ -1679,8 +1681,7 @@ X11_PumpEvents(_THIS) SDL_zero(xevent); /* Keep processing pending events */ - while (X11_Pending(data->display)) { - X11_XNextEvent(data->display, &xevent); + while (X11_PollEvent(data->display, &xevent)) { X11_DispatchEvent(_this, &xevent); } diff --git a/src/video/x11/SDL_x11sym.h b/src/video/x11/SDL_x11sym.h index 1dae060e7..835dc1503 100644 --- a/src/video/x11/SDL_x11sym.h +++ b/src/video/x11/SDL_x11sym.h @@ -97,7 +97,6 @@ SDL_X11_SYM(int,XMapRaised,(Display* a,Window b),(a,b),return) SDL_X11_SYM(Status,XMatchVisualInfo,(Display* a,int b,int c,int d,XVisualInfo* e),(a,b,c,d,e),return) SDL_X11_SYM(int,XMissingExtension,(Display* a,_Xconst char* b),(a,b),return) SDL_X11_SYM(int,XMoveWindow,(Display* a,Window b,int c,int d),(a,b,c,d),return) -SDL_X11_SYM(int,XNextEvent,(Display* a,XEvent* b),(a,b),return) SDL_X11_SYM(Display*,XOpenDisplay,(_Xconst char* a),(a),return) SDL_X11_SYM(Status,XInitThreads,(void),(),return) SDL_X11_SYM(int,XPeekEvent,(Display* a,XEvent* b),(a,b),return)