[PATCH v7 0/2] MR23: shell32: Sanitize Program Manager icon and group names
Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=52506 Signed-off-by: Alex Henrie <alexhenrie24(a)gmail.com> -- v7: shell32: Sanitize Program Manager icon and group names. shell32: Introduce combine_path helper for DDE. https://gitlab.winehq.org/wine/wine/-/merge_requests/23
From: Alex Henrie <alexhenrie24(a)gmail.com> --- dlls/shell32/dde.c | 44 ++++++++++++++++++++++++-------------------- 1 file changed, 24 insertions(+), 20 deletions(-) diff --git a/dlls/shell32/dde.c b/dlls/shell32/dde.c index 5ab63136092..c8b1f0280cd 100644 --- a/dlls/shell32/dde.c +++ b/dlls/shell32/dde.c @@ -78,20 +78,30 @@ static inline BOOL Dde_OnWildConnect(HSZ hszTopic, HSZ hszService) return FALSE; } +static WCHAR *combine_path(const WCHAR *directory, const WCHAR *name, const WCHAR *extension) +{ + WCHAR *path; + int len; + + len = wcslen(directory) + 1 + wcslen(name); + if (extension) len += wcslen(extension); + path = heap_alloc((len + 1) * sizeof(WCHAR)); + + 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) { 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); CoTaskMemFree(programs); return path; @@ -252,16 +262,14 @@ static DWORD PROGMAN_OnExecute(WCHAR *command, int argc, WCHAR **argv) } if (argc >= 2) { - 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"); } 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 = wcsdup(PathFindFileNameW(argv[0])); + *PathFindExtensionW(filename) = '\0'; + name = combine_path(last_group, filename, L".lnk"); + heap_free(filename); } hres = IPersistFile_Save(file, name, TRUE); @@ -278,12 +286,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"); ret = DeleteFileW(name); - heap_free(name); if (!ret) return DDE_FNOTPROCESSED; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/23
From: Alex Henrie <alexhenrie24(a)gmail.com> Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=52506 --- dlls/shell32/dde.c | 40 ++++++++++++++++++++++---------- dlls/shell32/tests/progman_dde.c | 3 +-- 2 files changed, 29 insertions(+), 14 deletions(-) diff --git a/dlls/shell32/dde.c b/dlls/shell32/dde.c index c8b1f0280cd..0f2f8763409 100644 --- a/dlls/shell32/dde.c +++ b/dlls/shell32/dde.c @@ -78,16 +78,32 @@ static inline BOOL Dde_OnWildConnect(HSZ hszTopic, HSZ hszService) return FALSE; } -static WCHAR *combine_path(const WCHAR *directory, const WCHAR *name, const WCHAR *extension) +static WCHAR *combine_path(const WCHAR *directory, const WCHAR *name, const WCHAR *extension, BOOL sanitize) { WCHAR *path; - int len; + int len, i; len = wcslen(directory) + 1 + wcslen(name); if (extension) len += wcslen(extension); path = heap_alloc((len + 1) * sizeof(WCHAR)); - PathCombineW(path, directory, name); + 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); @@ -96,12 +112,12 @@ static WCHAR *combine_path(const WCHAR *directory, const WCHAR *name, const WCHA } /* 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; SHGetKnownFolderPath(&FOLDERID_Programs, 0, NULL, &programs); - path = combine_path(programs, name, NULL); + path = combine_path(programs, name, NULL, sanitize); CoTaskMemFree(programs); return path; @@ -121,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) { @@ -172,7 +188,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); CreateDirectoryW(path, NULL); ShellExecuteW(NULL, NULL, path, NULL, NULL, SW_SHOWNORMAL); @@ -188,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); @@ -213,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); @@ -262,13 +278,13 @@ static DWORD PROGMAN_OnExecute(WCHAR *command, int argc, WCHAR **argv) } if (argc >= 2) { - name = combine_path(last_group, argv[1], L".lnk"); + name = combine_path(last_group, argv[1], L".lnk", TRUE); } else { WCHAR *filename = wcsdup(PathFindFileNameW(argv[0])); *PathFindExtensionW(filename) = '\0'; - name = combine_path(last_group, filename, L".lnk"); + name = combine_path(last_group, filename, L".lnk", TRUE); heap_free(filename); } hres = IPersistFile_Save(file, name, TRUE); @@ -286,7 +302,7 @@ static DWORD PROGMAN_OnExecute(WCHAR *command, int argc, WCHAR **argv) if (argc < 1) return DDE_FNOTPROCESSED; - name = combine_path(last_group, argv[0], L".lnk"); + name = combine_path(last_group, argv[0], L".lnk", FALSE); ret = DeleteFileW(name); heap_free(name); diff --git a/dlls/shell32/tests/progman_dde.c b/dlls/shell32/tests/progman_dde.c index cb1ff8ddb9f..5a7e8c912e5 100644 --- a/dlls/shell32/tests/progman_dde.c +++ b/dlls/shell32/tests/progman_dde.c @@ -459,8 +459,7 @@ static void test_name_sanitization(DWORD instance, HCONV hConv) error = dde_execute(instance, hConv, buf); ok(error == DMLERR_NO_ERROR, "expected DMLERR_NO_ERROR, got %#x\n", error); sprintf(buf, "Group%s", sanitized_name); - todo_wine ok(check_exists(buf), "directory not created\n"); - if (!check_exists(buf)) return; + ok(check_exists(buf), "directory not created\n"); ok(check_window_exists(buf), "window not created\n"); sprintf(buf, "[ShowGroup(\"Group%s\", 0)]", original_name); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/23
Hi, It looks like your patch introduced the new failures shown below. Please investigate and fix them before resubmitting your patch. If they are not new, fixing them anyway would help a lot. Otherwise please ask for the known failures list to be updated. The tests also ran into some preexisting test failures. If you know how to fix them that would be helpful. See the TestBot job for the details: The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=124922 Your paranoid android. === debian11 (32 bit zh:CN report) === shell32: autocomplete.c:614: Test failed: Expected (null), got L"ab" autocomplete.c:637: Test failed: Expected (null), got L"www.ax" === debian11 (build log) === Use of uninitialized value $Flaky in addition (+) at /home/testbot/lib/WineTestBot/LogUtils.pm line 720, <$LogFile> line 24684. Use of uninitialized value $Flaky in addition (+) at /home/testbot/lib/WineTestBot/LogUtils.pm line 720, <$LogFile> line 24684. Use of uninitialized value $Flaky in addition (+) at /home/testbot/lib/WineTestBot/LogUtils.pm line 720, <$LogFile> line 24684.
Huw Davies (@huw) commented about dlls/shell32/dde.c:
} if (argc >= 2) { - 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"); } 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 = wcsdup(PathFindFileNameW(argv[0])); + *PathFindExtensionW(filename) = '\0';
Personally, I find this rather ugly, could you use an intermediate variable to hold the `ext` ptr? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/23#note_10419
Huw Davies (@huw) commented about dlls/shell32/dde.c:
{ - 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"); } 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 = wcsdup(PathFindFileNameW(argv[0])); + *PathFindExtensionW(filename) = '\0'; + name = combine_path(last_group, filename, L".lnk"); + heap_free(filename);
This should be `free()` not `heap_free()` and suggests that what we should do first is to convert this file to using `malloc()`, `free()`, etc instead of the `heap_*()` helpers. I'd suggest a series something like this: 1. Move `strndupW()` into `dde.c` - it's only used in this file. 2. Convert calls to `heap_*()` helpers to their `malloc()`-style equivalents. 3. Add the `combine_path()` helper. 4. Add the sanitize option. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/23#note_10420
participants (4)
-
Alex Henrie -
Alex Henrie (＠alexhenrie)
-
Huw Davies (＠huw)
-
Marvin