[PATCH 0/2] MR707: shell32: Return NULL from ExtractIcon() for all error cases.

[Brendan Shanks (＠bshanks)]

Windows documents ExtractIcon() as [returning 1 in the case of an invalid file path](https://docs.microsoft.com/en-us/windows/win32/api/shellapi/nf-shellapi-extr...), but from the tests this does not seem to be true. Return NULL for all error cases (invalid path or no icons in file).

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/707

[PATCH 1/2] shell32/tests: Test ExtractIcon() with an empty/invalid path.

[Brendan Shanks]

From: Brendan Shanks <bshanks(a)codeweavers.com>

---
 dlls/shell32/tests/shelllink.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/dlls/shell32/tests/shelllink.c b/dlls/shell32/tests/shelllink.c
index c8a0308a9cb..13dce3302c7 100644
--- a/dlls/shell32/tests/shelllink.c
+++ b/dlls/shell32/tests/shelllink.c
@@ -1174,6 +1174,7 @@ static void test_ExtractIcon(void)
 {
     static const WCHAR nameW[] = {'\\','e','x','t','r','a','c','t','i','c','o','n','_','t','e','s','t','.','t','x','t',0};
     static const WCHAR shell32W[] = {'s','h','e','l','l','3','2','.','d','l','l',0};
+    static const WCHAR emptyW[] = {0};
     WCHAR pathW[MAX_PATH];
     HICON hicon, hicon2;
     char path[MAX_PATH];
@@ -1223,6 +1224,14 @@ static void test_ExtractIcon(void)
     r = DeleteFileA(path);
     ok(r, "failed to delete file %s (%ld)\n", path, GetLastError());
 
+    /* Empty file path */
+    hicon = ExtractIconA(NULL, "", -1);
+    ok(hicon == NULL, "Got icon %p\n", hicon);
+
+    hicon = ExtractIconA(NULL, "", 0);
+    todo_wine
+    ok(hicon == NULL, "Got icon %p\n", hicon);
+
     /* same for W variant */
 if (0)
 {
@@ -1270,6 +1279,14 @@ if (0)
 
     r = DeleteFileW(pathW);
     ok(r, "failed to delete file %s (%ld)\n", path, GetLastError());
+
+    /* Empty file path */
+    hicon = ExtractIconW(NULL, emptyW, -1);
+    ok(hicon == NULL, "Got icon %p\n", hicon);
+
+    hicon = ExtractIconW(NULL, emptyW, 0);
+    todo_wine
+    ok(hicon == NULL, "Got icon %p\n", hicon);
 }
 
 static void test_ExtractAssociatedIcon(void)
-- 
GitLab


https://gitlab.winehq.org/wine/wine/-/merge_requests/707

[PATCH 2/2] shell32: Return NULL from ExtractIcon() for all error cases.

[Brendan Shanks]

From: Brendan Shanks <bshanks(a)codeweavers.com>

---
 dlls/shell32/shell32_main.c    | 11 ++++-------
 dlls/shell32/tests/shelllink.c |  7 -------
 2 files changed, 4 insertions(+), 14 deletions(-)

diff --git a/dlls/shell32/shell32_main.c b/dlls/shell32/shell32_main.c
index 212de7445a4..2aac022f9e4 100644
--- a/dlls/shell32/shell32_main.c
+++ b/dlls/shell32/shell32_main.c
@@ -606,16 +606,13 @@ HICON WINAPI ExtractIconW(HINSTANCE hInstance, LPCWSTR lpszFile, UINT nIconIndex
         ret = PrivateExtractIconsW(lpszFile, 0, cx, cy, NULL, NULL, 0, LR_DEFAULTCOLOR);
         if (ret != (UINT)-1 && ret)
             return (HICON)(UINT_PTR)ret;
-        return NULL;
     }
     else
+    {
         ret = PrivateExtractIconsW(lpszFile, nIconIndex, cx, cy, &hIcon, NULL, 1, LR_DEFAULTCOLOR);
-
-    if (ret == (UINT)-1)
-        return (HICON)1;
-    else if (ret > 0 && hIcon)
-        return hIcon;
-
+        if (ret > 0 && hIcon)
+            return hIcon;
+    }
     return NULL;
 }
 
diff --git a/dlls/shell32/tests/shelllink.c b/dlls/shell32/tests/shelllink.c
index 13dce3302c7..345484d9a5d 100644
--- a/dlls/shell32/tests/shelllink.c
+++ b/dlls/shell32/tests/shelllink.c
@@ -1185,7 +1185,6 @@ static void test_ExtractIcon(void)
 
     /* specified instance handle */
     hicon = ExtractIconA(GetModuleHandleA("shell32.dll"), NULL, 0);
-    todo_wine
     ok(hicon == NULL, "Got icon %p\n", hicon);
     hicon2 = ExtractIconA(GetModuleHandleA("shell32.dll"), "shell32.dll", -1);
     ok(hicon2 != NULL, "Got icon %p\n", hicon2);
@@ -1211,14 +1210,12 @@ static void test_ExtractIcon(void)
     CloseHandle(file);
 
     hicon = ExtractIconA(NULL, path, 0);
-    todo_wine
     ok(hicon == NULL, "Got icon %p\n", hicon);
 
     hicon = ExtractIconA(NULL, path, -1);
     ok(hicon == NULL, "Got icon %p\n", hicon);
 
     hicon = ExtractIconA(NULL, path, 1);
-    todo_wine
     ok(hicon == NULL, "Got icon %p\n", hicon);
 
     r = DeleteFileA(path);
@@ -1229,7 +1226,6 @@ static void test_ExtractIcon(void)
     ok(hicon == NULL, "Got icon %p\n", hicon);
 
     hicon = ExtractIconA(NULL, "", 0);
-    todo_wine
     ok(hicon == NULL, "Got icon %p\n", hicon);
 
     /* same for W variant */
@@ -1267,14 +1263,12 @@ if (0)
     CloseHandle(file);
 
     hicon = ExtractIconW(NULL, pathW, 0);
-    todo_wine
     ok(hicon == NULL, "Got icon %p\n", hicon);
 
     hicon = ExtractIconW(NULL, pathW, -1);
     ok(hicon == NULL, "Got icon %p\n", hicon);
 
     hicon = ExtractIconW(NULL, pathW, 1);
-    todo_wine
     ok(hicon == NULL, "Got icon %p\n", hicon);
 
     r = DeleteFileW(pathW);
@@ -1285,7 +1279,6 @@ if (0)
     ok(hicon == NULL, "Got icon %p\n", hicon);
 
     hicon = ExtractIconW(NULL, emptyW, 0);
-    todo_wine
     ok(hicon == NULL, "Got icon %p\n", hicon);
 }
 
-- 
GitLab

https://gitlab.winehq.org/wine/wine/-/merge_requests/707