All of the new tests that are marked todo_wine fail because Wine's PathCombineW does not handle ".", "...", "....", or "foobar." correctly. They will pass once PathCombineW is fixed.
Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=52506 Signed-off-by: Alex Henrie alexhenrie24@gmail.com
-- v3: shell32: Sanitize Program Manager icon and group names
From: Alex Henrie alexhenrie24@gmail.com
All of the new tests that are marked todo_wine fail because Wine's PathCombineW does not handle ".", "...", "....", or "foobar." correctly. They will pass once PathCombineW is fixed.
Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=52506 Signed-off-by: Alex Henrie alexhenrie24@gmail.com --- dlls/shell32/dde.c | 79 +++++--- dlls/shell32/tests/progman_dde.c | 318 ++++++++++++++++++++++++++++++- 2 files changed, 365 insertions(+), 32 deletions(-)
diff --git a/dlls/shell32/dde.c b/dlls/shell32/dde.c index 5ab63136092..5dd3dff86f4 100644 --- a/dlls/shell32/dde.c +++ b/dlls/shell32/dde.c @@ -78,20 +78,46 @@ static inline BOOL Dde_OnWildConnect(HSZ hszTopic, HSZ hszService) return FALSE; }
+static WCHAR *combine_path(const WCHAR *directory, const WCHAR *name, const WCHAR *extension, BOOL sanitize) +{ + WCHAR *path; + int len, i; + + len = wcslen(directory) + 1 + wcslen(name); + if (extension) len += wcslen(extension); + path = heap_alloc((len + 1) * sizeof(WCHAR)); + + if (sanitize) + { + WCHAR *sanitized_name = wcsdup(name); + + for (i = 0; i < wcslen(name); i++) + { + if (name[i] < ' ' || wcschr(L"*/:<>?\|", name[i])) + sanitized_name[i] = '_'; + } + + PathCombineW(path, directory, sanitized_name); + heap_free(sanitized_name); + } + else + { + PathCombineW(path, directory, name); + } + + if (extension) + wcscat(path, extension); + + return path; +} + /* Returned string must be freed by caller */ -static WCHAR *get_programs_path(const WCHAR *name) +static WCHAR *get_programs_path(const WCHAR *name, BOOL sanitize) { WCHAR *programs, *path; - int len;
SHGetKnownFolderPath(&FOLDERID_Programs, 0, NULL, &programs); - - len = lstrlenW(programs) + 1 + lstrlenW(name); - path = heap_alloc((len + 1) * sizeof(*path)); - lstrcpyW(path, programs); - lstrcatW(path, L"/"); - lstrcatW(path, name); - + path = combine_path(programs, name, NULL, sanitize); CoTaskMemFree(programs);
return path; @@ -111,7 +137,7 @@ static inline HDDEDATA Dde_OnRequest(UINT uFmt, HCONV hconv, HSZ hszTopic, HDDEDATA ret;
groups_data[0] = 0; - programs = get_programs_path(L"*"); + programs = get_programs_path(L"*", FALSE); hfind = FindFirstFileW(programs, &finddata); if (hfind) { @@ -162,10 +188,10 @@ static DWORD PROGMAN_OnExecute(WCHAR *command, int argc, WCHAR **argv)
if (argc < 1) return DDE_FNOTPROCESSED;
- path = get_programs_path(argv[0]); + path = get_programs_path(argv[0], TRUE);
- CreateDirectoryW(path, NULL); - ShellExecuteW(NULL, NULL, path, NULL, NULL, SW_SHOWNORMAL); + if (CreateDirectoryW(path, NULL)) + ShellExecuteW(NULL, NULL, path, NULL, NULL, SW_SHOWNORMAL);
heap_free(last_group); last_group = path; @@ -178,7 +204,7 @@ static DWORD PROGMAN_OnExecute(WCHAR *command, int argc, WCHAR **argv)
if (argc < 1) return DDE_FNOTPROCESSED;
- path = get_programs_path(argv[0]); + path = get_programs_path(argv[0], TRUE);
path2 = heap_alloc((lstrlenW(path) + 2) * sizeof(*path)); lstrcpyW(path2, path); @@ -203,7 +229,7 @@ static DWORD PROGMAN_OnExecute(WCHAR *command, int argc, WCHAR **argv) * ignore its actual value. */ if (argc < 2) return DDE_FNOTPROCESSED;
- path = get_programs_path(argv[0]); + path = get_programs_path(argv[0], TRUE);
ShellExecuteW(NULL, NULL, path, NULL, NULL, SW_SHOWNORMAL);
@@ -250,26 +276,21 @@ static DWORD PROGMAN_OnExecute(WCHAR *command, int argc, WCHAR **argv) IShellLinkW_Release(link); return DDE_FNOTPROCESSED; } - if (argc >= 2) + if (argc >= 2 && argv[1][0]) { - len = lstrlenW(last_group) + 1 + lstrlenW(argv[1]) + 5; - name = heap_alloc(len * sizeof(*name)); - swprintf( name, len, L"%s/%s.lnk", last_group, argv[1] ); + name = combine_path(last_group, argv[1], L".lnk", TRUE); } else { - const WCHAR *filename = PathFindFileNameW(argv[0]); - len = PathFindExtensionW(filename) - filename; - name = heap_alloc((lstrlenW(last_group) + 1 + len + 5) * sizeof(*name)); - swprintf( name, lstrlenW(last_group) + 1 + len + 5, L"%s/%.*s.lnk", last_group, len, filename ); + WCHAR *filename = PathFindFileNameW(argv[0]); + *PathFindExtensionW(filename) = '\0'; + name = combine_path(last_group, filename, L".lnk", TRUE); } - hres = IPersistFile_Save(file, name, TRUE); + IPersistFile_Save(file, name, TRUE);
heap_free(name); IPersistFile_Release(file); IShellLinkW_Release(link); - - if (FAILED(hres)) return DDE_FNOTPROCESSED; } else if (!wcsicmp(command, L"DeleteItem") || !wcsicmp(command, L"ReplaceItem")) { @@ -278,12 +299,8 @@ static DWORD PROGMAN_OnExecute(WCHAR *command, int argc, WCHAR **argv)
if (argc < 1) return DDE_FNOTPROCESSED;
- len = lstrlenW(last_group) + 1 + lstrlenW(argv[0]) + 5; - name = heap_alloc(len * sizeof(*name)); - swprintf( name, len, L"%s/%s.lnk", last_group, argv[0]); - + name = combine_path(last_group, argv[0], L".lnk", FALSE); ret = DeleteFileW(name); - heap_free(name);
if (!ret) return DDE_FNOTPROCESSED; diff --git a/dlls/shell32/tests/progman_dde.c b/dlls/shell32/tests/progman_dde.c index 5d532f9222f..66b9ed0c87a 100644 --- a/dlls/shell32/tests/progman_dde.c +++ b/dlls/shell32/tests/progman_dde.c @@ -23,7 +23,7 @@ * functionality * - Todo: Handle CommonGroupFlag * Better AddItem Tests (Lots of parameters to test) - * Tests for Invalid Characters in Names / Invalid Parameters + * Tests for invalid parameters */
#include <stdio.h> @@ -425,6 +425,321 @@ static void test_request_groups(DWORD instance, HCONV hconv) FindClose(hfind); }
+static BOOL is_unsanitary(char c) +{ + return (c > 0 && c < ' ') || strchr("*/:<>?\|", c) != NULL; +} + +static void sanitize_name(const char *original_name, char *sanitized_name, BOOL group) +{ + BOOL at_end = TRUE; + int i; + + i = strlen(original_name); + sanitized_name[i] = 0; + + while (--i >= 0) + { + if (is_unsanitary(original_name[i])) + { + /* replaced in all positions */ + sanitized_name[i] = '_'; + at_end = FALSE; + } + else if (original_name[i] == '.' || (original_name[i] == ' ' && group)) + { + /* left alone if in the middle of the string, dropped if at the end of the string */ + sanitized_name[i] = at_end ? '\0' : original_name[i]; + } + else + { + /* left alone in all positions */ + sanitized_name[i] = original_name[i]; + at_end = FALSE; + } + } +} + +static void test_name_sanitization(DWORD instance, HCONV hConv) +{ + static const char test_chars[] = "\x01\x1F !#$%&'*+,-./:;<=>?@[\]^`{|}~\x7F\x80\xFF"; + char original_name[16], sanitized_icon_name[16], sanitized_group_name[16]; + char buf[64]; + UINT error; + int i; + char c; + + if (0) /* the directory isn't deleted on windows < 7 */ + { + error = dde_execute(instance, hConv, "[CreateGroup(" ")]"); + ok(error == DMLERR_NO_ERROR, "expected DMLERR_NO_ERROR, got %#x\n", error); + ok(check_exists(" "), "directory not created\n"); + ok(!check_window_exists(" "), "window should not exist\n"); + + error = dde_execute(instance, hConv, "[DeleteGroup(" ")]"); + ok(error == DMLERR_NO_ERROR, "expected DMLERR_NO_ERROR, got %#x\n", error); + ok(!check_exists(" "), "directory should not exist\n"); + } + + if (GetUserDefaultUILanguage() == MAKELANGID(LANG_ENGLISH, SUBLANG_DEFAULT)) + { + error = dde_execute(instance, hConv, "[CreateGroup("")]"); + ok(error == DMLERR_NO_ERROR, "expected DMLERR_NO_ERROR, got %#x\n", error); + todo_wine ok(check_window_exists("Programs") || broken(TRUE) /* 2003 */, "window not created\n"); + + error = dde_execute(instance, hConv, "[CreateGroup(".")]"); + ok(error == DMLERR_NO_ERROR, "expected DMLERR_NO_ERROR, got %#x\n", error); + ok(!check_window_exists("Programs"), "window should not exist\n"); + + error = dde_execute(instance, hConv, "[CreateGroup("..")]"); + ok(error == DMLERR_NO_ERROR, "expected DMLERR_NO_ERROR, got %#x\n", error); + todo_wine ok(check_window_exists("Start Menu") || broken(TRUE) /* 2003 */, "window not created\n"); + + error = dde_execute(instance, hConv, "[CreateGroup("...")]"); + ok(error == DMLERR_NO_ERROR, "expected DMLERR_NO_ERROR, got %#x\n", error); + todo_wine ok(check_window_exists("Programs") || broken(TRUE) /* XP */, "window not created\n"); + + error = dde_execute(instance, hConv, "[CreateGroup("....")]"); + ok(error == DMLERR_NO_ERROR, "expected DMLERR_NO_ERROR, got %#x\n", error); + ok(!check_window_exists("Programs"), "window should not exist\n"); + } + else + { + skip("Directory names are probably not in English\n"); + } + + if (0) /* these calls will actually delete the start menu */ + { + error = dde_execute(instance, hConv, "[DeleteGroup("")]"); + ok(error == DMLERR_NO_ERROR, "expected DMLERR_NO_ERROR, got %#x\n", error); + ok(!check_exists("../Programs"), "directory should not exist\n"); + + error = dde_execute(instance, hConv, "[DeleteGroup("..")]"); + ok(error == DMLERR_NO_ERROR, "expected DMLERR_NO_ERROR, got %#x\n", error); + ok(!check_exists("../../Start Menu"), "directory should not exist\n"); + } + + error = dde_execute(instance, hConv, "[CreateGroup(Group3)]"); + ok(error == DMLERR_NO_ERROR, "expected DMLERR_NO_ERROR, got %#x\n", error); + ok(check_exists("Group3"), "directory not created\n"); + ok(check_window_exists("Group3"), "window not created\n"); + + error = dde_execute(instance, hConv, "[AddItem(notepad,"")]"); + ok(error == DMLERR_NO_ERROR, "expected DMLERR_NO_ERROR, got %#x\n", error); + ok(check_exists("Group3/notepad.lnk"), "link not created\n"); + + error = dde_execute(instance, hConv, "[DeleteItem(notepad)]"); + ok(error == DMLERR_NO_ERROR, "expected DMLERR_NO_ERROR, got %#x\n", error); + ok(!check_exists("Group3/notepad.lnk"), "link should not exist\n"); + + error = dde_execute(instance, hConv, "[AddItem(notepad," ")]"); + ok(error == DMLERR_NO_ERROR, "expected DMLERR_NO_ERROR, got %#x\n", error); + ok(check_exists("Group3/ .lnk"), "link not created\n"); + + error = dde_execute(instance, hConv, "[DeleteItem(" ")]"); + ok(error == DMLERR_NO_ERROR, "expected DMLERR_NO_ERROR, got %#x\n", error); + ok(!check_exists("Group3/ .lnk"), "link should not exist\n"); + + error = dde_execute(instance, hConv, "[AddItem(notepad,".")]"); + ok(error == DMLERR_NO_ERROR, "expected DMLERR_NO_ERROR, got %#x\n", error); + todo_wine ok(check_exists("Group3.lnk"), "link not created\n"); + + error = dde_execute(instance, hConv, "[DeleteItem(".")]"); + ok(error == DMLERR_NO_ERROR, "expected DMLERR_NO_ERROR, got %#x\n", error); + ok(!check_exists("Group3.lnk"), "link should not exist\n"); + + error = dde_execute(instance, hConv, "[AddItem(notepad,"..")]"); + ok(error == DMLERR_NO_ERROR, "expected DMLERR_NO_ERROR, got %#x\n", error); + ok(check_exists("../Programs.lnk"), "link not created\n"); + + error = dde_execute(instance, hConv, "[DeleteItem("..")]"); + ok(error == DMLERR_NO_ERROR, "expected DMLERR_NO_ERROR, got %#x\n", error); + ok(!check_exists("../Programs.lnk"), "link should not exist\n"); + + error = dde_execute(instance, hConv, "[AddItem(notepad,"...")]"); + ok(error == DMLERR_NO_ERROR, "expected DMLERR_NO_ERROR, got %#x\n", error); + todo_wine ok(check_exists("Group3/.lnk") || broken(TRUE) /* XP */, "link not created\n"); + + error = dde_execute(instance, hConv, "[DeleteItem("...")]"); + ok(error == DMLERR_NO_ERROR, "expected DMLERR_NO_ERROR, got %#x\n", error); + ok(!check_exists("Group3/.lnk"), "link should not exist\n"); + + error = dde_execute(instance, hConv, "[AddItem(notepad,"....")]"); + ok(error == DMLERR_NO_ERROR, "expected DMLERR_NO_ERROR, got %#x\n", error); + todo_wine ok(check_exists("Group3/.lnk") || broken(TRUE) /* XP */, "link not created\n"); + + error = dde_execute(instance, hConv, "[DeleteItem("....")]"); + ok(error == DMLERR_NO_ERROR, "expected DMLERR_NO_ERROR, got %#x\n", error); + ok(!check_exists("Group3/.lnk"), "link should not exist\n"); + + /* Test sanitary group name with unsanitary icon names */ + + for (i = 0; i < sizeof(test_chars) - 1; i++) + { + c = test_chars[i]; + winetest_push_context("char %d '%c'", c, c); + + sprintf(original_name, "%03d_%c_.%c", c, c, c); + sanitize_name(original_name, sanitized_icon_name, FALSE); + + sprintf(buf, "[AddItem(notepad,"Notepad%s")]", original_name); + error = dde_execute(instance, hConv, buf); + ok(error == DMLERR_NO_ERROR, "expected DMLERR_NO_ERROR, got %#x\n", error); + sprintf(buf, "Group3/Notepad%s.lnk", sanitized_icon_name); + todo_wine_if(c == '.') ok(check_exists(buf) || broken(c == '.') /* XP */, "link not created\n"); + if (!check_exists(buf)) + { + winetest_pop_context(); + continue; + } + + if (is_unsanitary(c)) + { + sprintf(buf, "[ReplaceItem("Notepad%s")]", original_name); + error = dde_execute(instance, hConv, buf); + ok(error == DMLERR_NOTPROCESSED, "expected DMLERR_NOTPROCESSED, got %#x\n", error); + sprintf(buf, "Group3/Notepad%s.lnk", sanitized_icon_name); + ok(check_exists(buf), "link should still exist\n"); + + sprintf(buf, "[DeleteItem("Notepad%s")]", original_name); + error = dde_execute(instance, hConv, buf); + ok(error == DMLERR_NOTPROCESSED, "expected DMLERR_NOTPROCESSED, got %#x\n", error); + sprintf(buf, "Group3/Notepad%s.lnk", sanitized_icon_name); + ok(check_exists(buf), "link should still exist\n"); + } + else + { + sprintf(buf, "[ReplaceItem("Notepad%s")]", original_name); + error = dde_execute(instance, hConv, buf); + ok(error == DMLERR_NO_ERROR, "expected DMLERR_NO_ERROR, got %#x\n", error); + sprintf(buf, "Group3/Notepad%s.lnk", sanitized_icon_name); + ok(!check_exists(buf), "link should not exist\n"); + + sprintf(buf, "[AddItem(notepad,"Notepad%s")]", original_name); + error = dde_execute(instance, hConv, buf); + ok(error == DMLERR_NO_ERROR, "expected DMLERR_NO_ERROR, got %#x\n", error); + sprintf(buf, "Group3/Notepad%s.lnk", sanitized_icon_name); + ok(check_exists(buf), "link not created\n"); + + sprintf(buf, "[DeleteItem("Notepad%s")]", original_name); + error = dde_execute(instance, hConv, buf); + ok(error == DMLERR_NO_ERROR, "expected DMLERR_NO_ERROR, got %#x\n", error); + sprintf(buf, "Group3/Notepad%s.lnk", sanitized_icon_name); + ok(!check_exists(buf), "link should not exist\n"); + } + + winetest_pop_context(); + } + + error = dde_execute(instance, hConv, "[DeleteGroup(Group3)]"); + ok(error == DMLERR_NO_ERROR, "expected DMLERR_NO_ERROR, got %#x\n", error); + ok(!check_exists("Group3"), "directory should not exist\n"); + + for (i = 0; i < sizeof(test_chars) - 1; i++) + { + c = test_chars[i]; + winetest_push_context("char %d '%c'", c, c); + + sprintf(original_name, "%03d_%c_.%c", c, c, c); + sanitize_name(original_name, sanitized_group_name, TRUE); + sanitize_name(original_name, sanitized_icon_name, FALSE); + + sprintf(buf, "[CreateGroup("Group%s")]", original_name); + error = dde_execute(instance, hConv, buf); + ok(error == DMLERR_NO_ERROR, "expected DMLERR_NO_ERROR, got %#x\n", error); + sprintf(buf, "Group%s", sanitized_group_name); + ok(check_exists(buf), "directory not created\n"); + ok(check_window_exists(buf) || broken(c == ' ') /* vista */, "window not created\n"); + + sprintf(buf, "[ShowGroup("Group%s", 0)]", original_name); + error = dde_execute(instance, hConv, buf); + ok(error == DMLERR_NO_ERROR, "expected DMLERR_NO_ERROR, got %#x\n", error); + sprintf(buf, "Group%s", sanitized_group_name); + ok(check_window_exists(buf) || broken(c == ' ') /* vista */, "window not created\n"); + + /* Test unsanitary group name with sanitary icon name */ + + error = dde_execute(instance, hConv, "[AddItem(notepad,Notepad)]"); + ok(error == DMLERR_NO_ERROR, "expected DMLERR_NO_ERROR, got %#x\n", error); + sprintf(buf, "Group%s/Notepad.lnk", sanitized_group_name); + if (c == ' ') + { + /* Although no error is reported, no icon is created if the group name ends in a space */ + ok(!check_exists(buf), "link should not exist\n"); + goto delete_group; + } + todo_wine_if(c == '.') ok(check_exists(buf), "link not created\n"); + + error = dde_execute(instance, hConv, "[ReplaceItem(Notepad)]"); + todo_wine_if(c == '.') ok(error == DMLERR_NO_ERROR, "expected DMLERR_NO_ERROR, got %#x\n", error); + sprintf(buf, "Group%s/Notepad.lnk", sanitized_group_name); + ok(!check_exists(buf), "link should not exist\n"); + + error = dde_execute(instance, hConv, "[AddItem(notepad,Notepad)]"); + ok(error == DMLERR_NO_ERROR, "expected DMLERR_NO_ERROR, got %#x\n", error); + sprintf(buf, "Group%s/Notepad.lnk", sanitized_group_name); + todo_wine_if(c == '.') ok(check_exists(buf), "link not created\n"); + + error = dde_execute(instance, hConv, "[DeleteItem(Notepad)]"); + todo_wine_if(c == '.') ok(error == DMLERR_NO_ERROR, "expected DMLERR_NO_ERROR, got %#x\n", error); + sprintf(buf, "Group%s/Notepad.lnk", sanitized_group_name); + ok(!check_exists(buf), "link should not exist\n"); + + /* Test unsanitary group name with unsanitary icon name */ + + sprintf(buf, "[AddItem(notepad,"Notepad%s")]", original_name); + error = dde_execute(instance, hConv, buf); + ok(error == DMLERR_NO_ERROR, "expected DMLERR_NO_ERROR, got %#x\n", error); + sprintf(buf, "Group%s/Notepad%s.lnk", sanitized_group_name, sanitized_icon_name); + todo_wine_if(c == '.') ok(check_exists(buf) || broken(c == '.') /* XP */, "link not created\n"); + if (!check_exists(buf)) goto delete_group; + + if (is_unsanitary(c)) + { + sprintf(buf, "[ReplaceItem("Notepad%s")]", original_name); + error = dde_execute(instance, hConv, buf); + ok(error == DMLERR_NOTPROCESSED, "expected DMLERR_NOTPROCESSED, got %#x\n", error); + sprintf(buf, "Group%s/Notepad%s.lnk", sanitized_group_name, sanitized_icon_name); + ok(check_exists(buf), "link should still exist\n"); + + sprintf(buf, "[DeleteItem("Notepad%s")]", original_name); + error = dde_execute(instance, hConv, buf); + ok(error == DMLERR_NOTPROCESSED, "expected DMLERR_NOTPROCESSED, got %#x\n", error); + sprintf(buf, "Group%s/Notepad%s.lnk", sanitized_group_name, sanitized_icon_name); + ok(check_exists(buf), "link should still exist\n"); + } + else + { + sprintf(buf, "[ReplaceItem("Notepad%s")]", original_name); + error = dde_execute(instance, hConv, buf); + ok(error == DMLERR_NO_ERROR, "expected DMLERR_NO_ERROR, got %#x\n", error); + sprintf(buf, "Group%s/Notepad%s.lnk", sanitized_group_name, sanitized_icon_name); + ok(!check_exists(buf), "link should not exist\n"); + + sprintf(buf, "[AddItem(notepad,"Notepad%s")]", original_name); + error = dde_execute(instance, hConv, buf); + ok(error == DMLERR_NO_ERROR, "expected DMLERR_NO_ERROR, got %#x\n", error); + sprintf(buf, "Group%s/Notepad%s.lnk", sanitized_group_name, sanitized_icon_name); + ok(check_exists(buf), "link not created\n"); + + sprintf(buf, "[DeleteItem("Notepad%s")]", original_name); + error = dde_execute(instance, hConv, buf); + ok(error == DMLERR_NO_ERROR, "expected DMLERR_NO_ERROR, got %#x\n", error); + sprintf(buf, "Group%s/Notepad%s.lnk", sanitized_group_name, sanitized_icon_name); + ok(!check_exists(buf), "link should not exist\n"); + } + +delete_group: + sprintf(buf, "[DeleteGroup("Group%s")]", original_name); + error = dde_execute(instance, hConv, buf); + ok(error == DMLERR_NO_ERROR, "expected DMLERR_NO_ERROR, got %#x\n", error); + sprintf(buf, "Group%s", sanitized_group_name); + ok(!check_exists(buf), "directory should not exist\n"); + + winetest_pop_context(); + } +} + START_TEST(progman_dde) { DWORD instance = 0; @@ -479,6 +794,7 @@ START_TEST(progman_dde)
/* Run Tests */ test_progman_dde2(instance, hConv); + test_name_sanitization(instance, hConv);
/* Cleanup & Exit */ ret = DdeDisconnect(hConv);
I just pushed a new patch with two changes:
- Use wcslen, wcscat, wcsdup, and wcschr instead of lstrlenW, lstrcatW, strdupW, and StrChrW in new code (to take advantage of compiler optimizations). - Move allocation and path combination from get_programs_path to sanitize_and_combine and rename it to combine_path with a 'BOOL sanitize' argument.
I think the patch looks quite a bit nicer with those changes. Please let me know what else to do.
What can I do to get this patch accepted in whole or in part?
I have rebased this patch, dropping all of the tests that were not committed upstream in merge request !252, and dropping the fixes for the tests that were dropped. The resulting patch is much smaller and more focused on the main problem.
-
Alex Henrie -
Alex Henrie (＠alexhenrie)