Viewports support crash with multiple instances of Dear Imgui Apps

[sammyfreg]

Version/Branch of Dear ImGui:

Version 1.91.5 Branch: docking

Back-ends:

imgui_impl_dx11, imgui_impl_win32

Compiler, OS:

MSVC 2022

Full config/build information:

Dear ImGui 1.91.5 (19150)
--------------------------------
sizeof(size_t): 8, sizeof(ImDrawIdx): 2, sizeof(ImDrawVert): 20
define: __cplusplus=199711
define: _WIN32
define: _WIN64
define: _MSC_VER=1934
define: _MSVC_LANG=201402
define: IMGUI_HAS_VIEWPORT
define: IMGUI_HAS_DOCK
--------------------------------
io.BackendPlatformName: imgui_impl_win32
io.BackendRendererName: imgui_impl_dx11
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,64
io.DisplaySize: 1264.00,761.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 Issue/Question:

It seems that the current implementation of using SetPropA/GetPropA to store some pointers (IMGUI_CONTEXT, IMGUI_VIEWPORT) on win32 and glfw, doesn't take into account that there can be more than 1 Dear ImGui applications running. This cause read access violation when a process tries reading memory belonging to the 2nd process.

I believe that we should either use a unique string per process when using GetPropA/SetPropA or validate that the windows returned by '::WindowFromPoint' belongs to the process.

Screenshots/Video:

Minimal, Complete and Verifiable Example code:

Compile one of the win32 sample, launch it from visual studio, then launch the same exe from the file system. Notice that having your mouse hovering the 2nd launched application will cause a crash in the non hovered app.

[ocornut]

Ouch. I didn’t realize GetProp() was not process bound.

[sammyfreg]

I can confirm that this simple change fixes the problem.

Adding at the beginning of imgui_impl_win32.cpp

// @SAMPLE_EDIT : Fix for multi application crash
#include <stdio.h> 
char HWND_PROP_VIEWPORT_KEY[64]={};
char HWND_PROP_CONTEXT_KEY[64]={};

Then add this code to ImGui_ImplWin32_InitEx()

// @SAMPLE_EDIT : Fix for multi application crash
snprintf(HWND_PROP_VIEWPORT_KEY, sizeof(HWND_PROP_VIEWPORT_KEY), "IMGUI_VIEWPORT_%016llX", (unsigned long long)hwnd);
snprintf(HWND_PROP_CONTEXT_KEY, sizeof(HWND_PROP_CONTEXT_KEY), "IMGUI_CONTEXT_%016llX", (unsigned long long)hwnd);

And finally replace all GetPropA/SetPropA to using the 2 global variables as keys.

I can create a pull request sometime this week if you would like.

[ocornut]

This was caused by fedf45c (#8069)

Fix was a little bit trickier than suggested: we can't easily the same technique for IMGUI_CONTEXT.

• our ImGui_ImplWin32_WndProcHandler_PlatformWindow handler is expected to work even when imgui context is not bound.
• we can allow multiple imgui contexts in same process, so cannot use process id.
• i don't want to depend on primary hwnd because we are expected to ditch that concept eventually.
• it's fine to use for IMGUI_CONTEXT since we only ever call GetProp() on Windows that we own.

For IMGUI_VIEWPORT i stopped using GetProp() and iterating viewports from the context/IO structure.

As expected #8069 is quite causing trouble but happy to experiment with those aspects in a backend.

[ocornut]

Fixed by dad1047