[PATCH 0/1] MR5083: server: Try to retrieve the unix name on handles created from file descriptors.
From: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> It also fixes the game Unity of Command II (same bug). Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=46070 Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- server/fd.c | 39 +++++++++++++++++++++++++++++++++++++++ server/file.c | 1 + server/file.h | 2 ++ 3 files changed, 42 insertions(+) diff --git a/server/fd.c b/server/fd.c index 8576882aaa9..37c5bda0007 100644 --- a/server/fd.c +++ b/server/fd.c @@ -2071,6 +2071,45 @@ struct fd *create_anonymous_fd( const struct fd_ops *fd_user_ops, int unix_fd, s return NULL; } +void set_unix_name_of_fd( struct fd *fd, const struct stat *fd_st ) +{ +#ifdef __linux__ + static const char procfs_fmt[] = "/proc/self/fd/%d"; + + char path[PATH_MAX], procfs_path[sizeof(procfs_fmt) - 2 /* %d */ + 11]; + struct stat path_st; + ssize_t len; + + sprintf( procfs_path, procfs_fmt, fd->unix_fd ); + len = readlink( procfs_path, path, sizeof(path) ); + if (len == -1 || len >= sizeof(path) ) + return; + path[len] = '\0'; + + /* Make sure it's an absolute path, has at least one hardlink, and the same inode */ + if (path[0] != '/' || stat( path, &path_st ) || path_st.st_nlink < 1 || + path_st.st_dev != fd_st->st_dev || path_st.st_ino != fd_st->st_ino) + return; + + if (!(fd->unix_name = mem_alloc( len + 1 ))) + return; + memcpy( fd->unix_name, path, len + 1 ); + +#elif defined(F_GETPATH) + char path[PATH_MAX]; + size_t size; + + if (fcntl( fd->unix_fd, F_GETPATH, path ) == -1 || path[0] != '/') + return; + + size = strlen(path) + 1; + if (!(fd->unix_name = mem_alloc( size ))) + return; + memcpy( fd->unix_name, path, size ); + +#endif +} + /* retrieve the object that is using an fd */ void *get_fd_user( struct fd *fd ) { diff --git a/server/file.c b/server/file.c index 76c687833c9..5624e621eac 100644 --- a/server/file.c +++ b/server/file.c @@ -155,6 +155,7 @@ struct file *create_file_for_fd( int fd, unsigned int access, unsigned int shari release_object( file ); return NULL; } + set_unix_name_of_fd( file->fd, &st ); allow_fd_caching( file->fd ); return file; } diff --git a/server/file.h b/server/file.h index 7f2d1637863..b08f9e7acc4 100644 --- a/server/file.h +++ b/server/file.h @@ -22,6 +22,7 @@ #define __WINE_SERVER_FILE_H #include <sys/types.h> +#include <sys/stat.h> #include "object.h" @@ -85,6 +86,7 @@ extern struct fd *open_fd( struct fd *root, const char *name, struct unicode_str unsigned int sharing, unsigned int options ); extern struct fd *create_anonymous_fd( const struct fd_ops *fd_user_ops, int unix_fd, struct object *user, unsigned int options ); +extern void set_unix_name_of_fd( struct fd *fd, const struct stat *fd_st ); extern struct fd *dup_fd_object( struct fd *orig, unsigned int access, unsigned int sharing, unsigned int options ); extern struct fd *get_fd_object_for_mapping( struct fd *fd, unsigned int access, unsigned int sharing ); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/5083
This still doesn't work unfortunately when fd is for the file not accessible directly (sounds like weird condition, but that's what we have in Proton now with Steam runtime where your similar patch worked once but stopped doing so). Also there is sort of similar issue when the std handle relates to a special Unix file (like pipe or fifo), in that case it is exposed as pipe but pipe's operation doesn't work and that breaks some runtimes (not sure now about python specifically). I don't want to say that it necessarily makes this patch not useful, just probably the only sure way to avoid the issues with std handles coming from Unix side is to avoid those when it is important. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5083#note_61180
On Wed Feb 14 17:26:07 2024 +0000, Paul Gofman wrote:
This still doesn't work unfortunately when fd is for the file not accessible directly (sounds like weird condition, but that's what we have in Proton now with Steam runtime where your similar patch worked once but stopped doing so). Also there is sort of similar issue when the std handle relates to a special Unix file (like pipe or fifo), in that case it is exposed as pipe but pipe's operation doesn't work and that breaks some runtimes (not sure now about python specifically). I don't want to say that it necessarily makes this patch not useful, just probably the only sure way to avoid the issues with std handles coming from Unix side is to avoid those when it is important. Thanks for the reminder. I remember something similar long ago, but clearly i forgot. Can you give some examples what do you mean with "exposed as pipe but pipe's operation doesn't work"? Are you referring to the special fds like bash's `pipe:[1234]` fds?
Yeah, it doesn't handle those (purposefully). Do you think we should change `GetFileInformationByHandle` instead somehow? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5083#note_61181
Can you give some examples what do you mean with "exposed as pipe but pipe's operation doesn't work"?
Run Wine program with stderr piped somewhere through '|'. Or to a file created with mkfifo. That will be reported as pipe in GetFileInformationByHandle, but if an app (like some runtimes do) will then expect some pipe operations to work that will fail similarly to what happens with python vs files without name retrieval not working.
Yeah, it doesn't handle those (purposefully). Do you think we should change `GetFileInformationByHandle` instead somehow?
https://gitlab.winehq.org/wine/wine/-/merge_requests/1425 : I apparently though so, but everything happened as Alexandre had foresaw in the comment there. Same or different runtimes expect then normal file operations to work. And IIRC reporting something weird like unknown or serial device doesn't universally work too I am not sure if we have any good option here of both keeping Unix fd integration and making this fully correct. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5083#note_61182
On Wed Feb 14 17:55:41 2024 +0000, Paul Gofman wrote:
Can you give some examples what do you mean with "exposed as pipe but pipe's operation doesn't work"? Run Wine program with stderr piped somewhere through '|'. Or to a file created with mkfifo. That will be reported as pipe in GetFileInformationByHandle, but if an app (like some runtimes do) will then expect some pipe operations to work that will fail similarly to what happens with python vs files without name retrieval not working. Yeah, it doesn't handle those (purposefully). Do you think we should change `GetFileInformationByHandle` instead somehow? https://gitlab.winehq.org/wine/wine/-/merge_requests/1425 : I apparently thought so, but everything happened as Alexandre had foreseen in the comment there. Same or different runtimes expect then normal file operations to work. And IIRC reporting something weird like unknown or serial device doesn't universally work too I am not sure if we have any good option here of both keeping Unix fd integration and making this fully correct. That's true for pipes, but in the case of the linked bug, it happens when it's redirected to actual files. Python expects it to work because it's redirected to files, so it shouldn't be a problem?
Maybe it doesn't cover all cases with pipes, but can you give a small (even if contrived) example where my patch actually introduces *wrong* behavior here so I can investigate it more? I mean even if it doesn't fix all cases like this, it's still better than nothing… -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5083#note_61184
On Wed Feb 14 18:11:12 2024 +0000, Gabriel Ivăncescu wrote:
That's true for pipes, but in the case of the linked bug, it happens when it's redirected to actual files. Python expects it to work because it's redirected to files, so it shouldn't be a problem? Maybe it doesn't cover all cases with pipes, but can you give a small (even if contrived) example where my patch actually introduces *wrong* behavior here so I can investigate it more? I mean even if it doesn't fix all cases like this, it's still better than nothing… As I noted, I don't have reason to think that your patch is useless or introducing wrong behaviour. I am just saying that it doesn't fully solve the issue. And, e. g., in Proton, for the same files for the same python runtime we will still have to keep something like the present workaround (https://github.com/ValveSoftware/wine/commit/e930e5c0b12355948f38b61611c1d70...) because this name retrieval does not work from Steam runtime while Steam passes stderr fd for a file not accessible from runtine's filesystem, while write to the fd work.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/5083#note_61186
FWIW there is get_path_from_fd() already in the server. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5083#note_61197
On Thu Feb 15 16:00:32 2024 +0000, Zebediah Figura wrote:
FWIW there is get_path_from_fd() already in the server. I took a look, but I'm not sure how helpful that is since I'll have to still check for \_\_linux\_\_ anyway because I need readlink to get the actual file's path, not the symlink in procfs. It also would incur an extra allocation that we can avoid on the symlink's path.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/5083#note_61287
I took a look, but I'm not sure how helpful that is since I'll have to still check for \__linux_\_ anyway because I need readlink to get the actual file's path, not the symlink in procfs.
Why do you need to check for __linux__ to use readlink()?
It also would incur an extra allocation that we can avoid on the symlink's path.
I don't think one small allocation is going to matter in a server call. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5083#note_61359
On Thu Feb 15 19:40:57 2024 +0000, Zebediah Figura wrote:
I took a look, but I'm not sure how helpful that is since I'll have to still check for \__linux_\_ anyway because I need readlink to get the actual file's path, not the symlink in procfs. Why do you need to check for __linux__ to use readlink()? It also would incur an extra allocation that we can avoid on the symlink's path. I don't think one small allocation is going to matter in a server call. Because on Linux, /proc/self/fd/<fd> are symbolic links to the actual file path, so we need it to get the path of the file.
But on platforms that have F_GETPATH, the result is already the path of the file, so calling readlink on them is not just useless but also wrong, not least because it accesses the filesystem itself (rather than the stored name) which can even incur some race conditions. For example. file gets removed, a link gets placed in there to something completely unrelated, and then we read that link and whatever it points to as the file path; or something else gets mounted over and now we're reading an arbitrary link for the file path. It's like calling readlink on Linux twice, one to get the file path and then on the file itself. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5083#note_61364
On Thu Feb 15 19:40:57 2024 +0000, Gabriel Ivăncescu wrote:
Because on Linux, /proc/self/fd/<fd> are symbolic links to the actual file path, so we need it to get the path of the file. But on platforms that have F_GETPATH, the result is already the path of the file, so calling readlink on them is not just useless but also wrong, not least because it accesses the filesystem itself (rather than the stored name) which can even incur some race conditions. For example. file gets removed, a link gets placed in there to something completely unrelated, and then we read that link and whatever it points to as the file path; or something else gets mounted over and now we're reading an arbitrary link for the file path. It's like calling readlink on Linux twice, one to get the file path and then on the file itself. Well, the race condition exists as soon as you're calling readlink() even once, and I don't think they're meaningful anyway (what's the application going to do with this information?)
But I also don't understand why returning a symlink instead of the target is important? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5083#note_61374
On Thu Feb 15 20:45:58 2024 +0000, Zebediah Figura wrote:
Well, the race condition exists as soon as you're calling readlink() even once, and I don't think they're meaningful anyway (what's the application going to do with this information?) But I also don't understand why returning a symlink instead of the target is important? I think there's a misunderstanding here. I'll try with an example maybe it's clearer. Let's say the file's path is `/path/to/file` and we want to retrieve it.
With F_GETPATH, we just call it on the fd and we get /path/to/file — we're done. On Linux we need to do something like readlink("/proc/self/fd/42"), because `/proc/self/fd/42` is a symlink to `/path/to/file` (or whatever). How do you want to avoid checking for linux here? If we call readlink unconditionally, then we'll end up with readlink("/path/to/file") on other platforms which is the thing I was mentioning being wrong. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5083#note_61462
race condition
Of course someone can change the real filename without us noticing, but I believe this is already a problem for `unix_path` (which we want to populate) in general. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5083#note_61474
Why do you need to check for \_\_linux\_\_ to use readlink()?
/proc/self/fd is Linux-specific. Perhaps we should use /dev/fd on FreeBSD and macOS instead. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5083#note_61475
On Fri Feb 16 16:06:54 2024 +0000, Jinoh Kang wrote:
Why do you need to check for \_\_linux\_\_ to use readlink()? /proc/self/fd is Linux-specific. Perhaps we should use /dev/fd on FreeBSD instead. (macOS has F\_GETPATH.) My point is that we're just retrieving the file name here. We shouldn't access the filesystem…
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/5083#note_61476
On Fri Feb 16 16:10:09 2024 +0000, Gabriel Ivăncescu wrote:
My point is that we're just retrieving the file name here. We shouldn't access the filesystem… Those platforms support F_GETPATH afaik, which is simpler (it's already in the patch). Correct me if I'm wrong.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/5083#note_61477
Those platforms support F_GETPATH afaik, which is simpler (it's already in the patch). Correct me if I'm wrong.
I don't know much about FreeBSD, but [bug 198570 - Add fnctl(F_GETPATH) support to FreeBSD](<https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=198570>) is still open. Code search of `F_GETPATH` in FreeBSD trunk doesn't show interesting results, either. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5083#note_61479
We shouldn't access the filesystem…
I don't know who you're referring to, but just in case you don't recognize /dev/fd, on BSD it seems to work the same way as /proc/self/fd. **EDIT**: Take this with a grain of salt--they might not be symbolic links, so not a complete replacement. Yes, I'm aware /dev/fd is a symlink to /proc/self/fd on Linux. But, again, this is Linux-specific. (Is it part of LSB?) -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5083#note_61480
On Fri Feb 16 16:26:11 2024 +0000, Jinoh Kang wrote:
We shouldn't access the filesystem… I don't know who you're referring to, but just in case you don't recognize /dev/fd, on BSD it seems to work the same way as /proc/self/fd. **EDIT**: Take this with a grain of salt--[they might not be symbolic links](<https://man.freebsd.org/cgi/man.cgi?fdescfs>), so not a complete replacement. Yes, I'm aware /dev/fd is a symlink to /proc/self/fd on Linux. But, again, this is Linux-specific. (Is it part of LSB?) Sorry, I was talking about the race condition and using readlink on the actual file in my example (which accesses the filesystem), if used unconditionally.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/5083#note_61481
On Fri Feb 16 16:25:12 2024 +0000, Gabriel Ivăncescu wrote:
Sorry, I was talking about the race condition and using readlink on the actual file in my example (which accesses the filesystem), if used unconditionally. There are two levels of indirection in a symlink pathname that can be concurrently modified: the poointer (symlink filename itself) and the pointee (resolution of the link target).
- Pointer pathname: yes, this is a general problem if readlink() argument was arbitrary. `/proc/self/fd/<N>` on the other hand should be stable, since `/proc/self` refers to the current process (wineserver) which is single-threaded. - Pointee pathname: also yes, and this is a problem for `/proc/self/fd/<N>` too. `/path/to/pointee`, which was returned by readlink, might get deleted or replaced with an entirely different file, rendering `/proc/self/fd/<N>`'s link-target stale. Again, not a problem, since this is completely expected (we don't rely on `unix_name` to be up to date already.) -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5083#note_61483
Would you add a test? Is it correct that you're trying to get `GetFinalPathNameByHandle` work on certain type of FDs? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5083#note_61485
On Fri Feb 16 17:35:20 2024 +0000, Jinoh Kang wrote:
Would you add a test? Is it correct that you're trying to get `GetFinalPathNameByHandle` work on certain type of FDs? Yes, more specifically GetFileInformationByHandle (it's what Python uses), but I'm not sure how to add a test since this happens with unix integration when e.g. redirecting stdout to a file (from unix) in which case it won't have a name.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/5083#note_61494
How do you want to avoid checking for linux here? If we call readlink unconditionally, then we'll end up with readlink("/path/to/file") on other platforms which is the thing I was mentioning being wrong.
What I'm trying to ask is, why would this be wrong? Why is it important that we don't use readlink() on this path? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5083#note_61506
On Fri Feb 16 18:54:57 2024 +0000, Zebediah Figura wrote:
How do you want to avoid checking for linux here? If we call readlink unconditionally, then we'll end up with readlink("/path/to/file") on other platforms which is the thing I was mentioning being wrong. What I'm trying to ask is, why would this be wrong? Why is it important that we don't use readlink() on this path? The job of this function is to retrieve the file's path and filename for anonymous fds (such as redirected unix outputs). It shouldn't access the underlying filesystem at all: that's obviously wrong, since it shouldn't depend on when it's called, nor if the file was deleted or not (it's not the purpose of this function to deal with that).
For example, in theory, the file could get removed after retrieving its path (via F_GETPATH) and a symlink placed in place of it, then readlink would access that symlink and dereference it… Whether the rest of the code deals with such race conditions is another point, but I don't see why this function should have to concern itself with it, or access the filesystem at all? It's like needless corner cases to worry about. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5083#note_61507
The job of this function is to retrieve the file's path and filename for anonymous fds (such as redirected unix outputs). It shouldn't access the underlying filesystem at all: that's obviously wrong, since it shouldn't depend on when it's called, nor if the file was deleted or not (it's not the purpose of this function to deal with that).
But why? What's wrong with accessing the file system? If stdout is a symlink, how is that different from stdout being the target of the symlink? Why is returning the latter the wrong thing to do?
For example, in theory, the file could get removed after retrieving its path (via F_GETPATH) and a symlink placed in place of it, then readlink would access that symlink and dereference it… Whether the rest of the code deals with such race conditions is another point, but I don't see why this function should have to concern itself with it, or access the filesystem at all? It's like needless corner cases to worry about.
Why is this a meaningful race? If you assume that the filesystem can be modified, how can a program do *anything* meaningful with the output of GetFinalPathNameByHandle(), *ever*? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5083#note_61509
But why? What's wrong with accessing the file system?
If stdout is a symlink, how is that different from stdout being the target of the symlink? Why is returning the latter the wrong thing to do?
Why is it our job to do that instead of letting the app decide? I mean I don't know what you want me to answer, I'm trying to make it as simple and less burden on our side as possible so we don't have any extra things to worry about? Here's another one: Let's assume the user *does* decide to redirect output to a symlink. Why should we only go one level deep? Why not loop calling readlink until we find a non-link? Calling it once seems arbitrary to me. See what I mean?
Why is this a meaningful race? If you assume that the filesystem can be modified, how can a program do *anything* meaningful with the output of GetFinalPathNameByHandle(), *ever*?
Sorry I don't quite understand what you mean here. Of course things can happen, and apps can do anything they want with the filename; they can also check if it still exists and whatnot. Why is it our business to do that on behalf of the app? Sure, this race isn't particularly common, it was just off the top of my head. But I just don't get why must we compromise "correct" behavior here and get headaches later? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5083#note_61515
On Fri Feb 16 19:50:06 2024 +0000, Gabriel Ivăncescu wrote:
But why? What's wrong with accessing the file system?
If stdout is a symlink, how is that different from stdout being the target of the symlink? Why is returning the latter the wrong thing to do? Why is it our job to do that instead of letting the app decide? I mean I don't know what you want me to answer, I'm trying to make it as simple and less burden on our side as possible so we don't have any extra things to worry about? Here's another one: Let's assume the user *does* decide to redirect output to a symlink. Why should we only go one level deep? Why not loop calling readlink until we find a non-link? Calling it once seems arbitrary to me. See what I mean? Specifically, calling it once also is **different behavior** across platforms, because on Linux it **won't** be dereferenced, while on those with F_GETPATH it will. Why is this a meaningful race? If you assume that the filesystem can be modified, how can a program do *anything* meaningful with the output of GetFinalPathNameByHandle(), *ever*? Sorry I don't quite understand what you mean here. Of course things can happen, and apps can do anything they want with the filename; they can also check if it still exists and whatnot. Why is it our business to do that on behalf of the app? Sure, this race isn't particularly common, it was just off the top of my head. But I just don't get why must we compromise "correct" behavior here and get headaches later? How is there any "correct" behaviour in this situation? This patch is addressing a scenario that doesn't appear on Windows and has no analog, it's the case where a Unix fd, not opened by Wine, is used as a standard handle for a Wine process. Why is returning the symlink path "right", and the real path "wrong"? Why does it *matter* which path you return, when they're fundamentally the same file?
Why is this a meaningful race? If you assume that the filesystem can be modified, how can a program do *anything* meaningful with the output of GetFinalPathNameByHandle(), *ever*?
Sorry I don't quite understand what you mean here. Of course things can happen, and apps can do anything they want with the filename; they can also check if it still exists and whatnot. Why is it our business to do that on behalf of the app?
Sure, this race isn't particularly common, it was just off the top of my head. But I just don't get why must we compromise "correct" behavior here and get headaches later?
A link can be changed under someone's feet, but the same thing is true of *any* file path. Why does the first race matter, and the second part doesn't? As far as I can see, there is no race that you can come up with related to dereferencing a symlink, that doesn't exist already *anyway* in some other form. Trying to fix a race is only useful if the race can't be triggered some other way. Unless I'm missing something, the only way an application can safely use GetFinalPathNameByHandle() at all is if they assume the file system won't be changed. That's not to say that's a bad assumption—maybe it's their own config folder, and the user decides to mess with that while the application is running then she gets to keep the pieces. But if you assume that the file system won't be changed, then *no* TOCTOU races matter at all. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5083#note_61517
On Fri Feb 16 20:37:37 2024 +0000, Zebediah Figura wrote:
How is there any "correct" behaviour in this situation? This patch is addressing a scenario that doesn't appear on Windows and has no analog, it's the case where a Unix fd, not opened by Wine, is used as a standard handle for a Wine process. Why is returning the symlink path "right", and the real path "wrong"? Why does it *matter* which path you return, when they're fundamentally the same file?
Why is this a meaningful race? If you assume that the filesystem can be modified, how can a program do *anything* meaningful with the output of GetFinalPathNameByHandle(), *ever*?
Sorry I don't quite understand what you mean here. Of course things can happen, and apps can do anything they want with the filename; they can also check if it still exists and whatnot. Why is it our business to do that on behalf of the app?
Sure, this race isn't particularly common, it was just off the top of my head. But I just don't get why must we compromise "correct" behavior here and get headaches later? A link can be changed under someone's feet, but the same thing is true of *any* file path. Why does the first race matter, and the second part doesn't? As far as I can see, there is no race that you can come up with related to dereferencing a symlink, that doesn't exist already *anyway* in some other form. Trying to fix a race is only useful if the race can't be triggered some other way. Unless I'm missing something, the only way an application can safely use GetFinalPathNameByHandle() at all is if they assume the file system won't be changed. That's not to say that's a bad assumption—maybe it's their own config folder, and the user decides to mess with that while the application is running then she gets to keep the pieces. But if you assume that the file system won't be changed, then *no* TOCTOU races matter at all. Hilariously, I just tested, and if you use a symlink as a standard handle to a Linux program, /proc/.../fd/ points to the target *anyway*.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/5083#note_61518
On Fri Feb 16 20:39:45 2024 +0000, Zebediah Figura wrote:
Hilariously, I just tested, and if you use a symlink as a standard handle to a Linux program, /proc/.../fd/ points to the target *anyway*. @zfigura so you're saying we should return `/proc/<wineserver-pid>/fd/<wineserver-fd>` from `get_handle_unix_name` server req handler? Or make FileNameInfo return `Z:\proc\self\fd\<client-fd>` itself?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/5083#note_61557
happens with unix integration when e.g. redirecting stdout to a file (from unix) in which case it won't have a name.
You mean because stdout is redirected to an *anonymous* file? Is it also the case in the testbot? I think testbot redirects output to filesystem, unless I'm mistaken. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5083#note_61558
@zfigura so you're saying we should return `/proc/<wineserver-pid>/fd/<wineserver-fd>` from `get_handle_unix_name` server req handler? Or make FileNameInfo return `Z:\proc\self\fd\<client-fd>` itself?
Not necessarily, although I don't think there's any harm in it either? I just don't see the point of trying to dereference symlinks a specific number of times, and thereby complicating the code just to add specific logic for Linux. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5083#note_61565
On Sat Feb 17 00:53:57 2024 +0000, Zebediah Figura wrote:
@zfigura so you're saying we should return `/proc/<wineserver-pid>/fd/<wineserver-fd>` from `get_handle_unix_name` server req handler? Or make FileNameInfo return `Z:\proc\self\fd\<client-fd>` itself? Not necessarily, although I don't think there's any harm in it either? I just don't see the point of trying to dereference symlinks a specific number of times, and thereby complicating the code just to add specific logic for Linux. You would be right if the app used the resolved pathname (`\\?\unix\proc\123\fd\456`) for opening *only*. This will automatically dereference the symlink, and the # of dereferences (before or after `get_handle_unix_name`) will be irrelevant.
OTOH, what if the app doesn't want to dereference (à la O\_NOFOLLOW) it at all? If you allow me to speculate, it could assume the pathname to be fully resolved, and become unhappy if it found out `\\?\unix\proc\123\fd\456` is a symlink. I don't want to speculate here. I'd rather just wait for @insn to come up with a concrete use case,[^1] along with tests.[^2] [^1]: Looks like something along CPython calling GetFileInformationHandle, but I'm not sure how this is relevant. [^2]: Standalone tests (as an MR attachment, outside winetest) should be enough. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5083#note_61567
On Sat Feb 17 00:32:47 2024 +0000, Jinoh Kang wrote:
happens with unix integration when e.g. redirecting stdout to a file (from unix) in which case it won't have a name. You mean because stdout is redirected to an *anonymous* file? Is it also the case in the testbot? I think testbot redirects output to filesystem, unless I'm mistaken. Are you sure you don't actually mean GetFileInformationByHandle**Ex**? Then, perhaps we should just stop relying on whatever requires unix\_name.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/5083#note_61570
GetFileInformationByHandle (it's what Python uses)
Python has seen a number of changes regarding its `GetFileInformationByHandle` usage. - https://github.com/python/cpython/commit/0f175766e27642108c65bba04bbd54dcf87... - https://github.com/python/cpython/commit/29af7369dbbbba8cefafb196e977bce8189... Please share the exact CPython version you're having problem with. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5083#note_61571
On Sat Feb 17 01:55:57 2024 +0000, Jinoh Kang wrote:
You would be right if the app used the resolved pathname (`\\?\unix\proc\123\fd\456`) for opening *only*. This will automatically dereference the symlink, and the # of dereferences (before or after `get_handle_unix_name`) will be irrelevant. OTOH, what if the app doesn't want to dereference (à la O\_NOFOLLOW) it at all? If you allow me to speculate, it could assume the pathname to be fully resolved, and become unhappy if it found out `\\?\unix\proc\123\fd\456` is a symlink. I don't want to speculate here. I'd rather just wait for @insn to come up with a concrete use case,[^1] along with tests.[^2] [^1]: Looks like something along CPython calling GetFileInformationHandle, but I'm not sure how this is relevant. [^2]: Standalone tests (as an MR attachment, outside winetest) should be enough. Maybe, but that's very much a "what if". It also makes more sense than trying to dereference the symlink exactly once.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/5083#note_61572
Maybe, but that's very much a "what if". It also makes more sense than trying to dereference the symlink exactly once.
`/proc/.../fd/...` points to the final resolved pathname (unless `O_PATH|O_NOFOLLOW` is specified), so it should be the same as realpath (recursively follow link) in most cases. I don't see a problem here. Again, I'm not here to speculate. Let's just wait for the other thread, shall we? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5083#note_61573
On Sat Feb 17 02:40:58 2024 +0000, Jinoh Kang wrote: > > GetFileInformationByHandle (it's what Python uses) > Python has seen a number of changes regarding its > `GetFileInformationByHandle` usage. > - https://github.com/python/cpython/commit/0f175766e27642108c65bba04bbd54dcf8799b0e > - https://github.com/python/cpython/commit/29af7369dbbbba8cefafb196e977bce8189a527d > Please share the exact CPython version you're having a problem with, so > future wine hackers will understand why this patch is needed / shouldn't > be reverted. See the linked bug of the commit (46070). Some installers, launchers and games bundle it so it's not something we can control. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5083#note_61824
On Sat Feb 17 03:20:14 2024 +0000, Jinoh Kang wrote:
Maybe, but that's very much a "what if". It also makes more sense than trying to dereference the symlink exactly once. `/proc/.../fd/...` points to the final resolved pathname (unless `O_PATH|O_NOFOLLOW` is specified), so it should be the same as realpath (recursively follow link) in most cases. I don't see a problem here. Again, I'm not here to speculate. Let's just wait for the other thread, shall we? @iamahuman is this the kind of test you were looking for?
```c #include <windows.h> #include <stdio.h> int main(int argc, char **argv) { char buf[4096] = {0}; FILE_NAME_INFO *name_info = (FILE_NAME_INFO*)buf; if (!GetFileInformationByHandleEx(GetStdHandle(STD_OUTPUT_HANDLE), FileNameInfo, name_info, sizeof(buf))) { fprintf(stderr, "GetFileInformationByHandleEx failed %08lx\n", GetLastError()); return 1; } WriteFile(GetStdHandle(STD_ERROR_HANDLE), name_info->FileName, name_info->FileNameLength, NULL, NULL); return 0; } ``` On Windows: ``` test.exe > C:\Temp\test.txt ``` Output: `\Temp\test.txt` On wine: ``` wine test.exe > /tmp/wine/testprefix/drive_c/Temp/test.txt ``` Upstream output: `GetFileInformationByHandleEx failed 00000006` With this MR: `\Temp\test.txt` ``` -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5083#note_61825
participants (4)
-
Gabriel Ivăncescu -
Jinoh Kang (＠iamahuman)
-
Paul Gofman (＠gofman)
-
Zebediah Figura (＠zfigura)