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 <stephen@hexops.com>
This commit is contained in:
Stephen Gutekanst 2021-10-31 11:22:40 -07:00
parent fb0f2f92a3
commit b582cc576e

View File

@ -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);
}
}