[PATCH v6 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.
* 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...)
-- v6: server: Allow to open read-only files with FILE_WRITE_ATTRIBUTES server: Open files for writing with O_WRONLY not O_RDWR if FILE_READ_ATTRIBUTES is also requested
From: Joel Holdsworth joel@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@codeweavers.com Signed-off-by: Joel Holdsworth joel@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);
From: Joel Holdsworth joel@airwebreathe.org.uk
These defines are counter-productive for code clarity.
Signed-off-by: Joel Holdsworth joel@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 */
From: Joel Holdsworth joel@airwebreathe.org.uk
If a file is opened for write access, it is not necessary to open it with O_RDWR if FILE_READ_ATTRIBUTES has been requested in addition. File attributes can be read from any file open descriptor.
Signed-off-by: Joel Holdsworth joel@airwebreathe.org.uk --- server/fd.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/server/fd.c b/server/fd.c index 839a1dec914..f744e82c3f6 100644 --- a/server/fd.c +++ b/server/fd.c @@ -1944,7 +1944,7 @@ struct fd *open_fd( struct fd *root, const char *name, struct unicode_str nt_nam if ((access & (FILE_WRITE_DATA|FILE_APPEND_DATA|FILE_WRITE_ATTRIBUTES|FILE_WRITE_EA)) && !(options & FILE_DIRECTORY_FILE)) { - if (access & (FILE_READ_DATA|FILE_READ_ATTRIBUTES|FILE_READ_EA)) rw_mode = O_RDWR; + if (access & (FILE_READ_DATA|FILE_READ_EA)) rw_mode = O_RDWR; else rw_mode = O_WRONLY; } else rw_mode = O_RDONLY;
From: Joel Holdsworth joel@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 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 sebastian@fds-team.de Signed-off-by: Joel Holdsworth joel@airwebreathe.org.uk --- 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 f744e82c3f6..090839266ac 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_TRUNC))) && !(options & FILE_DIRECTORY_FILE)) { if (access & (FILE_READ_DATA|FILE_READ_EA)) rw_mode = O_RDWR;
Ok, I separated the change to the handling of `FILE_READ_ATTRIBUTES` into an extra patch.
I tried replacing `((access & FILE_WRITE_ATTRIBUTES) && (flags & O_TRUNC))` with `(flags & O_TRUNC)`, but doing that makes `test_NtCreateFile` tests #16 and #17 fail:
``` /*16*/{ FILE_SUPERSEDE, 0, 0, FILE_SUPERSEDED, FILE_ATTRIBUTE_ARCHIVE, FALSE }, /*17*/{ FILE_SUPERSEDE, FILE_ATTRIBUTE_READONLY, 0, FILE_SUPERSEDED, FILE_ATTRIBUTE_ARCHIVE|FILE_ATTRIBUTE_READONLY, TRUE }, ```
``` file.c:4296: Test failed: 16: expected 0 got 0xc0000022 file.c:4296: Test failed: 17: expected 0 got 0xc0000022 ```
Okay, so it turns out that:
(1) the man pages don't say this, but we do in fact need write permission to set xattr on a file, implying that we probably shouldn't keep translating READONLY to Unix permission bits,
(2) FILE_WRITE_ATTRIBUTES applies to FILE_SUPERSEDE but not FILE_OVERWRITE_IF, making that weird corner case work. This isn't really correct, though: as the tests show, both FILE_SUPERSEDE and FILE_OVERWRITE_IF are supposed to replace the file's attributes (contradicting the comment in server/file.c). The actual difference is apparently that FILE_OVERWRITE_IF will respect READONLY, but FILE_SUPERSEDE won't. (But on the other hand, trying to open a read-only file with FILE_SUPERSEDE and GENERIC_WRITE still fails???)
The thing is that those tests really are a corner case. By making that change we're only breaking the case where we're opening with FILE_SUPERSEDE but not FILE_WRITE_DATA, which is doubly rare because
(1) kernelbase doesn't even translate anything to FILE_SUPERSEDE, only FILE_OVERWRITE(_IF);
(2) it seems quite unlikely that an application would open a file to truncate it without also opening it for write access.
In light of that I'd propose we use logic that makes sense in the server rather than worry about that test, and hence do one of two things:
(a) require write access for O_TRUNC, but don't require it for FILE_WRITE_ATTRIBUTES—basically the same thing that this patch series already does, but with the aforementioned suggestion. Mark tests 16 and 17 as todo, because they don't really matter. This isn't really correct, because writing attributes *does* require POSIX write access, but I don't think it risks regressions—it just means we return success for opening files where we otherwise couldn't, and then we'll still return failure from NtSetAttributesFile() et al.
(b) Stop stripping POSIX write access from files with READONLY. Rely on xattr to store that. (Or perhaps, stop stripping POSIX write access *if* we can store with xattr.) This is more correct in general, although it's an open question whether the difference will ever actually matter. It arguably loses something in terms of host integration, although I also wonder whether that matters. I feel like there was a discussion about this earlier, but I've forgotten what or where it was...
(b) Stop stripping POSIX write access from files with READONLY. Rely on xattr to store that. (Or perhaps, stop stripping POSIX write access *if* we can store with xattr.) This is more correct in general, although it's an open question whether the difference will ever actually matter. It arguably loses something in terms of host integration, although I also wonder whether that matters. I feel like there was a discussion about this earlier, but I've forgotten what or where it was...
There are applications that depend on whether a file is marked read only or not, even if the underlying filesystem is (mounted) read-only. AkelPad for example checks if the configuration file is read-only or not. It will complain if the file is not marked read-only but the filesystem doesn't allow writes.
This would break if xattrs can't be stored while the filesystem itself is (mounted) read-only.
The short gist of it is that the read-only attribute should always be allowed to be changed or inspected, even on unix filesystems without xattrs. "stop stripping POSIX write access *if* we can store with xattr" should work.
This would break if xattrs can't be stored while the filesystem itself is (mounted) read-only.
I would assume we can't set xattrs if the filesystem is read-only?
The short gist of it is that the read-only attribute should always be allowed to be changed or inspected, even on unix filesystems without xattrs. "stop stripping POSIX write access *if* we can store with xattr" should work.
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.
On Mon Mar 27 21:41:10 2023 +0000, Zebediah Figura wrote:
This would break if xattrs can't be stored while the filesystem itself
is (mounted) read-only. I would assume we can't set xattrs if the filesystem is read-only?
The short gist of it is that the read-only attribute should always be
allowed to be changed or inspected, even on unix filesystems without xattrs. "stop stripping POSIX write access *if* we can store with xattr" should work. 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.
Ok I've spent some time thinking about this.
1. 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.
2. 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.
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 3-step process: chmod, open, fchmod. i.e. the path has to be resolved twice leaving the door open to TOCTOU race conditions. However, at least this is happening in server, so the other wine threads are implicitly excluded.
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?
3. 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`.
-
Gabriel Ivăncescu (＠insn) -
Joel Holdsworth
-
Joel Holdsworth (＠jhol)
-
Zebediah Figura (＠zfigura)