[PATCH] Fix GDI bitmap handle leak in Explorer context menus

TortoiseSVN-dev <[email protected]>
Newsgroups gmane.comp.version-control.subversion.tortoisesvn.devel
Message-ID <CA+_7EL2VC3G_n465HL6+yGj98-kqYiea4VsvNKZg3nfP9NCYXQ@mail.gmail.com> 
TortoiseSVN creates a new `HBITMAP` for each classic Explorer context-menu
icon via `IconBitmapUtils::IconToBitmapPARGB32()`. The handles are assigned
to `MENUITEMINFO::hbmpItem`, but they are not owned by the menu and are
never released by either `IconBitmapUtils` or `CShellExt`.

The same context-menu code path in TortoiseGit was reproduced at runtime
with ETW and Explorer crash dumps:

- Explorer terminated in `explorer!CTray::CheckGDIHandleLimit` with
exception `0xc0000409`.
- The final dump contained 8,945 GDI handles, including 8,347
bitmap/SURFACE handles.
- Across 40 classic context-menu opens, the Explorer GDI count increased by
169.
- 160 of 177 outstanding type-5 bitmap creation stacks included
`TortoiseGit.dll`.
- Disabling context-menu icons stopped the growth.

TortoiseGit tracking issue with the reproduction details:
https://gitlab.com/tortoisegit/tortoisegit/-/work_items/4278

TortoiseSVN 1.14.9 and current trunk r29822 contain the equivalent
allocation and ownership pattern. I have not installed TortoiseSVN on the
affected machine, so the TortoiseSVN-specific runtime reproduction is not
claimed here; the patch addresses the directly observable handle-lifetime
defect in the source.

The attached patch:

- tracks successfully created menu-item `HBITMAP` handles on `CShellExt`;
- releases them with `DeleteObject()` when the shell-extension object is
destroyed;
- avoids creating classic-menu bitmaps when no `HMENU` is supplied for the
modern Explorer command path;
- avoids setting `MIIM_BITMAP` when no bitmap is used.

Verification performed:

- based on TortoiseSVN trunk r29822;
- changed lines formatted with the repository `.clang-format` using
clang-format 15;
- unified diff checked by reversing and reapplying it against a clean
sparse SVN working copy;
- full Windows/MSVC build not run because the available isolated Docker
environment is Linux and cannot build the Windows ATL/MFC shell extension.

Microsoft's menu-item bitmap example explicitly frees application-created
bitmap resources with `DeleteObject()`:
https://learn.microsoft.com/en-us/windows/win32/menurc/using-menus

Please review the ownership lifetime and, if possible, run the existing
context-menu GDI-handle test against a TortoiseSVN build with this patch.

-- 
You received this message because you are subscribed to the Google Groups "TortoiseSVN-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [email protected].
To view this discussion visit https://groups.google.com/d/msgid/tortoisesvn-dev/CA%2B_7EL2VC3G_n465HL6%2ByGj98-kqYiea4VsvNKZg3nfP9NCYXQ%40mail.gmail.com.
TortoiseSVN-GDI-menu-bitmap-leak-r29822.patch (text/plain, 5.3 KB) 
Index: src/TortoiseShell/ContextMenu.cpp
===================================================================
--- src/TortoiseShell/ContextMenu.cpp	(revision 29822)
+++ src/TortoiseShell/ContextMenu.cpp	(working copy)
@@ -770,6 +770,14 @@
     return S_OK;
 }
 
+HBITMAP CShellExt::CreateMenuBitmap(UINT icon)
+{
+    auto bitmap = m_iconBitmapUtils.IconToBitmapPARGB32(g_hResInst, icon);
+    if (bitmap)
+        m_menuBitmaps.push_back(bitmap);
+    return bitmap;
+}
+
 void CShellExt::InsertSVNMenu(BOOL isTop, HMENU menu, UINT pos, UINT_PTR id, UINT stringId, UINT icon, UINT idCmdFirst, SVNCommands com, const std::wstring& verb)
 {
     wchar_t menuTextBuffer[255] = {0};
@@ -805,11 +813,14 @@
 
     MENUITEMINFO menuItemInfo = {0};
     menuItemInfo.cbSize       = sizeof(menuItemInfo);
-    menuItemInfo.fMask        = MIIM_FTYPE | MIIM_ID | MIIM_BITMAP | MIIM_STRING;
+    menuItemInfo.fMask        = MIIM_FTYPE | MIIM_ID | MIIM_STRING;
     menuItemInfo.fType        = MFT_STRING;
     menuItemInfo.dwTypeData   = menuTextBuffer;
-    if (icon)
-        menuItemInfo.hbmpItem = m_iconBitmapUtils.IconToBitmapPARGB32(g_hResInst, icon);
+    if (icon && menu)
+    {
+        menuItemInfo.fMask |= MIIM_BITMAP;
+        menuItemInfo.hbmpItem = CreateMenuBitmap(icon);
+    }
     menuItemInfo.wID = static_cast<UINT>(id);
     if (menu)
         InsertMenuItem(menu, pos, TRUE, &menuItemInfo);
@@ -1276,15 +1287,14 @@
         myIDMap[idCmd - idCmdFirst] = ShellSubMenu;
         myIDMap[idCmd]              = ShellSubMenu;
     }
