[PATCH v2 0/4] MR7993: shell32: Better handle error case for file operation.

[Ziqing Hui (＠zhui)]

--
  v2: shell32: Introduce parse_target_file_list helper.

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

[PATCH v2 1/4] shell32/tests: Add more wildcard target tests.

[Ziqing Hui]

From: Ziqing Hui

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

diff --git a/dlls/shell32/tests/shlfileop.c b/dlls/shell32/tests/shlfileop.c
index ff27ff06d42..3ec2757aca0 100644
--- a/dlls/shell32/tests/shlfileop.c
+++ b/dlls/shell32/tests/shlfileop.c
@@ -1457,6 +1457,28 @@ static void test_copy(void)
             ERROR_INVALID_NAME, FALSE, FALSE, FALSE);
     ok(!file_exists("two\aa.txt"), "Expected file to not exist\n");

+    check_file_operation(FO_COPY, FOF_NO_UI | FOF_MULTIDESTFILES,
+            "aa.txt\0", "one\0tw?\0",
+            ERROR_SUCCESS, FALSE, TRUE, FALSE);
+    todo_wine
+    ok(DeleteFileA("one\aa.txt"), "Expected file to exist\n");
+    ok(!DeleteFileA("two\aa.txt"), "Expected file to not exist\n");
+
+    check_file_operation(FO_COPY, FOF_NO_UI,
+            "aa.txt\0bb.txt\0", "one\0tw?\0",
+            ERROR_SUCCESS, FALSE, TRUE, FALSE);
+    todo_wine
+    ok(DeleteFileA("one\aa.txt"), "Expected file to exist\n");
+    todo_wine
+    ok(DeleteFileA("one\bb.txt"), "Expected file to exist\n");
+    ok(!DeleteFileA("two\bb.txt"), "Expected file to not exist\n");
+
+    check_file_operation(FO_COPY, FOF_NO_UI | FOF_MULTIDESTFILES,
+            "aa.txt\0bb.txt\0", "one\0tw?\0",
+            ERROR_INVALID_NAME, FALSE, FALSE, FALSE);
+    ok(!DeleteFileA("one\aa.txt"), "Expected file to not exist\n");
+    ok(!DeleteFileA("two\bb.txt"), "Expected file to not exist\n");
+
     ok(DeleteFileA("aa.txt"), "Expected file to exist\n");
     ok(DeleteFileA("ab.txt"), "Expected file to exist\n");
     ok(DeleteFileA("bb.txt"), "Expected file to exist\n");

[PATCH v2 2/4] shell32: Introduce has_wildcard helper.

[Ziqing Hui]

From: Ziqing Hui

---
 dlls/shell32/shlfileop.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/dlls/shell32/shlfileop.c b/dlls/shell32/shlfileop.c
index ed3c7f46743..fb8c73fa83a 100644
--- a/dlls/shell32/shlfileop.c
+++ b/dlls/shell32/shlfileop.c
@@ -1012,6 +1012,11 @@ static void file_list_destroy(FILE_LIST *flList)
     free(flList->feFiles);
 }

+static BOOL has_wildcard(const WCHAR *str)
+{
+    return !!wcspbrk(str, L"*?");
+}
+
 static LPWSTR wildcard_to_file(LPCWSTR szWildCard, LPCWSTR szFileName)
 {
     LPCWSTR ptr;
@@ -1074,7 +1079,7 @@ static HRESULT parse_file_list(FILE_LIST *flList, LPCWSTR szFiles, BOOL parse_wi

while (*ptr)
     {
-        BOOL from_wildcard = !!wcspbrk(ptr, L"*?");
+        BOOL from_wildcard = has_wildcard(ptr);

/* change relative to absolute path */
         if (PathIsRelativeW(ptr))
@@ -1198,7 +1203,7 @@ static DWORD do_copy(FILE_OPERATION *op, const FILE_ENTRY *from, const FILE_ENTR
         SHCreateDirectoryExW(NULL, target_dir, NULL);

/* Source contains wildcard. */
-    if (!!wcspbrk(from->szFullPath, L"*?"))
+    if (has_wildcard(from->szFullPath))
         return copy_wildcard(op, from, to);

/* Source is a dir. */

[PATCH v2 3/4] shell32: Handle invalid parameter correctly for SHFileOperationW.

[Ziqing Hui]

From: Ziqing Hui

---
 dlls/shell32/shlfileop.c       | 42 ++++++++++++++-----------------
 dlls/shell32/tests/shlfileop.c | 46 +++++++++++++++++-----------------
 2 files changed, 42 insertions(+), 46 deletions(-)

diff --git a/dlls/shell32/shlfileop.c b/dlls/shell32/shlfileop.c
index fb8c73fa83a..c330a94b3b1 100644
--- a/dlls/shell32/shlfileop.c
+++ b/dlls/shell32/shlfileop.c
@@ -1056,25 +1056,17 @@ static void parse_wildcard_files(FILE_LIST *file_list, LPCWSTR szFile)
 }

/* takes the null-separated file list and fills out the FILE_LIST */
-static HRESULT parse_file_list(FILE_LIST *flList, LPCWSTR szFiles, BOOL parse_wildcard)
+static void parse_file_list(FILE_LIST *flList, LPCWSTR szFiles, BOOL parse_wildcard)
 {
     LPCWSTR ptr = szFiles;
     WCHAR szCurFile[MAX_PATH];
     WCHAR *p;

-    if (!szFiles)
-        return ERROR_INVALID_PARAMETER;
-
     flList->bAnyFromWildcard = FALSE;
     flList->bAnyDirectories = FALSE;
     flList->bAnyDontExist = FALSE;
     flList->num_alloc = 32;
     flList->dwNumFiles = 0;
-
-    /* empty list */
-    if (!szFiles[0])
-        return ERROR_ACCESS_DENIED;
-
     flList->feFiles = calloc(flList->num_alloc, sizeof(FILE_ENTRY));

while (*ptr)
@@ -1103,8 +1095,6 @@ static HRESULT parse_file_list(FILE_LIST *flList, LPCWSTR szFiles, BOOL parse_wi
         /* advance to the next string */
         ptr += lstrlenW(ptr) + 1;
     }
-
-    return S_OK;
 }

static void create_dest_dirs(LPCWSTR szDestDir)
@@ -1133,8 +1123,7 @@ static DWORD copy_wildcard(FILE_OPERATION *op, const FILE_ENTRY *from, const FIL
     DWORD i, ret;

wcscpy(buffer, from->szFullPath);
-    if ((ret = parse_file_list(&from_files, buffer, TRUE)) != ERROR_SUCCESS)
-        return ret;
+    parse_file_list(&from_files, buffer, TRUE);

for (i = 0; i < from_files.dwNumFiles; ++i)
     {
@@ -1524,24 +1513,32 @@ int WINAPI SHFileOperationW(LPSHFILEOPSTRUCTW lpFileOp)
     FILE_LIST flFrom, flTo;
     int ret = 0;

-    if (!lpFileOp)
+    if (!lpFileOp || !lpFileOp->pFrom)
         return ERROR_INVALID_PARAMETER;

check_flags(lpFileOp->fFlags);

ZeroMemory(&flFrom, sizeof(FILE_LIST));
     ZeroMemory(&flTo, sizeof(FILE_LIST));
+    ZeroMemory(&op, sizeof(op));
+    op.req = lpFileOp;
+    lpFileOp->fAnyOperationsAborted = FALSE;

-    if ((ret = parse_file_list(&flFrom, lpFileOp->pFrom, parse_wildcard)))
-        return ret;
+    if (lpFileOp->wFunc != FO_MOVE
+            && lpFileOp->wFunc != FO_COPY
+            && lpFileOp->wFunc != FO_DELETE
+            && lpFileOp->wFunc != FO_RENAME)
+        return ERROR_INVALID_PARAMETER;
+
+    parse_file_list(&flFrom, lpFileOp->pFrom, parse_wildcard);
+    op.bManyItems = (flFrom.dwNumFiles > 1);

if (lpFileOp->wFunc != FO_DELETE)
+    {
+        if (!lpFileOp->pTo)
+            return ERROR_ACCESS_DENIED;
         parse_file_list(&flTo, lpFileOp->pTo, FALSE);
-
-    ZeroMemory(&op, sizeof(op));
-    op.req = lpFileOp;
-    op.bManyItems = (flFrom.dwNumFiles > 1);
-    lpFileOp->fAnyOperationsAborted = FALSE;
+    }

if (flTo.bAnyFromWildcard)
         return ERROR_INVALID_NAME;
@@ -1561,8 +1558,7 @@ int WINAPI SHFileOperationW(LPSHFILEOPSTRUCTW lpFileOp)
             ret = rename_files(lpFileOp, &flFrom, &flTo);
             break;
         default:
-            ret = ERROR_INVALID_PARAMETER;
-            break;
+            assert(0); /* Should never be here. */
     }

file_list_destroy(&flFrom);
diff --git a/dlls/shell32/tests/shlfileop.c b/dlls/shell32/tests/shlfileop.c
index 3ec2757aca0..712b002f9fd 100644
--- a/dlls/shell32/tests/shlfileop.c
+++ b/dlls/shell32/tests/shlfileop.c
@@ -675,7 +675,7 @@ static void test_delete(void)
     /* Empty filename. */
     init_shfo_tests();
     check_file_operation(FO_DELETE, FOF_NO_UI, "\0", NULL,
-            ERROR_SUCCESS, FALSE, TRUE, TRUE);
+            ERROR_SUCCESS, FALSE, FALSE, FALSE);
     ok(file_exists("test1.txt"), "Expected test1.txt to exist\n");

/* NULL filename. */
@@ -692,7 +692,7 @@ static void test_delete(void)
     /* Empty filename, only one null terminator */
     init_shfo_tests();
     check_file_operation(FO_DELETE, FOF_NO_UI, "", NULL,
-            ERROR_SUCCESS, FALSE, TRUE, TRUE);
+            ERROR_SUCCESS, FALSE, FALSE, FALSE);
     ok(file_exists("test1.txt"), "Expected test1.txt to exist\n");

/* Nonexistent file. */
@@ -840,13 +840,13 @@ static void test_rename(void)
     init_shfo_tests();
     check_file_operation(FO_RENAME, FOF_NO_UI,
             "\0", "test1.txt",
-            DE_MANYSRC1DEST, FALSE, TRUE, TRUE);
+            DE_MANYSRC1DEST, FALSE, FALSE, FALSE);
     check_file_operation(FO_RENAME, FOF_NO_UI,
             "\0", "testdir2",
-            DE_MANYSRC1DEST, FALSE, TRUE, TRUE);
+            DE_MANYSRC1DEST, FALSE, FALSE, FALSE);
     check_file_operation(FO_RENAME, FOF_NO_UI,
             "\0", "nonexistence",
-            DE_MANYSRC1DEST, FALSE, TRUE, TRUE);
+            DE_MANYSRC1DEST, FALSE, FALSE, FALSE);
     ok(!file_exists("nonexistence"), "Expected nonexistence to not exist\n");
     check_file_operation(FO_RENAME, FOF_NO_UI,
             "testdir2\0", "\0",
@@ -861,7 +861,7 @@ static void test_rename(void)
             ERROR_FILE_NOT_FOUND, FALSE, FALSE, FALSE);
     check_file_operation(FO_RENAME, FOF_NO_UI,
             "\0", "\0",
-            DE_MANYSRC1DEST, FALSE, TRUE, TRUE);
+            DE_MANYSRC1DEST, FALSE, FALSE, FALSE);

/* NULL source or target. */
     clean_after_shfo_tests();
@@ -877,13 +877,13 @@ static void test_rename(void)
             ERROR_INVALID_PARAMETER, 0xdeadbeef, FALSE, FALSE);
     check_file_operation(FO_RENAME, FOF_NO_UI,
             "test1.txt\0", NULL,
-            ERROR_ACCESS_DENIED, FALSE, TRUE, FALSE);
+            ERROR_ACCESS_DENIED, FALSE, FALSE, FALSE);
     check_file_operation(FO_RENAME, FOF_NO_UI,
             "testdir2\0", NULL,
-            ERROR_ACCESS_DENIED, FALSE, TRUE, FALSE);
+            ERROR_ACCESS_DENIED, FALSE, FALSE, FALSE);
     check_file_operation(FO_RENAME, FOF_NO_UI,
             "nonexistence\0", NULL,
-            ERROR_ACCESS_DENIED, FALSE, TRUE, FALSE);
+            ERROR_ACCESS_DENIED, FALSE, FALSE, FALSE);
     check_file_operation(FO_RENAME, FOF_NO_UI,
             NULL, NULL,
             ERROR_INVALID_PARAMETER, 0xdeadbeef, FALSE, FALSE);
@@ -1502,16 +1502,16 @@ static void test_copy(void)
             ERROR_FILE_NOT_FOUND, FALSE, FALSE, FALSE);
     check_file_operation(FO_COPY, FOF_NO_UI,
             "\0", "testdir2\0",
-            ERROR_SUCCESS, FALSE, TRUE, TRUE);
+            ERROR_SUCCESS, FALSE, FALSE, FALSE);
     check_file_operation(FO_COPY, FOF_NO_UI,
             "\0", "testfile.txt\0",
-            ERROR_SUCCESS, FALSE, TRUE, TRUE);
+            ERROR_SUCCESS, FALSE, FALSE, FALSE);
     check_file_operation(FO_COPY, FOF_NO_UI,
             "\0", "nonexistence",
-            ERROR_SUCCESS, FALSE, TRUE, TRUE);
+            ERROR_SUCCESS, FALSE, FALSE, FALSE);
     check_file_operation(FO_COPY, FOF_NO_UI,
             "\0", "\0",
-            ERROR_SUCCESS, FALSE, TRUE, TRUE);
+            ERROR_SUCCESS, FALSE, FALSE, FALSE);

