[PATCH v8 0/4] 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> -- v8: shell32: Sanitize Program Manager icon and group names. shell32: Introduce combine_path helper for DDE. shell32: Use standard C functions for memory allocation in dde.c. shell32: Move strndupW to dde.c. https://gitlab.winehq.org/wine/wine/-/merge_requests/23
From: Alex Henrie <alexhenrie24(a)gmail.com> --- dlls/shell32/dde.c | 13 +++++++++++++ dlls/shell32/shell32_main.h | 13 ------------- 2 files changed, 13 insertions(+), 13 deletions(-) diff --git a/dlls/shell32/dde.c b/dlls/shell32/dde.c index 5ab63136092..cd956def797 100644 --- a/dlls/shell32/dde.c +++ b/dlls/shell32/dde.c @@ -54,6 +54,19 @@ static const char *debugstr_hsz( HSZ hsz ) return debugstr_w( buffer ); } +static WCHAR *strndupW(const WCHAR *src, DWORD len) +{ + WCHAR *dest; + if (!src) return NULL; + dest = heap_alloc((len + 1) * sizeof(*dest)); + if (dest) + { + memcpy(dest, src, len * sizeof(WCHAR)); + dest[len] = '\0'; + } + return dest; +} + static inline BOOL Dde_OnConnect(HSZ hszTopic, HSZ hszService) { if ((hszTopic == hszProgmanTopic) && (hszService == hszProgmanService)) diff --git a/dlls/shell32/shell32_main.h b/dlls/shell32/shell32_main.h index 7bb26e46a6e..33b85ee2726 100644 --- a/dlls/shell32/shell32_main.h +++ b/dlls/shell32/shell32_main.h @@ -241,19 +241,6 @@ static inline WCHAR *strdupW(const WCHAR *src) return dest; } -static inline WCHAR *strndupW(const WCHAR *src, DWORD len) -{ - WCHAR *dest; - if (!src) return NULL; - dest = heap_alloc((len + 1) * sizeof(*dest)); - if (dest) - { - memcpy(dest, src, len * sizeof(WCHAR)); - dest[len] = '\0'; - } - return dest; -} - static inline WCHAR *strdupAtoW(const char *str) { WCHAR *ret; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/23
From: Alex Henrie <alexhenrie24(a)gmail.com> --- dlls/shell32/dde.c | 60 +++++++++++++++++++++++----------------------- 1 file changed, 30 insertions(+), 30 deletions(-) diff --git a/dlls/shell32/dde.c b/dlls/shell32/dde.c index cd956def797..39e7de9c2c9 100644 --- a/dlls/shell32/dde.c +++ b/dlls/shell32/dde.c @@ -58,7 +58,7 @@ static WCHAR *strndupW(const WCHAR *src, DWORD len) { WCHAR *dest; if (!src) return NULL; - dest = heap_alloc((len + 1) * sizeof(*dest)); + dest = malloc((len + 1) * sizeof(*dest)); if (dest) { memcpy(dest, src, len * sizeof(WCHAR)); @@ -100,7 +100,7 @@ static WCHAR *get_programs_path(const WCHAR *name) SHGetKnownFolderPath(&FOLDERID_Programs, 0, NULL, &programs); len = lstrlenW(programs) + 1 + lstrlenW(name); - path = heap_alloc((len + 1) * sizeof(*path)); + path = malloc((len + 1) * sizeof(*path)); lstrcpyW(path, programs); lstrcatW(path, L"/"); lstrcatW(path, name); @@ -119,7 +119,7 @@ static inline HDDEDATA Dde_OnRequest(UINT uFmt, HCONV hconv, HSZ hszTopic, WIN32_FIND_DATAW finddata; HANDLE hfind; int len = 1; - WCHAR *groups_data = heap_alloc(sizeof(WCHAR)); + WCHAR *groups_data = malloc(sizeof(WCHAR)); char *groups_dataA; HDDEDATA ret; @@ -134,7 +134,7 @@ static inline HDDEDATA Dde_OnRequest(UINT uFmt, HCONV hconv, HSZ hszTopic, wcscmp(finddata.cFileName, L".") && wcscmp(finddata.cFileName, L"..")) { len += lstrlenW(finddata.cFileName) + 2; - groups_data = heap_realloc(groups_data, len * sizeof(WCHAR)); + groups_data = realloc(groups_data, len * sizeof(WCHAR)); lstrcatW(groups_data, finddata.cFileName); lstrcatW(groups_data, L"\r\n"); } @@ -143,13 +143,13 @@ static inline HDDEDATA Dde_OnRequest(UINT uFmt, HCONV hconv, HSZ hszTopic, } len = WideCharToMultiByte(CP_ACP, 0, groups_data, -1, NULL, 0, NULL, NULL); - groups_dataA = heap_alloc(len * sizeof(WCHAR)); + groups_dataA = malloc(len * sizeof(WCHAR)); WideCharToMultiByte(CP_ACP, 0, groups_data, -1, groups_dataA, len, NULL, NULL); ret = DdeCreateDataHandle(dwDDEInst, (BYTE *)groups_dataA, len, 0, hszGroups, uFmt, 0); - heap_free(groups_dataA); - heap_free(groups_data); - heap_free(programs); + free(groups_dataA); + free(groups_data); + free(programs); return ret; } else if (hszTopic == hszProgmanTopic && hszItem == hszProgmanService && uFmt == CF_TEXT) @@ -180,7 +180,7 @@ static DWORD PROGMAN_OnExecute(WCHAR *command, int argc, WCHAR **argv) CreateDirectoryW(path, NULL); ShellExecuteW(NULL, NULL, path, NULL, NULL, SW_SHOWNORMAL); - heap_free(last_group); + free(last_group); last_group = path; } else if (!wcsicmp(command, L"DeleteGroup")) @@ -193,7 +193,7 @@ static DWORD PROGMAN_OnExecute(WCHAR *command, int argc, WCHAR **argv) path = get_programs_path(argv[0]); - path2 = heap_alloc((lstrlenW(path) + 2) * sizeof(*path)); + path2 = malloc((lstrlenW(path) + 2) * sizeof(*path)); lstrcpyW(path2, path); path2[lstrlenW(path) + 1] = 0; @@ -203,8 +203,8 @@ static DWORD PROGMAN_OnExecute(WCHAR *command, int argc, WCHAR **argv) ret = SHFileOperationW(&shfos); - heap_free(path2); - heap_free(path); + free(path2); + free(path); if (ret || shfos.fAnyOperationsAborted) return DDE_FNOTPROCESSED; } @@ -220,7 +220,7 @@ static DWORD PROGMAN_OnExecute(WCHAR *command, int argc, WCHAR **argv) ShellExecuteW(NULL, NULL, path, NULL, NULL, SW_SHOWNORMAL); - heap_free(last_group); + free(last_group); last_group = path; } else if (!wcsicmp(command, L"AddItem")) @@ -242,10 +242,10 @@ static DWORD PROGMAN_OnExecute(WCHAR *command, int argc, WCHAR **argv) IShellLinkW_Release(link); return DDE_FNOTPROCESSED; } - path = heap_alloc(len * sizeof(WCHAR)); + path = malloc(len * sizeof(WCHAR)); SearchPathW(NULL, argv[0], L".exe", len, path, NULL); IShellLinkW_SetPath(link, path); - heap_free(path); + free(path); if (argc >= 2) IShellLinkW_SetDescription(link, argv[1]); if (argc >= 4) IShellLinkW_SetIconLocation(link, argv[2], wcstol(argv[3], NULL, 10)); @@ -266,19 +266,19 @@ 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)); + name = malloc(len * sizeof(*name)); swprintf( name, len, L"%s/%s.lnk", last_group, argv[1] ); } else { const WCHAR *filename = PathFindFileNameW(argv[0]); len = PathFindExtensionW(filename) - filename; - name = heap_alloc((lstrlenW(last_group) + 1 + len + 5) * sizeof(*name)); + name = malloc((lstrlenW(last_group) + 1 + len + 5) * sizeof(*name)); swprintf( name, lstrlenW(last_group) + 1 + len + 5, L"%s/%.*s.lnk", last_group, len, filename ); } hres = IPersistFile_Save(file, name, TRUE); - heap_free(name); + free(name); IPersistFile_Release(file); IShellLinkW_Release(link); @@ -292,12 +292,12 @@ 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)); + name = malloc(len * sizeof(*name)); swprintf( name, len, L"%s/%s.lnk", last_group, argv[0]); ret = DeleteFileW(name); - heap_free(name); + free(name); if (!ret) return DDE_FNOTPROCESSED; } @@ -326,7 +326,7 @@ static DWORD parse_dde_command(HSZ hszTopic, WCHAR *command) while (*command == '[') { argc = 0; - argv = heap_alloc(sizeof(*argv)); + argv = malloc(sizeof(*argv)); command++; while (*command == ' ') command++; @@ -355,7 +355,7 @@ static DWORD parse_dde_command(HSZ hszTopic, WCHAR *command) } argc++; - argv = heap_realloc(argv, argc * sizeof(*argv)); + argv = realloc(argv, argc * sizeof(*argv)); argv[argc-1] = strndupW(command, p - command); command = p; @@ -381,9 +381,9 @@ static DWORD parse_dde_command(HSZ hszTopic, WCHAR *command) ret = DDE_FNOTPROCESSED; } - heap_free(opcode); - for (i = 0; i < argc; i++) heap_free(argv[i]); - heap_free(argv); + free(opcode); + for (i = 0; i < argc; i++) free(argv[i]); + free(argv); if (ret == DDE_FNOTPROCESSED) break; } @@ -392,9 +392,9 @@ static DWORD parse_dde_command(HSZ hszTopic, WCHAR *command) error: ERR("failed to parse command %s\n", debugstr_w(original)); - heap_free(opcode); - for (i = 0; i < argc; i++) heap_free(argv[i]); - heap_free(argv); + free(opcode); + for (i = 0; i < argc; i++) free(argv[i]); + free(argv); return DDE_FNOTPROCESSED; } @@ -406,14 +406,14 @@ static DWORD Dde_OnExecute(HCONV hconv, HSZ hszTopic, HDDEDATA hdata) len = DdeGetData(hdata, NULL, 0, 0); if (!len) return DDE_FNOTPROCESSED; - command = heap_alloc(len); + command = malloc(len); DdeGetData(hdata, (BYTE *)command, len, 0); TRACE("conv=%p topic=%s data=%s\n", hconv, debugstr_hsz(hszTopic), debugstr_w(command)); ret = parse_dde_command(hszTopic, command); - heap_free(command); + free(command); return ret; } -- GitLab 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 39e7de9c2c9..ce661c6a46b 100644 --- a/dlls/shell32/dde.c +++ b/dlls/shell32/dde.c @@ -91,20 +91,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 = malloc((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 = malloc((len + 1) * sizeof(*path)); - lstrcpyW(path, programs); - lstrcatW(path, L"/"); - lstrcatW(path, name); - + path = combine_path(programs, name, NULL); CoTaskMemFree(programs); return path; @@ -265,16 +275,14 @@ static DWORD PROGMAN_OnExecute(WCHAR *command, int argc, WCHAR **argv) } if (argc >= 2) { - len = lstrlenW(last_group) + 1 + lstrlenW(argv[1]) + 5; - name = malloc(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 = malloc((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]); + filename = strndupW(filename, PathFindExtensionW(filename) - filename); + name = combine_path(last_group, filename, L".lnk"); + free(filename); } hres = IPersistFile_Save(file, name, TRUE); @@ -291,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 = malloc(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); - 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 ce661c6a46b..fa20ca12b61 100644 --- a/dlls/shell32/dde.c +++ b/dlls/shell32/dde.c @@ -91,16 +91,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 = malloc((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); + free(sanitized_name); + } + else + { + PathCombineW(path, directory, name); + } if (extension) wcscat(path, extension); @@ -109,12 +125,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; @@ -134,7 +150,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) { @@ -185,7 +201,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); @@ -201,7 +217,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 = malloc((lstrlenW(path) + 2) * sizeof(*path)); lstrcpyW(path2, path); @@ -226,7 +242,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); @@ -275,13 +291,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 = PathFindFileNameW(argv[0]); filename = strndupW(filename, PathFindExtensionW(filename) - filename); - name = combine_path(last_group, filename, L".lnk"); + name = combine_path(last_group, filename, L".lnk", TRUE); free(filename); } hres = IPersistFile_Save(file, name, TRUE); @@ -299,7 +315,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); 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=124963 Your paranoid android. === debian11 (32 bit report) === ddraw: ddraw2.c:15138: Test succeeded inside todo block: Got unexpected caps 0x421350. === 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 24707. Use of uninitialized value $Flaky in addition (+) at /home/testbot/lib/WineTestBot/LogUtils.pm line 720, <$LogFile> line 24707. Use of uninitialized value $Flaky in addition (+) at /home/testbot/lib/WineTestBot/LogUtils.pm line 720, <$LogFile> line 24707.
On Thu Oct 13 15:59:16 2022 +0000, Alex Henrie wrote:
changed this line in [version 8 of the diff](/wine/wine/-/merge_requests/23/diffs?diff_id=13698&start_sha=4e95e24e5b1c00d421cc95075f0448f050d173a0#5e9b272a4b8973d33b8b2698e9f973594d3740c0_288_301) Thank you for the feedback. I have prepended the two new commits that you suggested to the beginning of this merge request. Please let me know what you think.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/23#note_10559
On Thu Oct 13 15:59:15 2022 +0000, Alex Henrie wrote:
changed this line in [version 8 of the diff](/wine/wine/-/merge_requests/23/diffs?diff_id=13698&start_sha=4e95e24e5b1c00d421cc95075f0448f050d173a0#5e9b272a4b8973d33b8b2698e9f973594d3740c0_286_298) I have changed it to use strndupW which I think is even less ugly, but if you still want an ext variable let me know; I am happy to add one.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/23#note_10560
On Thu Oct 13 16:03:46 2022 +0000, Alex Henrie wrote:
I have changed it to use strndupW which I think is even less ugly, but if you still want an ext variable let me know; I am happy to add one. I don't think adding another use case of the non-standard `strndupW()` makes things less ugly. Go back to using `wcsdup()` - it doesn't matter that we over-allocate a little as this is only a short-lived string.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/23#note_10570
participants (4)
-
Alex Henrie -
Alex Henrie (＠alexhenrie)
-
Huw Davies (＠huw)
-
Marvin