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

(cherry picked from commit 9cd4d2fa20)
This commit is contained in:
Stephen Gutekanst 2021-10-31 11:22:40 -07:00 committed by Camilla Löwy
parent 81d762bf66
commit 6281424988

View File

@ -2123,8 +2123,8 @@ void _glfwPlatformSetWindowIcon(_GLFWwindow* window,
for (i = 0; i < count; i++)
longCount += 2 + images[i].width * images[i].height;
long* icon = calloc(longCount, sizeof(long));
long* target = icon;
unsigned long* icon = calloc(longCount, sizeof(unsigned long));
unsigned long* target = icon;
for (i = 0; i < count; i++)
{
@ -2133,13 +2133,20 @@ void _glfwPlatformSetWindowIcon(_GLFWwindow* window,
for (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,