Skip to content

Commit

Permalink
X11: Fix undefined behavior in glfwSetWindowIcon
Browse files Browse the repository at this point in the history
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 9cd4d2f)
  • Loading branch information
slimsag authored and elmindreda committed Dec 8, 2021
1 parent 81d762b commit 6281424
Showing 1 changed file with 13 additions and 6 deletions.
19 changes: 13 additions & 6 deletions src/x11_window.c
Expand Up @@ -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++)
{
Expand All @@ -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,
Expand Down

0 comments on commit 6281424

Please sign in to comment.