search

ocornut / imgui_test_engine

Dear ImGui Automation Engine & Test Suite
386 stars 40 forks source link

TestEngine: heap use after free in ItemDragAndDrop #49

Closed mgerhardy closed 2 months ago

mgerhardy commented 2 months ago

Testcode at

https://github.com/vengi-voxel/vengi/blob/6916070fd1a4f246e711eec7a23ea642b6e63d89/src/tools/voxedit/modules/voxedit-ui/tests/AssetPanelTest.cpp#L14

The first Run-all worked fine, the second lead to this crash

void AssetPanel::registerUITests(ImGuiTestEngine *engine, const char *title) {
    IM_REGISTER_TEST(engine, testCategory(), "drag drop image")->TestFunc = [=](ImGuiTestContext *ctx) {
        if (_texturePool->cache().empty()) {
            ctx->LogInfo("No images found in asset panel");
            return;
        }
        const int viewportId = viewportEditMode(ctx, _app);
        IM_CHECK_SILENT(viewportId != -1);
        const core::String id = core::String::format("//%s", Viewport::viewportId(viewportId).c_str());

        const size_t n = core_min(3, _texturePool->cache().size());
        for (size_t i = 0; i < n; ++i) {
            IM_CHECK(focusWindow(ctx, title));
            ctx->ItemClick("##assetpaneltabs/Images");
            const core::String srcRef = core::string::format("##assetpaneltabs/Images/%i", (int)i);
            ctx->ItemDragAndDrop(srcRef.c_str(), id.c_str());
        }
    };
}

Maybe I'm doing something wrong here - but as the memory was allocated by testengine and also freed by testengine I thought it would make sense to report it.