/* NULL source or target. */
     clean_after_shfo_tests();
@@ -1527,13 +1527,13 @@ static void test_copy(void)
             ERROR_INVALID_PARAMETER, 0xdeadbeef, FALSE, FALSE);
     check_file_operation(FO_COPY, FOF_NO_UI,
             "test1.txt\0", NULL,
-            ERROR_ACCESS_DENIED, FALSE, TRUE, FALSE);
+            ERROR_ACCESS_DENIED, FALSE, FALSE, FALSE);
     check_file_operation(FO_COPY, FOF_NO_UI,
             "testdir2\0", NULL,
-            ERROR_ACCESS_DENIED, FALSE, TRUE, FALSE);
+            ERROR_ACCESS_DENIED, FALSE, FALSE, FALSE);
     check_file_operation(FO_COPY, FOF_NO_UI,
             "nonexistence\0", NULL,
-            ERROR_ACCESS_DENIED, FALSE, TRUE, FALSE);
+            ERROR_ACCESS_DENIED, FALSE, FALSE, FALSE);
     check_file_operation(FO_COPY, FOF_NO_UI,
             NULL, NULL,
             ERROR_INVALID_PARAMETER, 0xdeadbeef, FALSE, FALSE);
@@ -1847,13 +1847,13 @@ static void test_move(void)
     init_shfo_tests();
     check_file_operation(FO_MOVE, FOF_NO_UI,
             "\0", "test1.txt\0",
-            ERROR_SUCCESS, FALSE, TRUE, TRUE);
+            ERROR_SUCCESS, FALSE, FALSE, FALSE);
     check_file_operation(FO_MOVE, FOF_NO_UI,
             "\0", "testdir2\0",
-            ERROR_SUCCESS, FALSE, TRUE, TRUE);
+            ERROR_SUCCESS, FALSE, FALSE, FALSE);
     check_file_operation(FO_MOVE, FOF_NO_UI,
             "\0", "nonexistence\0",
-            ERROR_SUCCESS, FALSE, TRUE, TRUE);
+            ERROR_SUCCESS, FALSE, FALSE, FALSE);
     ok(!file_exists("nonexistence"), "Expected nonexistence to not exist.\n");
     check_file_operation(FO_MOVE, FOF_NO_UI,
             "test1.txt", "\0",
@@ -1866,7 +1866,7 @@ static void test_move(void)
             ERROR_FILE_NOT_FOUND, FALSE, FALSE, FALSE);
     check_file_operation(FO_MOVE, FOF_NO_UI,
             "\0", "\0",
-            ERROR_SUCCESS, FALSE, TRUE, TRUE);
+            ERROR_SUCCESS, FALSE, FALSE, FALSE);

