[PATCH v9 0/4] MR1895: Fixes for NtOpenFile of read-only files with FILE_WRITE_ATTRIBUTES
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...) -- v9: server: Use *at syscalls to implement temporary chmod behaviour server: Allow to open files without permission bits. https://gitlab.winehq.org/wine/wine/-/merge_requests/1895
From: Joel Holdsworth <joel(a)airwebreathe.org.uk> 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 <qhong(a)codeweavers.com> Signed-off-by: Joel Holdsworth <joel(a)airwebreathe.org.uk> --- 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); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/1895
From: Joel Holdsworth <joel(a)airwebreathe.org.uk> These defines are counter-productive for code clarity. Signed-off-by: Joel Holdsworth <joel(a)airwebreathe.org.uk> --- 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 */ -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/1895
From: Joel Holdsworth <joel(a)airwebreathe.org.uk> 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 <sebastian(a)fds-team.de> Signed-off-by: Joel Holdsworth <joel(a)airwebreathe.org.uk> --- dlls/ntdll/tests/file.c | 36 +++++++++++++++--------------------- server/fd.c | 36 ++++++++++++++++++++++++++++++++++-- 2 files changed, 49 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..8966c8e0648 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 restore_chmod = 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; + restore_chmod = 1; + } + else + { + set_error( STATUS_ACCESS_DENIED ); + goto error; + } + } if (fd->unix_fd == -1) { @@ -1966,6 +1990,8 @@ struct fd *open_fd( struct fd *root, const char *name, struct unicode_str nt_nam set_error( STATUS_OBJECT_NAME_INVALID ); else file_set_error(); + + if (restore_chmod) chmod( name, *mode ); goto error; } } @@ -1981,8 +2007,14 @@ 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 (restore_chmod) + chmod( name, *mode ); + else + { + 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)) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/1895
From: Joel Holdsworth <joel(a)airwebreathe.org.uk> When read-only files are opened with FILE_WRITE_ATTRIBUTES, server must temporarily chmod the file to grant sufficient permissions to allow the file to be opened. The previous implementation used stat(2), chmod(2), open(2) and fchmod(2) (or chmod(2) in the failure case). Therefore the file path was being resolved 3 or possible 4 times in rapid succession. These successive path resolutions provide an opportunity for a Time-of-Check/Time-of-Use (TOCTOU) race condition with other processes. This patch mitigates against this by first opening the fd of the parent directory, and then using fstatat(2), fchmodat(2) and openat(2) to open the file relative to its parent directory. This method does not completely eliminate the TOCTOU issue, but it does significantly reduce its extent. Signed-off-by: Joel Holdsworth <joel(a)airwebreathe.org.uk> --- server/fd.c | 70 ++++++++++++++++++++++++++++++++++++++--------------- 1 file changed, 50 insertions(+), 20 deletions(-) diff --git a/server/fd.c b/server/fd.c index 8966c8e0648..8e667f7b82a 100644 --- a/server/fd.c +++ b/server/fd.c @@ -25,6 +25,7 @@ #include <dirent.h> #include <errno.h> #include <fcntl.h> +#include <libgen.h> #include <limits.h> #include <signal.h> #include <stdarg.h> @@ -1888,6 +1889,51 @@ void get_nt_name( struct fd *fd, struct unicode_str *name ) name->len = fd->nt_namelen; } +static int try_chmod_open( const char *name, int flags, mode_t *mode, unsigned int access ) { + struct stat st; + int dirfd, fd = -1; + int tmp_errno; + char filename_buf[PATH_MAX]; + const char *file_base_name; + + if (!(access & FILE_WRITE_ATTRIBUTES) || + (access & (FILE_WRITE_DATA | FILE_APPEND_DATA | FILE_WRITE_EA | FILE_READ_DATA | FILE_READ_EA | DELETE))) + return -1; + + strcpy( filename_buf, name ); + dirfd = open( dirname( filename_buf ), +#ifdef O_PATH + O_PATH +#else + O_RDONLY +#endif + ); + + if (dirfd != -1) + { + strcpy( filename_buf, name ); + file_base_name = basename( filename_buf ); + + if (!fstatat( dirfd, file_base_name, &st, 0 ) && + st.st_uid == getuid() && + !fchmodat( dirfd, file_base_name, st.st_mode | S_IRUSR, 0 )) + { + fd = openat( dirfd, file_base_name, O_RDONLY | (flags & ~(O_TRUNC | O_CREAT | O_EXCL)), *mode ); + tmp_errno = errno; + if (fd == -1) + fchmod( fd, st.st_mode ); + else + fchmodat( dirfd, file_base_name, st.st_mode, 0 ); + errno = tmp_errno; + *mode = st.st_mode; + } + + close(dirfd); + } + + return fd; +} + /* open() wrapper that returns a struct fd with no fd user set */ struct fd *open_fd( struct fd *root, const char *name, struct unicode_str nt_name, int flags, mode_t *mode, unsigned int access, @@ -1899,7 +1945,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 restore_chmod = 0; + int mode_assigned = 0; if (((options & FILE_DELETE_ON_CLOSE) && !(access & DELETE)) || ((options & FILE_DIRECTORY_FILE) && (flags & O_TRUNC))) @@ -1962,20 +2008,8 @@ struct fd *open_fd( struct fd *root, const char *name, struct unicode_str nt_nam 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; - restore_chmod = 1; - } + if ((fd->unix_fd = try_chmod_open( name, flags, mode, access )) != -1) + mode_assigned = 1; else { set_error( STATUS_ACCESS_DENIED ); @@ -1990,8 +2024,6 @@ struct fd *open_fd( struct fd *root, const char *name, struct unicode_str nt_nam set_error( STATUS_OBJECT_NAME_INVALID ); else file_set_error(); - - if (restore_chmod) chmod( name, *mode ); goto error; } } @@ -2008,9 +2040,7 @@ struct fd *open_fd( struct fd *root, const char *name, struct unicode_str nt_nam closed_fd->unlink = 0; closed_fd->unix_name = fd->unix_name; - if (restore_chmod) - chmod( name, *mode ); - else + if (!mode_assigned) { fstat( fd->unix_fd, &st ); *mode = st.st_mode; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/1895
Just pushed a new version of the chmod patches with the following changes: * A fix to the error path. Previously `fchmod`/`chmod` was called to restore the access flags after `open` clobbering the value of `errno`. This patch delays restoring the flags - similar to the design in wine-staging. * An improved implementation that does the `stat`/`chmod`/`open` process using `fstatat`, `fchmodat` and `openat` with an fd of the parent directory that is locked in place from start to finish. This should significantly reduce the danger of TOCTOU, because now only the file name is being repeatedly resolved, rather than the full hierachy of directories. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1895#note_28321
On Wed Mar 29 12:44:35 2023 +0000, Joel Holdsworth wrote: > > 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 when needed. 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. FWIW I *really* dislike the idea of a hidden directory, it just feels wrong and clunky to me, and doesn't integrate well with the host at all. tmpfs not supporting reparse points seems fine if that's the worry here; the reason Read-Only is different is that it's a basic feature of all Windows filesystems, not just more advanced ones (like NTFS), so it never fails on Windows, anywhere, even on FAT filesystems. So apps on Windows can fail to use reparse points if used on a filesystem that doesn't support them, so they likely can expect it. But not the Read-Only attribute, that's why I find the current handling fine for tmpfs… -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1895#note_28322
Perhaps @ehoover can comment on this, but I'm worried that these concerns also apply to his patches.
Unless I'm misunderstanding, the only possible problem is with other unix programs (since the handling is all in the server). I don't see a way to avoid that, but maybe I'm not being creative enough.
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.
It is, IMO, better to use the samba extended attributes where possible. I only propose the hidden directories as a fallback mechanism for when that's not possible.
FWIW I really dislike the idea of a hidden directory, it just feels wrong and clunky to me, and doesn't integrate well with the host at all.
You could also replace the file with the directory of metadata, but then you have to change a lot of "is this a file or a directory" handling to read the contents of the metadata directory. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1895#note_28346
FWIW I *really* dislike the idea of a hidden directory, it just feels wrong and clunky to me, and doesn't integrate well with the host at all.
I mean, ultimately we need *some* way of storing arbitrary extra metadata. The nice thing about putting that in the file system is that it's perfectly portable, has no size limitations, and conceptually quite simple. Of course, as Joel mentioned, it's also a lot easier for that metadata to get lost. (I'm not sure that TOCTOU is a concern? Partly because "filesystem atomicity doesn't really matter unless you're doing something dumb". But I don't know, maybe there are real issues there.) It's worth mentioning that integration doesn't really matter for all of these attributes. I personally find that translating READONLY to mode bits gets in the way more often than not. Obviously SYSTEM and other attributes have no POSIX equivalent. Reparse points can potentially have a POSIX equivalent but I think we've seen it's not as simple as translating that directly, i.e. we still need metadata that's stored outside the symlink target.
tmpfs not supporting reparse points seems fine if that's the worry here; the reason Read-Only is different is that it's a basic feature of all Windows filesystems, not just more advanced ones (like NTFS), so it never fails on Windows, anywhere, even on FAT filesystems.
So apps on Windows can fail to use reparse points if used on a filesystem that doesn't support them, so they likely can expect it. But not the Read-Only attribute, that's why I find the current handling fine for tmpfs…
I don't think it works that way in practice; applications blithely assume they're running on NTFS all the time. Note that even on FAT there's more attributes than just READONLY; I think SYSTEM is the other one that actually matters in practice. That just can't work on tmpfs without storing some metadata. Frankly the answer could also easily be "don't do that", i.e. you get to keep the pieces if tmpfs breaks. I.e. we could print a winediag message if someone tries to set attributes on a file in tmpfs (or anything else that doesn't support xattr) and just ignore the request. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1895#note_28354
Can we come to a conclusion about the path forward for this patch-set? I still maintain that the design is reasonable within the limits of what the POSIX/Linux makes available. However, if the design is considered to be wrong, I would appreciate alternative suggestions. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1895#note_31129
As Zeb said, temporarily changing file modes is ugly and fragile, we don't want to go in that direction. Also there doesn't seem to be much evidence that it would be necessary. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1895#note_31191
On Wed Apr 26 12:34:16 2023 +0000, Alexandre Julliard wrote:
As Zeb said, temporarily changing file modes is ugly and fragile, we don't want to go in that direction. Also there doesn't seem to be much evidence that it would be necessary. Fair enough. So do you believe we should remove the usage of POSIX file-access flags to represent `FILE_ATTRIBUTE_READONLY` entirely? I don't see how we can open an fd of a non-user-writable file that allows filesystem extended attribute write access, unless we temporarily mark the file as writable.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/1895#note_31220
Fair enough. So do you believe we should remove the usage of POSIX file-access flags to represent `FILE_ATTRIBUTE_READONLY` entirely?
Not without seeing more evidence that this is necessary. It's not clear from the original report that opening for write is actually required to fix the bug. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1895#note_31221
On Wed Apr 26 13:40:24 2023 +0000, Alexandre Julliard wrote:
Fair enough. So do you believe we should remove the usage of POSIX file-access flags to represent `FILE_ATTRIBUTE_READONLY` entirely? Not without seeing more evidence that this is necessary. It's not clear from the original report that opening for write is actually required to fix the bug. I wrote a bit more information about it in my commit message here:
https://gitlab.winehq.org/wine/wine/-/merge_requests/1895/diffs?commit_id=ce...
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.
What do you think? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1895#note_31235
On Wed Apr 26 14:49:02 2023 +0000, Joel Holdsworth wrote:
I wrote a bit more information about it in my commit message here: https://gitlab.winehq.org/wine/wine/-/merge_requests/1895/diffs?commit_id=ce...
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. What do you think? I don't see anything in there that would require write access. The most obvious fix would be to support FILE_DISPOSITION_IGNORE_READONLY_ATTRIBUTE, but opening the file read-only and faking the attributes setting could probably also be made to work.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/1895#note_31243
On Wed Apr 26 15:12:29 2023 +0000, Alexandre Julliard wrote:
I don't see anything in there that would require write access. The most obvious fix would be to support FILE_DISPOSITION_IGNORE_READONLY_ATTRIBUTE, but opening the file read-only and faking the attributes setting could probably also be made to work. I'll have a look, and see how it works out. Thanks.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/1895#note_31246
I don't see anything in there that would require write access. The most obvious fix would be to support FILE_DISPOSITION_IGNORE_READONLY_ATTRIBUTE,
I don't understand, how do you propose to support that flag while still mapping READONLY to ~0222?
but opening the file read-only and faking the attributes setting could probably also be made to work.
I guess we could do that, but what if we come across an application that tries to set attributes like HIDDEN or SYSTEM on a read-only file and expects that to persist? I can see the argument for "it's not worth worrying about", though... -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1895#note_31262
I don't see anything in there that would require write access. The most obvious fix would be to support FILE_DISPOSITION_IGNORE_READONLY_ATTRIBUTE,
I don't understand, how do you propose to support that flag while still mapping READONLY to \~0222?
Ah, never mind, I made assumptions about what this flag does. That would work for Cygwin. I don't like the idea of leaving our code broken in this way otherwise, but... -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1895#note_31263
On Wed Apr 26 17:13:14 2023 +0000, Zebediah Figura wrote:
I don't see anything in there that would require write access. The most obvious fix would be to support FILE_DISPOSITION_IGNORE_READONLY_ATTRIBUTE,
I don't understand, how do you propose to support that flag while still mapping READONLY to \~0222? Ah, never mind, I made assumptions about what this flag does. That would work for Cygwin. I don't like the idea of leaving our code broken in this way otherwise, but... The thought occurs to me that this patch set might be a usful addition to wine-staging for the time being.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/1895#note_31578
The thought occurs to me that this patch set might be a usful addition to wine-staging for the time being.
Don't we already have a different (if perhaps worse) version of this in wine-staging already? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1895#note_31602
Don't we already have a different (if perhaps worse) version of this in wine-staging already?
We do indeed. This would be an upgrade - especially with the extra "server: Use *at syscalls to implement temporary chmod behaviour" patch. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1895#note_31626
For the record, - bug #50771 was ultimately resolved with MR !3122 so this MR here can be therefore closed. Many thanks @jhol for the great work! Any by the way, there exist also a long standing issue #52171 around `NtCreateFile` which seems to affect especially emulators like CXBX-R and to some degree also dolphin. :wink: -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1895#note_38397
Superseded by !3122. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1895#note_38672
This merge request was closed by Alexandre Julliard. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1895
participants (7)
-
Alexandre Julliard (＠julliard) -
Clemens Leu (＠lorn10)
-
Erich Hoover (＠ehoover)
-
Gabriel Ivăncescu
-
Joel Holdsworth
-
Joel Holdsworth (＠jhol)
-
Zebediah Figura (＠zfigura)