=================================================================
==154641==ERROR: AddressSanitizer: heap-use-after-free on address 0x50f00004c618 at pc 0x557aa63d8ca6 bp 0x7f37b765f710 sp 0x7f37b765f708
READ of size 4 at 0x50f00004c618 thread T10
    #0 0x557aa63d8ca5 in ImGuiTestContext::ItemDragAndDrop(ImGuiTestRef, ImGuiTestRef, int) /home/mgerhardy/dev/oss/engine/src/modules/ui/dearimgui/imgui_test_engine/imgui_te_context.cpp:3032
    #1 0x557aa3e19e30 in operator() /home/mgerhardy/dev/oss/engine/src/tools/voxedit/modules/voxedit-ui/tests/AssetPanelTest.cpp:27
    #2 0x557aa3e1b03e in __invoke_impl<void, voxedit::AssetPanel::registerUITests(ImGuiTestEngine*, char const*)::<lambda(ImGuiTestContext*)>&, ImGuiTestContext*> /usr/include/c++/13/bits/invoke.h:61
    #3 0x557aa3e1ac49 in __invoke_r<void, voxedit::AssetPanel::registerUITests(ImGuiTestEngine*, char const*)::<lambda(ImGuiTestContext*)>&, ImGuiTestContext*> /usr/include/c++/13/bits/invoke.h:111
    #4 0x557aa3e1a95c in _M_invoke /usr/include/c++/13/bits/std_function.h:290
    #5 0x557aa6439256 in std::function<void (ImGuiTestContext*)>::operator()(ImGuiTestContext*) const (/home/mgerhardy/dev/oss/engine/build/voxedit/vengi-voxedit+0x86ff256) (BuildId: aa8b0276cbe3a526c136e6b69891c16b23fd7758)
    #6 0x557aa64123b3 in ImGuiTestEngine_RunTest(ImGuiTestEngine*, ImGuiTestContext*, ImGuiTest*, int) /home/mgerhardy/dev/oss/engine/src/modules/ui/dearimgui/imgui_test_engine/imgui_te_engine.cpp:1625
    #7 0x557aa64093b4 in ImGuiTestEngine_ProcessTestQueue /home/mgerhardy/dev/oss/engine/src/modules/ui/dearimgui/imgui_test_engine/imgui_te_engine.cpp:1092
    #8 0x557aa6406623 in ImGuiTestEngine_TestQueueCoroutineMain /home/mgerhardy/dev/oss/engine/src/modules/ui/dearimgui/imgui_test_engine/imgui_te_engine.cpp:909
    #9 0x557aa64c171b in CoroutineThreadMain /home/mgerhardy/dev/oss/engine/src/modules/ui/dearimgui/imgui_test_engine/imgui_te_coroutine.cpp:57
    #10 0x557aa64c69f1 in void std::__invoke_impl<void, void (*)(Coroutine_ImplStdThreadData*, void (*)(void*), void*), Coroutine_ImplStdThreadData*, void (*)(void*), void*>(std::__invoke_other, void (*&&)(Coroutine_ImplStdThreadData*, void (*)(void*), void*), Coroutine_ImplStdThreadData*&&, void (*&&)(void*), void*&&) /usr/include/c++/13/bits/invoke.h:61
    #11 0x557aa64c66e2 in std::__invoke_result<void (*)(Coroutine_ImplStdThreadData*, void (*)(void*), void*), Coroutine_ImplStdThreadData*, void (*)(void*), void*>::type std::__invoke<void (*)(Coroutine_ImplStdThreadData*, void (*)(void*), void*), Coroutine_ImplStdThreadData*, void (*)(void*), void*>(void (*&&)(Coroutine_ImplStdThreadData*, void (*)(void*), void*), Coroutine_ImplStdThreadData*&&, void (*&&)(void*), void*&&) (/home/mgerhardy/dev/oss/engine/build/voxedit/vengi-voxedit+0x878c6e2) (BuildId: aa8b0276cbe3a526c136e6b69891c16b23fd7758)
    #12 0x557aa64c6277 in void std::thread::_Invoker<std::tuple<void (*)(Coroutine_ImplStdThreadData*, void (*)(void*), void*), Coroutine_ImplStdThreadData*, void (*)(void*), void*> >::_M_invoke<0ul, 1ul, 2ul, 3ul>(std::_Index_tuple<0ul, 1ul, 2ul, 3ul>) (/home/mgerhardy/dev/oss/engine/build/voxedit/vengi-voxedit+0x878c277) (BuildId: aa8b0276cbe3a526c136e6b69891c16b23fd7758)
    #13 0x557aa64c5f8b in std::thread::_Invoker<std::tuple<void (*)(Coroutine_ImplStdThreadData*, void (*)(void*), void*), Coroutine_ImplStdThreadData*, void (*)(void*), void*> >::operator()() (/home/mgerhardy/dev/oss/engine/build/voxedit/vengi-voxedit+0x878bf8b) (BuildId: aa8b0276cbe3a526c136e6b69891c16b23fd7758)
    #14 0x557aa64c5f43 in std::thread::_State_impl<std::thread::_Invoker<std::tuple<void (*)(Coroutine_ImplStdThreadData*, void (*)(void*), void*), Coroutine_ImplStdThreadData*, void (*)(void*), void*> > >::_M_run() (/home/mgerhardy/dev/oss/engine/build/voxedit/vengi-voxedit+0x878bf43) (BuildId: aa8b0276cbe3a526c136e6b69891c16b23fd7758)
    #15 0x7f37dc0dee63  (/lib/x86_64-linux-gnu/libstdc++.so.6+0xdee63) (BuildId: 332d8df09457200979fca5abf449dd4629a19f78)
    #16 0x7f37dc65ae65 in asan_thread_start ../../../../src/libsanitizer/asan/asan_interceptors.cpp:234
    #17 0x7f37db6a645b in start_thread nptl/pthread_create.c:444
    #18 0x7f37db726bbb in clone3 ../sysdeps/unix/sysv/linux/x86_64/clone3.S:81

