ReplaceFileW should fail with ERROR_ACCESS_DENIED when either the replaced or replacement file is set to read only using SetFileAttributes. Testing for the return values of ReplaceFileW was performed on a Windows XP SP3 and Windows 10 vm.
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@gmail.com --- dlls/kernel32/file.c | 29 ++++++++++++++++++++++++++++- dlls/kernel32/tests/file.c | 4 ++-- 2 files changed, 30 insertions(+), 3 deletions(-)
diff --git a/dlls/kernel32/file.c b/dlls/kernel32/file.c index 081f0760a5..b31b054f35 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; + BY_HANDLE_FILE_INFORMATION file_info;
TRACE("%s %s %s 0x%08x %p %p\n", debugstr_w(lpReplacedFileName), debugstr_w(lpReplacementFileName), debugstr_w(lpBackupFileName), @@ -1772,7 +1773,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); @@ -1788,6 +1789,19 @@ BOOL WINAPI ReplaceFileW(LPCWSTR lpReplacedFileName, LPCWSTR lpReplacementFileNa goto fail; }
+ /* Replacement should fail if replaced is READ_ONLY */ + if (!GetFileInformationByHandle(hReplaced, &file_info)) + { + error = GetLastError(); + goto fail; + } + + if (file_info.dwFileAttributes & 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 +1825,19 @@ BOOL WINAPI ReplaceFileW(LPCWSTR lpReplacedFileName, LPCWSTR lpReplacementFileNa goto fail; }
+ /* Replacement should fail if replacement is READ_ONLY */ + if (!GetFileInformationByHandle(hReplacement, &file_info)) + { + error = GetLastError(); + goto fail; + } + + if (file_info.dwFileAttributes & FILE_ATTRIBUTE_READONLY) + { + error = ERROR_ACCESS_DENIED; + goto fail; + } + /* If the user wants a backup then that needs to be performed first */ if (lpBackupFileName) { diff --git a/dlls/kernel32/tests/file.c b/dlls/kernel32/tests/file.c index 33cae4b7c4..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); - todo_wine ok(ret == 0 && GetLastError() == ERROR_ACCESS_DENIED, "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 || @@ -3698,7 +3698,7 @@ static void test_ReplaceFileA(void) "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); - todo_wine ok(ret, "ReplaceFileA: unexpected error %d\n", GetLastError()); + ok(ret, "ReplaceFileA: unexpected error %d\n", GetLastError()); CloseHandle(hReplacedFile);
/* replacement file still exists, make pass w/o "replaced" */
Brock York twunknown@gmail.com wrote:
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);
@@ -1788,6 +1789,19 @@ BOOL WINAPI ReplaceFileW(LPCWSTR lpReplacedFileName, LPCWSTR lpReplacementFileNa goto fail; }
- /* Replacement should fail if replaced is READ_ONLY */
- if (!GetFileInformationByHandle(hReplaced, &file_info))
- {
error = GetLastError();goto fail;- }
- if (file_info.dwFileAttributes & FILE_ATTRIBUTE_READONLY)
- {
error = ERROR_ACCESS_DENIED;goto fail;- }
There is an existing test for NtCreateFile that it's supposed to fail if the file has read-only attribute. Probably NtOpenFile with WRITE|DELETE access supposed to behave in a similar way. Adding such workarounds to high level API like ReplaceFile only clutters the code with not appropriate hacks.
On Sat, Sep 08, 2018 at 10:45:16AM +0800, Dmitry Timoshkov wrote:
Brock York twunknown@gmail.com wrote:
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);
@@ -1788,6 +1789,19 @@ BOOL WINAPI ReplaceFileW(LPCWSTR lpReplacedFileName, LPCWSTR lpReplacementFileNa goto fail; }
- /* Replacement should fail if replaced is READ_ONLY */
- if (!GetFileInformationByHandle(hReplaced, &file_info))
- {
error = GetLastError();goto fail;- }
- if (file_info.dwFileAttributes & FILE_ATTRIBUTE_READONLY)
- {
error = ERROR_ACCESS_DENIED;goto fail;- }
There is an existing test for NtCreateFile that it's supposed to fail if the file has read-only attribute. Probably NtOpenFile with WRITE|DELETE access supposed to behave in a similar way. Adding such workarounds to high level API like ReplaceFile only clutters the code with not appropriate hacks.
-- Dmitry.
Hello Dmitry
I had written a test to check that exact case before I implemented this patch but it turns out that I had screwed up my test program and was trying to NtOpenFile the wrong file... Which made me then think that the ACCESS_DENIED was coming from the ReplaceFileW call itself.
The MSDN documentation for ReplaceFileW states that the replaced file is opened with GENERIC_READ, DELETE and SYNCHRONISE. But the next sentence after mentions that the user must have write access, is this implying that the GENERIC_WRITE flag in passed in the like original author of ReplaceFileW for wine had implemented or is the MSDN probably wrong? ? If so the fix is easy modifying the check from NtOpenFile to handle the access_denied case which you were correct that when WRITE | DELETE is passed as the access flags to open a read only file NtOpenFile correctly returns ACCESS_DENIED
Thanks for the good pickup and feedback.
Warm regards Brock
-
Brock York -
Dmitry Timoshkov