[PATCH 1/3] kernel32/tests: Test ReplaceFileW on open "exe"
Replicate the scenario of an exe calling ReplaceFileW on itself. An exe calling ReplaceFileW on itself will fail in Wine currently but succeed in Windows 7. This seems to be the problem seen in bug 33845. Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=33845 Signed-off-by: Brock York <twunknown(a)gmail.com> --- dlls/kernel32/tests/file.c | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/dlls/kernel32/tests/file.c b/dlls/kernel32/tests/file.c index 1c4157279b..d496e9273b 100644 --- a/dlls/kernel32/tests/file.c +++ b/dlls/kernel32/tests/file.c @@ -3677,6 +3677,30 @@ static void test_ReplaceFileA(void) ok(ret || GetLastError() == ERROR_ACCESS_DENIED, "SetFileAttributesA: error setting to normal %d\n", GetLastError()); + /* re-create replacement file for pass w/ replaced opened with + * the same permissions as an exe (Replicating an exe trying to + * replace itself) + */ + ret = GetTempFileNameA(temp_path, prefix, 0, replacement); + ok(ret != 0, "GetTempFileNameA error (replacement) %d\n", GetLastError()); + + SetLastError(0xdeadbeef); + /* make sure that the replacement file still exists */ + hReplacementFile = CreateFileA(replacement, GENERIC_READ, FILE_SHARE_READ, NULL, OPEN_EXISTING, 0, 0); + ok(hReplacementFile != INVALID_HANDLE_VALUE || + broken(GetLastError() == ERROR_FILE_NOT_FOUND), /* win2k */ + "unexpected error, replacement file should still exist %d\n", GetLastError()); + CloseHandle(hReplacementFile); + + /* make sure that the replaced file is opened like an exe*/ + hReplacedFile = CreateFileA(replaced, GENERIC_READ | SYNCHRONIZE, FILE_SHARE_READ | FILE_SHARE_DELETE, NULL, OPEN_EXISTING, 0, 0); + ok(hReplacedFile != INVALID_HANDLE_VALUE, + "unexpected error, replaced file should be able to be opened %d\n", GetLastError()); + /*Calling ReplaceFileA on an exe should succeed*/ + ret = pReplaceFileA(replaced, replacement, NULL, 0, 0, 0); + ok(ret, "ReplaceFileA: unexpected error %d\n", GetLastError()); + CloseHandle(hReplacedFile); + /* replacement file still exists, make pass w/o "replaced" */ ret = DeleteFileA(replaced); ok(ret || GetLastError() == ERROR_ACCESS_DENIED, -- 2.18.0
Check that ReplaceFileW returned 0 indicating failure and that the error code was access denied. This corrects the error checking for the test case where ReplaceFileW should fail with ACCESS_DENIED on a file with the READ_ONLY attribute set. Tested on a Windows 10 and Windows XP vm and also passed on all vm's on the wine test bot Signed-off-by: Brock York <twunknown(a)gmail.com> --- dlls/kernel32/tests/file.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dlls/kernel32/tests/file.c b/dlls/kernel32/tests/file.c index d496e9273b..8e4a994142 100644 --- a/dlls/kernel32/tests/file.c +++ b/dlls/kernel32/tests/file.c @@ -3666,7 +3666,7 @@ static void test_ReplaceFileA(void) */ SetLastError(0xdeadbeef); ret = pReplaceFileA(replaced, replacement, backup, 0, 0, 0); - ok(ret != ERROR_UNABLE_TO_REMOVE_REPLACED, "ReplaceFileA: unexpected error %d\n", GetLastError()); + ok(ret == 0 && GetLastError() == ERROR_ACCESS_DENIED, "ReplaceFileA: unexpected error %d\n", GetLastError()); /* make sure that the replacement file still exists */ hReplacementFile = CreateFileA(replacement, GENERIC_READ, FILE_SHARE_READ, NULL, OPEN_EXISTING, 0, 0); ok(hReplacementFile != INVALID_HANDLE_VALUE || -- 2.18.0
Remove GENERIC_WRITE flag when opening replaced file. ReplaceFileW will fail when called to replace the current executable. This is not the same behaviour as tested on Windows 7, 10 and XP this is because the "replaced" file is opened with the GENERIC_WRITE flag. The MSDN also mentions that the replaced file is only opened with GENERIC_READ, DELETE and SYNCHRONIZE. This patch will fix the following bug once it is merged into Wine-Staging as the WarFrame launcher requires patches from Wine-Staging to work. Wine-Bug:https://bugs.winehq.org/show_bug.cgi?id=33845 Signed-off-by: Brock York <twunknown(a)gmail.com> --- dlls/kernel32/file.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dlls/kernel32/file.c b/dlls/kernel32/file.c index b558f8e205..3fdac41c50 100644 --- a/dlls/kernel32/file.c +++ b/dlls/kernel32/file.c @@ -1772,7 +1772,7 @@ BOOL WINAPI ReplaceFileW(LPCWSTR lpReplacedFileName, LPCWSTR lpReplacementFileNa } replaced_flags = lpBackupFileName ? FILE_OPEN : FILE_OPEN_IF; attr.ObjectName = &nt_replaced_name; - status = NtOpenFile(&hReplaced, GENERIC_READ|GENERIC_WRITE|DELETE|SYNCHRONIZE, + status = NtOpenFile(&hReplaced, GENERIC_READ|DELETE|SYNCHRONIZE, &attr, &io, FILE_SHARE_READ|FILE_SHARE_WRITE|FILE_SHARE_DELETE, FILE_SYNCHRONOUS_IO_NONALERT|FILE_NON_DIRECTORY_FILE); -- 2.18.0
Brock York <twunknown(a)gmail.com> writes:
Remove GENERIC_WRITE flag when opening replaced file. ReplaceFileW will fail when called to replace the current executable. This is not the same behaviour as tested on Windows 7, 10 and XP this is because the "replaced" file is opened with the GENERIC_WRITE flag. The MSDN also mentions that the replaced file is only opened with GENERIC_READ, DELETE and SYNCHRONIZE.
This patch will fix the following bug once it is merged into Wine-Staging as the WarFrame launcher requires patches from Wine-Staging to work. Wine-Bug:https://bugs.winehq.org/show_bug.cgi?id=33845
Signed-off-by: Brock York <twunknown(a)gmail.com> --- dlls/kernel32/file.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
It breaks the tests: ../../../tools/runtest -q -P wine -T ../../.. -M kernel32.dll -p kernel32_test.exe.so file && touch file.ok file.c:904: Tests skipped: CopyFile2 is not available file.c:1328: Tests skipped: Either no authority to volume, or is todo_wine for C:\users\julliard\Temp\ err=5 should be 3 file.c:1328: Tests skipped: Either no authority to volume, or is todo_wine for C:\users\julliard\Temp\removeme\ err=5 should be 3 file.c:1328: Tests skipped: Either no authority to volume, or is todo_wine for C:\ err=5 should be 3 file.c:1328: Tests skipped: Either no authority to volume, or is todo_wine for \\?\C:\ err=5 should be 3 file.c:3669: Test failed: ReplaceFileA: unexpected error -559038737 file.c:3672: Test failed: unexpected error, replacement file should still exist 2 file.c:3737: Test failed: DeleteFileA: error (backup) 5 file.c:3825: Test failed: DeleteFileW: error (backup) 5 make: *** [Makefile:477: file.ok] Error 4 -- Alexandre Julliard julliard(a)winehq.org
Replicate the scenario of an exe calling ReplaceFileW on itself. An exe calling ReplaceFileW on itself will fail in Wine currently but succeed in Windows 7. This seems to be the problem seen in bug 33845. Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=33845 Signed-off-by: Brock York <twunknown(a)gmail.com> --- dlls/kernel32/tests/file.c | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/dlls/kernel32/tests/file.c b/dlls/kernel32/tests/file.c index 1c4157279b..d496e9273b 100644 --- a/dlls/kernel32/tests/file.c +++ b/dlls/kernel32/tests/file.c @@ -3677,6 +3677,30 @@ static void test_ReplaceFileA(void) ok(ret || GetLastError() == ERROR_ACCESS_DENIED, "SetFileAttributesA: error setting to normal %d\n", GetLastError()); + /* re-create replacement file for pass w/ replaced opened with + * the same permissions as an exe (Replicating an exe trying to + * replace itself) + */ + ret = GetTempFileNameA(temp_path, prefix, 0, replacement); + ok(ret != 0, "GetTempFileNameA error (replacement) %d\n", GetLastError()); + + SetLastError(0xdeadbeef); + /* make sure that the replacement file still exists */ + hReplacementFile = CreateFileA(replacement, GENERIC_READ, FILE_SHARE_READ, NULL, OPEN_EXISTING, 0, 0); + ok(hReplacementFile != INVALID_HANDLE_VALUE || + broken(GetLastError() == ERROR_FILE_NOT_FOUND), /* win2k */ + "unexpected error, replacement file should still exist %d\n", GetLastError()); + CloseHandle(hReplacementFile); + + /* make sure that the replaced file is opened like an exe*/ + hReplacedFile = CreateFileA(replaced, GENERIC_READ | SYNCHRONIZE, FILE_SHARE_READ | FILE_SHARE_DELETE, NULL, OPEN_EXISTING, 0, 0); + ok(hReplacedFile != INVALID_HANDLE_VALUE, + "unexpected error, replaced file should be able to be opened %d\n", GetLastError()); + /*Calling ReplaceFileA on an exe should succeed*/ + ret = pReplaceFileA(replaced, replacement, NULL, 0, 0, 0); + ok(ret, "ReplaceFileA: unexpected error %d\n", GetLastError()); + CloseHandle(hReplacedFile); + /* replacement file still exists, make pass w/o "replaced" */ ret = DeleteFileA(replaced); ok(ret || GetLastError() == ERROR_ACCESS_DENIED, -- 2.18.0
Brock York <twunknown(a)gmail.com> writes:
+ /* make sure that the replaced file is opened like an exe*/ + hReplacedFile = CreateFileA(replaced, GENERIC_READ | SYNCHRONIZE, FILE_SHARE_READ | FILE_SHARE_DELETE, NULL, OPEN_EXISTING, 0, 0); + ok(hReplacedFile != INVALID_HANDLE_VALUE, + "unexpected error, replaced file should be able to be opened %d\n", GetLastError()); + /*Calling ReplaceFileA on an exe should succeed*/ + ret = pReplaceFileA(replaced, replacement, NULL, 0, 0, 0); + ok(ret, "ReplaceFileA: unexpected error %d\n", GetLastError()); + CloseHandle(hReplacedFile);
This fails on Wine: file.c:3701: Test failed: ReplaceFileA: unexpected error 1175 Please note that tests that currently fail on Wine must be marked todo_wine, with the todo removed in the patch that fixes the bug. All the tests need to succeed after every patch. -- Alexandre Julliard julliard(a)winehq.org
Check that ReplaceFileW returned 0 indicating failure and that the error code was access denied. This corrects the error checking for the test case where ReplaceFileW should fail with ACCESS_DENIED on a file with the READ_ONLY attribute set. Tested on a Windows 10 and Windows XP vm and also passed on all vm's on the wine test bot Signed-off-by: Brock York <twunknown(a)gmail.com> --- dlls/kernel32/tests/file.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dlls/kernel32/tests/file.c b/dlls/kernel32/tests/file.c index d496e9273b..8e4a994142 100644 --- a/dlls/kernel32/tests/file.c +++ b/dlls/kernel32/tests/file.c @@ -3666,7 +3666,7 @@ static void test_ReplaceFileA(void) */ SetLastError(0xdeadbeef); ret = pReplaceFileA(replaced, replacement, backup, 0, 0, 0); - ok(ret != ERROR_UNABLE_TO_REMOVE_REPLACED, "ReplaceFileA: unexpected error %d\n", GetLastError()); + ok(ret == 0 && GetLastError() == ERROR_ACCESS_DENIED, "ReplaceFileA: unexpected error %d\n", GetLastError()); /* make sure that the replacement file still exists */ hReplacementFile = CreateFileA(replacement, GENERIC_READ, FILE_SHARE_READ, NULL, OPEN_EXISTING, 0, 0); ok(hReplacementFile != INVALID_HANDLE_VALUE || -- 2.18.0
Remove GENERIC_WRITE flag when opening replaced file. ReplaceFileW will fail when called to replace the current executable. This is not the same behaviour as tested on Windows 7, 10 and XP this is because the "replaced" file is opened with the GENERIC_WRITE flag. The MSDN also mentions that the replaced file is only opened with GENERIC_READ, DELETE and SYNCHRONIZE. This patch will fix the following bug once it is merged into Wine-Staging as the WarFrame launcher requires patches from Wine-Staging to work. Wine-Bug:https://bugs.winehq.org/show_bug.cgi?id=33845 Signed-off-by: Brock York <twunknown(a)gmail.com> --- dlls/kernel32/file.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dlls/kernel32/file.c b/dlls/kernel32/file.c index b558f8e205..3fdac41c50 100644 --- a/dlls/kernel32/file.c +++ b/dlls/kernel32/file.c @@ -1772,7 +1772,7 @@ BOOL WINAPI ReplaceFileW(LPCWSTR lpReplacedFileName, LPCWSTR lpReplacementFileNa } replaced_flags = lpBackupFileName ? FILE_OPEN : FILE_OPEN_IF; attr.ObjectName = &nt_replaced_name; - status = NtOpenFile(&hReplaced, GENERIC_READ|GENERIC_WRITE|DELETE|SYNCHRONIZE, + status = NtOpenFile(&hReplaced, GENERIC_READ|DELETE|SYNCHRONIZE, &attr, &io, FILE_SHARE_READ|FILE_SHARE_WRITE|FILE_SHARE_DELETE, FILE_SYNCHRONOUS_IO_NONALERT|FILE_NON_DIRECTORY_FILE); -- 2.18.0
ReplaceFileW should fail with ERROR_ACCESS_DENIED when either the replaced or replacement file is set to read only using SetFileAttributes. This solves the issue of ReplaceFileW tests failing with another patch in this series because replacement was occuring on a file that is set to read only. Testing for the return values of ReplaceFileW was performed on a Windows XP SP3 and Windows 10 vm. Signed-off-by: Brock York <twunknown(a)gmail.com> --- dlls/kernel32/file.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/dlls/kernel32/file.c b/dlls/kernel32/file.c index 3fdac41c50..1858866cf3 100644 --- a/dlls/kernel32/file.c +++ b/dlls/kernel32/file.c @@ -1738,6 +1738,7 @@ BOOL WINAPI ReplaceFileW(LPCWSTR lpReplacedFileName, LPCWSTR lpReplacementFileNa NTSTATUS status; IO_STATUS_BLOCK io; OBJECT_ATTRIBUTES attr; + DWORD fattribs; TRACE("%s %s %s 0x%08x %p %p\n", debugstr_w(lpReplacedFileName), debugstr_w(lpReplacementFileName), debugstr_w(lpBackupFileName), @@ -1788,6 +1789,14 @@ BOOL WINAPI ReplaceFileW(LPCWSTR lpReplacedFileName, LPCWSTR lpReplacementFileNa goto fail; } + /* Replacement should fail if replaced is READ_ONLY */ + fattribs = GetFileAttributesW(lpReplacedFileName); + if (fattribs & FILE_ATTRIBUTE_READONLY) + { + error = ERROR_ACCESS_DENIED; + goto fail; + } + /* * Open the replacement file for reading, writing, and deleting * (writing and deleting are needed when finished) @@ -1811,6 +1820,14 @@ BOOL WINAPI ReplaceFileW(LPCWSTR lpReplacedFileName, LPCWSTR lpReplacementFileNa goto fail; } + /* Replacement should fail if replacement is READ_ONLY */ + fattribs = GetFileAttributesW(lpReplacementFileName); + if (fattribs & FILE_ATTRIBUTE_READONLY) + { + error = ERROR_ACCESS_DENIED; + goto fail; + } + /* If the user wants a backup then that needs to be performed first */ if (lpBackupFileName) { -- 2.18.0
participants (2)
-
Alexandre Julliard -
Brock York