0x50f00004c618 is located 72 bytes inside of 176-byte region [0x50f00004c5d0,0x50f00004c680)
freed by thread T0 here:
    #0 0x7f37dc6f2878 in free ../../../../src/libsanitizer/asan/asan_malloc_linux.cpp:52
    #1 0x7f37dc4ba7f2  (/lib/x86_64-linux-gnu/libSDL2-2.0.so.0+0xa57f2) (BuildId: 0fde3d17eb240615a6c99354e493de4c6dea2d72)
    #2 0x557aa4a01157 in ImGui::MemFree(void*) /home/mgerhardy/dev/oss/engine/src/modules/ui/dearimgui/imgui.cpp:4316
    #3 0x557aa64367e5 in void IM_DELETE<ImGuiTestInfoTask>(ImGuiTestInfoTask*) /home/mgerhardy/dev/oss/engine/src/modules/ui/dearimgui/imgui.h:1980
    #4 0x557aa6404aaf in ImGuiTestEngine_PostNewFrame /home/mgerhardy/dev/oss/engine/src/modules/ui/dearimgui/imgui_test_engine/imgui_te_engine.cpp:810
    #5 0x557aa63f95e5 in operator() /home/mgerhardy/dev/oss/engine/src/modules/ui/dearimgui/imgui_test_engine/imgui_te_engine.cpp:162
    #6 0x557aa63f960d in _FUN /home/mgerhardy/dev/oss/engine/src/modules/ui/dearimgui/imgui_test_engine/imgui_te_engine.cpp:162
    #7 0x557aa49f637c in ImGui::CallContextHooks(ImGuiContext*, ImGuiContextHookType) /home/mgerhardy/dev/oss/engine/src/modules/ui/dearimgui/imgui.cpp:3835
    #8 0x557aa4a14087 in ImGui::NewFrame() /home/mgerhardy/dev/oss/engine/src/modules/ui/dearimgui/imgui.cpp:5015
    #9 0x557aa486aebe in ui::IMGUIApp::onRunning() /home/mgerhardy/dev/oss/engine/src/modules/ui/IMGUIApp.cpp:597
    #10 0x557aa3d06951 in VoxEdit::onRunning() /home/mgerhardy/dev/oss/engine/src/tools/voxedit/VoxEdit.cpp:545
    #11 0x557aa65df29c in app::App::onFrame() /home/mgerhardy/dev/oss/engine/src/modules/app/App.cpp:303
    #12 0x557aa65dc3aa in app::App::startMainLoop(int, char**) /home/mgerhardy/dev/oss/engine/src/modules/app/App.cpp:164
    #13 0x557aa3d06ff1 in main /home/mgerhardy/dev/oss/engine/src/tools/voxedit/VoxEdit.cpp:574
    #14 0x7f37db6456c9 in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58

