As MSDN says, the replaced file is opened with the GENERIC_READ, DELETE, and SYNCHRONIZE access rights. If GENERIC_WRITE is added, then it will fail with previous file opens without FILE_SHARE_WRITE.
And then we also need to check if the replaced file has readonly attributes now that GENERIC_WRITE is gone. It seems fine to add a check in kernel32, otherwise we need add an extra server call to rename files and checking the attribute in wineserver.
Wine-Bug:https://bugs.winehq.org/show_bug.cgi?id=33845 Signed-off-by: Zhiyi Zhang zzhang@codeweavers.com --- dlls/kernel32/file.c | 19 +++++++++++++++++-- dlls/kernel32/tests/file.c | 4 ++-- 2 files changed, 19 insertions(+), 4 deletions(-)
diff --git a/dlls/kernel32/file.c b/dlls/kernel32/file.c index c66073b820..64c3e3e826 100644 --- a/dlls/kernel32/file.c +++ b/dlls/kernel32/file.c @@ -1746,6 +1746,7 @@ BOOL WINAPI ReplaceFileW(LPCWSTR lpReplacedFileName, LPCWSTR lpReplacementFileNa NTSTATUS status; IO_STATUS_BLOCK io; OBJECT_ATTRIBUTES attr; + FILE_BASIC_INFORMATION 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 attr.SecurityDescriptor = NULL; attr.SecurityQualityOfService = NULL;
- /* Open the "replaced" file for reading and writing */ + /* Open the "replaced" file for reading */ if (!(RtlDosPathNameToNtPathName_U(lpReplacedFileName, &nt_replaced_name, NULL, NULL))) { error = ERROR_PATH_NOT_FOUND; @@ -1780,7 +1781,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); @@ -1796,6 +1797,20 @@ BOOL WINAPI ReplaceFileW(LPCWSTR lpReplacedFileName, LPCWSTR lpReplacementFileNa goto fail; }
+ /* Replacement should fail if replaced is READ_ONLY */ + status = NtQueryInformationFile(hReplaced, &io, &info, sizeof(info), FileBasicInformation); + if (status != STATUS_SUCCESS) + { + error = RtlNtStatusToDosError(status); + goto fail; + } + + if (info.FileAttributes & 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) diff --git a/dlls/kernel32/tests/file.c b/dlls/kernel32/tests/file.c index 65ef4a6b43..4df31c15c7 100644 --- a/dlls/kernel32/tests/file.c +++ b/dlls/kernel32/tests/file.c @@ -3691,7 +3691,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 || @@ -3727,7 +3727,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);
/* replace file while replacement is opened */
Zhiyi Zhang zzhang@codeweavers.com wrote:
And then we also need to check if the replaced file has readonly attributes now that GENERIC_WRITE is gone. It seems fine to add a check in kernel32, otherwise we need add an extra server call to rename files and checking the attribute in wineserver.
I'd guess that NtOpenFile(DELETE) should fail for read-only files making this check on client side redundant.
On 4/9/19 9:41 PM, Dmitry Timoshkov wrote:
Zhiyi Zhang zzhang@codeweavers.com wrote:
And then we also need to check if the replaced file has readonly attributes now that GENERIC_WRITE is gone. It seems fine to add a check in kernel32, otherwise we need add an extra server call to rename files and checking the attribute in wineserver.
I'd guess that NtOpenFile(DELETE) should fail for read-only files making this check on client side redundant.
NtOpenFile(DELETE) fails for read-only files only when FILE_DELETE_ON_CLOSE is used together. We can't use FILE_DELETE_ON_CLOSE when opening the replaced file because we need to keep the file if there is any error.
We also can't use NtOpenFile to open the replaced file first without checking for readonly and later use NtOpenFile(DELETE) with FILE_DELETE_ON_CLOSE to delete the file. Because such a way will allow a backup to be created when replaced is readonly and then ReplaceFileW fail. I tested this behavior but I didn't include it in the tests.
-
Dmitry Timoshkov -
Zhiyi Zhang