[reactos] 01/01: [SHELL32] Do not return -1 if a file is not valid or not found - ExtractIconEx() (#2113)

[Bișoc George]

https://git.reactos.org/?p=reactos.git;a=commitdiff;h=8ec05f5fa1108a50f378e5...

commit 8ec05f5fa1108a50f378e57b76b5015ab7613ffe
Author:     Bișoc George [e-mail]
AuthorDate: Thu Dec 12 00:24:20 2019 +0100
Commit:     Katayama Hirofumi MZ [e-mail]
CommitDate: Thu Dec 12 08:24:20 2019 +0900

[SHELL32] Do not return -1 if a file is not valid or not found - ExtractIconEx() (#2113)

ExtractIconEx() returns the number of successfully extracted icons from a file. The routine may return 0 in case no icons could be extracted but it could also return 0 if the file is not a valid PE image file or the file couldn't be found.

PrivateExtractIcons and the internal USER32 routine, ICO_ExtractIconExW(), return -1 in such scenarios. The behaviour is correct however we do not want that ExtractIconEx() returns -1 as well as it doesn't comply with the general documentation. In such cases, simply return 0 as no successful icons have been extracted due to related file failures.

CORE-16535
---
 dll/win32/shell32/iconcache.cpp                    | 20 +++++++++---
 modules/rostests/apitests/shell32/CMakeLists.txt   |  1 +
 .../rostests/apitests/shell32/ExtractIconEx.cpp    | 37 ++++++++++++++++++++++
 modules/rostests/apitests/shell32/testlist.c       |  2 ++
 4 files changed, 56 insertions(+), 4 deletions(-)

diff --git a/dll/win32/shell32/iconcache.cpp b/dll/win32/shell32/iconcache.cpp
index 400f75cc456..5b35326a2b7 100644
--- a/dll/win32/shell32/iconcache.cpp
+++ b/dll/win32/shell32/iconcache.cpp
@@ -866,12 +866,13 @@ EXTERN_C INT WINAPI Shell_GetCachedImageIndex(LPCWSTR szPath, INT nIndex, UINT b
 /*************************************************************************
  * ExtractIconExW            [SHELL32.@]
  * RETURNS
- *  0 no icon found
- *  -1 file is not valid
+ *  0 no icon found (or the file is not valid)
  *  or number of icons extracted
  */
 UINT WINAPI ExtractIconExW(LPCWSTR lpszFile, INT nIconIndex, HICON * phiconLarge, HICON * phiconSmall, UINT nIcons)
 {
+    UINT ret = 0;
+
     /* get entry point of undocumented function PrivateExtractIconExW() in user32 */
 #if defined(__CYGWIN__) || defined (__MINGW32__) || defined(_MSC_VER)
     static UINT (WINAPI*PrivateExtractIconExW)(LPCWSTR,int,HICON*,HICON*,UINT) = NULL;
@@ -881,13 +882,24 @@ UINT WINAPI ExtractIconExW(LPCWSTR lpszFile, INT nIconIndex, HICON * phiconLarge
         PrivateExtractIconExW = (UINT(WINAPI*)(LPCWSTR,int,HICON*,HICON*,UINT)) GetProcAddress(hUser32, "PrivateExtractIconExW");

if (!PrivateExtractIconExW)
-        return 0;
+        return ret;
     }
 #endif

TRACE("%s %i %p %p %i\n", debugstr_w(lpszFile), nIconIndex, phiconLarge, phiconSmall, nIcons);
+    ret = PrivateExtractIconExW(lpszFile, nIconIndex, phiconLarge, phiconSmall, nIcons);

-    return PrivateExtractIconExW(lpszFile, nIconIndex, phiconLarge, phiconSmall, nIcons);
+    /* PrivateExtractIconExW() may return -1 if the provided file is not a valid PE image file or the said
+     * file couldn't be found. The behaviour is correct although ExtractIconExW() only returns the successfully
+     * extracted icons from a file. In such scenario, simply return 0.
+    */
+    if (ret == 0xFFFFFFFF)
+    {
+        WARN("Invalid file or couldn't be found - %s\n", debugstr_w(lpszFile));
+        ret = 0;
+    }
+
+    return ret;
 }

/*************************************************************************
diff --git a/modules/rostests/apitests/shell32/CMakeLists.txt b/modules/rostests/apitests/shell32/CMakeLists.txt
index f1cd11fefaa..22dbadf9dfb 100644
--- a/modules/rostests/apitests/shell32/CMakeLists.txt
+++ b/modules/rostests/apitests/shell32/CMakeLists.txt
@@ -18,6 +18,7 @@ list(APPEND SOURCE
     CUserNotification.cpp
     Control_RunDLLW.cpp
     DragDrop.cpp
+    ExtractIconEx.cpp
     IShellFolderViewCB.cpp
     OpenAs_RunDLL.cpp
     PathResolve.cpp
diff --git a/modules/rostests/apitests/shell32/ExtractIconEx.cpp b/modules/rostests/apitests/shell32/ExtractIconEx.cpp
new file mode 100644
index 00000000000..721c749350b
--- /dev/null
+++ b/modules/rostests/apitests/shell32/ExtractIconEx.cpp
@@ -0,0 +1,37 @@
+/*
+ * PROJECT:     ReactOS API Tests
+ * LICENSE:     GPL-2.0-or-later (https://spdx.org/licenses/GPL-2.0-or-later)
+ * PURPOSE:     Tests for ExtractIconEx routine
+ * COPYRIGHT:   Copyright 2019 Bișoc George (fraizeraust99 at gmail dot com)
+ */
+
+#include "shelltest.h"
+
+typedef struct
+{
+    PCWSTR pszFilePath;
+    UINT nIcons;
+} EXTRACTICONTESTS;
+
+EXTRACTICONTESTS IconTests[] =
+{
+    /* Executable file with icon */
+    {L"%SystemRoot%\System32\cmd.exe", 1},
+
+    /* Executable file without icon */
+    {L"%SystemRoot%\System32\autochk.exe", 0},
+
+    /* Non-existing files */
+    {L"%SystemRoot%\non-existent-file.sdf", 0}
+};
+
+START_TEST(ExtractIconEx)
+{
+    UINT i, nReturnedIcons;
+
+    for (i = 0; i < _countof(IconTests); ++i)
+    {
+        nReturnedIcons = ExtractIconExW(IconTests[i].pszFilePath, 0, NULL, NULL, IconTests[i].nIcons);
+        ok(nReturnedIcons == IconTests[i].nIcons, "ExtractIconExW(%u): Expected %u icons, got %u\n", i, IconTests[i].nIcons, nReturnedIcons);
+    }
+}
diff --git a/modules/rostests/apitests/shell32/testlist.c b/modules/rostests/apitests/shell32/testlist.c
index a274688b0f5..18939f998e3 100644
--- a/modules/rostests/apitests/shell32/testlist.c
+++ b/modules/rostests/apitests/shell32/testlist.c
@@ -12,6 +12,7 @@ extern void func_CShellDesktop(void);
 extern void func_CShellLink(void);
 extern void func_CUserNotification(void);
 extern void func_DragDrop(void);
+extern void func_ExtractIconEx(void);
 extern void func_IShellFolderViewCB(void);
 extern void func_menu(void);
 extern void func_OpenAs_RunDLL(void);
@@ -35,6 +36,7 @@ const struct test winetest_testlist[] =
     { "CShellLink", func_CShellLink },
     { "CUserNotification", func_CUserNotification },
     { "DragDrop", func_DragDrop },
+    { "ExtractIconEx", func_ExtractIconEx },
     { "IShellFolderViewCB", func_IShellFolderViewCB },
     { "menu", func_menu },
     { "OpenAs_RunDLL", func_OpenAs_RunDLL },