-    HBITMAP bmp        = nullptr;
-
-    menuItemInfo.fMask = MIIM_FTYPE | MIIM_ID | MIIM_SUBMENU | MIIM_DATA | MIIM_BITMAP | MIIM_STRING;
-    if (bShowIcons)
-        menuItemInfo.hbmpItem = m_iconBitmapUtils.IconToBitmapPARGB32(g_hResInst, uIcon);
-    menuItemInfo.hbmpChecked   = bmp;
-    menuItemInfo.hbmpUnchecked = bmp;
-    menuItemInfo.hSubMenu      = subMenu;
-    menuItemInfo.wID           = idCmd++;
+    menuItemInfo.fMask = MIIM_FTYPE | MIIM_ID | MIIM_SUBMENU | MIIM_DATA | MIIM_STRING;
+    if (uIcon && hMenu)
+    {
+        menuItemInfo.fMask |= MIIM_BITMAP;
+        menuItemInfo.hbmpItem = CreateMenuBitmap(uIcon);
+    }
+    menuItemInfo.hSubMenu = subMenu;
+    menuItemInfo.wID      = idCmd++;
     if (hMenu)
     {
         InsertMenuItem(hMenu, indexMenu++, TRUE, &menuItemInfo);
@@ -2531,15 +2541,15 @@
     {
         MENUITEMINFO menuiteminfo = {0};
         menuiteminfo.cbSize       = sizeof(menuiteminfo);
-        menuiteminfo.fMask        = MIIM_FTYPE | MIIM_ID | MIIM_SUBMENU | MIIM_DATA | MIIM_BITMAP | MIIM_STRING;
+        menuiteminfo.fMask        = MIIM_FTYPE | MIIM_ID | MIIM_SUBMENU | MIIM_DATA | MIIM_STRING;
         menuiteminfo.fType        = MFT_STRING;
-        HBITMAP bmp               = m_iconBitmapUtils.IconToBitmapPARGB32(g_hResInst, icon);
-        if (icon)
-            menuiteminfo.hbmpItem = bmp;
-        menuiteminfo.hbmpChecked   = bmp;
-        menuiteminfo.hbmpUnchecked = bmp;
-        menuiteminfo.hSubMenu      = ignoreSubMenu;
-        menuiteminfo.wID           = idCmd;
+        if (icon && hMenu)
+        {
+            menuiteminfo.fMask |= MIIM_BITMAP;
+            menuiteminfo.hbmpItem = CreateMenuBitmap(icon);
+        }
+        menuiteminfo.hSubMenu = ignoreSubMenu;
+        menuiteminfo.wID      = idCmd;
         SecureZeroMemory(stringTableBuffer, sizeof(stringTableBuffer));
         if (itemStates & ITEMIS_IGNORED)
             GetMenuTextFromResource(ShellMenuUnIgnoreSub);
Index: src/TortoiseShell/ShellExt.cpp
===================================================================
--- src/TortoiseShell/ShellExt.cpp	(revision 29822)
+++ src/TortoiseShell/ShellExt.cpp	(working copy)
@@ -58,6 +58,10 @@
 
 CShellExt::~CShellExt()
 {
+    // Destroying a menu does not release the bitmaps assigned to its items.
+    for (const auto bitmap : m_menuBitmaps)
+        ::DeleteObject(bitmap);
+
     AutoLocker lock(g_csGlobalComGuard);
     InterlockedDecrement(&g_cRefThisDll);
     g_shellObjects.Erase(this);
Index: src/TortoiseShell/ShellExt.h
===================================================================
--- src/TortoiseShell/ShellExt.h	(revision 29822)
+++ src/TortoiseShell/ShellExt.h	(working copy)
@@ -225,9 +225,11 @@
     CString                                               columnFolder;    ///< current folder of ColumnProvider
     std::vector<std::pair<std::wstring, std::string>>     columnUserProps; ///< user properties of ColumnProvider
     std::vector<Microsoft::WRL::ComPtr<CExplorerCommand>> m_explorerCommands;
+    std::vector<HBITMAP>                                  m_menuBitmaps;
 
 #define MAKESTRING(ID) LoadStringEx(g_hResInst, ID, stringTableBuffer, _countof(stringTableBuffer), (WORD)CRegStdDWORD(L"Software\\TortoiseSVN\\LanguageID", MAKELANGID(LANG_NEUTRAL, SUBLANG_DEFAULT)))
 private:
+    HBITMAP             CreateMenuBitmap(UINT icon);
     void                InsertSVNMenu(BOOL isTop, HMENU menu, UINT pos, UINT_PTR id, UINT stringId, UINT icon, UINT idCmdFirst, SVNCommands com, const std::wstring& verb);
     void                InsertIgnoreSubmenus(UINT& idCmd, UINT idCmdFirst, HMENU hMenu, HMENU subMenu, UINT& indexMenu, int& indexSubMenu, unsigned __int64 topMenu, bool bShowIcons);
     static std::wstring WriteFileListToTempFile(const std::vector<std::wstring>& files, const std::wstring folder);
lmpx.com only provides a reader for public news (NNTP) servers. It is not affiliated with the servers or forums shown here and is not responsible for the content of articles, which is written by their respective authors.