[PATCH v3 0/3] MR1895: Fixes for NtOpenFile of read-only files with FILE_WRITE_ATTRIBUTES

[Joel Holdsworth (＠jhol)]

Previously, the Wine implementation of `NtOpenFile` would fail to open read-only files for
`FILE_WRITE_ATTRIBUTES` access in wine-server, because it would attempt to open the file with `O_RDWR`
or `O_WRONLY` access.

This patch-set adds test cases to highlight the issue, and solves it by only opening the file with write access where necessary.

* Wine Bug: [#50771](https://bugs.winehq.org/show_bug.cgi?id=50771)
  * Related Wine-Staging Patches:
      * [0002-server-Allow-to-open-files-without-any-permission-bi.patch](https://gitlab.winehq.org/wine/wine-staging/-/blob/master/patches/server-Fil...)
      * [0006-ntdll-tests-Added-tests-for-open-behaviour-on-readon.patch](https://gitlab.winehq.org/wine/wine-staging/-/blob/master/patches/server-Fil...)
      * [0007-server-FILE_WRITE_ATTRIBUTES-should-succeed-for-read.patch](https://gitlab.winehq.org/wine/wine-staging/-/blob/master/patches/server-Fil...)

--
  v3: server: Allow to open read-only files with FILE_WRITE_ATTRIBUTES

https://gitlab.winehq.org/wine/wine/-/merge_requests/1895

[PATCH v3 1/3] ntdll/tests: Added tests for attributes access on read-only files.

[Joel Holdsworth]

From: Joel Holdsworth

This patch was inspired by the wine-staging patch:

server-File_Permissions/0006-ntdll-tests-Added-tests-for-open-behaviour-on-readon.patch

Co-authored-by: Qian Hong 
Signed-off-by: Joel Holdsworth 
---
 dlls/ntdll/tests/file.c | 44 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 44 insertions(+)

diff --git a/dlls/ntdll/tests/file.c b/dlls/ntdll/tests/file.c
index 6186afdfb63..57dee54076a 100644
--- a/dlls/ntdll/tests/file.c
+++ b/dlls/ntdll/tests/file.c
@@ -5279,6 +5279,7 @@ static void test_file_readonly_access(void)
     static const WCHAR fooW[] = {'f', 'o', 'o', 0};
     WCHAR path[MAX_PATH];
     OBJECT_ATTRIBUTES attr;
+    FILE_BASIC_INFORMATION fbi;
     UNICODE_STRING nameW;
     IO_STATUS_BLOCK io;
     HANDLE handle;
@@ -5303,6 +5304,49 @@ static void test_file_readonly_access(void)
     ok(status == STATUS_SUCCESS, "expected STATUS_SUCCESS, got %#lx.\n", status);
     CloseHandle(handle);

+    /* NtOpenFile FILE_READ_ATTRIBUTES */
+    status = pNtOpenFile(&handle, FILE_READ_ATTRIBUTES,  &attr, &io, default_sharing, FILE_NON_DIRECTORY_FILE);
+    ok(status == STATUS_SUCCESS, "got %#lx\n", status);
+
+    memset(&fbi, 0, sizeof(fbi));
+    status = pNtQueryInformationFile(handle, &io, &fbi, sizeof fbi, FileBasicInformation);
+    ok(status == STATUS_SUCCESS, "can't get attributes, status %#lx\n", status);
+    ok((fbi.FileAttributes & FILE_ATTRIBUTE_READONLY) == FILE_ATTRIBUTE_READONLY,
+       "attribute %lx not expected\n", fbi.FileAttributes );
+
+    CloseHandle(handle);
+
+    /* NtOpenFile FILE_WRITE_ATTRIBUTES */
+    status = pNtOpenFile(&handle, FILE_WRITE_ATTRIBUTES,  &attr, &io, default_sharing, FILE_NON_DIRECTORY_FILE);
+    todo_wine ok(status == STATUS_SUCCESS, "got %#lx\n", status);
+
+    if (status == STATUS_SUCCESS) {
+        memset(&fbi, 0, sizeof(fbi));
+        fbi.FileAttributes = FILE_ATTRIBUTE_NORMAL;
+        U(io).Status = 0xdeadbeef;
+        status = pNtSetInformationFile(handle, &io, &fbi, sizeof fbi, FileBasicInformation);
+        todo_wine ok(status == STATUS_SUCCESS, "can't set system attribute, NtSetInformationFile returned %#lx\n",
+                     status);
+        todo_wine ok(U(io).Status == STATUS_SUCCESS, "can't set system attribute, io.Status is %lx\n",
+                     U(io).Status);
+
+        fbi.FileAttributes = FILE_ATTRIBUTE_READONLY;
+        U(io).Status = 0xdeadbeef;
+        status = pNtSetInformationFile(handle, &io, &fbi, sizeof fbi, FileBasicInformation);
+        todo_wine ok(status == STATUS_SUCCESS, "can't set system attribute, NtSetInformationFile returned %#lx\n",
+                     status);
+        todo_wine ok(U(io).Status == STATUS_SUCCESS, "can't set system attribute, io.Status is %lx\n",
+                     U(io).Status);
+    }
+
+    CloseHandle(handle);
+
+    /* NtOpenFile FILE_READ_ATTRIBUTES and FILE_WRITE_ATTRIBUTES */
+    status = pNtOpenFile(&handle, FILE_READ_ATTRIBUTES|FILE_WRITE_ATTRIBUTES,  &attr, &io,
+                         default_sharing, FILE_NON_DIRECTORY_FILE);
+    todo_wine ok(status == STATUS_SUCCESS, "got %#lx\n", status);
+    CloseHandle(handle);
+
     /* NtCreateFile FILE_GENERIC_WRITE */
     status = pNtCreateFile(&handle, FILE_GENERIC_WRITE, &attr, &io, NULL, FILE_ATTRIBUTE_NORMAL, default_sharing,
                            FILE_OPEN, FILE_NON_DIRECTORY_FILE, NULL, 0);

[PATCH v3 2/3] server: Open code FILE_UNIX_{READ,WRITE}_ACCESS

[Joel Holdsworth]

From: Joel Holdsworth

These defines are counter-productive for code clarity.

Signed-off-by: Joel Holdsworth 
---
 server/fd.c   | 8 +++++---
 server/file.h | 7 -------
 2 files changed, 5 insertions(+), 10 deletions(-)

diff --git a/server/fd.c b/server/fd.c
index eaebe044f37..839a1dec914 100644
--- a/server/fd.c
+++ b/server/fd.c
@@ -1941,9 +1941,10 @@ struct fd *open_fd( struct fd *root, const char *name, struct unicode_str nt_nam
         flags &= ~(O_CREAT | O_EXCL | O_TRUNC);
     }

-    if ((access & FILE_UNIX_WRITE_ACCESS) && !(options & FILE_DIRECTORY_FILE))
+    if ((access & (FILE_WRITE_DATA|FILE_APPEND_DATA|FILE_WRITE_ATTRIBUTES|FILE_WRITE_EA)) &&
+        !(options & FILE_DIRECTORY_FILE))
     {
-        if (access & FILE_UNIX_READ_ACCESS) rw_mode = O_RDWR;
+        if (access & (FILE_READ_DATA|FILE_READ_ATTRIBUTES|FILE_READ_EA)) rw_mode = O_RDWR;
         else rw_mode = O_WRONLY;
     }
     else rw_mode = O_RDONLY;
@@ -1953,7 +1954,8 @@ struct fd *open_fd( struct fd *root, const char *name, struct unicode_str nt_nam
         /* if we tried to open a directory for write access, retry read-only */
         if (errno == EISDIR)
         {
-            if ((access & FILE_UNIX_WRITE_ACCESS) || (flags & O_CREAT))
+            if ((access & (FILE_WRITE_DATA|FILE_APPEND_DATA|FILE_WRITE_ATTRIBUTES|FILE_WRITE_EA)) ||
+                (flags & O_CREAT))
                 fd->unix_fd = open( name, O_RDONLY | (flags & ~(O_TRUNC | O_CREAT | O_EXCL)), *mode );
         }

diff --git a/server/file.h b/server/file.h
index 0ffe0e2c8dc..a0c739f3605 100644
--- a/server/file.h
+++ b/server/file.h
@@ -257,13 +257,6 @@ static inline int async_queued( struct async_queue *queue )
     return !list_empty( &queue->queue );
 }

-
-/* access rights that require Unix read permission */
-#define FILE_UNIX_READ_ACCESS (FILE_READ_DATA|FILE_READ_ATTRIBUTES|FILE_READ_EA)
-
-/* access rights that require Unix write permission */
-#define FILE_UNIX_WRITE_ACCESS (FILE_WRITE_DATA|FILE_APPEND_DATA|FILE_WRITE_ATTRIBUTES|FILE_WRITE_EA)
-
 /* magic file access rights for mappings */
 #define FILE_MAPPING_IMAGE  0x80000000  /* set for SEC_IMAGE mappings */
 #define FILE_MAPPING_WRITE  0x40000000  /* set for writable shared mappings */

[PATCH v3 3/3] server: Allow to open read-only files with FILE_WRITE_ATTRIBUTES

[Joel Holdsworth]

From: Joel Holdsworth

The Msys2 port of the pacman package manager creates a read-only lock file during package
installation. When it is time to delete this files, pacman calls the Msys2 implementation of
unlink(2). This function is implemented in msys2-runtime:

https://github.com/msys2/msys2-runtime/blob/msys2-3.4.3/winsup/cygwin/syscal...

A similar implementation exists in Cygwin:

https://www.cygwin.com/git/?p=newlib-cygwin.git;a=blob;f=winsup/cygwin/sysca...

The implementation works by first opening the file using NtOpenFile with FILE_WRITE_ATTRIBUTES
permissions only, it then clears the FILE_ATTRIBUTE_READONLY flag using NtSetAttributesFile, then
repoens the file with DELETE permissions.

Previously, the Wine implementation of NtOpenFile would fail to open read-only files for
FILE_WRITE_ATTRIBUTES access in wine-server, because it would attempt to open the file with O_RDWR
or O_WRONLY access.

This patch solves this issue by only opening the file for writing if O_CREAT or O_TRUNC is
requested, or if the caller has requested other write-access flags in addition. Otherwise, read-only
access is sufficient to allow NtSetAttributesFile to operate using fchmod(2).

This patch was inspired by the following wine-staging patches:

server-File_Permissions/0007-server-FILE_WRITE_ATTRIBUTES-should-succeed-for-read.patch
server-File_Permissions/0002-server-Allow-to-open-files-without-any-permission-bi.patch

These staging patches address the case where a file has neither read or write access flags, and
allows the file to be opened by temporarily modifying the flags to grant access.

This patch takes a simpler approach of assuming that read-only files will have the read access flag
already granted. In this case a read-only file can be opened with O_RDONLY access, providing an fd
that is usable with fchmod(2) etc.

Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=50771
Co-authored-by: Sebastian Lackner 
Signed-off-by: Joel Holdsworth 
---
 dlls/ntdll/tests/file.c | 36 +++++++++++++++---------------------
 server/fd.c             |  7 ++++++-
 2 files changed, 21 insertions(+), 22 deletions(-)

diff --git a/dlls/ntdll/tests/file.c b/dlls/ntdll/tests/file.c
index 57dee54076a..fb02fd4a527 100644
--- a/dlls/ntdll/tests/file.c
+++ b/dlls/ntdll/tests/file.c
@@ -5318,33 +5318,27 @@ static void test_file_readonly_access(void)

/* NtOpenFile FILE_WRITE_ATTRIBUTES */
     status = pNtOpenFile(&handle, FILE_WRITE_ATTRIBUTES,  &attr, &io, default_sharing, FILE_NON_DIRECTORY_FILE);
-    todo_wine ok(status == STATUS_SUCCESS, "got %#lx\n", status);
-
-    if (status == STATUS_SUCCESS) {
-        memset(&fbi, 0, sizeof(fbi));
-        fbi.FileAttributes = FILE_ATTRIBUTE_NORMAL;
-        U(io).Status = 0xdeadbeef;
-        status = pNtSetInformationFile(handle, &io, &fbi, sizeof fbi, FileBasicInformation);
-        todo_wine ok(status == STATUS_SUCCESS, "can't set system attribute, NtSetInformationFile returned %#lx\n",
-                     status);
-        todo_wine ok(U(io).Status == STATUS_SUCCESS, "can't set system attribute, io.Status is %lx\n",
-                     U(io).Status);
-
-        fbi.FileAttributes = FILE_ATTRIBUTE_READONLY;
-        U(io).Status = 0xdeadbeef;
-        status = pNtSetInformationFile(handle, &io, &fbi, sizeof fbi, FileBasicInformation);
-        todo_wine ok(status == STATUS_SUCCESS, "can't set system attribute, NtSetInformationFile returned %#lx\n",
-                     status);
-        todo_wine ok(U(io).Status == STATUS_SUCCESS, "can't set system attribute, io.Status is %lx\n",
-                     U(io).Status);
-    }
+    ok(status == STATUS_SUCCESS, "got %#lx\n", status);
+
+    memset(&fbi, 0, sizeof(fbi));
+    fbi.FileAttributes = FILE_ATTRIBUTE_NORMAL;
+    U(io).Status = 0xdeadbeef;
+    status = pNtSetInformationFile(handle, &io, &fbi, sizeof fbi, FileBasicInformation);
+    ok(status == STATUS_SUCCESS, "can't set system attribute, NtSetInformationFile returned %#lx\n", status);
+    ok(U(io).Status == STATUS_SUCCESS, "can't set system attribute, io.Status is %lx\n", U(io).Status);
+
+    fbi.FileAttributes = FILE_ATTRIBUTE_READONLY;
+    U(io).Status = 0xdeadbeef;
+    status = pNtSetInformationFile(handle, &io, &fbi, sizeof fbi, FileBasicInformation);
+    ok(status == STATUS_SUCCESS, "can't set system attribute, NtSetInformationFile returned %#lx\n", status);
+    ok(U(io).Status == STATUS_SUCCESS, "can't set system attribute, io.Status is %lx\n", U(io).Status);

CloseHandle(handle);

/* NtOpenFile FILE_READ_ATTRIBUTES and FILE_WRITE_ATTRIBUTES */
     status = pNtOpenFile(&handle, FILE_READ_ATTRIBUTES|FILE_WRITE_ATTRIBUTES,  &attr, &io,
                          default_sharing, FILE_NON_DIRECTORY_FILE);
-    todo_wine ok(status == STATUS_SUCCESS, "got %#lx\n", status);
+    ok(status == STATUS_SUCCESS, "got %#lx\n", status);
     CloseHandle(handle);

/* NtCreateFile FILE_GENERIC_WRITE */
diff --git a/server/fd.c b/server/fd.c
index 839a1dec914..9f902c871be 100644
--- a/server/fd.c
+++ b/server/fd.c
@@ -1941,7 +1941,12 @@ struct fd *open_fd( struct fd *root, const char *name, struct unicode_str nt_nam
         flags &= ~(O_CREAT | O_EXCL | O_TRUNC);
     }

-    if ((access & (FILE_WRITE_DATA|FILE_APPEND_DATA|FILE_WRITE_ATTRIBUTES|FILE_WRITE_EA)) &&
+    /* If FILE_WRITE_ATTRIBUTES has been requested, write access is only required if the file
+     * will be created or modified. Otherwise, read-only access is sufficient, which is all
+     * that will be available if the file has the FILE_ATTRIBUTE_READONLY attribute.
+     */
+    if (((access & (FILE_WRITE_DATA|FILE_APPEND_DATA|FILE_WRITE_EA)) ||
+        ((access & FILE_WRITE_ATTRIBUTES) && (flags & (O_CREAT | O_TRUNC)))) &&
         !(options & FILE_DIRECTORY_FILE))
     {
         if (access & (FILE_READ_DATA|FILE_READ_ATTRIBUTES|FILE_READ_EA)) rw_mode = O_RDWR;

[Zebediah Figura (＠zfigura)]

Sorry for the late review here.

Do we actually need write access for anything that FILE_WRITE_ATTRIBUTES does? And if not, the condition from 3/3 seems a little odd.

I guess O_TRUNC implies write access, but then it shouldn't involve FILE_WRITE_ATTRIBUTES. (Put another way: can we create a file with FILE_SUPERSEDE and (only) FILE_READ_DATA, and is that then broken in current Wine?)

And from a quick reading of the documentation, I don't think O_CREAT requires POSIX write access? I guess it's here to make the file writable, but again, that seems like it'd be broken in cases unrelated to FILE_WRITE_ATTRIBUTES? In which case we should probably either fchmod() the file after creation, or fcntl() the handle to give it the right access. [Or is the latter even necessary? I don't remember how our access checking works.]