[PATCH v8 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~~ temporarily modifying file access permisions to allow the file to be opened.

* 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...)

--
  v8: server: Allow to open files without permission bits.

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

[PATCH v8 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 v8 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 210fcec5e78..e97c9cc13fd 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 v8 3/3] server: Allow to open files without permission bits.

[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 file, 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.

Wine uses POSIX file permissions as one its possible methods of storing the READONLY DOS attribute
in addition to storing attributes in Samba-formatted user extentded attributes.

Files that are opened with FILE_WRITE_ATTRIBUTES must be opened with O_WRONLY or O_RDWR, because
write access is required to modify the extended attribute. However, this will fail if the POSIX file
permissions are set to read-only.

This patch solves the problem by temporarily modifying the file access permissions to enable it to
be opened before restoring the permissions to their previous values.

There are risks involved in doing this. Modifying the file permissions is a 4-step process:

1. stat - read the existing access flags
  2. chmod - modify to allow write permission.
  3. open - attempt to open the file.
  4. fchmod/chmod - restore the access flags using the fd if possible.

The process involves resolving the file path at least 3 times, which is somewhat vulnerable to
TOCTOU race conditions. However, the timing will be very brief, and this process occurs in server
meaning that other Wine threads will be excluded. Therefore, the impact is judged to be low.

This patch was derived 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

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             | 32 ++++++++++++++++++++++++++++++--
 2 files changed, 45 insertions(+), 23 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..9a4f2297329 100644
--- a/server/fd.c
+++ b/server/fd.c
@@ -1899,6 +1899,7 @@ struct fd *open_fd( struct fd *root, const char *name, struct unicode_str nt_nam
     int root_fd = -1;
     int rw_mode;
     char *path;
+    int mode_assigned = 0;

if (((options & FILE_DELETE_ON_CLOSE) && !(access & DELETE)) ||
         ((options & FILE_DIRECTORY_FILE) && (flags & O_TRUNC)))
@@ -1958,6 +1959,29 @@ struct fd *open_fd( struct fd *root, const char *name, struct unicode_str nt_nam
                 (flags & O_CREAT))
                 fd->unix_fd = open( name, O_RDONLY | (flags & ~(O_TRUNC | O_CREAT | O_EXCL)), *mode );
         }
+        else if (errno == EACCES)
+        {
+            /* try to change permissions temporarily to open a file descriptor */
+            if ((access & FILE_WRITE_ATTRIBUTES) &&
+                !(access & (FILE_WRITE_DATA | FILE_APPEND_DATA | FILE_WRITE_EA |
+                    FILE_READ_DATA | FILE_READ_EA | DELETE)) &&
+                !stat( name, &st ) && st.st_uid == getuid() &&
+                !chmod( name, st.st_mode | S_IRUSR ))
+            {
+                fd->unix_fd = open( name, O_RDONLY | (flags & ~(O_TRUNC | O_CREAT | O_EXCL)), *mode );
+                if (fd->unix_fd == -1)
+                    chmod( name, st.st_mode );
+                else
+                    fchmod( fd->unix_fd, st.st_mode );
+                *mode = st.st_mode;
+                mode_assigned = 1;
+            }
+            else
+            {
+                set_error( STATUS_ACCESS_DENIED );
+                goto error;
+            }
+        }

if (fd->unix_fd == -1)
         {
@@ -1981,8 +2005,12 @@ struct fd *open_fd( struct fd *root, const char *name, struct unicode_str nt_nam
     closed_fd->unix_fd = fd->unix_fd;
     closed_fd->unlink = 0;
     closed_fd->unix_name = fd->unix_name;
-    fstat( fd->unix_fd, &st );
-    *mode = st.st_mode;
+
+    if (!mode_assigned)
+    {
+        fstat( fd->unix_fd, &st );
+        *mode = st.st_mode;
+    }

/* only bother with an inode for normal files and directories */
     if (S_ISREG(st.st_mode) || S_ISDIR(st.st_mode))

[PATCH v8 3/3] server: Allow to open files without permission bits.

[Marvin]

Hi,

It looks like your patch introduced the new failures shown below.
Please investigate and fix them before resubmitting your patch.
If they are not new, fixing them anyway would help a lot. Otherwise
please ask for the known failures list to be updated.

The tests also ran into some preexisting test failures. If you know how
to fix them that would be helpful. See the TestBot job for the details:

The full results can be found at:
https://testbot.winehq.org/JobDetails.pl?Key=131141

Your paranoid android.

=== debian11 (32 bit report) ===

quartz:
dsoundrender: Timeout

[Gabriel Ivăncescu (＠insn)]

> Not to argue against having a fallback, but what filesystems don't support xattrs? There's FAT, but we could easily add support for FAT_IOCTL_SET_ATTRIBUTES.

FWIW, I think tmpfs doesn't support user xattrs by design.

[Joel Holdsworth (＠jhol)]

On Tue Mar 28 14:01:21 2023 +0000, Gabriel Ivăncescu wrote:
> > Not to argue against having a fallback, but what filesystems don't
>
> support xattrs? There's FAT, but we could easily add support for FAT_IOCTL_SET_ATTRIBUTES.
> FWIW, I think tmpfs doesn't support user xattrs by design.

@ehoover suggested using hidden directories to store this information. In Wine staging he experimented with storing data of reparse points in a `.REPARSE_POINT/` adjacent subdirectory:

https://gitlab.winehq.org/wine/wine-staging/-/blob/master/patches/ntdll-Junc...

I wondered if a more general version of this could be used to store DOS attributes and EA data on filesystems that don't support xattrs.

[Joel Holdsworth (＠jhol)]

On Tue Mar 28 14:01:20 2023 +0000, Joel Holdsworth wrote:
> @ehoover suggested using hidden directories to store this information.
> In Wine staging he experimented with storing data of reparse points in a
> `.REPARSE_POINT/` adjacent subdirectory:
> https://gitlab.winehq.org/wine/wine-staging/-/blob/master/patches/ntdll-Junc...
> I wondered if a more general version of this could be used to store DOS
> attributes and EA data on filesystems that don't support xattrs.

```
       The tmpfs filesystem supports extended attributes (see xattr(7)),
       but user extended attributes are not permitted.
```
https://www.man7.org/linux/man-pages/man5/tmpfs.5.html

[Joel Holdsworth (＠jhol)]

On Tue Mar 28 16:17:45 2023 +0000, Joel Holdsworth wrote:
> Ok - I've updated the patch set to adopt the design from (2a) in my
> above comment. Any thoughts?

On Linux, we could reduce the TOCTOU attack-space by opening the parent directory with `O_PATH`, then using `fstatat`, `fchmodat`, and `openat` to work on the file.

We could further reduce the attack-space by doing the `stat`ing and `chmod`ing with an `O_PATH` file fd. Unfortunately, we can't reopen an `O_PATH` file fd for read/write, so the file would still have to be resolved by name twice.

Still better than resolving the full path 3 or 4-times over.

[Erich Hoover (＠ehoover)]

On Tue Mar 28 14:55:47 2023 +0000, Joel Holdsworth wrote:
>        The tmpfs filesystem supports extended attributes (see xattr(7)),
>        but user extended attributes are not permitted.
>
> https://www.man7.org/linux/man-pages/man5/tmpfs.5.html

I have been summoned!  Yes, I have suggested that we could treat this all as [alternative stream data](https://learn.microsoft.com/en-us/openspecs/windows_protocols/ms-fscc/c54dec...) in an adjacent subdirectory.  So, files with additional metadata would look something like this:
```
$ ls -haltr . .NT_STREAMS/myfile/
.:
<snip . and ..>
drwxrwxr-x  4 ehoover ehoover 4.0K Oct 20 09:58 .NT_STREAMS
lrwxrwxrwx  1 ehoover ehoover   20 Oct 20 13:02 myfile -> .NT_STREAMS/myfile/:
.NT_STREAMS/myfile/:
<snip . and ..>
lrwxrwxrwx 1 ehoover ehoover    2 Oct 20 13:02  : -> ::
lrwxrwxrwx 1 ehoover ehoover    7 Oct 20 13:02  :: -> '::$DATA'
-rw-rw-r-- 1 ehoover ehoover    8 Oct 20 13:03 '::$DATA'
```
[Additional attributes](https://learn.microsoft.com/en-us/openspecs/windows_protocols/ms-fscc/a82e91...) (`$REPARSE_POINT`, etc.) would all be stored here according to the NTFS spec.  In this case, I believe that you would want to use `$STANDARD_INFORMATION` ([source](http://inform.pucp.edu.pe/~inf232/Ntfs/ntfs_doc_v0.5/attributes/standard_inf...)).  I put together some patches for this, but it gets a little tricky to find the folder under some circumstances.  I don't remember where I left off with it, but I can look into it if folks are interested.

[Zebediah Figura (＠zfigura)]

> Ok I've spent some time thinking about this.
> -
>
> As I mentioend previously I think we should make use of other storage methods in addition:
> - Samba v4 User xattrs
> - Linux NTFS DOS Attribute System xattrs
> - Linux `FAT_IOCTL_SET_ATTRIBUTES`
> - Perhaps BSD/MacOS `st_flags` `UF_IMMUTABLE`, though this presents a less severe version of the POSIX access flags problme.
>
> I have some experimental patches lined up to cover these things.
> -
>
> I agree with you that we have a big problem with using POSIX file permissions to store the `READONLY` DOS attribute.
>
> We have made a comittment to storing DOS attributes in User xattrs. In this case we need to be able to call fsetxattr on any file that is opened with `FILE_WRITE_ATTRIBUTES`, which requires that the fd would be openned with write access.

Yeah, although maybe it's not a *big* problem? I don't remember the Cygwin code, but if this patch set works, then it doesn't actually need NtSetAttributesFile() to succeed, and there's no other known users of FILE_WRITE_ATTRIBUTES without FILE_WRITE_DATA, which is a weird and corner-case-ish thing to do anyway.
> I thought about delayed approaches i.e. increasing access permissions temporarily if fsetxattr fails with `EACCESS`, but this won't work. We cannot change the access of an existing fd. fcntl doesn't allow us to do that, and we cannot reopen the file, because we won't know its path.
>
> So as I see it, there are two possible approaches:
>
> a) As per the wine-staging patch, temporarily chmod the file in server when opening the fd so as to get a writable fd of an otherwise read-only file.
>
> This would involve a 4-step process: `stat`, `chmod`, `open`, `fchmod`. i.e. the path has to be resolved three times leaving the door open to TOCTOU race conditions. However, at least this is happening in server, so the other wine threads are implicitly excluded.

I still really don't like this solution. TOCTOU bothers me less than that it just feels too much like a hack.
> b) Stop using POSIX file permissions, and rely solely on Samba User xattrs, and other methods.
>
> Out of the two, (a) seems more better to me, but I think either approach is quite reasonable. What do you think?
> - Either one of these approaches supercedes my pigging-backing approach. As before, `FILE_READ_ATTRIBUTES` would require `O_RDONLY`. `FILE_WRITE_ATTRIBUTES` would require `O_WRONLY`. `FILE_READ_ATTRIBUTES|FILE_WRITE_ATTRIBUTES` would require `O_RDWR`.

[Zebediah Figura (＠zfigura)]

Gitlab somehow posted that before I was done typing, so let me expand:

I don't like temporarily changing the file permissions. It feels too much like a hack: if we really need to change the file mode, why didn't we just have the more permissive mode in the first place? I know the answer is "so that we can deny FILE_WRITE_DATA given READONLY", but it ends up feeling very complex and fragile. The way we translate permissions has already come up with enough snags already...

I like the idea of always relying on metadata (including Erich's hidden-directory proposal) better.

[Joel Holdsworth (＠jhol)]

> Yeah, although maybe it's not a *big* problem?

Well to be more precise, it's a tricky problem. Fortunately it's quite a well-contained problem, that - as you correctly say, only happens in a corner case.

I agree with you my patch has a hacky feel to it. However, the issue with bad hacks is that they tend to get out of hand leading to wider behavioural problems and development headaches. By contrast, I think the hackiness of my patch is quite well contained:

* It only executes in an edge case.
  * It's not much code.
  * It's execute quickly.
  * And it has unit tests.

To me, the impact seems a lot less than throwing out the usage of POSIX access flags.
> if we really need to change the file mode, why didn't we just have the more permissive mode in the first place?

To me it's two reasons:

1. Host integration. From a user stand-point, if a UNIX application wanted to make a file read-only, it would do it using `chmod`. If a Windows application running in Wine wants to do the same thing, I would expect Wine to do it in the same way.

2.  Limitations of the POSIX/Linux API. When you open a file you have to declare all the access you will ever want up front, and you can't upgrade or downgrade it later. Furthermore, UNIX doesn't have the same access granularity as NT. You can't open a file for writing extended attributes, while also opening it read-only. This is why we have to open non-user-writable files for writing using `chmod`.
> I like the idea of always relying on metadata (including Erich's hidden-directory proposal) better.

Ok, lets talk about that.

So I think if we did implement all the storage methods I listed above, we would still need some kind of fallback, so I think we certainly would sometimes need hidden directories - or something similar.

But I'm really not sure about it.

In regard to fragility, it seems to me that having the hidden directory is going to be more fragile. It would be very easy for the files to be separated from their attributes, for example. Users would have to know to keep each file together with its hidden directory.

But the bigger question I have is how to implement it? The issue I see is that if the file is opened with `FILE_READ_ATTRIBUTES` or `FILE_WRITE_ATTRIBUTES` we need to open both the file and the file containing the DOS attributes i.e. we have to hold on to two files with two fds.

Alternatively, we could use the `get_handle_unix_name` server request to get the path from the NT file handle. But then we have a different kind of TOCTOU issue, because the file might have been moved on the UNIX side since it was opened.

Also, are there threading issues? Is it possible to have multiple accessors to the attributes file? Or would the attributes have to be read/updated in server?

Perhaps @ehoover can comment on this, but I'm worried that these concerns also apply to his patches. Shouldn't reparse points be using (Samba inspired?) extended attributes instead? But if we care to support reparse points on tmpfs, then we will need hidden directories some of the time at least.