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

ProgressBar with fraction == NaN leads to gigantic allocation #7451

Closed
alektron opened this issue Mar 28, 2024 · 1 comment
Closed

ProgressBar with fraction == NaN leads to gigantic allocation #7451

alektron opened this issue Mar 28, 2024 · 1 comment

Comments

@alektron
Copy link

Version/Branch of Dear ImGui:

Version 1.90.5 (19046), Branch: docking (master/docking/etc.)

Back-ends:

Backend independent

Compiler, OS:

Visual Studio 2022 MSVC C++ 20

Full config/build information:

Dear ImGui 1.90.5 WIP (19046)
--------------------------------
sizeof(size_t): 8, sizeof(ImDrawIdx): 2, sizeof(ImDrawVert): 20
define: __cplusplus=199711
define: _WIN32
define: _WIN64
define: _MSC_VER=1938
define: _MSVC_LANG=202002
define: IMGUI_HAS_VIEWPORT
define: IMGUI_HAS_DOCK
--------------------------------
io.BackendPlatformName: imgui_impl_win32
io.BackendRendererName: imgui_impl_opengl3
io.ConfigFlags: 0x00000443
 NavEnableKeyboard
 NavEnableGamepad
 DockingEnable
 ViewportsEnable
io.ConfigViewportsNoDecoration
io.ConfigInputTextCursorBlink
io.ConfigWindowsResizeFromEdges
io.ConfigMemoryCompactTimer = 60.0
io.BackendFlags: 0x00001C0E
 HasMouseCursors
 HasSetMousePos
 PlatformHasViewports
 HasMouseHoveredViewport
 RendererHasVtxOffset
 RendererHasViewports
--------------------------------
io.Fonts: 1 fonts, Flags: 0x00000000, TexSize: 512,64
io.DisplaySize: 1264.00,761.00
io.DisplayFramebufferScale: 1.00,1.00
--------------------------------
style.FrameRounding: (See explanation)

Details:

Reproduction:

volatile float zero = 0.0f;
ImGui::ProgressBar(0 / zero);

Explanation:
Passing NaN to a ProgressBar widget with FrameRounding != 0 leads to a giant allocation in PathArcTo.
In my specific case this happens due to us calculating the fraction for the progress bar as Count / (float)MaxCount, which happens to be able to become a 0 / (float)0.
Now, shame on us for dividing through 0 but it seems to me like that would be a pretty common expression to use for a progress bar and therefore a mistake that is easily made.

Now ProgressBar calls ImSaturate internally on fraction which e.g. properly prevents issues when fraction equals infinity.
However NaN seems to stay NaN.
This NaN gets then passed down to RenderRectFilledRangeH.
IMPORTANT: If FrameRounding is set to 0, RenderRectFilledRangeH will exit out early and while the values calculated from NaN are still wrong, it is not critical for AddRectFilled. It may look wrong but doesn't crash.
With frame rounding enabled however, a variation of Path...To will get used to draw the rounded rectangle.
And PathArcTo for example will allocate. I did not look exactly into how the NaN influences the number of path points. I also had different results in our application as opposed to a fresh imgui clone example.
However from that point it is easy to see that things go wrong and lead to a giant allocation in _Path.reserve.

The fix is not exactly difficult (just a check in ProgressBar) but it raises the quesiton if maybe PathArcTo itself should be more robust against this kind of input. That may be up to discussion and probably better decided by someone else.

Screenshots/Video:

No response

Minimal, Complete and Verifiable Example code:

No response

@ocornut
Copy link
Owner

ocornut commented Mar 29, 2024

Thank you for the report and suggestion!
I made ProgressBar() support NaN without leading to a crash (commit 25a492f) and display the NaN within the progress bar.
I however left it as crashing/UB to directly call e.g. PathArcTo() with NaN value. I think it's fine that it crashes in this context.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants