Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

signed overflow in "slider s32 full" demo widget #6675

Closed
jbarthelmes opened this issue Aug 2, 2023 · 3 comments
Closed

signed overflow in "slider s32 full" demo widget #6675

jbarthelmes opened this issue Aug 2, 2023 · 3 comments

Comments

@jbarthelmes
Copy link
Contributor

Version/Branch of Dear ImGui:

Dear ImGui 1.89.8 WIP (18974)
--------------------------------
sizeof(size_t): 8, sizeof(ImDrawIdx): 2, sizeof(ImDrawVert): 20
define: __cplusplus=201703
define: _WIN32
define: _WIN64
define: __MINGW32__
define: __MINGW64__
define: __GNUC__=4
define: __clang_version__=16.0.1 (https://github.com/ziglang/zig-bootstrap bf1b2cdb83141ad9336eec42160c9fe87f90198d)
define: IMGUI_HAS_VIEWPORT
define: IMGUI_HAS_DOCK

My Issue/Question:

The demo widget "slider s32 full" creates an integer overflow that my compiler in debug traps with an illegal instruction. Signed overflow is UB IIRC.

Concretely, the issue is with v_range + 1 here:

imgui/imgui_widgets.cpp

Lines 2781 to 2782 in f7eea63

if (!is_floating_point && v_range >= 0) // v_range < 0 may happen on integer overflows
grab_sz = ImMax((float)(slider_sz / (v_range + 1)), style.GrabMinSize); // For integer sliders: if possible have the grab size represent 1 unit

With v_range for "slider s32 full" being 2147483647 = 231-1.

@GamingMinds-DanielC
Copy link
Contributor

Oh, quite true that, same for 64 bits. The expected behavior would be a division by a large negative number, resulting in a small negative number that then gets clamped to style.GrabMinSize, so no actual problem. But while the actual behavior is known, it technically is undefined, and that should be avoided.

@ocornut: replacing (float)(slider_sz / (v_range + 1)) with slider_sz / ((float)v_range + 1.0f)) should be safe there.

@jbarthelmes
Copy link
Contributor Author

I changed the definition of v_range to float because the only other usage

imgui/imgui_widgets.cpp

Lines 2851 to 2852 in f7eea63

if ((v_range >= -100.0f && v_range <= 100.0f) || tweak_slow)
input_delta = ((input_delta < 0.0f) ? -1.0f : +1.0f) / (float)v_range; // Gamepad/keyboard tweak speeds in integer steps
treats it like float anyway. That makes it a one word change and the typecast on the division can also be removed.

@ocornut
Copy link
Owner

ocornut commented Aug 3, 2023

Merged b7a7d67, thanks!

@ocornut ocornut closed this as completed Aug 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants