[PATCH 4/4] kernel32: Open replaced file without GENERIC_WRITE in ReplaceFileW().

[Zhiyi Zhang]

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(a)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 */
-- 
2.20.1

[Dmitry Timoshkov]

Zhiyi Zhang <zzhang(a)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.

-- 
Dmitry.

[Zhiyi Zhang]

On 4/9/19 9:41 PM, Dmitry Timoshkov wrote:
> Zhiyi Zhang <zzhang(a)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.