do not rely on undefined behavior in glfwSetWindowIcon for X11

[slimsag]

While working on Zig bindings for GLFW me and @Andoryuuta noticed that glfwSetWindowIcon was crashing. I wrote about debugging this and the cause in an article 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:

                *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:

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. See do not rely on undefined behavior in glfwSetWindowIcon for X11 #1986 (comment)

In an equal snippet of code in Godbolt, 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

[Maato]

The explanation of why this is undefined behavior is incorrect.

In an expression such as this: images[i].pixels[j * 4 + 0] << 16, the integer promotions are performed on each of the operands. So the left operand, which starts out as unsigned char, will get promoted to int. Shifting a value known to be at most 255 to the left by 16 bits will not cause undefined behavior in this case.

Instead the undefined behavior comes from the following expression: images[i].pixels[j * 4 + 3] << 24. Again the left operand will be promoted to int. If the value happens to be >= 0x80 shifting left by 24 will cause the sign bit of the int to be set (or really, the mathematical result of the shift operation will not be representable in the int).

The standard says the following (E1 is the left operand and E2 is the right operand of the << operator):

If E1 has a signed type and nonnegative value, and E1x2^E2 is representable in the result type, then that is the resulting value; otherwise, the behavior is undefined.

[slimsag]

Oh wow, I totally missed that @Maato - thank you for pointing that out. I should've caught that, too, because I knew << 24 was the shift directly before the crash but I guess I muddied the water and got confused :)

[martinhath]

Wouldn't it be better to cast to unsigned int or something similar (like uint32_t), since long can still be 32 bits? If the size of long and int is the same, wouldn't the cast as proposed in the PR be semantically identical to the current state?

[dcousens]

If the size of long and int is the same, wouldn't the cast as proposed in the PR be semantically identical to the current state?

// gcc -fsanitize=undefined
#include <stdint.h>
#include <stdio.h>

int main () {
	unsigned char foo = 0x80;
	int x =
		(((int) foo) << 16) |
		(((int) foo) <<  8) |
		(((int) foo) <<  0) |
		(((int) foo) << 24);

	printf("x: %i \n", x);
	return 0;
}
// test.c:10:16: runtime error: left shift of 128 by 24 places cannot be represented in type 'int'
//   x: -2139062144

Yes

I wonder if there is a set of warning flags that could have helped with this.
Unfortunately -Wconversion misses the mark.

[drfuchs]

Wouldn't it be better to cast to unsigned int or something similar (like uint32_t), since long can still be 32 bits? If the size of long and int is the same, wouldn't the cast as proposed in the PR be semantically identical to the current state?

In case there's a thought of fixing the incorrect "cast to long" fix by casting to "long long" instead (which would at least work properly everywhere), another reason that this suggestion by martinhath (to cast to unsigned int) is the correct solution is that it's potentially more efficient in various architectures and optimization levels, with zero extra machine code generated for widening into a 64-bit value, never mind 64-bit shifts being slower in some situations. It also more clearly indicates what the real issue is (shifting a one into the sign bit, which an unsigned int doesn't have).

[slimsag]

Pushed a commit to switch to unsigned int, I really appreciate the corrections to my understanding & feedback here everyone!

[Validark]

To repeat what @Maato said but in arithmetic terms:
(byte ≥ 2⁷) × 2²⁴ ⟹ (result ≥ 2³¹)

The maximum signed 32-bit integer is 2³¹- 1, hence you get an overflow.

[linkmauve]

Hi, the proper type for an unsigned 32-bit integer is uint32_t.

Not that it matters much here because GLFW is unlikely to ever be compiled for a 16-bit CPU, but the C standard only guarantees unsigned int to be at least 16-bit wide, which would still make two of these values not representable.

Also note that target is still a long*, which means the conversion from uint32_t to long still happens, and on 32-bit platforms (or platforms using LLP64) the sign bit will still be the most significant bit of the alpha value.

[elmindreda]

Thank you @slimsag and @Andoryuuta for making and documenting this bug fix, and thank you everyone for helping to refine it!

I agree that uint32_t is a more appropriate type to cast to before shifting, even if unsigned int is probably okay for anything we will run on.

[slimsag]

Appreciate the feedback, I've swapped the cast for uint32_t and also changed target and icon from long to uint32_t types which if I understand correctly should be more correct due to XChangeProperty accepting a 32-bit format regardless of platform.

Let me know what you think!

[linkmauve]

Hi, I believe the change from long to uint32_t is incorrect, Xlib defines the data as a series of long, which would be 64-bit on ILP64 platforms like Linux.

[martinhath]

Hi, I believe the change from long to uint32_t is incorrect, Xlib defines the data as a series of long, which would be 64-bit on ILP64 platforms like Linux.

I'm not sure this is true, do you have a source for this? Here is, from what I can tell, the spec, and it says (under _NET_WM_ICON):

This is an array of 32bit packed CARDINAL ARGB with high byte being A, low byte being B. The first two cardinals are width, height. Data is in rows, left to right and top to bottom.

Further, this is reflected in the actual call to X that GLFW already does:

XChangeProperty(_glfw.x11.display, window->x11.handle,
                _glfw.x11.NET_WM_ICON,
                XA_CARDINAL, 32,
                PropModeReplace,
                (unsigned char*) icon,
                elements);

where we explicitly specify that we're passing in unsigned 32-bit numbers.

I'm no Xpert though, so I might be mistaken :)

[slimsag]

I thought similar to what @martinhath said above based on the XChangeProperty docs, as we pass 32 for the format which is defined as:

Specifies whether the data should be viewed as a list of 8-bit, 16-bit, or 32-bit quantities. Possible values are 8, 16, and 32. This information allows the X server to correctly perform byte-swap operations as necessary. If the format is 16-bit or 32-bit, you must explicitly cast your data pointer to an (unsigned char *) in the call to XChangeProperty().

And because the signature takes unsigned char* not long*.

However, looking into it more it seems like Xlib casts the pointer back to long*, and passes it to this Data32 function internally right here:

Data32 (dpy, (_Xconst long *) data, len);

And on IL64 platforms that ends up defined as a function which takes long* data and converts the 64-bit longs to 32-bit data:

https://sourcegraph.com/github.com/mirror/libX11@2356e59/-/blob/src/XlibInt.c?L1670-1697

So it would seem the Xlib docs are wrong, the function does take long* data despite the signature being unsigned char* data, and despite it claiming to take 32-bit data.

Will update soon.

[slimsag]

OK, I believe this is finally good to go, we now cast to unsigned long. The unsigned should avoid the UBSan complaint about overflowing into the sign bit, while long should match the internal expectations of Xlib which internally treats the pointer as a long (64-bit integer on IL64 platforms, even if the data is only 32-bit.)

I have tested with UBSan as well and this passes.

[elmindreda]

Thank you everyone for the fix!