previously allocated by thread T10 here:
    #0 0x7f37dc6f3bd7 in malloc ../../../../src/libsanitizer/asan/asan_malloc_linux.cpp:69
    #1 0x7f37dc4ba706  (/lib/x86_64-linux-gnu/libSDL2-2.0.so.0+0xa5706) (BuildId: 0fde3d17eb240615a6c99354e493de4c6dea2d72)
    #2 0x557aa4a00ee3 in ImGui::MemAlloc(unsigned long) /home/mgerhardy/dev/oss/engine/src/modules/ui/dearimgui/imgui.cpp:4300
    #3 0x557aa63fddd5 in ImGuiTestEngine_FindItemInfo(ImGuiTestEngine*, unsigned int, char const*) /home/mgerhardy/dev/oss/engine/src/modules/ui/dearimgui/imgui_test_engine/imgui_te_engine.cpp:450
    #4 0x557aa63b305f in ImGuiTestContext::ItemInfo(ImGuiTestRef, int) /home/mgerhardy/dev/oss/engine/src/modules/ui/dearimgui/imgui_test_engine/imgui_te_context.cpp:992
    #5 0x557aa63d8760 in ImGuiTestContext::ItemDragAndDrop(ImGuiTestRef, ImGuiTestRef, int) /home/mgerhardy/dev/oss/engine/src/modules/ui/dearimgui/imgui_test_engine/imgui_te_context.cpp:3013
    #6 0x557aa3e19e30 in operator() /home/mgerhardy/dev/oss/engine/src/tools/voxedit/modules/voxedit-ui/tests/AssetPanelTest.cpp:27
    #7 0x557aa3e1b03e in __invoke_impl<void, voxedit::AssetPanel::registerUITests(ImGuiTestEngine*, char const*)::<lambda(ImGuiTestContext*)>&, ImGuiTestContext*> /usr/include/c++/13/bits/invoke.h:61
    #8 0x557aa3e1ac49 in __invoke_r<void, voxedit::AssetPanel::registerUITests(ImGuiTestEngine*, char const*)::<lambda(ImGuiTestContext*)>&, ImGuiTestContext*> /usr/include/c++/13/bits/invoke.h:111
    #9 0x557aa3e1a95c in _M_invoke /usr/include/c++/13/bits/std_function.h:290
    #10 0x557aa6439256 in std::function<void (ImGuiTestContext*)>::operator()(ImGuiTestContext*) const (/home/mgerhardy/dev/oss/engine/build/voxedit/vengi-voxedit+0x86ff256) (BuildId: aa8b0276cbe3a526c136e6b69891c16b23fd7758)
    #11 0x557aa64123b3 in ImGuiTestEngine_RunTest(ImGuiTestEngine*, ImGuiTestContext*, ImGuiTest*, int) /home/mgerhardy/dev/oss/engine/src/modules/ui/dearimgui/imgui_test_engine/imgui_te_engine.cpp:1625
    #12 0x557aa64093b4 in ImGuiTestEngine_ProcessTestQueue /home/mgerhardy/dev/oss/engine/src/modules/ui/dearimgui/imgui_test_engine/imgui_te_engine.cpp:1092
    #13 0x557aa6406623 in ImGuiTestEngine_TestQueueCoroutineMain /home/mgerhardy/dev/oss/engine/src/modules/ui/dearimgui/imgui_test_engine/imgui_te_engine.cpp:909
    #14 0x557aa64c171b in CoroutineThreadMain /home/mgerhardy/dev/oss/engine/src/modules/ui/dearimgui/imgui_test_engine/imgui_te_coroutine.cpp:57
    #15 0x557aa64c69f1 in void std::__invoke_impl<void, void (*)(Coroutine_ImplStdThreadData*, void (*)(void*), void*), Coroutine_ImplStdThreadData*, void (*)(void*), void*>(std::__invoke_other, void (*&&)(Coroutine_ImplStdThreadData*, void (*)(void*), void*), Coroutine_ImplStdThreadData*&&, void (*&&)(void*), void*&&) /usr/include/c++/13/bits/invoke.h:61
    #16 0x557aa64c66e2 in std::__invoke_result<void (*)(Coroutine_ImplStdThreadData*, void (*)(void*), void*), Coroutine_ImplStdThreadData*, void (*)(void*), void*>::type std::__invoke<void (*)(Coroutine_ImplStdThreadData*, void (*)(void*), void*), Coroutine_ImplStdThreadData*, void (*)(void*), void*>(void (*&&)(Coroutine_ImplStdThreadData*, void (*)(void*), void*), Coroutine_ImplStdThreadData*&&, void (*&&)(void*), void*&&) (/home/mgerhardy/dev/oss/engine/build/voxedit/vengi-voxedit+0x878c6e2) (BuildId: aa8b0276cbe3a526c136e6b69891c16b23fd7758)
    #17 0x557aa64c6277 in void std::thread::_Invoker<std::tuple<void (*)(Coroutine_ImplStdThreadData*, void (*)(void*), void*), Coroutine_ImplStdThreadData*, void (*)(void*), void*> >::_M_invoke<0ul, 1ul, 2ul, 3ul>(std::_Index_tuple<0ul, 1ul, 2ul, 3ul>) (/home/mgerhardy/dev/oss/engine/build/voxedit/vengi-voxedit+0x878c277) (BuildId: aa8b0276cbe3a526c136e6b69891c16b23fd7758)
    #18 0x557aa64c5f8b in std::thread::_Invoker<std::tuple<void (*)(Coroutine_ImplStdThreadData*, void (*)(void*), void*), Coroutine_ImplStdThreadData*, void (*)(void*), void*> >::operator()() (/home/mgerhardy/dev/oss/engine/build/voxedit/vengi-voxedit+0x878bf8b) (BuildId: aa8b0276cbe3a526c136e6b69891c16b23fd7758)
    #19 0x557aa64c5f43 in std::thread::_State_impl<std::thread::_Invoker<std::tuple<void (*)(Coroutine_ImplStdThreadData*, void (*)(void*), void*), Coroutine_ImplStdThreadData*, void (*)(void*), void*> > >::_M_run() (/home/mgerhardy/dev/oss/engine/build/voxedit/vengi-voxedit+0x878bf43) (BuildId: aa8b0276cbe3a526c136e6b69891c16b23fd7758)
    #20 0x7f37dc0dee63  (/lib/x86_64-linux-gnu/libstdc++.so.6+0xdee63) (BuildId: 332d8df09457200979fca5abf449dd4629a19f78)

