[PATCH v5 0/3] 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...)
-- v5: server: Allow to open read-only files with FILE_WRITE_ATTRIBUTES
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
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 | 9 +++++++-- 2 files changed, 22 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..090839266ac 100644 --- a/server/fd.c +++ b/server/fd.c @@ -1941,10 +1941,15 @@ 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_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;
@zfigura Thanks for your review. I'm sorry myself for taking so long to address your points.
I think you're right about `O_CREAT`. My tests pass just as well without it, so I rebased my patches with it removed.
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.
No we don't need write access for attribute writing. Any access will do.
Therefore we should "piggy-back" on whatever access we are getting via other flags. So previously `O_RDONLY` access would become `O_RDWR` if `FILE_WRITE_ATTRIBUTES` was specified, which is impossible for read-only files. In that case `O_RDONY` is sufficient.
However, in the case where `O_TRUNC` is also required, we need write-access. I this case attribute writing can piggy-back on `O_WRONLY` or `O_RDWR`.
Does this sound reasonable to you?
A test is failing, but I don't see how it's related to these patches.
I guess what I'm trying to get at is, why isn't the condition just
"if ((access & (FILE_WRITE_DATA | FILE_APPEND_DATA | FILE_WRITE_EA)) || (flags & O_TRUNC))"
(plus the FILE_DIRECTORY_FILE part which I'm leaving out for simplicity).
I.e. why does FILE_WRITE_ATTRIBUTES even matter?
[And, if that is the right condition, this should probably be two separate patches—one to check O_TRUNC, and then the second to remove FILE_WRITE_ATTRIBUTES.]
Similarly the FILE_READ_ATTRIBUTES part should probably be its own patch.
-
Joel Holdsworth -
Joel Holdsworth (＠jhol)
-
Zebediah Figura (＠zfigura)