From b582cc576e0afc10f729ade7d6a66287b3c28686 Mon Sep 17 00:00:00 2001 From: Stephen Gutekanst Date: Sun, 31 Oct 2021 11:22:40 -0700 Subject: [PATCH] do not rely on undefined behavior in glfwSetWindowIcon for X11 While working on [Zig bindings for GLFW](https://github.com/hexops/mach-glfw) me and @Andoryuuta noticed that `glfwSetWindowIcon` was crashing. I wrote about debugging this and the cause [in an article](https://devlog.hexops.com/2021/perfecting-glfw-for-zig-and-finding-undefined-behavior#finding-lurking-undefined-behavior-in-6-year-old-glfw-code) but the summary is that when compiling with UBSan (which Zig does by default) clang will insert `asm { ud1 }` traps when it thinks there is undefined behavior. This code in particular is problematic: ```c *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); ``` We see in IDA Pro that clang inserted a jump (pictured below) to an `asm { ud1 }` instruction: ![image](https://user-images.githubusercontent.com/3173176/139594073-b2159e4c-6764-44b1-882d-802724f424e8.png) What is happening here is that: * `images[i].pixels[j * 4 + 0]` is returning an `unsigned char` (8 bits) * It is then being shifted left by `<< 16` bits. !!! That's further than an 8-bit number can be shifted left by, hence undefined behavior. In [an equal snippet of code in Godbolt](https://godbolt.org/z/ddq75WsYK), we can see that without UBSan clang merely uses the 32-bit EAX register as an optimization. It loads the 8-bit number into the 32-bit register, and then performs the left shift. Although the shift exceeds 8 bits, it _does not get truncated to zero_ - instead it is effectively as if the number was converted to a `long` (32 bits) prior to the left-shift operation. This explains why nobody has caught this UB in GLFW yet, too: it works by nature of compilers liking to use 32-bit registers in this context. So, to fix this, ensure we cast to `long` first before shifting. Helps hexops/mach#20 Signed-off-by: Stephen Gutekanst --- src/x11_window.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/x11_window.c b/src/x11_window.c index 616f9fd8..be1fb502 100644 --- a/src/x11_window.c +++ b/src/x11_window.c @@ -2115,10 +2115,10 @@ 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++ = (((long)images[i].pixels[j * 4 + 0]) << 16) | + (((long)images[i].pixels[j * 4 + 1]) << 8) | + (((long)images[i].pixels[j * 4 + 2]) << 0) | + (((long)images[i].pixels[j * 4 + 3]) << 24); } }