Support for xattrs on kio copy/move
AbandonedPublic

Authored by arrowd on Dec 27 2018, 9:38 AM.

Details

Summary

This patch adds support to KIO to preserve xattr of files and directories on copy or move.
Working for copy/move including overwrite and rename cases.
Not working for "write into" when coping folders have name conflict. I'm against adding to this user case as the name of the action don't imply overwrite.

Tested only on Linux.

FEATURE: 116617

Diff Detail

Branch
arcpatch-D17816
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 27193
Build 27211: arc lint + arc unit
There are a very large number of changes, so older changes are hidden. Show Older Changes
bruns added inline comments.Jun 2 2020, 11:13 PM
src/ioslaves/file/file_unix.cpp
561

"does not"

arrowd updated this revision to Diff 83207.Jun 3 2020, 5:55 PM
arrowd marked 8 inline comments as done.
  • Address comments.
arrowd added a comment.Jun 3 2020, 5:56 PM

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

arrowd updated this revision to Diff 83302.Jun 20 2020, 4:30 PM
  • Rebase on master.
bruns added inline comments.Jun 22 2020, 2:51 PM
src/ioslaves/file/file_unix.cpp
566

src_fd is quite meaningless as debug output

611

Infinite loop on valuelen == 0

arrowd updated this revision to Diff 83309.Jun 23 2020, 4:44 PM
arrowd marked an inline comment as done.
  • Improve comment.
arrowd added inline comments.Jun 23 2020, 5:23 PM
src/ioslaves/file/file_unix.cpp
611

Why? ERANGE means we need to come up with new value for valuelen, so we set it to zero and start over. On the new iteration it gets passed into fgetxattr/extattr_get_fd to find out sufficient value.

bruns added inline comments.Jun 24 2020, 12:29 PM
src/ioslaves/file/file_unix.cpp
611

"0" is a regular return value. Try your code with the following file:

touch t
setfattr -n user.foo -v "" t

and preferably add a test case ...

arrowd updated this revision to Diff 83318.Jun 29 2020, 6:09 PM
arrowd marked 3 inline comments as done.
  • Handle attrs with empty values.
  • Add test for it.
  • Fix syscalls for FreeBSD case.
bruns added inline comments.Jun 29 2020, 8:15 PM
autotests/jobtest.cpp
459

not done ...

src/ioslaves/file/file_unix.cpp
606

No extra spaces for qDebug, inserts spaces itself.

arrowd updated this revision to Diff 83319.Jun 30 2020, 8:07 AM
arrowd marked an inline comment as done.
  • Remove extraneous debugging output.
arrowd added inline comments.Jun 30 2020, 8:08 AM
autotests/jobtest.cpp
459

There are no arrays of 512 size anymore. Or am I missing something?

arrowd updated this revision to Diff 83334.Jul 30 2020, 9:00 AM

Rebase on master.

