From 9cd4d2fa204fc9fb884b9bb9096cedb7f1484cf7 Mon Sep 17 00:00:00 2001 From: Stephen Gutekanst Date: Sun, 31 Oct 2021 11:22:40 -0700 Subject: [PATCH] X11: Fix undefined behavior in glfwSetWindowIcon The conversion of window icon image data involves unsigned char color values being promoted to int and then shifted to the left by 24. For 32-bit ints this is just far enough to trigger undefined behavior. It worked by accident because of how current compilers translate this piece of code. This was caught by @slimsag while working on [Zig bindings for GLFW][1], and diagnosed together with @Andoryuuta, as described [in an article][2]. Zig has UBSan enabled by default, which caught this undefined behavior. [1]: https://github.com/hexops/mach-glfw [2]: https://devlog.hexops.com/2021/perfecting-glfw-for-zig-and-finding-undefined-behavior#finding-lurking-undefined-behavior-in-6-year-old-glfw-code Thanks to Maato, martinhath, dcousens, drfuchs and Validark for helping to refine the solution. This commit message was rewritten by @elmindreda to hopefully reflect the conclusions of the pull request thread. Related to hexops/mach#20 Closes #1986 --- src/x11_window.c | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/src/x11_window.c b/src/x11_window.c index 616f9fd8..ea775cd4 100644 --- a/src/x11_window.c +++ b/src/x11_window.c @@ -2105,8 +2105,8 @@ void _glfwSetWindowIconX11(_GLFWwindow* window, int count, const GLFWimage* imag for (int i = 0; i < count; i++) longCount += 2 + images[i].width * images[i].height; - long* icon = _glfw_calloc(longCount, sizeof(long)); - long* target = icon; + unsigned long* icon = _glfw_calloc(longCount, sizeof(unsigned long)); + unsigned long* target = icon; for (int i = 0; i < count; i++) { @@ -2115,13 +2115,20 @@ void _glfwSetWindowIconX11(_GLFWwindow* window, int count, const GLFWimage* imag for (int j = 0; j < images[i].width * images[i].height; j++) { - *target++ = (images[i].pixels[j * 4 + 0] << 16) | - (images[i].pixels[j * 4 + 1] << 8) | - (images[i].pixels[j * 4 + 2] << 0) | - (images[i].pixels[j * 4 + 3] << 24); + *target++ = (((unsigned long)images[i].pixels[j * 4 + 0]) << 16) | + (((unsigned long)images[i].pixels[j * 4 + 1]) << 8) | + (((unsigned long)images[i].pixels[j * 4 + 2]) << 0) | + (((unsigned long)images[i].pixels[j * 4 + 3]) << 24); } } + // Important: Despite XChangeProperty docs indicating that `icon` (unsigned char*) would be + // in the format of the icon image, e.g. 32-bit below, the function actually casts the ptr + // (unsigned char*) internally to (long*) and then if long is defined as 64-bits, as on IL64 + // platforms, extracts only 32 bits from the long leaving the other 32 unused. That is, on a + // 64-bit platform XChangeProperty expects 64-bit integers representing 32-bit pixels. + // + // See https://github.com/glfw/glfw/pull/1986#issuecomment-962445299 XChangeProperty(_glfw.x11.display, window->x11.handle, _glfw.x11.NET_WM_ICON, XA_CARDINAL, 32,