[PATCH v4 0/1] MR1414: ntdll: Add integration with Linux FAT file attributes
On Linux, the FAT filesystem implementation allows DOS file attributes to be applied to files and queried through a family of ioctls. Note that these ioctls are not yet supported by the NTFS or CIFS drivers. Signed-off-by: Joel Holdsworth <joel(a)airwebreathe.org.uk> -- v4: ntdll: Add integration with Linux FAT file attributes https://gitlab.winehq.org/wine/wine/-/merge_requests/1414
From: Joel Holdsworth <joel(a)airwebreathe.org.uk> On Linux, the FAT filesystem implementation allows DOS file attributes to be applied to files and queried through a family of ioctls. Note that these ioctls are not yet supported by the NTFS or CIFS drivers. This patch implements a design where Wine ntdll will attempt to store attributes in every available storage locations (currently the Samba-style user.DOSATTRIB extended attribute, and the Linux FAT filesystem ioctl). DOS Attibute flags are then retrieved from every available storage location and OR'd together. In future, additional filesystem and OS-specific storage locations can be added in similar fashion. Signed-off-by: Joel Holdsworth <joel(a)airwebreathe.org.uk> --- configure | 6 ++ configure.ac | 1 + dlls/ntdll/unix/file.c | 190 ++++++++++++++++++++++++++++++++++------- include/config.h.in | 3 + 4 files changed, 167 insertions(+), 33 deletions(-) diff --git a/configure b/configure index aaebcc553f5..b755a36416a 100755 --- a/configure +++ b/configure @@ -8014,6 +8014,12 @@ if test "x$ac_cv_header_linux_major_h" = xyes then : printf "%s\n" "#define HAVE_LINUX_MAJOR_H 1" >>confdefs.h +fi +ac_fn_c_check_header_compile "$LINENO" "linux/msdos_fs.h" "ac_cv_header_linux_msdos_fs_h" "$ac_includes_default" +if test "x$ac_cv_header_linux_msdos_fs_h" = xyes +then : + printf "%s\n" "#define HAVE_LINUX_MSDOS_FS_H 1" >>confdefs.h + fi ac_fn_c_check_header_compile "$LINENO" "linux/param.h" "ac_cv_header_linux_param_h" "$ac_includes_default" if test "x$ac_cv_header_linux_param_h" = xyes diff --git a/configure.ac b/configure.ac index fe6a773d1c3..5e7f963478f 100644 --- a/configure.ac +++ b/configure.ac @@ -437,6 +437,7 @@ AC_CHECK_HEADERS(\ linux/input.h \ linux/ioctl.h \ linux/major.h \ + linux/msdos_fs.h \ linux/param.h \ linux/serial.h \ linux/types.h \ diff --git a/dlls/ntdll/unix/file.c b/dlls/ntdll/unix/file.c index e80eb368e8a..133fb4d85ce 100644 --- a/dlls/ntdll/unix/file.c +++ b/dlls/ntdll/unix/file.c @@ -87,6 +87,9 @@ #ifdef HAVE_LINUX_MAJOR_H # include <linux/major.h> #endif +#ifdef HAVE_LINUX_MSDOS_FS_H +# include <linux/msdos_fs.h> +#endif #ifdef HAVE_SYS_PARAM_H #include <sys/param.h> #endif @@ -1570,11 +1573,51 @@ static BOOL fd_is_mount_point( int fd, const struct stat *st ) } +#ifdef HAVE_LINUX_MSDOS_FS_H +static BOOL fd_get_fat_dos_attrib( int fd, ULONG *attr ) +{ + uint32_t fat_attrib; + + if (ioctl( fd, FAT_IOCTL_GET_ATTRIBUTES, &fat_attrib ) == 0) + { + if (fat_attrib & ATTR_HIDDEN) *attr |= FILE_ATTRIBUTE_HIDDEN; + if (fat_attrib & ATTR_SYS) *attr |= FILE_ATTRIBUTE_SYSTEM; + return TRUE; + } + + return FALSE; +} +#endif + + +static BOOL fd_get_xattr_dos_attrib( int fd, ULONG *attr ) +{ + char data[65]; + int len; + + len = xattr_fget( fd, SAMBA_XATTR_DOS_ATTRIB, data, sizeof(data)-1 ); + if (len != -1) + { + *attr |= parse_samba_dos_attrib_data( data, len ); + return TRUE; + } +#ifdef ENODATA + else if (errno == ENODATA) + return TRUE; +#endif + else if (errno == ENOTSUP) + return TRUE; + + WARN( "Failed to get extended attribute " SAMBA_XATTR_DOS_ATTRIB ". errno %d (%s)\n", + errno, strerror( errno ) ); + + return FALSE; +} + /* get the stat info and file attributes for a file (by file descriptor) */ static int fd_get_file_info( int fd, unsigned int options, struct stat *st, ULONG *attr ) { - char attr_data[65]; - int attr_len, ret; + int ret; *attr = 0; ret = fstat( fd, st ); @@ -1584,23 +1627,38 @@ static int fd_get_file_info( int fd, unsigned int options, struct stat *st, ULON if ((options & FILE_OPEN_REPARSE_POINT) && fd_is_mount_point( fd, st )) *attr |= FILE_ATTRIBUTE_REPARSE_POINT; - attr_len = xattr_fget( fd, SAMBA_XATTR_DOS_ATTRIB, attr_data, sizeof(attr_data)-1 ); - if (attr_len != -1) - *attr |= parse_samba_dos_attrib_data( attr_data, attr_len ); - else - { - if (errno == ENOTSUP) return ret; -#ifdef ENODATA - if (errno == ENODATA) return ret; +#ifdef HAVE_LINUX_MSDOS_FS_H + /* try retrieving FAT file attributes */ + fd_get_fat_dos_attrib( fd, attr ); #endif - WARN( "Failed to get extended attribute " SAMBA_XATTR_DOS_ATTRIB ". errno %d (%s)\n", - errno, strerror( errno ) ); - } + + /* try retrieving DOS attributes from extended attributes */ + fd_get_xattr_dos_attrib( fd, attr ); + return ret; } -static int fd_set_dos_attrib( int fd, ULONG attr ) +#ifdef HAVE_LINUX_MSDOS_FS_H +static int fd_set_fat_dos_attrib( int fd, ULONG attr ) +{ + int ret; + uint32_t fat_attrib; + + /* read flags so that flags other than HIDDEN and SYSTEM e.g. READONLY can be preserved */ + ret = ioctl( fd, FAT_IOCTL_GET_ATTRIBUTES, &fat_attrib ); + if (ret != 0) + return ret; + + fat_attrib &= ~(ATTR_HIDDEN | ATTR_SYS); + if (attr & FILE_ATTRIBUTE_HIDDEN) fat_attrib |= ATTR_HIDDEN; + if (attr & FILE_ATTRIBUTE_SYSTEM) fat_attrib |= ATTR_SYS; + return ioctl( fd, FAT_IOCTL_SET_ATTRIBUTES, &fat_attrib ); +} +#endif + + +static int fd_set_xattr_dos_attrib( int fd, ULONG attr ) { /* we only store the HIDDEN and SYSTEM attributes */ attr &= XATTR_ATTRIBS_MASK; @@ -1617,6 +1675,40 @@ static int fd_set_dos_attrib( int fd, ULONG attr ) } +static void fd_set_dos_attrib( int fd, ULONG attr ) +{ +#ifdef HAVE_LINUX_MSDOS_FS_H + int fat_errno = 0; +#endif + int xattr_errno = 0; + +#ifdef HAVE_LINUX_MSDOS_FS_H + /* try setting FAT file attributes */ + if (fd_set_fat_dos_attrib( fd, attr ) == -1) + fat_errno = errno; +#endif + + /* try setting DOS attributes into extended attributes */ + if (fd_set_xattr_dos_attrib( fd, attr ) == -1) + xattr_errno = errno; + + if ( +#ifdef HAVE_LINUX_MSDOS_FS_H + fat_errno || +#endif + xattr_errno) { + WARN( "No method was found to store DOS attributes. "); +#ifdef HAVE_LINUX_MSDOS_FS_H + WARN( "Storage with FAT_IOCTL_SET_ATTRIBUTES failed with errno %d (%s). ", + fat_errno, strerror( fat_errno ) ); +#endif + WARN( "Storage with " SAMBA_XATTR_DOS_ATTRIB + " extended attribute failed with errno %d (%s).\n", + xattr_errno, strerror( xattr_errno ) ); + } +} + + /* set the stat info and file attributes for a file (by file descriptor) */ NTSTATUS fd_set_file_info( int fd, ULONG attr ) { @@ -1637,20 +1729,58 @@ NTSTATUS fd_set_file_info( int fd, ULONG attr ) } if (fchmod( fd, st.st_mode ) == -1) return errno_to_status( errno ); - if (fd_set_dos_attrib( fd, attr ) == -1 && errno != ENOTSUP) - WARN( "Failed to set extended attribute " SAMBA_XATTR_DOS_ATTRIB ". errno %d (%s)\n", - errno, strerror( errno ) ); + fd_set_dos_attrib( fd, attr ); return STATUS_SUCCESS; } +#ifdef HAVE_LINUX_MSDOS_FS_H +static BOOL get_fat_dos_attrib( const char *path, ULONG *attr ) +{ + BOOL ret = FALSE; + int fd = open( path, O_PATH ); + + if (fd) + { + if (fd_get_fat_dos_attrib( fd, attr )) ret = TRUE; + close( fd ); + } + + return ret; +} +#endif + + +static BOOL get_xattr_dos_attrib( const char *path, ULONG *attr ) +{ + char data[65]; + int len; + + len = xattr_get( path, SAMBA_XATTR_DOS_ATTRIB, data, sizeof(data)-1 ); + if (len != -1) + { + *attr |= parse_samba_dos_attrib_data( data, len ); + return TRUE; + } +#ifdef ENODATA + else if (errno == ENODATA) + return TRUE; +#endif + else if (errno == ENOTSUP) + return TRUE; + + WARN( "Failed to get extended attribute " SAMBA_XATTR_DOS_ATTRIB " from \"%s\". errno %d (%s)\n", + path, errno, strerror( errno ) ); + + return FALSE; +} + /* get the stat info and file attributes for a file (by name) */ static int get_file_info( const char *path, struct stat *st, ULONG *attr ) { char *parent_path; - char attr_data[65]; - int attr_len, ret; + int ret; *attr = 0; ret = lstat( path, st ); @@ -1677,18 +1807,14 @@ static int get_file_info( const char *path, struct stat *st, ULONG *attr ) } *attr |= get_file_attributes( st ); - attr_len = xattr_get( path, SAMBA_XATTR_DOS_ATTRIB, attr_data, sizeof(attr_data)-1 ); - if (attr_len != -1) - *attr |= parse_samba_dos_attrib_data( attr_data, attr_len ); - else - { - if (errno == ENOTSUP) return ret; -#ifdef ENODATA - if (errno == ENODATA) return ret; +#ifdef HAVE_LINUX_MSDOS_FS_H + /* try retrieving FAT file attributes */ + get_fat_dos_attrib( path, attr ); #endif - WARN( "Failed to get extended attribute " SAMBA_XATTR_DOS_ATTRIB " from \"%s\". errno %d (%s)\n", - path, errno, strerror( errno ) ); - } + + /* try retrieving DOS attributes from extended attributes */ + get_xattr_dos_attrib( path, attr ); + return ret; } @@ -4042,9 +4168,7 @@ NTSTATUS WINAPI NtCreateFile( HANDLE *handle, ACCESS_MASK access, OBJECT_ATTRIBU /* set any DOS extended attributes */ if (!server_get_unix_fd( *handle, 0, &fd, &needs_close, NULL, NULL )) { - if (fd_set_dos_attrib( fd, attributes ) == -1 && errno != ENOTSUP) - WARN( "Failed to set extended attribute " SAMBA_XATTR_DOS_ATTRIB ". errno %d (%s)", - errno, strerror( errno ) ); + fd_set_dos_attrib( fd, attributes ); if (needs_close) close( fd ); } } diff --git a/include/config.h.in b/include/config.h.in index 3a06d36bd02..763c1bf29f6 100644 --- a/include/config.h.in +++ b/include/config.h.in @@ -209,6 +209,9 @@ /* Define to 1 if you have the <linux/major.h> header file. */ #undef HAVE_LINUX_MAJOR_H +/* Define to 1 if you have the <linux/msdos_fs.h> header file. */ +#undef HAVE_LINUX_MSDOS_FS_H + /* Define to 1 if you have the <linux/param.h> header file. */ #undef HAVE_LINUX_PARAM_H -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/1414
This merge request was closed by Joel Holdsworth. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1414
Sorry for getting to review this so late; any particular reason this was closed? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1414#note_18123
Zebediah Figura (@zfigura) commented about dlls/ntdll/unix/file.c:
+static void fd_set_dos_attrib( int fd, ULONG attr ) +{ +#ifdef HAVE_LINUX_MSDOS_FS_H + int fat_errno = 0; +#endif + int xattr_errno = 0; + +#ifdef HAVE_LINUX_MSDOS_FS_H + /* try setting FAT file attributes */ + if (fd_set_fat_dos_attrib( fd, attr ) == -1) + fat_errno = errno; +#endif + + /* try setting DOS attributes into extended attributes */ + if (fd_set_xattr_dos_attrib( fd, attr ) == -1) + xattr_errno = errno; Why try to set both attributes?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/1414#note_18130
Zebediah Figura (@zfigura) commented about dlls/ntdll/unix/file.c:
return STATUS_SUCCESS; }
+#ifdef HAVE_LINUX_MSDOS_FS_H +static BOOL get_fat_dos_attrib( const char *path, ULONG *attr ) +{ + BOOL ret = FALSE; + int fd = open( path, O_PATH ); + + if (fd) + { + if (fd_get_fat_dos_attrib( fd, attr )) ret = TRUE; + close( fd ); + }
This isn't going to work, FAT_IOCTL_GET_ATTRIBUTES needs a real fd with read access. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1414#note_18131
Zebediah Figura (@zfigura) commented about dlls/ntdll/unix/file.c:
- else - { - if (errno == ENOTSUP) return ret; -#ifdef ENODATA - if (errno == ENODATA) return ret; +#ifdef HAVE_LINUX_MSDOS_FS_H + /* try retrieving FAT file attributes */ + get_fat_dos_attrib( path, attr ); #endif - WARN( "Failed to get extended attribute " SAMBA_XATTR_DOS_ATTRIB " from \"%s\". errno %d (%s)\n", - path, errno, strerror( errno ) ); - } + + /* try retrieving DOS attributes from extended attributes */ + get_xattr_dos_attrib( path, attr ); + Same here, why try both methods?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/1414#note_18132
On Thu Dec 1 10:46:41 2022 +0000, Zebediah Figura wrote:
Sorry for getting to review this so late; any particular reason this was closed? Hi Zeb,
Thanks for taking a look. I closed this MR because after more testing I found some problems and a couple of other things I wanted to improve about it, but I'm still experimenting with the concept. At the moment we are storing DOS attributes (`FILE_ATTRIBUTE_HIDDEN` and `FILE_ATTRIBUTE_SYSTEM`) in the Samba-style extended attributes only, however depending on the OS and filesystem type these may not be available, or other methods of storage may be available instead. The full list as I currently understand it is: * Samba-style user extended attributes - works on Linux and MacOS with modern filesystems. * The `system.dos_attrib` extended attribute for the Linux ntfs3 driver * The `FAT_IOCTL_SET_ATTRIBUTES` `ioctl` for the Linux FAT filesystem driver. * `fchflags`/`st_flags` on MacOs, FreeBSD and NetBSD can represent `FILE_ATTRIBUTE_READONLY` with `UF_IMMUTABLE` and `FILE_ATTRIBUTE_HIDDEN` with `UF_HIDDEN`. The show up in the UI on MacOS. (See Wine Bug [#39366](https://bugs.winehq.org/show_bug.cgi?id=39366)) I'm working on patches that take an opportunitistic approach of storing the attributes in any place that might be available. When reloading the attributes, my plan was to load them from all the places that are available and OR them together. I also think we should revisit `FILE_ATTRIBUTE_READONLY`. Currently this is excluded from the set of attributes that is stored in the Samba attribute. However, I think this design might have issues with interoperability - it would strip off the attribute if one was already present on a file. As you know `FILE_ATTRIBUTE_READONLY` is currently stored in the file's access mod flags. However, I think it would be better to add this to the mix for the `READONLY` attribute along with the other backends listed above. At the same time I want to polish up this patch: [server: Allow to open files without any permission bits.](https://github.com/wine-staging/wine-staging/blob/master/patches/server-File...). We spoke about this one previously - it temporarily `chmod`s a read-only file to allow it to be openned. (This is fixes Wine Bug [#50771](https://bugs.winehq.org/show_bug.cgi?id=50771)) The biggest catch I can see with my concept for the `READONLY` attribute is that some of the backends would add additional layers of locking to a file, so to open read-only file, we would need to try to unlock it via `chmod`, `fchflags` and `FAT_IOCTL_SET_ATTRIBUTES` - just because we don't know how the attribute might have been set. Forunately, `FAT_IOCTL_SET_ATTRIBUTES` is Linux-only, and `fchflags` is BSD-only, but still it would add to code complexity. Even though what I'm proposing is fairly complex, the problem is that the current implementation is limited to Linux and MacOS, and will only work on modern filesystems, and won't properly interopate with FAT and NTFS filesystems. I would appreciate your thoughts about this design. If you like the concept, I can finish it off and submit it with all the backends. Thanks Joel -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1414#note_18227
At the moment we are storing DOS attributes (`FILE_ATTRIBUTE_HIDDEN` and `FILE_ATTRIBUTE_SYSTEM`) in the Samba-style extended attributes only, however depending on the OS and filesystem type these may not be available, or other methods of storage may be available instead.
The full list as I currently understand it is:
* Samba-style user extended attributes - works on Linux and MacOS with modern filesystems. * The `system.dos_attrib` extended attribute for the Linux ntfs3 driver * The `FAT_IOCTL_SET_ATTRIBUTES` `ioctl` for the Linux FAT filesystem driver. * `fchflags`/`st_flags` on MacOs, FreeBSD and NetBSD can represent `FILE_ATTRIBUTE_READONLY` with `UF_IMMUTABLE` and `FILE_ATTRIBUTE_HIDDEN` with `UF_HIDDEN`. The show up in the UI on MacOS. (See Wine Bug [#39366](https://bugs.winehq.org/show_bug.cgi?id=39366))
I'm working on patches that take an opportunitistic approach of storing the attributes in any place that might be available. When reloading the attributes, my plan was to load them from all the places that are available and OR them together.
I'm not sure that it makes sense to use multiple approaches. Consider ntfs in particular, which IIRC has a dedicated DOS attribute field for all files, but also supports extended attributes. We probably don't want to store extended attributes as that'd be redundant. Same with Apple, whatever file system that is.
I also think we should revisit `FILE_ATTRIBUTE_READONLY`. Currently this is excluded from the set of attributes that is stored in the Samba attribute. However, I think this design might have issues with interoperability - it would strip off the attribute if one was already present on a file.
I think the proper solution to that is to not remove the attributes currently on a file, i.e. basically what this patch does for FAT, but for xattrs as well. (Along the same lines we should probably introduce support for parsing other Samba attribute versions, and avoid writing over the file times. But one thing at a time.)
As you know `FILE_ATTRIBUTE_READONLY` is currently stored in the file's access mod flags. However, I think it would be better to add this to the mix for the `READONLY` attribute along with the other backends listed above.
At the same time I want to polish up this patch: [server: Allow to open files without any permission bits.](https://github.com/wine-staging/wine-staging/blob/master/patches/server-File...). We spoke about this one previously - it temporarily `chmod`s a read-only file to allow it to be openned. (This is fixes Wine Bug [#50771](https://bugs.winehq.org/show_bug.cgi?id=50771))
The biggest catch I can see with my concept for the `READONLY` attribute is that some of the backends would add additional layers of locking to a file, so to open read-only file, we would need to try to unlock it via `chmod`, `fchflags` and `FAT_IOCTL_SET_ATTRIBUTES` - just because we don't know how the attribute might have been set. Forunately, `FAT_IOCTL_SET_ATTRIBUTES` is Linux-only, and `fchflags` is BSD-only, but still it would add to code complexity.
Even though what I'm proposing is fairly complex, the problem is that the current implementation is limited to Linux and MacOS, and will only work on modern filesystems, and won't properly interopate with FAT and NTFS filesystems.
I think chmod is definitely the wrong approach. I don't understand why we'd need to remove the READONLY attribute in order to open a file without any access bits, though? I suspect the right answer is to open the file with O_PATH if available, and possibly to open it without a backing FD (i.e. with only a path name) if not. That latter might take some extra work, though. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1414#note_18297
On Thu Dec 1 19:21:46 2022 +0000, Zebediah Figura wrote:
At the moment we are storing DOS attributes (`FILE_ATTRIBUTE_HIDDEN` and `FILE_ATTRIBUTE_SYSTEM`) in the Samba-style extended attributes only, however depending on the OS and filesystem type these may not be available, or other methods of storage may be available instead.
The full list as I currently understand it is:
* Samba-style user extended attributes - works on Linux and MacOS with modern filesystems. * The `system.dos_attrib` extended attribute for the Linux ntfs3 driver * The `FAT_IOCTL_SET_ATTRIBUTES` `ioctl` for the Linux FAT filesystem driver. * `fchflags`/`st_flags` on MacOs, FreeBSD and NetBSD can represent `FILE_ATTRIBUTE_READONLY` with `UF_IMMUTABLE` and `FILE_ATTRIBUTE_HIDDEN` with `UF_HIDDEN`. The show up in the UI on MacOS. (See Wine Bug [#39366](https://bugs.winehq.org/show_bug.cgi?id=39366))
I'm working on patches that take an opportunitistic approach of storing the attributes in any place that might be available. When reloading the attributes, my plan was to load them from all the places that are available and OR them together. I'm not sure that it makes sense to use multiple approaches. Consider ntfs in particular, which IIRC has a dedicated DOS attribute field for all files, but also supports extended attributes. We probably don't want to store extended attributes as that'd be redundant. Same with Apple, whatever file system that is. I also think we should revisit `FILE_ATTRIBUTE_READONLY`. Currently this is excluded from the set of attributes that is stored in the Samba attribute. However, I think this design might have issues with interoperability - it would strip off the attribute if one was already present on a file. I think the proper solution to that is to not remove the attributes currently on a file, i.e. basically what this patch does for FAT, but for xattrs as well. (Along the same lines we should probably introduce support for parsing other Samba attribute versions, and avoid writing over the file times. But one thing at a time.) As you know `FILE_ATTRIBUTE_READONLY` is currently stored in the file's access mod flags. However, I think it would be better to add this to the mix for the `READONLY` attribute along with the other backends listed above.
At the same time I want to polish up this patch: [server: Allow to open files without any permission bits.](https://github.com/wine-staging/wine-staging/blob/master/patches/server-File...). We spoke about this one previously - it temporarily `chmod`s a read-only file to allow it to be openned. (This is fixes Wine Bug [#50771](https://bugs.winehq.org/show_bug.cgi?id=50771))
The biggest catch I can see with my concept for the `READONLY` attribute is that some of the backends would add additional layers of locking to a file, so to open read-only file, we would need to try to unlock it via `chmod`, `fchflags` and `FAT_IOCTL_SET_ATTRIBUTES` - just because we don't know how the attribute might have been set. Forunately, `FAT_IOCTL_SET_ATTRIBUTES` is Linux-only, and `fchflags` is BSD-only, but still it would add to code complexity.
Even though what I'm proposing is fairly complex, the problem is that the current implementation is limited to Linux and MacOS, and will only work on modern filesystems, and won't properly interopate with FAT and NTFS filesystems. I think chmod is definitely the wrong approach. I don't understand why we'd need to remove the READONLY attribute in order to open a file without any access bits, though? I suspect the right answer is to open the file with O_PATH if available, and possibly to open it without a backing FD (i.e. with only a path name) if not. That latter might take some extra work, though. Hi Zeb, thanks for your thoughts. Sorry for the delay in getting back to you - I've been rather busy these last few days.
I'm not sure that it makes sense to use multiple approaches. Consider ntfs in particular, which IIRC has a dedicated DOS attribute field for all files, but also supports extended attributes. We probably don't want to store extended attributes as that'd be redundant. Same with Apple, whatever file system that is.
Yes - ideally every file would only ever have the single most optimal form of attribute marking. However, it seems to me that it's going to be difficult to ensure this without doing a lot of probing. This will involve a lot more OS-specific code, and I'm not thrilled about the possibility of TOCTOU issues. It seems more elegant to simply try various methods, and see what works. You are correct to say that on NTFS on Linux, every file will be labbelled by both a user extended attribute, and a system DOS attribute extended attrbite. But aside from being slightly inelegant, is this really a problem? It would mean that the file would retain its attribute if it were moved to another filesystem.
I think the proper solution to that is to not remove the attributes currently on a file, i.e. basically what this patch does for FAT, but for xattrs as well.
(Along the same lines we should probably introduce support for parsing other Samba attribute versions, and avoid writing over the file times. But one thing at a time.)
Completely agree.
I think chmod is definitely the wrong approach. I don't understand why we'd need to remove the READONLY attribute in order to open a file without any access bits, though?
This is my partially updated version of the patch from staging: https://gitlab.winehq.org/jhol/wine/-/commit/2d980a3c45b12d707b1a21f30e7af47... Just looking at the logic, the only scenario that could trigger the chmod behaviour is `FILE_WRITE_ATTRIBUTES` on a read-only file. I think `O_PATH` should work in this scenario? But we would need a fallback path for the other UNIXs. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1414#note_19446
I'm not sure that it makes sense to use multiple approaches. Consider ntfs in particular, which IIRC has a dedicated DOS attribute field for all files, but also supports extended attributes. We probably don't want to store extended attributes as that'd be redundant. Same with Apple, whatever file system that is.
Yes - ideally every file would only ever have the single most optimal form of attribute marking. However, it seems to me that it's going to be difficult to ensure this without doing a lot of probing. This will involve a lot more OS-specific code, and I'm not thrilled about the possibility of TOCTOU issues.
It seems more elegant to simply try various methods, and see what works. You are correct to say that on NTFS on Linux, every file will be labbelled by both a user extended attribute, and a system DOS attribute extended attrbite. But aside from being slightly inelegant, is this really a problem?
Well, it'd take up more space, presumably, whereas we get the system DOS attribute for free.
It would mean that the file would retain its attribute if it were moved to another filesystem.
Hmm, that's a fair point, though...
I think chmod is definitely the wrong approach. I don't understand why we'd need to remove the READONLY attribute in order to open a file without any access bits, though?
This is my partially updated version of the patch from staging: https://gitlab.winehq.org/jhol/wine/-/commit/2d980a3c45b12d707b1a21f30e7af47...
Just looking at the logic, the only scenario that could trigger the chmod behaviour is `FILE_WRITE_ATTRIBUTES` on a read-only file.
Does that actually matter in practice? I.e. are there cases where a Windows program sets FILE_WRITE_ATTRIBUTES access but not FILE_WRITE_DATA access? If it's just FILE_ATTRIBUTE_READONLY that's stripping Unix write access, I would agree that the better thing to do there is just to use the DOS attributes to store it, instead of translating it to Unix ~write access.
I think `O_PATH` should work in this scenario? But we would need a fallback path for the other UNIXs.
Ugh, I'm surprised this isn't a more standardized feature, it's missing from basically everything but Linux and FreeBSD. Maybe we just leave it as unimplemented; it wouldn't be a regression in that case. Or maybe we just implement a fallback based on storing path names and hang the TOCTOU issues. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1414#note_19505
Well, it'd take up more space, presumably, whereas we get the system DOS attribute for free.
True - I guess we could fall back to Sambe extended attributes only if no more suitable native method is found. The problem is that we probably need to update every available method that we implement, because it's the only way to be sure that file is in a consistent state at the end of the process - both in terms of Wine is doing with the file, and what other applications are doing. I suppose we could skip and where necessary remove any redundant Samba attributes if a better method of storage was found.
Does that actually matter in practice? I.e. are there cases where a Windows program sets FILE_WRITE_ATTRIBUTES access but not FILE_WRITE_DATA access?
Msys2 does this in its implementation of the `unlink()` syscall: https://github.com/msys2/msys2-runtime/blob/msys2-3_3_6-release/winsup/cygwi... See this bug: https://bugs.winehq.org/show_bug.cgi?id=50771 Msys2 pacman creates a database lock file, which it fails to delete after use, because Wine can't open the file handle for deletion.
If it's just FILE_ATTRIBUTE_READONLY that's stripping Unix write access, I would agree that the better thing to do there is just to use the DOS attributes to store it, instead of translating it to Unix \~write access.
I hadn't considered the possibility of doing away with he UNIX file access flags support entirely. It certainly does make things tricky. There will be similar issues if we add additional support for MacOS/BSD `st_flags` `UF_IMMUTABLE` (as per [Wine Bug #39366](https://bugs.winehq.org/show_bug.cgi?id=39366)). The benefit is that you get idiomatic behaviour for the host OS. Windows read-only files will appear read-only in the GUIs, for example. The problem is that mismatch in the semantics of these features compared to `FILE_ATTRIBUTE_READONLY` makes the implementation awkward. I am prepared to put in the work to build as clean an implementation as is possible, but it would be good to have a rough agreement around the design before I go too much futher down the rabbit hole.
Ugh, I'm surprised this isn't a more standardized feature, it's missing from basically everything but Linux and FreeBSD. Maybe we just leave it as unimplemented; it wouldn't be a regression in that case. Or maybe we just implement a fallback based on storing path names and hang the TOCTOU issues.
I think the latter is acceptable. It would be good to have this working for MacOS users. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1414#note_19509
Well, it'd take up more space, presumably, whereas we get the system DOS attribute for free.
True - I guess we could fall back to Sambe extended attributes only if no more suitable native method is found.
The problem is that we probably need to update every available method that we implement, because it's the only way to be sure that file is in a consistent state at the end of the process - both in terms of Wine is doing with the file, and what other applications are doing.
I suppose we could skip and where necessary remove any redundant Samba attributes if a better method of storage was found.
Yeah, maybe it is better to be redundant after all. I mean, it's not like we're going to be setting the attribute on every file anyway...
If it's just FILE_ATTRIBUTE_READONLY that's stripping Unix write access, I would agree that the better thing to do there is just to use the DOS attributes to store it, instead of translating it to Unix \~write access.
I hadn't considered the possibility of doing away with he UNIX file access flags support entirely.
It certainly does make things tricky.
There will be similar issues if we add additional support for MacOS/BSD `st_flags` `UF_IMMUTABLE` (as per [Wine Bug #39366](https://bugs.winehq.org/show_bug.cgi?id=39366)).
The benefit is that you get idiomatic behaviour for the host OS. Windows read-only files will appear read-only in the GUIs, for example.
The problem is that mismatch in the semantics of these features compared to `FILE_ATTRIBUTE_READONLY` makes the implementation awkward.
I am prepared to put in the work to build as clean an implementation as is possible, but it would be good to have a rough agreement around the design before I go too much futher down the rabbit hole.
Yeah, personally I agree that translating these semantics to the host would be *nice*, but I'd rather have programs just work than have that integration [well, and without having to rely on runtime chmod() to do it.] Honestly I'm not even sure that translating FILE_ATTRIBUTE_READONLY to ~0222 is even that nice of a feature? In my experience it's only really set on internal files like DLLs, which I wouldn't expect normal end users to be messing with *anyway*, and it actually kind of gets in the way by virtue of preventing prefix removal (without rm -f, anyway.) I don't know what UF_IMMUTABLE actually does, but if it's the same kind of thing I'm not sure it's worthwhile either. Of course, this is all *my* opinion (on what we should do and how we should do it), this probably deserves a weigh-in from others as well. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1414#note_19512
On Mon Dec 12 22:02:40 2022 +0000, Zebediah Figura wrote:
Well, it'd take up more space, presumably, whereas we get the system DOS attribute for free.
True - I guess we could fall back to Sambe extended attributes only if no more suitable native method is found.
The problem is that we probably need to update every available method that we implement, because it's the only way to be sure that file is in a consistent state at the end of the process - both in terms of Wine is doing with the file, and what other applications are doing.
I suppose we could skip and where necessary remove any redundant Samba attributes if a better method of storage was found. Yeah, maybe it is better to be redundant after all. I mean, it's not like we're going to be setting the attribute on every file anyway...
If it's just FILE_ATTRIBUTE_READONLY that's stripping Unix write access, I would agree that the better thing to do there is just to use the DOS attributes to store it, instead of translating it to Unix \~write access.
I hadn't considered the possibility of doing away with he UNIX file access flags support entirely.
It certainly does make things tricky.
There will be similar issues if we add additional support for MacOS/BSD `st_flags` `UF_IMMUTABLE` (as per [Wine Bug #39366](https://bugs.winehq.org/show_bug.cgi?id=39366)).
The benefit is that you get idiomatic behaviour for the host OS. Windows read-only files will appear read-only in the GUIs, for example.
The problem is that mismatch in the semantics of these features compared to `FILE_ATTRIBUTE_READONLY` makes the implementation awkward.
I am prepared to put in the work to build as clean an implementation as is possible, but it would be good to have a rough agreement around the design before I go too much futher down the rabbit hole. Yeah, personally I agree that translating these semantics to the host would be *nice*, but I'd rather have programs just work than have that integration [well, and without having to rely on runtime chmod() to do it.] Honestly I'm not even sure that translating FILE_ATTRIBUTE_READONLY to ~0222 is even that nice of a feature? In my experience it's only really set on internal files like DLLs, which I wouldn't expect normal end users to be messing with *anyway*, and it actually kind of gets in the way by virtue of preventing prefix removal (without rm -f, anyway.) I don't know what UF_IMMUTABLE actually does, but if it's the same kind of thing I'm not sure it's worthwhile either. Of course, this is all *my* opinion (on what we should do and how we should do it), this probably deserves a weigh-in from others as well. I personally use read only files a lot, it's nice to also access them from Wine file explorers/managers. Of course, Wine can't set read-only on directories, because Windows can't (at least not with same semantics as Unix), so that's fine.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/1414#note_19561
I personally use read only files a lot, it's nice to also access them from Wine file explorers/managers. Of course, Wine can't set read-only on directories, because Windows can't (at least not with same semantics as Unix), so that's fine.
You mean files on the Unix side which you've manually set as read-only via chmod? I don't think we'd be breaking that case, it'd only be if a program tries to open those files with FILE_WRITE_ATTRIBUTES, which should be relatively rare. The only known example is Cygwin's unlink(), which is going to need write access to delete the file anyway. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1414#note_19581
On Tue Dec 13 20:50:43 2022 +0000, Zebediah Figura wrote:
I personally use read only files a lot, it's nice to also access them from Wine file explorers/managers. Of course, Wine can't set read-only on directories, because Windows can't (at least not with same semantics as Unix), so that's fine. You mean files on the Unix side which you've manually set as read-only via chmod? I don't think we'd be breaking that case, it'd only be if a program tries to open those files with FILE_WRITE_ATTRIBUTES, which should be relatively rare. The only known example is Cygwin's unlink(), which is going to need write access to delete the file anyway. If I remember correctly, AkelPad was one of the apps that checks if its settings file (.ini) is read-only or not, and only complains if it's not read-only but it failed to write to it. For example, if the file isn't marked read-only, but is placed on read-only fs or made immutable, it will complain.
But also it means that Windows applications won't be able to "unset" the unix `read` permission (by marking it as read-only), which is something I'm more concerned about. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1414#note_19582
If I remember correctly, AkelPad was one of the apps that checks if its settings file (.ini) is read-only or not, and only complains if it's not read-only but it failed to write to it. For example, if the file isn't marked read-only, but is placed on read-only fs or made immutable, it will complain.
Hmm, in that case you could manually set the file as read-only using Wine explorer?
But also it means that Windows applications won't be able to "unset" the unix `read` permission (by marking it as read-only), which is something I'm more concerned about.
I assume you meant to say "'unset' the unix write permission"? My point above is that I'm not sure this is actually a desirable feature, i.e. at least in my experience it gets in the way more often than not, and I have a hard time imagining a circumstance in which it's a good idea. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1414#note_19583
On Tue Dec 13 21:54:19 2022 +0000, Zebediah Figura wrote:
If I remember correctly, AkelPad was one of the apps that checks if its settings file (.ini) is read-only or not, and only complains if it's not read-only but it failed to write to it. For example, if the file isn't marked read-only, but is placed on read-only fs or made immutable, it will complain. Hmm, in that case you could manually set the file as read-only using Wine explorer? But also it means that Windows applications won't be able to "unset" the unix `read` permission (by marking it as read-only), which is something I'm more concerned about. I assume you meant to say "'unset' the unix write permission"? My point above is that I'm not sure this is actually a desirable feature, i.e. at least in my experience it gets in the way more often than not, and I have a hard time imagining a circumstance in which it's a good idea. Yeah, I meant the write permission, sorry. I personally like the feature, but, I don't feel strongly about it, anyway.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/1414#note_19634
participants (4)
-
Gabriel Ivăncescu -
Joel Holdsworth
-
Joel Holdsworth (＠jhol)
-
Zebediah Figura (＠zfigura)