Re: [1/2] shell32: Check IsWow64Process() before calling Wow64 functions.
On 21.04.2016 3:12, Olivier F. R. Dierick wrote:
See bug 36838 for comments and discussion.
Supersedes patch submission 121402.
Better version of the patch. Call IsWow64Process() only once on dll initialization and setup a global variable.
From e7b7317755faff75b960f9143b2195344113a9a4 Mon Sep 17 00:00:00 2001 From: "Olivier F. R. Dierick" <o.dierick(a)piezo-forte.be> Date: Wed, 20 Apr 2016 06:45:38 +0200 Subject: shell32: Check IsWow64Process() before calling Wow64 functions.
Signed-off-by: Olivier F. R. Dierick <o.dierick(a)piezo-forte.be> --- dlls/shell32/shell32_main.c | 3 +++ dlls/shell32/shell32_main.h | 1 + dlls/shell32/shfldr_unixfs.c | 6 ++++-- 3 files changed, 8 insertions(+), 2 deletions(-)
diff --git a/dlls/shell32/shell32_main.c b/dlls/shell32/shell32_main.c index 74deecd..98e7fbf 100644 --- a/dlls/shell32/shell32_main.c +++ b/dlls/shell32/shell32_main.c @@ -1255,6 +1255,7 @@ HRESULT WINAPI DllGetVersion (DLLVERSIONINFO *pdvi) * */ HINSTANCE shell32_hInstance = 0; +BOOL is_wow64 = FALSE;
/************************************************************************* @@ -1277,6 +1278,8 @@ BOOL WINAPI DllMain(HINSTANCE hinstDLL, DWORD fdwReason, LPVOID fImpLoad) GetModuleFileNameW(hinstDLL, swShell32Name, MAX_PATH); swShell32Name[MAX_PATH - 1] = '\0';
+ if (!IsWow64Process( GetCurrentProcess(), &is_wow64 )) is_wow64 = FALSE; + InitChangeNotifications(); break;
diff --git a/dlls/shell32/shell32_main.h b/dlls/shell32/shell32_main.h index 492f79f..7a4d9b2 100644 --- a/dlls/shell32/shell32_main.h +++ b/dlls/shell32/shell32_main.h @@ -42,6 +42,7 @@ */ extern HMODULE huser32 DECLSPEC_HIDDEN; extern HINSTANCE shell32_hInstance DECLSPEC_HIDDEN; +extern BOOL is_wow64 DECLSPEC_HIDDEN;
/* Iconcache */ #define INVALID_INDEX -1 diff --git a/dlls/shell32/shfldr_unixfs.c b/dlls/shell32/shfldr_unixfs.c index be1ba81..d9b1320 100644 --- a/dlls/shell32/shfldr_unixfs.c +++ b/dlls/shell32/shfldr_unixfs.c @@ -414,7 +414,8 @@ static BOOL UNIXFS_get_unix_path(LPCWSTR pszDosPath, char *pszCanonicalPath) dospath_end = dospath + lstrlenW(dospath); /* search for the most valid UNIX path possible, then append missing * path parts */ - Wow64DisableWow64FsRedirection(&redir); + if(is_wow64) + Wow64DisableWow64FsRedirection(&redir); while(!(pszUnixPath = wine_get_unix_file_name(dospath))){ if(has_failed){ *dospath_end = '/'; @@ -428,7 +429,8 @@ static BOOL UNIXFS_get_unix_path(LPCWSTR pszDosPath, char *pszCanonicalPath) } *dospath_end = '\0'; } - Wow64RevertWow64FsRedirection(redir); + if(is_wow64) + Wow64RevertWow64FsRedirection(redir); if(dospath_end < dospath) return FALSE; strcat(szPath, pszUnixPath + cDriveSymlinkLen);
Like I said in bug comment, doing this is depending on a side effect. SHFileOperation() could do anything internally, if you want to have consistent ERROR_SUCCESS last error (even though it's kind of wrong to check it, since it returns error code already), why not force it in SHFileOperation() directly? Changing internal path is wrong in my opinion.
Le jeudi 21 avril 2016 à 03:28 +0300, Nikolay Sivov a écrit :
On 21.04.2016 3:12, Olivier F. R. Dierick wrote:
See bug 36838 for comments and discussion.
Supersedes patch submission 121402.
Better version of the patch. Call IsWow64Process() only once on dll initialization and setup a global variable.
Like I said in bug comment, doing this is depending on a side effect. SHFileOperation() could do anything internally, if you want to have consistent ERROR_SUCCESS last error (even though it's kind of wrong to check it, since it returns error code already), why not force it in SHFileOperation() directly? Changing internal path is wrong in my opinion.
I don't know if resetting last error in SHFileOperation() is conformant. That's why I took the route to fix one error at a time until there were none left. More errors may be introduced later, but for now this patch is the last one needed to fix bug 36838. I find forcing ERROR_SUCCESS in SHFileOperation() a brute and lazy way to fix things. It may hide other errors. Fixing the internal code itself may avoid bugs in other functions and is the way to go in my opinion. -- Olivier F. R. Dierick o.dierick(a)piezo-forte.be
On 21.04.2016 7:04, Olivier F. R. Dierick wrote:
Le jeudi 21 avril 2016 à 03:28 +0300, Nikolay Sivov a écrit :
On 21.04.2016 3:12, Olivier F. R. Dierick wrote:
See bug 36838 for comments and discussion.
Supersedes patch submission 121402.
Better version of the patch. Call IsWow64Process() only once on dll initialization and setup a global variable.
Like I said in bug comment, doing this is depending on a side effect. SHFileOperation() could do anything internally, if you want to have consistent ERROR_SUCCESS last error (even though it's kind of wrong to check it, since it returns error code already), why not force it in SHFileOperation() directly? Changing internal path is wrong in my opinion.
I don't know if resetting last error in SHFileOperation() is conformant. That's why I took the route to fix one error at a time until there were none left. More errors may be introduced later, but for now this patch is the last one needed to fix bug 36838.
I find forcing ERROR_SUCCESS in SHFileOperation() a brute and lazy way to fix things. It may hide other errors. Fixing the internal code itself may avoid bugs in other functions and is the way to go in my opinion.
Well, it's clearly an application bug to check it in a first place. Fixing it this way you're now depending on implementation details, and like you said it can affect more than you tested.
"Olivier F. R. Dierick" <o.dierick(a)piezo-forte.be> writes:
I don't know if resetting last error in SHFileOperation() is conformant. That's why I took the route to fix one error at a time until there were none left. More errors may be introduced later, but for now this patch is the last one needed to fix bug 36838.
I find forcing ERROR_SUCCESS in SHFileOperation() a brute and lazy way to fix things. It may hide other errors. Fixing the internal code itself may avoid bugs in other functions and is the way to go in my opinion.
If the application depends on this, setting it explicitly is better than depending on side effects of internal code paths. -- Alexandre Julliard julliard(a)winehq.org
Le jeudi 21 avril 2016 à 16:40 +0900, Alexandre Julliard a écrit :
"Olivier F. R. Dierick" <o.dierick(a)piezo-forte.be> writes:
I find forcing ERROR_SUCCESS in SHFileOperation() a brute and lazy way to fix things. It may hide other errors. Fixing the internal code itself may avoid bugs in other functions and is the way to go in my opinion.
If the application depends on this, setting it explicitly is better than depending on side effects of internal code paths.
Ok, the patch does not fix bug 36838. I'll follow your recommandation and make another one. Still, I found code that generates errors for no purpose on 32 bit wine and made a patch to fix it, avoiding possible error leaks and side effects. Will it be considered? -- Olivier F. R. Dierick o.dierick(a)piezo-forte.be
"Olivier F. R. Dierick" <o.dierick(a)piezo-forte.be> writes:
Ok, the patch does not fix bug 36838. I'll follow your recommandation and make another one.
Still, I found code that generates errors for no purpose on 32 bit wine and made a patch to fix it, avoiding possible error leaks and side effects. Will it be considered?
In general, errors generated in internal code paths don't matter. If the function fails in the end, it should set a proper error code. If it doesn't fail, the error code is irrelevant and isn't supposed to be checked by the app. In cases where an app mistakenly does check it, again we have to set it explicitly. Complicating internal code paths to avoid changing last error is not worth the trouble; in many cases the same thing happens on Windows anyway. -- Alexandre Julliard julliard(a)winehq.org
participants (3)
-
Alexandre Julliard -
Nikolay Sivov -
Olivier F. R. Dierick