Found by address sanitizer: global-buffer-overflow triggered from ImGui::InputTextWithHint()

[m9710797]

Version/Branch of Dear ImGui:

Version 1.91.7-docking

Back-ends:

imgui_impl_sdl2.cpp + imgui_impl_opengl3.cpp

Compiler, OS:

Linux, gcc

Full config/build information:

Dear ImGui 1.91.7 (19170)
--------------------------------
sizeof(size_t): 8, sizeof(ImDrawIdx): 2, sizeof(ImDrawVert): 20
define: __cplusplus=201103
define: __linux__
define: __GNUC__=14
define: IMGUI_HAS_VIEWPORT
define: IMGUI_HAS_DOCK
--------------------------------
io.BackendPlatformName: imgui_impl_sdl2
io.BackendRendererName: imgui_impl_opengl3
io.ConfigFlags: 0x00000483
 NavEnableKeyboard
 NavEnableGamepad
 DockingEnable
 ViewportsEnable
io.ConfigViewportsNoDecoration
io.ConfigNavCaptureKeyboard
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,128
io.DisplaySize: 1280.00,720.00
io.DisplayFramebufferScale: 1.00,1.00
--------------------------------
style.WindowPadding: 8.00,8.00
style.WindowBorderSize: 1.00
style.FramePadding: 4.00,3.00
style.FrameRounding: 0.00
style.FrameBorderSize: 0.00
style.ItemSpacing: 8.00,4.00
style.ItemInnerSpacing: 4.00,4.00

Details:

My best guess is that back-end, compiler and OS don't matter.
But to most easily demonstrate the bug it's best to use gcc or clang with the address sanitizer enabled.

I can trigger the bug with the example application (e.g. git/imgui/examples/example_sdl2_opengl3, git revision: v1.91.7-docking), with these two source code changes (I've also included a patch below):

• In imgui_demo.cpp, line 8627, change ImGui::InputText(...) to ImGui::InputTextWithHint(..., "Enter command", ...).
• Change the Makefile to also include -fsanitize=address,undefined in CXXFLAGS.

Then re-compile and start the example application:

• In the "Dear ImGui Demo" window, enable "Menu bar > Examples > Console".
• In the command input field at the bottom (which now shows "Enter command") enter some command. For the problem to trigger the command must long enough, longer than the string "Enter command" (plus a few bytes padding??).
• Then press cursor-up to recall the (long) command.

Normally this should show the previously entered (long) command. But instead it triggers an address-sanitizer crash, see below for the full crash-report.

I've done some preliminary investigation. Here's what I think is happening:

• The crash triggers in imgui.cpp:2071 in ImStrbol(char const* buf_mid_line, char const* buf_begin) on the statement buf_mid_line[-1].
• The buf_begin parameter is valid and points to the string literal "Enter command".
• The buf_mid_line parameters points too far past buf_begin.
• More specifically in imgui_widgets.cpp:3871 this parameter is calculated as const char* cursor_ptr = ... text_begin + state->Stb->cursor ..., with Stb->cursor a value larger than the length of the "Enter command" string literal.
• So we're reading at some location past the end of the global string-literal buffer (just like the address sanitizer error report is telling).

I hope that's sufficient info. Thanks in advance for investigating this.

Background info:
I think this is a real bug, but in practice this will very often appear to work just fine, without using address-sanitizer. Often the hint is a string-literal, and often the compiler groups many string literals together in the executable. So reading past the end of a string-literal does often not trigger a crash (you just read data from an adjacent literal). I discovered this bug by accident while using address-sanitizer to debug a problem in my code. (Un)fortunately I was not able to do that because this bug in Dear ImGui triggered before my bug had a chance :-)

---

Here's the full patch:

> git diff
diff --git a/examples/example_sdl2_opengl3/Makefile b/examples/example_sdl2_opengl3/Makefile
index 5b4f9419c..3ac8d865b 100644
--- a/examples/example_sdl2_opengl3/Makefile
+++ b/examples/example_sdl2_opengl3/Makefile
@@ -23,7 +23,7 @@ OBJS = $(addsuffix .o, $(basename $(notdir $(SOURCES))))
 UNAME_S := $(shell uname -s)
 LINUX_GL_LIBS = -lGL
 
-CXXFLAGS = -std=c++11 -I$(IMGUI_DIR) -I$(IMGUI_DIR)/backends
+CXXFLAGS = -std=c++11 -I$(IMGUI_DIR) -I$(IMGUI_DIR)/backends -fsanitize=address,undefined
 CXXFLAGS += -g -Wall -Wformat
 LIBS =
 
diff --git a/imgui_demo.cpp b/imgui_demo.cpp
index 17703750b..46185f2de 100644
--- a/imgui_demo.cpp
+++ b/imgui_demo.cpp
@@ -8624,7 +8624,7 @@ struct ExampleAppConsole
         // Command-line
         bool reclaim_focus = false;
         ImGuiInputTextFlags input_text_flags = ImGuiInputTextFlags_EnterReturnsTrue | ImGuiInputTextFlags_EscapeClearsAll | ImGuiInputTextFlags_CallbackCompletion | ImGuiInputTextFlags_CallbackHistory;
