Zengtudor/glfw
1
0
Fork 0
mirror of https://github.com/glfw/glfw.git synced 2024-11-11 13:03:52 +00:00
Code Issues Packages Projects Releases Wiki Activity

X11: Fix undefined behavior in glfwSetWindowIcon

Browse Source 
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
This commit is contained in:
Stephen Gutekanst 2021-10-31 11:22:40 -07:00 committed by Camilla Löwy
parent 37fc28bff6
commit 9cd4d2fa20
1 changed files with 13 additions and 6 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

19
src/x11_window.c
View File

@ -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,