[PATCH v2 0/4] MR7993: shell32: Better handle error case for file operation.
-- v2: shell32: Introduce parse_target_file_list helper. https://gitlab.winehq.org/wine/wine/-/merge_requests/7993
From: Ziqing Hui <zhui(a)codeweavers.com> --- 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"); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/7993
From: Ziqing Hui <zhui(a)codeweavers.com> --- 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. */ -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/7993
From: Ziqing Hui <zhui(a)codeweavers.com> --- 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); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/7993
From: Ziqing Hui <zhui(a)codeweavers.com> 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"); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/7993
participants (2)
-
Ziqing Hui -
Ziqing Hui (＠zhui)