[PATCH 0/1] MR3445: server: Open files in blocking mode in create_file
This fixes a bug in wine when running on 9pfs where short reads would occur, causing binaries to be loaded incorrectly. All other filesystems on Linux ignore O_NONBLOCK for regular files, so this should not affect them. Thanks to Paul Gofman for the advice on this fix. --- Ref https://gitlab.winehq.org/wine/wine/-/merge_requests/3390 for my original attempt at fixing this, particularly https://gitlab.winehq.org/wine/wine/-/merge_requests/3390#note_40093 which describes the strange situation `O_NONBLOCK` finds itself in on filesystems. The risk with this MR is that fifo, unix sockets or other special files opened via `open_unix_file` in ntdll will now have changed behaviour - but my quick look suggests that this is unlikely, as `open_unix_file` is only used for: - `dlls/ntdll/unix/file.c`: `NtCreateFile` and `NtDeleteFile` - `dlls/ntdll/unix/process.c`: loading PE information and getting a dirfd for the current directory - `dlls/ntdll/unix/env.c`: `open_nls_data_file` - `dlls/ntdll/unix/loader.c`: dll load functions - `dlls/ntdll/unix/registry.c`: `NtLoadKeyEx` cc @gofman -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3445
From: Aidan Hobson Sayers <aidanhs(a)cantab.net> This fixes a bug in wine when running on 9pfs where short reads would occur, causing binaries to be loaded incorrectly. All other filesystems on Linux ignore O_NONBLOCK for regular files, so this should not affect them. Thanks to Paul Gofman for the advice on this fix. --- server/file.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/file.c b/server/file.c index 76c687833c9..a96a55ffab3 100644 --- a/server/file.c +++ b/server/file.c @@ -266,7 +266,7 @@ static struct object *create_file( struct fd *root, const char *nameptr, data_si access = map_access( access, &file_type.mapping ); /* FIXME: should set error to STATUS_OBJECT_NAME_COLLISION if file existed before */ - fd = open_fd( root, name, nt_name, flags | O_NONBLOCK, &mode, access, sharing, options ); + fd = open_fd( root, name, nt_name, flags, &mode, access, sharing, options ); if (!fd) goto done; if (S_ISDIR(mode)) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3445
For the record, I didn't say that it is a bug in Wine, I think it is a bug in 9pfs as docs suggest that O_NONBLOCK has no effect on regular files. What I noted is that the same docs advises against using O_NONBLOCK with regular files and expect sync behaviour because it can in theory change in the future (but not so far). Unfortunately I can't easily comment on correctness of this change. I suspect that O_NONBLOCK might be actually needed for something and unconditionally removing it will break that something, but can't immediately guess which case is that. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3445#note_40661
And for added context from previous thread - the change to make `O_NONBLOCK` behave differently on 9pfs in the kernel was a deliberate change. As a result, I personally am not inclined to go upstream to ask them to fix it. If the decision for wine is that this is a WONTFIX (which I'd understand) then I'm offering to fix docs/wiki to indicate 9pfs isn't currently supported (if pointed at roughly the pieces that need changing). -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3445#note_40663
How about opening with O\_NONBLOCK, but putting back blocking flag via fcntl(F\_SETFL)? Would that work? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3445#note_40679
On Sat Jul 29 06:51:48 2023 +0000, Paul Gofman wrote:
For the record, I didn't say that it is a bug in Wine, I think it is a bug in 9pfs as docs suggest that O_NONBLOCK has no effect on regular files. What I noted is that the same docs advises against using O_NONBLOCK with regular files and expect sync behaviour because it can in theory change in the future (but not so far). Unfortunately I can't easily comment on correctness of this change. I suspect that O_NONBLOCK might be actually needed for something and unconditionally removing it will break that something, but can't immediately guess which case is that. For one, it'll indefinitely block the wineserver if an app accidentally tries to open a FIFO with the other half not open.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3445#note_40680
participants (4)
-
Aidan Hobson Sayers -
Aidan Hobson Sayers (＠aidanhs)
-
Jinoh Kang (＠iamahuman)
-
Paul Gofman (＠gofman)