From: Serge Gautherie Date: Tue, 05 Apr 2017 18:14:00 +0200 Subject: [SHELL32] iconcache.cpp: *A few rewrites. *Document SHELL32_SicCS var. Use it more or document why not. *Add/Improve some debug messages. *SIC_OverlayShortcutImage(): Merge duplicate code. *SIC_Initialize() failure code: Simply call SIC_Destroy() (which fixes (properly) destroying ShellBigIconList and sic_hdpa). *SIC_Destroy(): Review if()s. CORE-13033 Index: reactos/dll/win32/shell32/iconcache.cpp --- a/reactos/dll/win32/shell32/iconcache.cpp +++ b/reactos/dll/win32/shell32/iconcache.cpp @@ -36,12 +36,13 @@ } SIC_ENTRY, * LPSIC_ENTRY; static HDPA sic_hdpa = 0; - static HIMAGELIST ShellSmallIconList; static HIMAGELIST ShellBigIconList; namespace { +// NB: To account for SIC_Initialize() and SIC_Destroy(), +// always use SHELL32_SicCS to protect the internal SIC data structures. extern CRITICAL_SECTION SHELL32_SicCS; CRITICAL_SECTION_DEBUG critsect_debug = { @@ -90,15 +91,16 @@ */ static HICON SIC_OverlayShortcutImage(HICON SourceIcon, BOOL large) { + static int s_imgListIdx = INVALID_INDEX; + ICONINFO ShortcutIconInfo, TargetIconInfo; - HICON ShortcutIcon = NULL, TargetIcon; + HICON ShortcutIcon = NULL, TargetIcon = NULL; BITMAP TargetBitmapInfo, ShortcutBitmapInfo; HDC ShortcutDC = NULL, TargetDC = NULL; HBITMAP OldShortcutBitmap = NULL, OldTargetBitmap = NULL; - static int s_imgListIdx = -1; ZeroMemory(&ShortcutIconInfo, sizeof(ShortcutIconInfo)); ZeroMemory(&TargetIconInfo, sizeof(TargetIconInfo)); @@ -121,20 +123,29 @@ } /* search for the shortcut icon only once */ - if (s_imgListIdx == -1) + if (s_imgListIdx == INVALID_INDEX) + { + // NB: IDI_SHELL_SHORTCUT is assumed to be valid/permanent, + // thus not bothering to protect this block, + // nor to run it once only (in case of failure). s_imgListIdx = SIC_LoadOverlayIcon(- IDI_SHELL_SHORTCUT); /* FIXME should use icon index 29 instead of the resource id, but not all icons are present yet so we can't use icon indices */ + } - if (s_imgListIdx != -1) + if (s_imgListIdx != INVALID_INDEX) { - if (large) - ShortcutIcon = ImageList_GetIcon(ShellBigIconList, s_imgListIdx, ILD_TRANSPARENT); - else - ShortcutIcon = ImageList_GetIcon(ShellSmallIconList, s_imgListIdx, ILD_TRANSPARENT); - } else - ShortcutIcon = NULL; + EnterCriticalSection(&SHELL32_SicCS); + ShortcutIcon = ImageList_GetIcon( + (large ? ShellBigIconList : ShellSmallIconList), + s_imgListIdx, ILD_TRANSPARENT); + LeaveCriticalSection(&SHELL32_SicCS); + } + else + { + ERR("Failed to load '-IDI_SHELL_SHORTCUT' overlay icon.\n"); + } if (!ShortcutIcon || !GetIconInfo(ShortcutIcon, &ShortcutIconInfo)) { @@ -282,39 +293,24 @@ goto fail; } - /* Clean up, we're not goto'ing to 'fail' after this so we can be lazy and not set - handles to NULL */ - SelectObject(TargetDC, OldTargetBitmap); - DeleteDC(TargetDC); - SelectObject(ShortcutDC, OldShortcutBitmap); - DeleteDC(ShortcutDC); - /* Create the icon using the bitmaps prepared earlier */ TargetIcon = CreateIconIndirect(&TargetIconInfo); - /* CreateIconIndirect copies the bitmaps, so we can release our bitmaps now */ - DeleteObject(TargetIconInfo.hbmColor); - DeleteObject(TargetIconInfo.hbmMask); - /* Delete what GetIconInfo gave us */ - DeleteObject(ShortcutIconInfo.hbmColor); - DeleteObject(ShortcutIconInfo.hbmMask); - DestroyIcon(ShortcutIcon); - - return TargetIcon; - fail: /* Clean up scratch resources we created */ if (NULL != OldTargetBitmap) SelectObject(TargetDC, OldTargetBitmap); if (NULL != TargetDC) DeleteDC(TargetDC); if (NULL != OldShortcutBitmap) SelectObject(ShortcutDC, OldShortcutBitmap); if (NULL != ShortcutDC) DeleteDC(ShortcutDC); + /* CreateIconIndirect copies the bitmaps, so we can release our bitmaps now */ if (NULL != TargetIconInfo.hbmColor) DeleteObject(TargetIconInfo.hbmColor); if (NULL != TargetIconInfo.hbmMask) DeleteObject(TargetIconInfo.hbmMask); + /* Delete what GetIconInfo gave us */ if (NULL != ShortcutIconInfo.hbmColor) DeleteObject(ShortcutIconInfo.hbmColor); if (NULL != ShortcutIconInfo.hbmMask) DeleteObject(ShortcutIconInfo.hbmMask); if (NULL != ShortcutIcon) DestroyIcon(ShortcutIcon); - return NULL; + return TargetIcon; } /***************************************************************************** @@ -378,7 +374,9 @@ HeapFree(GetProcessHeap(), 0, lpsice->sSourceFile); SHFree(lpsice); } + LeaveCriticalSection(&SHELL32_SicCS); + return ret; } /**************************************************************************** @@ -453,11 +451,11 @@ sice.dwSourceIndex = dwSourceIndex; sice.dwFlags = dwFlags; + EnterCriticalSection(&SHELL32_SicCS); + if (!sic_hdpa) SIC_Initialize(); - EnterCriticalSection(&SHELL32_SicCS); - if (NULL != DPA_GetPtr (sic_hdpa, 0)) { /* search linear from position 0*/ @@ -475,6 +473,7 @@ } LeaveCriticalSection(&SHELL32_SicCS); + return ret; } @@ -493,15 +492,20 @@ TRACE("Entered SIC_Initialize\n"); + EnterCriticalSection(&SHELL32_SicCS); + if (sic_hdpa) { TRACE("Icon cache already initialized\n"); + LeaveCriticalSection(&SHELL32_SicCS); return TRUE; } sic_hdpa = DPA_Create(16); if (!sic_hdpa) { + ERR("Failed to create icon cache.\n"); + LeaveCriticalSection(&SHELL32_SicCS); return FALSE; } @@ -513,6 +517,7 @@ } bpp = GetDeviceCaps(hDC, BITSPIXEL); + DeleteDC(hDC); if (bpp <= 4) @@ -584,12 +589,13 @@ if(SIC_IconAppend(swShell32Name, IDI_SHELL_DOCUMENT-1, hSm, hLg, 0) == INVALID_INDEX) { - ERR("Failed to add IDI_SHELL_DOCUMENT icon to cache.\n"); + ERR("Failed to add 'IDI_SHELL_DOCUMENT - 1' icon to cache.\n"); goto end; } + if(SIC_IconAppend(swShell32Name, -IDI_SHELL_DOCUMENT, hSm, hLg, 0) == INVALID_INDEX) { - ERR("Failed to add IDI_SHELL_DOCUMENT icon to cache.\n"); + ERR("Failed to add '-IDI_SHELL_DOCUMENT' icon to cache.\n"); goto end; } @@ -603,25 +609,16 @@ /* Clean everything if something went wrong */ if(!result) - { - if(sic_hdpa) DPA_Destroy(sic_hdpa); - if(ShellSmallIconList) ImageList_Destroy(ShellSmallIconList); - if(ShellBigIconList) ImageList_Destroy(ShellSmallIconList); - sic_hdpa = NULL; - ShellSmallIconList = NULL; - ShellBigIconList = NULL; - } + SIC_Destroy(); TRACE("hIconSmall=%p hIconBig=%p\n",ShellSmallIconList, ShellBigIconList); + LeaveCriticalSection(&SHELL32_SicCS); + return result; } -/************************************************************************* - * SIC_Destroy - * - * frees the cache - */ +// Callback passed to DPA_DestroyCallback() (in SIC_Destroy()). static INT CALLBACK sic_free( LPVOID ptr, LPVOID lparam ) { HeapFree(GetProcessHeap(), 0, ((LPSIC_ENTRY)ptr)->sSourceFile); @@ -629,20 +626,35 @@ return TRUE; } +/************************************************************************* + * SIC_Destroy + * + * Frees the cache. + */ void SIC_Destroy(void) { - TRACE("\n"); + TRACE("Entered SIC_Destroy\n"); EnterCriticalSection(&SHELL32_SicCS); - if (sic_hdpa) DPA_DestroyCallback(sic_hdpa, sic_free, NULL ); + if (ShellBigIconList) + { + ImageList_Destroy(ShellBigIconList); + ShellBigIconList = NULL; + } - sic_hdpa = NULL; - ImageList_Destroy(ShellSmallIconList); - ShellSmallIconList = 0; - ImageList_Destroy(ShellBigIconList); - ShellBigIconList = 0; + if (ShellSmallIconList) + { + ImageList_Destroy(ShellSmallIconList); + ShellSmallIconList = NULL; + } + if (sic_hdpa) + { + DPA_DestroyCallback(sic_hdpa, sic_free, NULL); + sic_hdpa = NULL; + } + LeaveCriticalSection(&SHELL32_SicCS); //DeleteCriticalSection(&SHELL32_SicCS); //static } @@ -690,8 +702,10 @@ RegCloseKey(hKeyShellIcons); } + EnterCriticalSection(&SHELL32_SicCS); if (!sic_hdpa) SIC_Initialize(); + LeaveCriticalSection(&SHELL32_SicCS); return SIC_LoadIcon(iconPath, iconIdx, 0); } @@ -707,6 +721,8 @@ { TRACE("(%p,%p)\n",lpBigList,lpSmallList); + EnterCriticalSection(&SHELL32_SicCS); + if (!sic_hdpa) SIC_Initialize(); @@ -716,6 +732,8 @@ if (lpSmallList) *lpSmallList = ShellSmallIconList; + LeaveCriticalSection(&SHELL32_SicCS); + return TRUE; } /************************************************************************* @@ -745,8 +763,10 @@ TRACE("sf=%p pidl=%p %s\n", sh, pidl, bBigIcon?"Big":"Small"); + EnterCriticalSection(&SHELL32_SicCS); if (!sic_hdpa) SIC_Initialize(); + LeaveCriticalSection(&SHELL32_SicCS); if (SUCCEEDED (sh->GetUIObjectOf(0, 1, &pidl, IID_NULL_PPV_ARG(IExtractIconW, &ei)))) { @@ -877,11 +897,16 @@ static UINT (WINAPI*PrivateExtractIconExW)(LPCWSTR,int,HICON*,HICON*,UINT) = NULL; if (!PrivateExtractIconExW) { + // NB: user32+PrivateExtractIconExW is assumed to be valid/permanent, + // thus not bothering to protect this block, + // nor to run it once only (in case of failure). HMODULE hUser32 = GetModuleHandleA("user32"); PrivateExtractIconExW = (UINT(WINAPI*)(LPCWSTR,int,HICON*,HICON*,UINT)) GetProcAddress(hUser32, "PrivateExtractIconExW"); - if (!PrivateExtractIconExW) - return 0; + { + ERR("Failed to retrieve PrivateExtractIconExW() address (hUser32=%p).\n", hUser32); + return 0; + } } #endif