From cd22e2849512a88d0ab77bc7a3458646625f2c50 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Camilla=20L=C3=B6wy?= Date: Fri, 18 Feb 2022 14:29:43 +0100 Subject: [PATCH] X11: Fix empty event race condition with a pipe There is a seemingly unavoidable race condition when waiting for data on the X11 display connection, as long as any other thread is also making Xlib calls. The event data we are waiting for could be read by the other thread as part of looking for the reply to its request, before our poll has begun. This commit replaces the X11 event sent by glfwPostEmptyEvent with writing to an unnamed pipe. The race condition remains if other Xlib calls are made on other threads, but glfwPostEmptyEvent should now be race-free. This commit is based on work by pcwalton, OlivierSohn, kovidgoyal and joaodasilva. Closes #2033 Related to #379 Related to #1281 Related to #1285 --- CONTRIBUTORS.md | 2 ++ README.md | 2 ++ docs/news.dox | 6 ++++++ src/x11_init.c | 42 ++++++++++++++++++++++++++++++++++++++++++ src/x11_platform.h | 1 + src/x11_window.c | 44 +++++++++++++++++++++++++++++++++++--------- 6 files changed, 88 insertions(+), 9 deletions(-) diff --git a/CONTRIBUTORS.md b/CONTRIBUTORS.md index 34ca624f..f20422ec 100644 --- a/CONTRIBUTORS.md +++ b/CONTRIBUTORS.md @@ -185,6 +185,7 @@ video tutorials. - Ali Sherief - Yoshiki Shibukawa - Dmitri Shuralyov + - Joao da Silva - Daniel Sieger - Daniel Skorupski - Anthony Smith @@ -192,6 +193,7 @@ video tutorials. - Cliff Smolinsky - Patrick Snape - Erlend Sogge Heggen + - Olivier Sohn - Julian Squires - Johannes Stein - Pontus Stenetorp diff --git a/README.md b/README.md index fefaf500..d1dcb600 100644 --- a/README.md +++ b/README.md @@ -272,6 +272,8 @@ information on what to include when reporting a bug. (#2024) - [X11] Bugfix: Joystick events could lead to busy-waiting (#1872) - [X11] Bugfix: `glfwWaitEvents*` did not continue for joystick events + - [X11] Bugfix: `glfwPostEmptyEvent` could be ignored due to race condition + (#379,#1281,#1285,#2033) - [Wayland] Added dynamic loading of all Wayland libraries - [Wayland] Added support for key names via xkbcommon - [Wayland] Removed support for `wl_shell` (#1443) diff --git a/docs/news.dox b/docs/news.dox index 6c55e65f..fbf60319 100644 --- a/docs/news.dox +++ b/docs/news.dox @@ -138,6 +138,12 @@ GLFW_TRANSPARENT_FRAMEBUFFER on Windows 7 if DWM transparency is off (the Transparency setting under Personalization > Window Color). +@subsubsection emptyevents_34 Empty events on X11 no longer roundtrip to server + +Events posted with @ref glfwPostEmptyEvent now use a separate unnamed pipe +instead of sending an X11 client event to the helper window. + + @subsection deprecations_34 Deprecations in version 3.4 @subsection removals_34 Removals in 3.4 diff --git a/src/x11_init.c b/src/x11_init.c index acfa7933..203828fc 100644 --- a/src/x11_init.c +++ b/src/x11_init.c @@ -35,6 +35,8 @@ #include #include #include +#include +#include // Translate the X11 KeySyms for a key to a GLFW key code @@ -1042,6 +1044,37 @@ static Window createHelperWindow(void) CWEventMask, &wa); } +// Create the pipe for empty events without assumuing the OS has pipe2(2) +// +static GLFWbool createEmptyEventPipe(void) +{ + if (pipe(_glfw.x11.emptyEventPipe) != 0) + { + _glfwInputError(GLFW_PLATFORM_ERROR, + "X11: Failed to create empty event pipe: %s", + strerror(errno)); + return GLFW_FALSE; + } + + for (int i = 0; i < 2; i++) + { + const int sf = fcntl(_glfw.x11.emptyEventPipe[i], F_GETFL, 0); + const int df = fcntl(_glfw.x11.emptyEventPipe[i], F_GETFD, 0); + + if (sf == -1 || df == -1 || + fcntl(_glfw.x11.emptyEventPipe[i], F_SETFL, sf | O_NONBLOCK) == -1 || + fcntl(_glfw.x11.emptyEventPipe[i], F_SETFD, df | FD_CLOEXEC) == -1) + { + _glfwInputError(GLFW_PLATFORM_ERROR, + "X11: Failed to set flags for empty event pipe: %s", + strerror(errno)); + return GLFW_FALSE; + } + } + + return GLFW_TRUE; +} + // X error handler // static int errorHandler(Display *display, XErrorEvent* event) @@ -1491,6 +1524,9 @@ int _glfwInitX11(void) getSystemContentScale(&_glfw.x11.contentScaleX, &_glfw.x11.contentScaleY); + if (!createEmptyEventPipe()) + return GLFW_FALSE; + if (!initExtensions()) return GLFW_FALSE; @@ -1604,5 +1640,11 @@ void _glfwTerminateX11(void) _glfwPlatformFreeModule(_glfw.x11.xlib.handle); _glfw.x11.xlib.handle = NULL; } + + if (_glfw.x11.emptyEventPipe[0] || _glfw.x11.emptyEventPipe[1]) + { + close(_glfw.x11.emptyEventPipe[0]); + close(_glfw.x11.emptyEventPipe[1]); + } } diff --git a/src/x11_platform.h b/src/x11_platform.h index 61a0b17d..8da3a2e4 100644 --- a/src/x11_platform.h +++ b/src/x11_platform.h @@ -582,6 +582,7 @@ typedef struct _GLFWlibraryX11 double restoreCursorPosX, restoreCursorPosY; // The window whose disabled cursor mode is active _GLFWwindow* disabledCursorWindow; + int emptyEventPipe[2]; // Window manager atoms Atom NET_SUPPORTED; diff --git a/src/x11_window.c b/src/x11_window.c index 50fba5e8..76becd6c 100644 --- a/src/x11_window.c +++ b/src/x11_window.c @@ -114,8 +114,12 @@ static GLFWbool waitForX11Event(double* timeout) // static GLFWbool waitForAnyEvent(double* timeout) { - nfds_t count = 1; - struct pollfd fds[2] = { { ConnectionNumber(_glfw.x11.display), POLLIN } }; + nfds_t count = 2; + struct pollfd fds[3] = + { + { ConnectionNumber(_glfw.x11.display), POLLIN }, + { _glfw.x11.emptyEventPipe[0], POLLIN } + }; #if defined(__linux__) if (_glfw.joysticksInitialized) @@ -137,6 +141,32 @@ static GLFWbool waitForAnyEvent(double* timeout) return GLFW_TRUE; } +// Writes a byte to the empty event pipe +// +static void writeEmptyEvent(void) +{ + for (;;) + { + const char byte = 0; + const int result = write(_glfw.x11.emptyEventPipe[1], &byte, 1); + if (result == 1 || (result == -1 && errno != EINTR)) + break; + } +} + +// Drains available data from the empty event pipe +// +static void drainEmptyEvents(void) +{ + for (;;) + { + char dummy[64]; + const int result = read(_glfw.x11.emptyEventPipe[0], dummy, sizeof(dummy)); + if (result == -1 && errno != EINTR) + break; + } +} + // Waits until a VisibilityNotify event arrives for the specified window or the // timeout period elapses (ICCCM section 4.2.2) // @@ -2778,6 +2808,8 @@ GLFWbool _glfwRawMouseMotionSupportedX11(void) void _glfwPollEventsX11(void) { + drainEmptyEvents(); + #if defined(__linux__) if (_glfw.joysticksInitialized) _glfwDetectJoystickConnectionLinux(); @@ -2823,13 +2855,7 @@ void _glfwWaitEventsTimeoutX11(double timeout) void _glfwPostEmptyEventX11(void) { - XEvent event = { ClientMessage }; - event.xclient.window = _glfw.x11.helperWindowHandle; - event.xclient.format = 32; // Data is 32-bit longs - event.xclient.message_type = _glfw.x11.NULL_; - - XSendEvent(_glfw.x11.display, _glfw.x11.helperWindowHandle, False, 0, &event); - XFlush(_glfw.x11.display); + writeEmptyEvent(); } void _glfwGetCursorPosX11(_GLFWwindow* window, double* xpos, double* ypos)