[PATCH v3 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...) -- v3: server: Allow to open read-only files with FILE_WRITE_ATTRIBUTES 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 0ffe0e2c8dc..a0c739f3605 100644 --- a/server/file.h +++ b/server/file.h @@ -257,13 +257,6 @@ static inline int async_queued( struct async_queue *queue ) return !list_empty( &queue->queue ); } - -/* access rights that require Unix read permission */ -#define FILE_UNIX_READ_ACCESS (FILE_READ_DATA|FILE_READ_ATTRIBUTES|FILE_READ_EA) - -/* access rights that require Unix write permission */ -#define FILE_UNIX_WRITE_ACCESS (FILE_WRITE_DATA|FILE_APPEND_DATA|FILE_WRITE_ATTRIBUTES|FILE_WRITE_EA) - /* magic file access rights for mappings */ #define FILE_MAPPING_IMAGE 0x80000000 /* set for SEC_IMAGE mappings */ #define FILE_MAPPING_WRITE 0x40000000 /* set for writable shared mappings */ -- 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 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(a)fds-team.de> Signed-off-by: Joel Holdsworth <joel(a)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 839a1dec914..9f902c871be 100644 --- a/server/fd.c +++ b/server/fd.c @@ -1941,7 +1941,12 @@ struct fd *open_fd( struct fd *root, const char *name, struct unicode_str nt_nam flags &= ~(O_CREAT | O_EXCL | O_TRUNC); } - if ((access & (FILE_WRITE_DATA|FILE_APPEND_DATA|FILE_WRITE_ATTRIBUTES|FILE_WRITE_EA)) && + /* If FILE_WRITE_ATTRIBUTES has been requested, write access is only required if the file + * will be created or modified. Otherwise, read-only access is sufficient, which is all + * that will be available if the file has the FILE_ATTRIBUTE_READONLY attribute. + */ + if (((access & (FILE_WRITE_DATA|FILE_APPEND_DATA|FILE_WRITE_EA)) || + ((access & FILE_WRITE_ATTRIBUTES) && (flags & (O_CREAT | O_TRUNC)))) && !(options & FILE_DIRECTORY_FILE)) { if (access & (FILE_READ_DATA|FILE_READ_ATTRIBUTES|FILE_READ_EA)) rw_mode = O_RDWR; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/1895
Sorry for the late review here. Do we actually need write access for anything that FILE_WRITE_ATTRIBUTES does? And if not, the condition from 3/3 seems a little odd. I guess O_TRUNC implies write access, but then it shouldn't involve FILE_WRITE_ATTRIBUTES. (Put another way: can we create a file with FILE_SUPERSEDE and (only) FILE_READ_DATA, and is that then broken in current Wine?) And from a quick reading of the documentation, I don't think O_CREAT requires POSIX write access? I guess it's here to make the file writable, but again, that seems like it'd be broken in cases unrelated to FILE_WRITE_ATTRIBUTES? In which case we should probably either fchmod() the file after creation, or fcntl() the handle to give it the right access. [Or is the latter even necessary? I don't remember how our access checking works.] -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1895#note_23539
participants (3)
-
Joel Holdsworth -
Joel Holdsworth (＠jhol)
-
Zebediah Figura (＠zfigura)