-        if (ImGui::InputText("Input", InputBuf, IM_ARRAYSIZE(InputBuf), input_text_flags, &TextEditCallbackStub, (void*)this))
+        if (ImGui::InputTextWithHint("Input", "Enter command", InputBuf, IM_ARRAYSIZE(InputBuf), input_text_flags, &TextEditCallbackStub, (void*)this))
         {
             char* s = InputBuf;
             Strtrim(s);

Here's the address-sanitizer crash report (without colors):

==283856==ERROR: AddressSanitizer: global-buffer-overflow on address 0x6104a17a7ed8 at pc 0x6104a116faf2 bp 0x7ffdb88e2870 sp 0x7ffdb88e2860
READ of size 1 at 0x6104a17a7ed8 thread T0
    #0 0x6104a116faf1 in ImStrbol(char const*, char const*) ../../imgui.cpp:2071
    #1 0x6104a1606c19 in ImGui::InputTextEx(char const*, char const*, char*, int, ImVec2 const&, int, int (*)(ImGuiInputTextCallbackData*), void*) ../../imgui_widgets.cpp:5202
    #2 0x6104a15da282 in ImGui::InputTextWithHint(char const*, char const*, char*, unsigned long, int, int (*)(ImGuiInputTextCallbackData*), void*) ../../imgui_widgets.cpp:3871
    #3 0x6104a14220c5 in ExampleAppConsole::Draw(char const*, bool*) ../../imgui_demo.cpp:8627
    #4 0x6104a14086dd in ShowExampleAppConsole ../../imgui_demo.cpp:8799
    #5 0x6104a13bd9e9 in ImGui::ShowDemoWindow(bool*) ../../imgui_demo.cpp:423
    #6 0x6104a1154fb7 in main /home/wouter/git/imgui/examples/example_sdl2_opengl3/main.cpp:181
    #7 0x7f01baa2a3b7 in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58
    #8 0x7f01baa2a47a in __libc_start_main_impl ../csu/libc-start.c:360
    #9 0x6104a1154464 in _start (/home/wouter/git/imgui/examples/example_sdl2_opengl3/example_sdl2_opengl3+0x792464) (BuildId: 939fd9981100f808be6fadd1e7f8f94aeaba3c4d)

0x6104a17a7ed8 is located 10 bytes after global variable '*.LC2183' defined in '../../imgui_demo.cpp' (0x6104a17a7ec0) of size 14
  '*.LC2183' is ascii string 'Enter command'
0x6104a17a7ed8 is located 40 bytes before global variable '*.LC2184' defined in '../../imgui_demo.cpp' (0x6104a17a7f00) of size 6
  '*.LC2184' is ascii string '# %s
'
SUMMARY: AddressSanitizer: global-buffer-overflow ../../imgui.cpp:2071 in ImStrbol(char const*, char const*)
Shadow bytes around the buggy address:
  0x6104a17a7c00: f9 f9 f9 f9 00 07 f9 f9 f9 f9 f9 f9 00 00 00 00
  0x6104a17a7c80: 05 f9 f9 f9 f9 f9 f9 f9 00 00 f9 f9 f9 f9 f9 f9
  0x6104a17a7d00: 00 00 00 05 f9 f9 f9 f9 06 f9 f9 f9 f9 f9 f9 f9
  0x6104a17a7d80: 00 04 f9 f9 f9 f9 f9 f9 00 00 00 00 f9 f9 f9 f9
  0x6104a17a7e00: 00 00 f9 f9 f9 f9 f9 f9 00 f9 f9 f9 f9 f9 f9 f9
=>0x6104a17a7e80: 03 f9 f9 f9 f9 f9 f9 f9 00 06 f9[f9]f9 f9 f9 f9
  0x6104a17a7f00: 06 f9 f9 f9 f9 f9 f9 f9 00 02 f9 f9 f9 f9 f9 f9
  0x6104a17a7f80: 05 f9 f9 f9 f9 f9 f9 f9 00 01 f9 f9 f9 f9 f9 f9
  0x6104a17a8000: 00 00 07 f9 f9 f9 f9 f9 00 00 00 00 00 00 06 f9
  0x6104a17a8080: f9 f9 f9 f9 02 f9 f9 f9 f9 f9 f9 f9 00 00 03 f9
  0x6104a17a8100: f9 f9 f9 f9 06 f9 f9 f9 f9 f9 f9 f9 00 00 01 f9
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07 
  Heap left redzone:       fa
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
==283856==ABORTING

Screenshots/Video:

No response

Minimal, Complete and Verifiable Example code:

See above, can be reproduced with the example application with minimal code changes, and compiled with address-sanitizer enabled.

[ocornut]

Thanks for the report and thorough investigation. I can confirm that there is a bug.
I couldn't get it to repro with MSVC's Clang's Address Sanitizer, but could easily confirm it by adding asserts in ImStrbol():

const char* ImStrbol(const char* buf_mid_line, const char* buf_begin) // find beginning-of-line
{
    IM_ASSERT(buf_mid_line >= buf_begin);
    IM_ASSERT(buf_mid_line <= buf_begin + strlen(buf_begin));

The problem is that we lock bool is_displaying_hint earlier in the function, but the callback replacement invalidates it.
So mid-function we end up with

    if (is_displaying_hint)
    {
        buf_display = hint;
        buf_display_end = hint + strlen(hint);
    }

Even though the cursor is not zero.

What's particularly tricky about the situation is that is_displaying_hint is also needed early for this codepath:

// Password pushes a temporary font with only a fallback glyph
if (is_password && !is_displaying_hint)
    PushPasswordFont();

The native fix for the issue could make that a callback change from non-empty to empty could display a _Password field without the special *** font for one frame, which sounds particularly bad. I'll work on getting this resolved correctly :)

[ocornut]

Pushed a fix 5dd8408.

Thanks again!

[m9710797]

Thanks for fixing!

Background info:
I was curious why MSVC's address sanitizer did not caught this problem, so I did some investigation:
https://godbolt.org/z/966e9o6vn
It seems gcc does put extra padding between string literals (when address sanitizer is enabled), MSVC does not.

Your IM_ASSERT_PARANOID is also a good idea of course (and will catch even more).