bruns added inline comments.Aug 26 2020, 10:10 PM
src/ioslaves/file/file_unix.cpp
583 ↗(On Diff #83334)

Should be true

arrowd updated this revision to Diff 83361.Sep 6 2020, 6:28 PM
arrowd marked an inline comment as done.
  • Return "true" if the file doesn't have any xattrs.
bruns added inline comments.Sep 8 2020, 5:15 PM
autotests/jobtest.cpp
467

const QString

And if you just call this with the target directory (e.g. homeTmpDir()), you can skip the split/join dance.

780

This will set m_SkipXattr unconditionally even if the source FS does not support Xattrs.

arrowd updated this revision to Diff 83364.Sep 10 2020, 11:18 AM
arrowd marked 2 inline comments as done.
  • Make checkXattrFsSupport accept directory as a parameter.
  • Consitify QStrings.
usta added inline comments.Sep 10 2020, 1:43 PM
autotests/jobtest.cpp
477 ↗(On Diff #83334)

const ?

src/ioslaves/file/file_unix.cpp
1531 ↗(On Diff #83334)

isnt this ignoring acl_from_mode part ? I mean not sure but i think we need to check if it is nullptr or not before assigning it otherwise we will ignore the acl_from_mode part.

bruns added inline comments.Sep 10 2020, 5:03 PM
autotests/jobtest.cpp
780

And now you only check the source FS, but no longer the destination FS.

arrowd added inline comments.Sep 17 2020, 4:44 AM
autotests/jobtest.cpp
780

To be honest, I was confused by your first comment. What was wrong with the first version of this code?

My understanding was that we want to skip xattr stuff if either source or destination doesn't support it

arrowd updated this revision to Diff 83365.Sep 17 2020, 5:25 AM
arrowd marked an inline comment as done.
  • More const QString.
arrowd added inline comments.Sep 17 2020, 5:26 AM
src/ioslaves/file/file_unix.cpp
1531 ↗(On Diff #83334)

You seem to be right. I blamed the code down to svn->git import and this bug was present all the time.

To properly fix this I'd start with a test for KIO::chmod job that changes ACL permissions, but I can't test this on FreeBSD as we don't have acl_from_mode.

Anyways, it is out of scope of this review.

bruns requested changes to this revision.Sep 17 2020, 2:03 PM
bruns added inline comments.
autotests/jobtest.cpp
483 ↗(On Diff #83334)

should be dest.

501 ↗(On Diff #83334)

Should have a break or return here.

780

setXattr, called from checkXattrFsSupport, unconditionally sets m_SkipXattr.
You want something like

bool canRead =  checkXattrFsSupport(homeDir);
bool canWrite =  checkXattrFsSupport(otherHomeDir);
if (canRead && canWrite) {

and get rid of the m_SkipXAttr variable.

This revision now requires changes to proceed.Sep 17 2020, 2:03 PM
arrowd updated this revision to Diff 83366.Sep 21 2020, 7:12 AM
arrowd marked 5 inline comments as done.
  • Address comments.

Another bump.

bruns added a comment.Oct 26 2020, 6:56 PM

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

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

dfaure added inline comments.Oct 28 2020, 6:54 PM
src/ioslaves/file/ConfigureChecks.cmake
12 ↗(On Diff #83334)

This is already done by cmake/FindACL.cmake, why not add the other one (extattr.h) to that file as well?

The CMakeLists.txt in this directory calls find_package(ACL) so it'll be used.

Even though this isn't technically about ACLs, I'm guessing it all links *because* FindACL.cmake links to libattr, right?
And then this might also mean that the condition in FindACL.cmake needs to be updated, it currently *requires* HAVE_SYS_XATTR_H instead of allowing both variants.

But then I guess this means kpropertiesdialog.cpp needs to be ported to the sys/extattr.h API.

Has this been tested on a system with sys/extattr.h? I'm wondering if what will happen is: FindACL didn't find sys/attr.h so it doesn't link to libattr, and then the new code here fails to link. Or am I missing something?

src/ioslaves/file/file_unix.cpp
549 ↗(On Diff #83334)

This whole method should be in #if HAVE_SYS_XATTR_H || HAVE_SYS_EXTATTR_H.

Otherwise, on systems with neither defined, a lot of compiler warnings will happen, like "keyLen isn't initialized" on line 591.

You can leave the method in the .h without #if (those defines aren't known there).
Here you can either #if the method itself (declared and not defined and not used, is OK, although some might say, a bit unclean)
or #if the whole method *contents*, but then in the #else you need Q_UNUSED(src_fd); Q_UNUSED(dest_fd); to avoid compiler warnings about unused parameters.

arrowd updated this revision to Diff 83375.Oct 29 2020, 5:31 PM
arrowd marked an inline comment as done.
  • Put FileProtocol::copyXattrs inside a #if
arrowd added inline comments.Oct 29 2020, 5:32 PM
src/ioslaves/file/ConfigureChecks.cmake
12 ↗(On Diff #83334)

Has this been tested on a system with sys/extattr.h?

I was working on this revision on such system all the time. FreeBSD contains extattr bits in its libc, so no extra libraries are required.

So, what should be done here, then? Just move HAVE_SYS_EXTATTR_H check to FindACL.cmake module?

dfaure accepted this revision.Oct 29 2020, 8:29 PM
dfaure added inline comments.
src/ioslaves/file/ConfigureChecks.cmake
12 ↗(On Diff #83334)

Ah, I see, OK. No extra lib needed makes it simple.

The FindACL.cmake stuff is a bit of a mess now, with the need for extended attributes outside the ACL related code.
Maybe this could be split up into "find extended attribute stuff" and "find ACL stuff", the latter relying on the former.
But this merge request has been pending for long enough, let's do buildsystem refactoring as part of it.

Let's leave this part as is for now.

If you feel like it, I suggest followup commits to 1) enable the ACL stuff on systems with extattr, see the little bit of code in kpropertiesdialog.cpp, and 2) separate the cmake stuff for ACLs from the cmake stuff for extended attributes.

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!

arrowd added inline comments.Oct 30 2020, 5:12 AM
src/ioslaves/file/ConfigureChecks.cmake
12 ↗(On Diff #83334)
  1. enable the ACL stuff on systems with extattr, see the little bit of code in kpropertiesdialog.cpp

By that you mean that I should edit the CMake module to define HAVE_POSIX_ACL when extattr headers are found?

Or should I change checks in kpropertiesdialog.cpp from HAVE_POSIX_ACL to HAVE_*ATTR_H?

dfaure added inline comments.Oct 30 2020, 7:37 AM
src/ioslaves/file/ConfigureChecks.cmake
12 ↗(On Diff #83334)

I'm talking about the implementation of fileSystemSupportsACL in kpropertiesdialog.cpp, which uses getxattr.
But now that I take another look at it, I see that it's already implemented on FreeBSD, without the use of extattr, it uses statfs instead.
So I think I was wrong, this part is fine.

I guess the only thing that's a bit weird is that kio_file's attr code (unrelated to ACLs) relies on FindACL linking to the right lib (on Linux). It would be cleaner if the attr stuff was separate. But no big deal I guess.

arrowd added inline comments.Nov 17 2020, 7:08 AM
src/ioslaves/file/ConfigureChecks.cmake
12 ↗(On Diff #83334)

There are other parts of code that are guarded with HAVE_POSIX_ACLs, and these aren't enabled ATM for FreeBSD. So, the change is needed and I'm willing to implement it.

I plan to move set(HAVE_POSIX_ACL ...) part into FindACL.cmake and use this module everywhere. Does that sound OK?

dfaure added inline comments.Nov 18 2020, 11:49 AM
src/ioslaves/file/ConfigureChecks.cmake
12 ↗(On Diff #83334)

Yep.

kdudka added a subscriber: kdudka.Mar 2 2021, 7:49 AM

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.

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.

bruns added a comment.Mar 2 2021, 12:48 PM

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.)

kdudka added a comment.Mar 2 2021, 1:05 PM

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.

bruns added a comment.Mar 2 2021, 1:18 PM

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.

kdudka added a comment.Mar 2 2021, 2:19 PM

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.

bruns added a comment.Mar 2 2021, 3:06 PM

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?

kdudka added a comment.EditedMar 2 2021, 3:53 PM

@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 +++
bruns added a comment.Mar 2 2021, 5:21 PM

@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.

kdudka added a comment.Mar 2 2021, 6:40 PM

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. :)

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

arrowd abandoned this revision.Mar 31 2021, 4:45 AM

Mark the revision as closed to reflect reality.

Gotcha, thanks!