floooh / sokol

minimal cross-platform standalone C headers
https://floooh.github.io/sokol-html5
zlib License
7.08k stars 498 forks source link

sokol_app.h: UBSAN: I am too lazy to find out what is happening here, it happened only once and I don't care enough. #1153

Closed OetkenPurveyorOfCode closed 2 hours ago

OetkenPurveyorOfCode commented 3 hours ago
.\sokol_app.h:7569:109: runtime error: left shift of 4294967295 by 16 places cannot be represented in type 'uint32_t' (aka 'unsigned int')
                if (SendMessage(_sapp.win32.hwnd, WM_NCHITTEST, wParam, lParam) == HTCAPTION) {
                    POINT point;
                    GetCursorPos(&point);
                    ScreenToClient(_sapp.win32.hwnd, &point);
                    PostMessage(_sapp.win32.hwnd, WM_MOUSEMOVE, 0, ((uint32_t)point.x)|(((uint32_t)point.y) << 16)); // overflow happens here
                }
floooh commented 2 hours ago

Thanks! 4294967295 is hex 0xFFFFFFFF, looks like point.y can be -1 (or generally negative) in some situations. The component types in POINT are signed numbers, and left shifting a negative signed number is UB.

I guess I'll just skip the entire PostMessage when the x or y values in POINT are negative.

OetkenPurveyorOfCode commented 2 hours ago

Hang on, its acutally defined behaviour. I guess UBSAN also warns against unsigned integer overflow.

floooh commented 2 hours ago

Ah right, it's casted to unsigned before the shift... then I guess shifting 1-bits out of a value is also UB, that's kinda crazy though (e.g. IMHO shifting bits out of a value is an entirely normal operation, but looks like UBSAN or the C standard disagrees).

OetkenPurveyorOfCode commented 2 hours ago

Thanks! 4294967295 is hex 0xFFFFFFFF, looks like point.y can be -1 (or generally negative) in some situations.

The documentation says that GetCursorPos can fail and return zero in the BOOL return value.

OetkenPurveyorOfCode commented 2 hours ago

Ah right, it's casted to unsigned before the shift... then I guess shifting 1-bits out of a value is also UB, that's kinda crazy though (e.g. IMHO shifting bits out of a value is an entirely normal operation, but looks like UBSAN or the C standard disagrees).

I guess I left integer sanitizer on in my build.

floooh commented 2 hours ago

Hmm, could be the -fsanitize=unsigned-integer-overflow, which says in the UBSAN docs "is not undefined behaviour but often unintentional".

floooh commented 1 hour ago

The documentation says that GetCursorPos can fail and return zero in the BOOL return value.

...good point, I'll use the return value to skip the rest of the code. POINT point; being uninitialized is also kind of a code-smell.

floooh commented 1 hour ago

Ok, see https://github.com/floooh/sokol/commit/a5329cf1ca56408687c7771772846b0c38094ac4. Not sure if it actually fixes the unsigned integer overflow, but it's definitely better behaviour than before.

Thanks for the report!