I'd appreciate if someone test this on Linux and MacOS. I got FreeBSD covered.

@bruns?

Bump.

Good now, @bruns?

Another bump.

@bruns?

Looks ok to me now. Can someone else please double check?

Can you change your status to approved? @dfaure, one final look maybe?

After almost two years, I'm so happy to see this land!

Phabricator didn't actually close the revision after I landed it because @bruns forgot to change his status to accepted. You can close this now, @arrowd. Great work!

I was wondering why copying files in Krusader or Dolphin from a vfat-formatted memory card stopped working for me after update. After copying the first file, the transfer stopped progressing and the process ended up consuming 100% CPU forever. Later I discovered that the cause of the breakage was this patch: If fgetxattr() fails with ENOTSUP, the code loops indefinitely calling fgetxattr().

The following hotfix made copying of files in Krusader work again for me:

--- a/src/ioslaves/file/file_unix.cpp
+++ b/src/ioslaves/file/file_unix.cpp
@@ -642,35 +642,35 @@ bool FileProtocol::copyXattrs(const int src_fd, const int dest_fd)
         ssize_t valuelen = 0;
         do {
             value.resize(valuelen);
 #if HAVE_SYS_XATTR_H && !defined(__stub_getxattr) && !defined(Q_OS_MAC)
             valuelen = fgetxattr(src_fd, key.constData(), value.data(), valuelen);
 #elif defined(Q_OS_MAC)
             valuelen = fgetxattr(src_fd, key.constData(), value.data(), valuelen, 0, 0);
 #elif HAVE_SYS_EXTATTR_H
             valuelen = extattr_get_fd(src_fd, EXTATTR_NAMESPACE_USER, key.constData(), valuelen == 0 ? nullptr : value.data(), valuelen);
 #endif
             if (valuelen > 0 && value.size() == 0) {
                 continue;
             }
             if (valuelen > 0 && value.size() > 0) {
                 break;
             }
-            if (valuelen == -1 && errno == ERANGE) {
+            if (valuelen == -1 && errno != ERANGE) {
                 valuelen = 0;
-                continue;
+                break;
             }
             // happens when attr value is an empty string
             if (valuelen == 0) {
                 break;
             }
         } while (true);

         // Write key:value pair on destination
 #if HAVE_SYS_XATTR_H && !defined(__stub_getxattr) && !defined(Q_OS_MAC)
         ssize_t destlen = fsetxattr(dest_fd, key.constData(), value.constData(), valuelen, 0);
 #elif defined(Q_OS_MAC)
         ssize_t destlen = fsetxattr(dest_fd, key.constData(), value.constData(), valuelen, 0, 0);
 #elif HAVE_SYS_EXTATTR_H
         ssize_t destlen = extattr_set_fd(dest_fd, EXTATTR_NAMESPACE_USER, key.constData(), value.constData(), valuelen);
 #endif
         if (destlen == -1 && errno == ENOTSUP) {

However, the code might need bigger changes to cover all the cases. I do not fully understand the (IMO over-complicated) loops around flistxattr() and fgetxattr(). Note that the fact that one of them uses while (true) { ... } whereas the other one uses do { ... } while (true) does not improve code readability either.

In D17816#677446, @kdudka wrote:

I was wondering why copying files in Krusader or Dolphin from a vfat-formatted memory card stopped working for me after update. After copying the first file, the transfer stopped progressing and the process ended up consuming 100% CPU forever. Later I discovered that the cause of the breakage was this patch: If fgetxattr() fails with ENOTSUP, the code loops indefinitely calling fgetxattr().

The following hotfix made copying of files in Krusader work again for me:
...

However, the code might need bigger changes to cover all the cases. I do not fully understand the (IMO over-complicated) loops around flistxattr() and fgetxattr(). Note that the fact that one of them uses while (true) { ... } whereas the other one uses do { ... } while (true) does not improve code readability either.

I think, the correct fix is something like

diff --git a/src/ioslaves/file/file_unix.cpp b/src/ioslaves/file/file_unix.cpp
index 74767123..4bc87c57 100644
--- a/src/ioslaves/file/file_unix.cpp
+++ b/src/ioslaves/file/file_unix.cpp
@@ -614,8 +614,8 @@ bool FileProtocol::copyXattrs(const int src_fd, const int dest_fd)
                 valuelen = 0;
                 continue;
             }
-            // happens when attr value is an empty string
-            if (valuelen == 0) {
+            // valuelen=0 happens when attr value is an empty string
+            if (valuelen == 0 || valuelen == -1) {
                 break;
             }
         } while (true);

Can you check if this works for you too?

Even after applying the proposed patch, the code still looks problematic to me. I would prefer to have it explained first. When fgetxattr(..., 0) returns -1/ERANGE, what is the point of calling fgetxattr(..., 0) again? It is still going to busy-loop indefinitely in this case, doesn't it? How many times do we actually need to call fgetxattr() on a single file descriptor? Twice? Then unbounded loop is not the best construction to begin with.

In D17816#677449, @kdudka wrote:

Even after applying the proposed patch, the code still looks problematic to me. I would prefer to have it explained first. When fgetxattr(..., 0) returns -1/ERANGE, what is the point of calling fgetxattr(..., 0) again? It is still going to busy-loop indefinitely in this case, doesn't it? How many times do we actually need to call fgetxattr() on a single file descriptor? Twice? Then unbounded loop is not the best construction to begin with.

Ever heard of a TOCTOU race?

Quoting from man 2 getxattr:

If size is specified as zero, these calls return the current size of the named extended attribute (and leave value unchanged). This can be used to determine the size of the buffer that should be supplied in a subsequent call. (But, bear in mind that there is a possibility that the attribute value may change between the two calls, so that it is still necessary to check the return status from the second call.)

The man page says that one has to check return value of the second call. It does not say that the function needs to be called in a loop indefinitely until it succeeds.

In D17816#677451, @kdudka wrote:

The man page says that one has to check return value of the second call. It does not say that the function needs to be called in a loop indefinitely until it succeeds.

And if the second call then returns ERANGE you have to start from the beginning.

I do not think we have to. Please have a look at how attr_copy_fd() from <attr/libattr.h> is implemented: http://git.savannah.nongnu.org/cgit/attr.git/tree/libattr/attr_copy_fd.c?id=a4187bec#n111 This code is quite mature and it is used by cp(1) on GNU/Linux for instance. I do not think that KDE users will appreciate some blindly coded fancy features on top of that when the basic functionality gets totally broken as a result of these experiments. Please keep the code simple and reliable.

In D17816#677453, @kdudka wrote:

I do not think we have to. Please have a look at how attr_copy_fd() from <attr/libattr.h> is implemented: http://git.savannah.nongnu.org/cgit/attr.git/tree/libattr/attr_copy_fd.c?id=a4187bec#n111 This code is quite mature and it is used by cp(1) on GNU/Linux for instance. I do not think that KDE users will appreciate some blindly coded fancy features on top of that when the basic functionality gets totally broken as a result of these experiments. Please keep the code simple and reliable.

Its not blindly coded. Please keep your arrogant comments to yourself.

And obviously, there is something strange going on on your system - flistxattr returns a list of keys, but fgetxattr then returns ENOTSUP?

@bruns I find your attitude unnecessarily hostile. If you think that the code is perfect as it is, feel free to patch it case by case until it eventually works for everybody. That is your choice.

Anyway, strace of cp --preserve=xattr on the same device looks like this:

$ LC_ALL=C strace -e '%file,/.*xattr' cp --preserve=xattr /mnt/mmc/file .
execve("/bin/cp", ["cp", "--preserve=xattr", "/mnt/mmc/file", "."], 0x7ffdf8529c88 /* 81 vars */) = 0
access("/etc/ld.so.preload", R_OK)      = -1 ENOENT (No such file or directory)
openat(AT_FDCWD, "/etc/ld.so.cache", O_RDONLY|O_CLOEXEC) = 3
openat(AT_FDCWD, "/lib64/libacl.so.1", O_RDONLY|O_CLOEXEC) = 3
openat(AT_FDCWD, "/lib64/libattr.so.1", O_RDONLY|O_CLOEXEC) = 3
openat(AT_FDCWD, "/lib64/libc.so.6", O_RDONLY|O_CLOEXEC) = 3
stat(".", {st_mode=S_IFDIR|S_ISVTX|0777, st_size=1522, ...}) = 0
newfstatat(AT_FDCWD, "/mnt/mmc/file", {st_mode=S_IFREG|0755, st_size=0, ...}, 0) = 0
newfstatat(AT_FDCWD, "./file", 0x7ffea3f59f50, 0) = -1 ENOENT (No such file or directory)
openat(AT_FDCWD, "/mnt/mmc/file", O_RDONLY) = 3
openat(AT_FDCWD, "./file", O_WRONLY|O_CREAT|O_EXCL, 0755) = 4
flistxattr(3, NULL, 0)                  = 17
flistxattr(3, "security.selinux\0", 17) = 17
openat(AT_FDCWD, "/etc/xattr.conf", O_RDONLY) = 5
fgetxattr(3, "security.selinux", NULL, 0) = -1 EOPNOTSUPP (Operation not supported)
cp: getting attribute 'security.selinux' of 'security.selinux': Operation not supported
+++ exited with 1 +++

In D17816#677455, @kdudka wrote:

@bruns I find your attitude unnecessarily hostile. If you think that the code is perfect as it is, feel free to patch it case by case until it eventually works for everybody. That is your choice.

I never said its perfect. There is exactly one case which was missed (and only in this one location, for both listxattr and setxattr a return value of -1 is handled correctly).

I do not think that KDE users will appreciate some blindly coded fancy features on top of that when the basic functionality gets totally broken as a result of these experiments.

Such well chosen words ...

Btw, there are plenty of more cases where kio deliberately deviates from the behavior of low level system tools.

I am fine with kio implementing things differently as long as the basic functionality keeps working. I used to use Krusader to get images out of my camera and it simply stopped working for me with no obvious indication of what went wrong. This code tries to implement some advanced error handling, yet it is written in a way that it passed a review with a fundamental programming mistake in the basic error handling.

The fix proposed in https://phabricator.kde.org/D17816#677448 is incomplete, as I understand it. If we break the loop with valuelen == -1, the value will be passed as size_t to the size argument of fsetxattr(), which may lead to reading value.constData() out of bounds.

What is the status of this? How do we move forwards? I know this has gone on a long time and we're all getting tired, but I think we can push this past the finish line without too much trouble, hopefully. :)

In D17816#677552, @ngraham wrote:

What is the status of this? How do we move forwards? I know this has gone on a long time and we're all getting tired, but I think we can push this past the finish line without too much trouble, hopefully. :)

This has landed long time ago. Unfortunately, in combination with SELINUX getfattr returns an error not mentioned in any man page, which causes an inifinite loop in the current code.

Fixed by https://invent.kde.org/frameworks/kio/-/merge_requests/368

Gotcha, thanks!