/* NULL source or target. */
     clean_after_shfo_tests();
@@ -1882,13 +1882,13 @@ static void test_move(void)
             ERROR_INVALID_PARAMETER, 0xdeadbeef, FALSE, FALSE);
     check_file_operation(FO_MOVE, FOF_NO_UI,
             "test1.txt\0", NULL,
-            ERROR_ACCESS_DENIED, FALSE, TRUE, FALSE);
+            ERROR_ACCESS_DENIED, FALSE, FALSE, FALSE);
     check_file_operation(FO_MOVE, FOF_NO_UI,
             "testdir2\0", NULL,
-            ERROR_ACCESS_DENIED, FALSE, TRUE, FALSE);
+            ERROR_ACCESS_DENIED, FALSE, FALSE, FALSE);
     check_file_operation(FO_MOVE, FOF_NO_UI,
             "nonexistent\0", NULL,
-            ERROR_ACCESS_DENIED, FALSE, TRUE, FALSE);
+            ERROR_ACCESS_DENIED, FALSE, FALSE, FALSE);
     check_file_operation(FO_MOVE, FOF_NO_UI,
             NULL, NULL,
             ERROR_INVALID_PARAMETER, 0xdeadbeef, FALSE, FALSE);

[PATCH v2 4/4] shell32: Introduce parse_target_file_list helper.

[Ziqing Hui]

From: Ziqing Hui

Also correctly check wildcard in target file names.
---
 dlls/shell32/shlfileop.c       | 45 +++++++++++++++++++++++++---------
 dlls/shell32/tests/shlfileop.c |  7 ++----
 2 files changed, 35 insertions(+), 17 deletions(-)

diff --git a/dlls/shell32/shlfileop.c b/dlls/shell32/shlfileop.c
index c330a94b3b1..f24c7b7df23 100644
--- a/dlls/shell32/shlfileop.c
+++ b/dlls/shell32/shlfileop.c
@@ -1097,6 +1097,32 @@ static void parse_file_list(FILE_LIST *flList, LPCWSTR szFiles, BOOL parse_wildc
     }
 }

