[PATCH 1/4] shell32/shfldr_mycomp: Fail properly when parsing display name
Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- Currently the code doesn't fail at all if an invalid path is specified. dlls/shell32/shfldr_mycomp.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dlls/shell32/shfldr_mycomp.c b/dlls/shell32/shfldr_mycomp.c index 6a31e8c..1b987a7 100644 --- a/dlls/shell32/shfldr_mycomp.c +++ b/dlls/shell32/shfldr_mycomp.c @@ -228,7 +228,7 @@ static HRESULT WINAPI ISF_MyComputer_fnParseDisplayName (IShellFolder2 *iface, hr = SHELL32_ParseNextElement (iface, hwndOwner, pbc, &pidlTemp, (LPOLESTR) szNext, pchEaten, pdwAttributes); } - else + else if (pidlTemp) { if (pdwAttributes && *pdwAttributes) SHELL32_GetItemAttributes (&This->IShellFolder2_iface, pidlTemp, pdwAttributes); -- 2.21.0
Besides PathGetDriveNumber being dangerous and having a completely messed up result with \\?\ prefix, a backslash is not required anymore on newer Windows versions. So e.g. C: should succeed to be parsed. Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- Testcase at end of series. dlls/shell32/shfldr_mycomp.c | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/dlls/shell32/shfldr_mycomp.c b/dlls/shell32/shfldr_mycomp.c index 1b987a7..97d259a 100644 --- a/dlls/shell32/shfldr_mycomp.c +++ b/dlls/shell32/shfldr_mycomp.c @@ -193,7 +193,7 @@ static HRESULT WINAPI ISF_MyComputer_fnParseDisplayName (IShellFolder2 *iface, IMyComputerFolderImpl *This = impl_from_IShellFolder2(iface); HRESULT hr = E_INVALIDARG; LPCWSTR szNext = NULL; - WCHAR szElement[MAX_PATH]; + WCHAR c, szElement[MAX_PATH]; LPITEMIDLIST pidlTemp = NULL; CLSID clsid; @@ -213,13 +213,18 @@ static HRESULT WINAPI ISF_MyComputer_fnParseDisplayName (IShellFolder2 *iface, SHCLSIDFromStringW (szElement + 2, &clsid); pidlTemp = _ILCreateGuid (PT_GUID, &clsid); } - /* do we have an absolute path name ? */ - else if (PathGetDriveNumberW (lpszDisplayName) >= 0 && - lpszDisplayName[2] == (WCHAR) '\\') + /* do we have an absolute path name? note that we can't use + PathGetDriveNumberW because we can't have the \\?\ prefix */ + else if ((c = toupperW(lpszDisplayName[0])) >= 'A' && c <= 'Z' && + lpszDisplayName[1] == ':' && + (lpszDisplayName[2] == '\\' || lpszDisplayName[2] == '\0')) { - szNext = GetNextElementW (lpszDisplayName, szElement, MAX_PATH); - /* make drive letter uppercase to enable PIDL comparison */ - szElement[0] = toupper(szElement[0]); + /* drive letter has to be uppercase to enable PIDL comparison */ + szElement[0] = c; + szElement[1] = ':'; + szElement[2] = '\\'; /* make sure it has a backslash here */ + szElement[3] = '\0'; + szNext = &lpszDisplayName[2] + (lpszDisplayName[2] == '\\'); pidlTemp = _ILCreateDrive (szElement); } -- 2.21.0
Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/shell32/shfldr_desktop.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/dlls/shell32/shfldr_desktop.c b/dlls/shell32/shfldr_desktop.c index 938871e..bdfc460 100644 --- a/dlls/shell32/shfldr_desktop.c +++ b/dlls/shell32/shfldr_desktop.c @@ -154,7 +154,7 @@ static HRESULT WINAPI ISF_Desktop_fnParseDisplayName (IShellFolder2 * iface, DWORD * pchEaten, LPITEMIDLIST * ppidl, DWORD * pdwAttributes) { IDesktopFolderImpl *This = impl_from_IShellFolder2(iface); - WCHAR szElement[MAX_PATH]; + WCHAR c, szElement[MAX_PATH]; LPCWSTR szNext = NULL; LPITEMIDLIST pidlTemp = NULL; PARSEDURLW urldata; @@ -182,7 +182,8 @@ static HRESULT WINAPI ISF_Desktop_fnParseDisplayName (IShellFolder2 * iface, SHCLSIDFromStringW (szElement + 2, &clsid); pidlTemp = _ILCreateGuid (PT_GUID, &clsid); } - else if (PathGetDriveNumberW (lpszDisplayName) >= 0) + /* we can't use PathGetDriveNumberW because we can't have the \\?\ prefix */ + else if ((c = toupperW(lpszDisplayName[0])) >= 'A' && c <= 'Z' && lpszDisplayName[1] == ':') { /* it's a filesystem path with a drive. Let MyComputer/UnixDosFolder parse it */ if (UNIXFS_is_rooted_at_desktop()) -- 2.21.0
Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/shell32/tests/shfldr_special.c | 69 +++++++++++++++++++++++++++++ 1 file changed, 69 insertions(+) diff --git a/dlls/shell32/tests/shfldr_special.c b/dlls/shell32/tests/shfldr_special.c index b73759e..69b7ab7 100644 --- a/dlls/shell32/tests/shfldr_special.c +++ b/dlls/shell32/tests/shfldr_special.c @@ -39,6 +39,74 @@ static inline BOOL SHELL_OsIsUnicode(void) return !(GetVersion() & 0x80000000); } +/* Tests for My Computer */ +static void test_parse_for_my_computer(void) +{ + WCHAR path[] = { '\\','\\','?','\\','C',':','\\',0 }; + WCHAR *drive = path + 4; + IShellFolder *mycomp, *sf; + ITEMIDLIST *pidl; + DWORD eaten; + HRESULT hr; + + hr = SHGetDesktopFolder(&sf); + ok(hr == S_OK, "SHGetDesktopFolder failed with error 0x%08x\n", hr); + if (hr != S_OK) return; + hr = SHGetSpecialFolderLocation(NULL, CSIDL_DRIVES, &pidl); + ok(hr == S_OK, "SHGetSpecialFolderLocation failed with error 0x%08x\n", hr); + if (hr != S_OK) + { + IShellFolder_Release(sf); + return; + } + hr = IShellFolder_BindToObject(sf, pidl, NULL, &IID_IShellFolder, (void**)&mycomp); + ok(hr == S_OK, "IShellFolder::BindToObject failed with error 0x%08x\n", hr); + IShellFolder_Release(sf); + ILFree(pidl); + if (hr != S_OK) return; + + while (drive[0] <= 'Z' && GetDriveTypeW(drive) == DRIVE_NO_ROOT_DIR) drive[0]++; + if (drive[0] > 'Z') + { + skip("No drive found, skipping My Computer tests...\n"); + goto done; + } + + pidl = NULL; + eaten = 0xdeadbeef; + hr = IShellFolder_ParseDisplayName(mycomp, NULL, NULL, drive, &eaten, &pidl, NULL); + ok(hr == S_OK, "IShellFolder::ParseDisplayName failed with error 0x%08x\n", hr); +todo_wine + ok(eaten == 0xdeadbeef, "eaten should not have been set to %u\n", eaten); + ok(pidl != NULL, "pidl is NULL\n"); + ILFree(pidl); + + /* \\?\ prefix is not valid */ + pidl = NULL; + eaten = 0xdeadbeef; + hr = IShellFolder_ParseDisplayName(mycomp, NULL, NULL, path, &eaten, &pidl, NULL); + ok(hr == E_INVALIDARG, "IShellFolder::ParseDisplayName should have failed with E_INVALIDARG, got 0x%08x\n", hr); +todo_wine + ok(eaten == 0xdeadbeef, "eaten should not have been set to %u\n", eaten); + ok(pidl == NULL, "pidl is not NULL\n"); + ILFree(pidl); + + /* Try without backslash */ + drive[2] = '\0'; + pidl = NULL; + eaten = 0xdeadbeef; + hr = IShellFolder_ParseDisplayName(mycomp, NULL, NULL, drive, &eaten, &pidl, NULL); + ok(hr == S_OK || broken(hr == E_INVALIDARG) /* WinXP */, + "IShellFolder::ParseDisplayName failed with error 0x%08x\n", hr); +todo_wine + ok(eaten == 0xdeadbeef, "eaten should not have been set to %u\n", eaten); + ok(pidl != NULL || broken(!pidl) /* WinXP */, "pidl is NULL\n"); + ILFree(pidl); + +done: + IShellFolder_Release(mycomp); +} + /* Tests for My Network Places */ static void test_parse_for_entire_network(void) { @@ -294,6 +362,7 @@ static void test_desktop_displaynameof(void) START_TEST(shfldr_special) { + test_parse_for_my_computer(); test_parse_for_entire_network(); test_parse_for_control_panel(); test_printers_folder(); -- 2.21.0
Hi, While running your changed tests, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=55692 Your paranoid android. === debian10 (32 bit WoW report) === shell32: shelllink.c:841: Test failed: save failed (0x80070020) shelllink.c:841: Test failed: got 0x00000001 shelllink.c:841: Test failed: Didn't expect NULL shelllink.c:506: Test failed: GetCurFile fails on shell32 < 5.0 shelllink.c:843: Test failed: GetDescription returned 'non-executable file' instead of 'batch file' shelllink.c:843: Test failed: GetIconLocation returned 'C:\users\winetest\Temp\\test.txt' instead of 'C:\users\winetest\Temp\test'
So I missed this earlier. These seem totally unrelated and a random hiccup on the testbot (failed to save a file?), especially since they only happened on one Debian VM. On 8/21/19 8:49 PM, Marvin wrote:
Hi,
While running your changed tests, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check?
Full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=55692
Your paranoid android.
=== debian10 (32 bit WoW report) ===
shell32: shelllink.c:841: Test failed: save failed (0x80070020) shelllink.c:841: Test failed: got 0x00000001 shelllink.c:841: Test failed: Didn't expect NULL shelllink.c:506: Test failed: GetCurFile fails on shell32 < 5.0 shelllink.c:843: Test failed: GetDescription returned 'non-executable file' instead of 'batch file' shelllink.c:843: Test failed: GetIconLocation returned 'C:\users\winetest\Temp\\test.txt' instead of 'C:\users\winetest\Temp\test'
participants (2)
-
Gabriel Ivăncescu -
Marvin