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

New issue

Closed

opened 2023-08-02 08:13:13 +00:00 by jbarthelmes · 3 comments

jbarthelmes commented

2023-08-02 08:13:13 +00:00

(Migrated from github.com)

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: ocornut/imgui@f7eea63872/imgui_widgets.cpp (L2781-L2782)
With v_range for "slider s32 full" being 2147483647 = 2³¹-1.

**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: https://github.com/ocornut/imgui/blob/f7eea6387202f1174fdb7072311156d667a6826c/imgui_widgets.cpp#L2781-L2782 With `v_range` for "slider s32 full" being 2147483647 = 2<sup>31</sup>-1.

GamingMinds-DanielC commented

2023-08-02 09:26:23 +00:00

(Migrated from github.com)

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.

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 commented

2023-08-02 20:27:22 +00:00

(Migrated from github.com)

I changed the definition of v_range to float because the only other usage ocornut/imgui@f7eea63872/imgui_widgets.cpp (L2851-L2852) treats it like float anyway. That makes it a one word change and the typecast on the division can also be removed.

I changed the definition of `v_range` to float because the only other usage https://github.com/ocornut/imgui/blob/f7eea6387202f1174fdb7072311156d667a6826c/imgui_widgets.cpp#L2851-L2852 treats it like float anyway. That makes it a one word change and the typecast on the division can also be removed.

ocornut commented

2023-08-03 18:15:15 +00:00

(Migrated from github.com)

Merged b7a7d673b9, thanks!

Merged b7a7d673b96f08b1e8a7195aa59307730bbf3c54, thanks!