Run cp -a with --no-preserve=xattr, to avoid problems with copied xattrs

Including them breaking permissions setting on some NFS servers.

Sponsored-by: Dartmouth College's Datalad project

diff --git a/Build/Configure.hs b/Build/Configure.hs
index 5fd172d3dba8fdaecd261662a7b07d7f181223d0..83ef161ced25d41020f3d3a227aefc535f7e55cc 100644 (file)
--- a/Build/Configure.hs
+++ b/Build/Configure.hs
@@ -25,6 +25,7 @@ tests =
        , testCp "cp_p" "-p"
        , testCp "cp_preserve_timestamps" "--preserve=timestamps"
        , testCpReflinkAuto
+       , testCpNoPreserveXattr
        , TestCase "xargs -0" $ testCmd "xargs_0" "xargs -0 </dev/null"
        , TestCase "rsync" $ testCmd "rsync" "rsync --version >/dev/null"
        , TestCase "curl" $ testCmd "curl" "curl --version >/dev/null"
@@ -62,6 +63,11 @@ testCpReflinkAuto = testCp k "--reflink=auto"
   where
        k = "cp_reflink_supported"
 
+testCpNoPreserveXattr :: TestCase
+testCpNoPreserveXattr = testCp 
+       "cp_no_preserve_xattr_supported" 
+       "--no-preserve=xattr"
+
 getUpgradeLocation :: Test
 getUpgradeLocation = do
        e <- getEnv "UPGRADE_LOCATION"

diff --git a/CHANGELOG b/CHANGELOG
index 66d577fc53a8b75b4e385d1c19b8e5ccad8c073d..2c6cdce457758ce843958877463df0cf6f3820c1 100644 (file)
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -19,6 +19,9 @@ git-annex (8.20210804) UNRELEASED; urgency=medium
     get, drop, move, copy, whereis
   * Added annex.youtube-dl-command config. This can be used to run some
     forks of youtube-dl.
+  * Run cp -a with --no-preserve=xattr, to avoid problems with copied
+    xattrs, including them breaking permissions setting on some NFS
+    servers.
 
  -- Joey Hess <id@joeyh.name>  Tue, 03 Aug 2021 12:22:45 -0400
 

diff --git a/Utility/CopyFile.hs b/Utility/CopyFile.hs
index f851326b90b6f9c0d35705afa4c7484bf1182dd4..ed2da15e5ce45d6bdd771422399013f0d58948f6 100644 (file)
--- a/Utility/CopyFile.hs
+++ b/Utility/CopyFile.hs
@@ -1,6 +1,6 @@
 {- file copying
  -
- - Copyright 2010-2019 Joey Hess <id@joeyh.name>
+ - Copyright 2010-2021 Joey Hess <id@joeyh.name>
  -
  - License: BSD-2-clause
  -}
@@ -30,6 +30,12 @@ copyMetaDataParams meta = map snd $ filter fst
                , Param "-p")
        , (not allmeta && BuildInfo.cp_preserve_timestamps
                , Param "--preserve=timestamps")
+       -- cp -a may preserve xattrs that have special meaning,
+       -- eg to NFS, and have even been observed to prevent later
+       -- changing the permissions of the file. So prevent preserving
+       -- xattrs.
+       , (allmeta && BuildInfo.cp_a && BuildInfo.cp_no_preserve_xattr_supported
+               , Param "--no-preserve=xattr")
        ]
   where
        allmeta = meta == CopyAllMetaData

diff --git a/doc/bugs/__34__357_out_of_984_tests_failed__34___on_NFS_lustre_mount.mdwn b/doc/bugs/__34__357_out_of_984_tests_failed__34___on_NFS_lustre_mount.mdwn
index 0d6ff8e6ba5d6c86e1c6ec5c147bbebbe39a8693..c5c982a2a83b737cccfc717735f19a85d26bac41 100644 (file)
--- a/doc/bugs/__34__357_out_of_984_tests_failed__34___on_NFS_lustre_mount.mdwn
+++ b/doc/bugs/__34__357_out_of_984_tests_failed__34___on_NFS_lustre_mount.mdwn
@@ -134,3 +134,5 @@ details of the mount with minimal sensoring with XXXs:
 any ideas Joey?  or how to troubleshoot on this beast further?
 
 [[!meta title="357 out of 984 tests fail on NFS isilon mount"]]
+
+> [[fixed|done]] --[[Joey]]

diff --git a/doc/bugs/__34__357_out_of_984_tests_failed__34___on_NFS_lustre_mount/comment_11_541e257ea04d6a7647a337998d79232c._comment b/doc/bugs/__34__357_out_of_984_tests_failed__34___on_NFS_lustre_mount/comment_11_541e257ea04d6a7647a337998d79232c._comment
new file mode 100644 (file)
index 0000000..9cf2c56
--- /dev/null
+++ b/doc/bugs/__34__357_out_of_984_tests_failed__34___on_NFS_lustre_mount/comment_11_541e257ea04d6a7647a337998d79232c._comment
@@ -0,0 +1,44 @@
+[[!comment format=mdwn
+ username="joey"
+ subject="""comment 11"""
+ date="2021-08-27T16:41:45Z"
+ content="""
+I've made the change to use that option. I think this should clear the test
+suite.
+
+I see that Isilon is a NAS, so some proprietary and broken NFS server
+apparently. Kind of explains why cp -a would do this, because I found
+threads about cp -a preserving NFS xattrs, and people wanted it to,
+presumably for other reasons on less broken NFS servers.
+
+This does raise the question of what might happen if someone copies
+a file themselves with cp -a on this NFS and then git-annex adds the copy.
+Probably git-annex would then be unable to remove the write bit
+from the annex object file. 
+
+For that matter, the same could happen with ACLs. Eg, I was able to
+use setfacl to make this happen:
+
+       joey@darkstar:~>chmod -w foo
+       joey@darkstar:~>setfacl -m g:nogroup:rw foo
+       joey@darkstar:~>ls -l foo
+       -r--rw-r--+ 1 joey joey 0 Aug 27 12:53 foo
+       joey@darkstar:~>chmod -w foo
+       chmod: foo: new permissions are r--rw-r--, not r--r--r--
+       - exit 1
+       joey@darkstar:~>perl -e 'chmod(400)' foo
+       joey@darkstar:~>ls -l foo
+       -r--rw-r--+ 1 joey joey 0 Aug 27 12:53 foo
+
+So git-annex would be unable to clear the write bit, and would not be able
+to effectively lock down the file for all users, eg user nobody can write
+to the file in the above example. There's probably a way to let user joey
+also write to it, but my attempt to do that with an ACL failed.
+
+Perhaps git-annex should clear ACLs when ingesting (and locking) files.
+But perhaps users use ACLs for other purposes that would not prevent
+lockdown, and so they should not be cleared. And as far as internal-use NFS
+xattrs, it doesn't seem wise for git-annex to try to unset them from files
+its ingesting. So I guess I'm going to punt on this broader question,
+if users want to use the ACL rope, it's over there..
+"""]]