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

DragScalar clamp segment fault bug if only one of min/max value is provided #3824

Closed
harry75369 opened this issue Feb 18, 2021 · 1 comment
Closed

Comments

@harry75369
Copy link
Contributor

harry75369 commented Feb 18, 2021

Version/Branch of Dear ImGui:

Dear ImGui 1.80 (18000)
--------------------------------
sizeof(size_t): 8, sizeof(ImDrawIdx): 2, sizeof(ImDrawVert): 20
define: __cplusplus=201703
define: __linux__
define: __GNUC__=10
--------------------------------
io.BackendPlatformName: imgui_impl_sdl
io.BackendRendererName: imgui_impl_opengl3
io.ConfigFlags: 0x00000020
 NoMouseCursorChange
io.ConfigInputTextCursorBlink
io.ConfigWindowsResizeFromEdges
io.ConfigMemoryCompactTimer = 60.0
io.BackendFlags: 0x0000000E
 HasMouseCursors
 HasSetMousePos
 RendererHasVtxOffset
--------------------------------
io.Fonts: 1 fonts, Flags: 0x00000000, TexSize: 512,1024
io.DisplaySize: 2000.00,1600.00
io.DisplayFramebufferScale: 1.00,1.00
--------------------------------
style.WindowPadding: 16.00,16.00
style.WindowBorderSize: 1.00
style.FramePadding: 8.00,6.00
style.FrameRounding: 0.00
style.FrameBorderSize: 0.00
style.ItemSpacing: 16.00,8.00
style.ItemInnerSpacing: 8.00,8.00

My Issue/Question:

If ImGui::DragScalar is provided only one of min/max values and ImGuiSliderFlags_AlwaysClamp is set, the program will segfault when input an out-of-bound value.

This is due to the bug in ImGui::TempInputScalar function where it attempts to invoke DataTypeCompare even if only one of min/max value is provided.

But for DataTypeCompare, both min/max values must be provided.

  3250 bool ImGui::TempInputScalar(const ImRect& bb, ImGuiID id, const char* label, ImGuiDataType data_type, void* p_data, const char* format, const void* p_clamp_min, const void* p_clamp_max)
  3251 {
  3252     ...
  3263     if (TempInputText(bb, id, label, data_buf, IM_ARRAYSIZE(data_buf), flags))
  3264     {
  3265         // Backup old value
  3266         size_t data_type_size = DataTypeGetInfo(data_type)->Size;
  3267         ImGuiDataTypeTempStorage data_backup;
  3268         memcpy(&data_backup, p_data, data_type_size);
  3269 
  3270         // Apply new value (or operations) then clamp
  3271         DataTypeApplyOpFromText(data_buf, g.InputTextState.InitialTextA.Data, data_type, p_data, NULL);
  3272         if (p_clamp_min || p_clamp_max) // NOTE HERE!
  3273         {
  3274             if (DataTypeCompare(data_type, p_clamp_min, p_clamp_max) > 0) // NOTE HERE!
  3275                 ImSwap(p_clamp_min, p_clamp_max);
  3276             DataTypeClamp(data_type, p_data, p_clamp_min, p_clamp_max);
  3277         }

Related commit: 36af398

My current fix is

  3272         if (p_clamp_min || p_clamp_max)
  3273         {
~ 3274             if (p_clamp_min && p_clamp_max && DataTypeCompare(data_type, p_clamp_min, p_clamp_max) > 0)
  3275                 ImSwap(p_clamp_min, p_clamp_max);
  3276             DataTypeClamp(data_type, p_data, p_clamp_min, p_clamp_max);
  3277         }

Hope it helps.

ocornut added a commit that referenced this issue Feb 18, 2021
…iderFlags_AlwaysClamp + only one of either p_min or p_max set. (#3824) [@harry75369]
@ocornut
Copy link
Owner

ocornut commented Feb 18, 2021

Hello Chaoya,
Thanks for your report. I've looked at this and confirm that your suggested fix is correct.
The fix has been pushed to 0ecdf81.

Thank you!

@ocornut ocornut closed this as completed Feb 18, 2021
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