+static DWORD parse_target_file_list(const SHFILEOPSTRUCTW *op, DWORD source_file_count, FILE_LIST *out)
+{
+    DWORD i, check_count = 1;
+
+    if (op->wFunc == FO_DELETE)
+        return ERROR_SUCCESS;
+    if (!op->pTo)
+        return ERROR_ACCESS_DENIED;
+
+    parse_file_list(out, op->pTo, FALSE);
+
+    if (op->fFlags & FOF_MULTIDESTFILES)
+        check_count = min(source_file_count, out->dwNumFiles);
+
+    for (i = 0; i < check_count; ++i)
+    {
+        if (out->feFiles[i].bFromWildcard)
+        {
+            file_list_destroy(out);
+            return ERROR_INVALID_NAME;
+        }
+    }
+
+    return ERROR_SUCCESS;
+}
+
 static void create_dest_dirs(LPCWSTR szDestDir)
 {
     WCHAR dir[MAX_PATH];
@@ -1518,8 +1544,6 @@ int WINAPI SHFileOperationW(LPSHFILEOPSTRUCTW lpFileOp)

check_flags(lpFileOp->fFlags);

-    ZeroMemory(&flFrom, sizeof(FILE_LIST));
-    ZeroMemory(&flTo, sizeof(FILE_LIST));
     ZeroMemory(&op, sizeof(op));
     op.req = lpFileOp;
     lpFileOp->fAnyOperationsAborted = FALSE;
@@ -1530,19 +1554,18 @@ int WINAPI SHFileOperationW(LPSHFILEOPSTRUCTW lpFileOp)
             && lpFileOp->wFunc != FO_RENAME)
         return ERROR_INVALID_PARAMETER;

+    /* Parse source file list. */
+    memset(&flFrom, 0, sizeof(flFrom));
     parse_file_list(&flFrom, lpFileOp->pFrom, parse_wildcard);
     op.bManyItems = (flFrom.dwNumFiles > 1);

-    if (lpFileOp->wFunc != FO_DELETE)
+    memset(&flTo, 0, sizeof(flTo));
+    if ((ret = parse_target_file_list(lpFileOp, flFrom.dwNumFiles, &flTo)) != ERROR_SUCCESS)
     {
-        if (!lpFileOp->pTo)
-            return ERROR_ACCESS_DENIED;
-        parse_file_list(&flTo, lpFileOp->pTo, FALSE);
+        file_list_destroy(&flFrom);
+        return ret;
     }

-    if (flTo.bAnyFromWildcard)
-        return ERROR_INVALID_NAME;
-
     switch (lpFileOp->wFunc)
     {
         case FO_COPY:
@@ -1562,9 +1585,7 @@ int WINAPI SHFileOperationW(LPSHFILEOPSTRUCTW lpFileOp)
     }

file_list_destroy(&flFrom);
-
-    if (lpFileOp->wFunc != FO_DELETE)
-        file_list_destroy(&flTo);
+    file_list_destroy(&flTo);

if (ret == ERROR_CANCELLED)
         lpFileOp->fAnyOperationsAborted = TRUE;
diff --git a/dlls/shell32/tests/shlfileop.c b/dlls/shell32/tests/shlfileop.c
index 712b002f9fd..8e81483fc1c 100644
--- a/dlls/shell32/tests/shlfileop.c
+++ b/dlls/shell32/tests/shlfileop.c
@@ -1459,17 +1459,14 @@ static void test_copy(void)

check_file_operation(FO_COPY, FOF_NO_UI | FOF_MULTIDESTFILES,
             "aa.txt\0", "one\0tw?\0",
-            ERROR_SUCCESS, FALSE, TRUE, FALSE);
-    todo_wine
+            ERROR_SUCCESS, FALSE, FALSE, FALSE);
     ok(DeleteFileA("one\aa.txt"), "Expected file to exist\n");
     ok(!DeleteFileA("two\aa.txt"), "Expected file to not exist\n");

check_file_operation(FO_COPY, FOF_NO_UI,
             "aa.txt\0bb.txt\0", "one\0tw?\0",
-            ERROR_SUCCESS, FALSE, TRUE, FALSE);
-    todo_wine
+            ERROR_SUCCESS, FALSE, FALSE, FALSE);
     ok(DeleteFileA("one\aa.txt"), "Expected file to exist\n");
-    todo_wine
     ok(DeleteFileA("one\bb.txt"), "Expected file to exist\n");
     ok(!DeleteFileA("two\bb.txt"), "Expected file to not exist\n");