Thread T10 created by T0 here:
    #0 0x7f37dc6ebaf1 in pthread_create ../../../../src/libsanitizer/asan/asan_interceptors.cpp:245
    #1 0x7f37dc0def38 in std::thread::_M_start_thread(std::unique_ptr<std::thread::_State, std::default_delete<std::thread::_State> >, void (*)()) (/lib/x86_64-linux-gnu/libstdc++.so.6+0xdef38) (BuildId: 332d8df09457200979fca5abf449dd4629a19f78)
    #2 0x557aa64c1dba in Coroutine_ImplStdThread_Create /home/mgerhardy/dev/oss/engine/src/modules/ui/dearimgui/imgui_test_engine/imgui_te_coroutine.cpp:77
    #3 0x557aa63fb4e3 in ImGuiTestEngine_Start(ImGuiTestEngine*, ImGuiContext*) /home/mgerhardy/dev/oss/engine/src/modules/ui/dearimgui/imgui_test_engine/imgui_te_engine.cpp:258
    #4 0x557aa4865cea in ui::IMGUIApp::onInit() /home/mgerhardy/dev/oss/engine/src/modules/ui/IMGUIApp.cpp:324
    #5 0x557aa3d04587 in VoxEdit::onInit() /home/mgerhardy/dev/oss/engine/src/tools/voxedit/VoxEdit.cpp:454
    #6 0x557aa65deb42 in app::App::onFrame() /home/mgerhardy/dev/oss/engine/src/modules/app/App.cpp:281
    #7 0x557aa65dc3aa in app::App::startMainLoop(int, char**) /home/mgerhardy/dev/oss/engine/src/modules/app/App.cpp:164
    #8 0x557aa3d06ff1 in main /home/mgerhardy/dev/oss/engine/src/tools/voxedit/VoxEdit.cpp:574
    #9 0x7f37db6456c9 in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58

SUMMARY: AddressSanitizer: heap-use-after-free /home/mgerhardy/dev/oss/engine/src/modules/ui/dearimgui/imgui_test_engine/imgui_te_context.cpp:3032 in ImGuiTestContext::ItemDragAndDrop(ImGuiTestRef, ImGuiTestRef, int)
Shadow bytes around the buggy address:
  0x50f00004c380: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x50f00004c400: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x50f00004c480: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x50f00004c500: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x50f00004c580: fa fa fa fa fa fa fa fa fa fa fd fd fd fd fd fd
=>0x50f00004c600: fd fd fd[fd]fd fd fd fd fd fd fd fd fd fd fd fd
  0x50f00004c680: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x50f00004c700: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x50f00004c780: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x50f00004c800: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x50f00004c880: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
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
==154641==ABORTING
FAILED: src/tools/voxedit/CMakeFiles/voxedit-run /home/mgerhardy/dev/oss/engine/build/src/tools/voxedit/CMakeFiles/voxedit-run 
cd /home/mgerhardy/dev/oss/engine/build/voxedit && /home/mgerhardy/dev/oss/engine/build/voxedit/vengi-voxedit
ninja: build stopped: subcommand failed.
make: *** [Makefile:179: voxedit-run] Fehler 1

https://github.com/vengi-voxel/vengi/issues/454

ocornut commented 2 months ago

Thank you! I pushed the fix b7e8d66 and I am now investigating how to better detect those issues and sturdy code against them.

ocornut commented 2 months ago

Following this I did the aggressive move of pushing 7bcdcdb d905355 & co. aka

- TestEngine: *EXCEPTIONALLY BREAKING CHANGE*
  Changed ItemInfo(), WindowInfo() and variants signatures to return
    a full ImGuiTestItemInfo struct instead of a pointer.
    Before:
      ImGuiTestItemInfo*  ItemInfo(ImGuiTestRef ref, ....);
      ImGuiTestItemInfo*  WindowInfo(ImGuiTestRef window_ref, ...);
    After:
      ImGuiTestItemInfo   ItemInfo(ImGuiTestRef ref, ....);
      ImGuiTestItemInfo   WindowInfo(ImGuiTestRef window_ref, ...);
  - The initial design suggested that the data could be updated on subsequent
    frames, but it technically required to manipulate the RefCount field and it was
    very easy to get wrong and mistakes were difficult to notice and debug,
    occasionally leading to dangling pointers.
  - We are instead returning a copy of the value at the current time.
    You may call ItemInfo()/WindowInfo() again if you need refreshed values.
       item = ItemInfo(item.ID); // refresh values
  - If you were using info->RefCount++/info->RefCount-- is likely means that
    you wanted to reuse an ItemInfo pointers accross frames and might
    want to request the refreshed data.
  - Removed the ImGuiTestItemInfo::RefCount field since it is now unused,
    and its removal makes it easier to spot cases that might need fixing.
  APOLOGIES FOR THE BREAKING CHANGE.
  PLEASE CONTACT US IF YOU HAVE ISSUES OR FEEDBACK OR ANY QUESTION.

This will prevent this class of issue and I think the design makes better sense.

I'd be interested in your feedback about the breakage incurred.

mgerhardy commented 2 months ago

Thank you very much for the fix. Works fine for me now.

The breakage wasn't a big deal - I had a few places where I've used WindowInfo() for child id resolving.