avoid using removePathForcibly everywhere, it is unsafe

If the temp directory can somehow contain a hard link, it changes the
mode, which affects all other hard linked files. So, it's too unsafe
to use everywhere in git-annex, since hard links are possible in
multiple ways and it would be very hard to prove that every place that
uses a temp directory cannot possibly put a hard link in it.

Added a call to removeDirectoryForCleanup to test_crypto, which will
fix the problem that commit 17b20a24502aee3bfc5683146c3899a233295aea
was intending to fix, with a much smaller hammer.

Sponsored-by: Dartmouth College's Datalad project

diff --git a/Test.hs b/Test.hs
index d10ef8b4cffd397dac4899b7401c98347c8be9cc..a8d01bf6df593bcfeba4703215a3be74de2c9fcc 100644 (file)
--- a/Test.hs
+++ b/Test.hs
@@ -1794,7 +1794,12 @@ test_crypto = do
                -- it needs to be able to store the agent socket there,
                -- which can be problimatic when testing some filesystems.
                absgpgtmp <- fromRawFilePath <$> absPath (toRawFilePath gpgtmp)
-               testscheme' scheme absgpgtmp
+               res <- testscheme' scheme absgpgtmp
+               -- gpg may still be running and would prevent
+               -- removeDirectoryRecursive from succeeding, so
+               -- force removal of the temp directory.
+               liftIO $ removeDirectoryForCleanup gpgtmp
+               return res
        testscheme' scheme absgpgtmp = intmpclonerepo $ do
                -- Since gpg uses a unix socket, which is limited to a
                -- short path, use whichever is shorter of absolute

diff --git a/Utility/Tmp/Dir.hs b/Utility/Tmp/Dir.hs
index 4deda1297ecf858d84951e3b98ea3d878a11bfb0..904b65a52676c476abe9493d28635f4c2d2b4784 100644 (file)
--- a/Utility/Tmp/Dir.hs
+++ b/Utility/Tmp/Dir.hs
@@ -69,11 +69,4 @@ removeTmpDir tmpdir = liftIO $ whenM (doesDirectoryExist tmpdir) $ do
        go tmpdir
 #endif
   where
-       -- Use removePathForcibly when available, to avoid crashing
-       -- if some other process is removing files in the directory at the
-       -- same time.
-#if MIN_VERSION_directory(1,2,7)
-       go = removePathForcibly
-#else
        go = removeDirectoryRecursive
-#endif

diff --git a/doc/bugs/Tests_v8_locked__58___rsync_remote__58___FAIL.mdwn b/doc/bugs/Tests_v8_locked__58___rsync_remote__58___FAIL.mdwn
index 70ba96f0e1ddd9cdcf891cf9021b5f8fcc9d4ac4..d7137d7cce64c8c5c7310e9cad7db3b5cbc142a7 100644 (file)
--- a/doc/bugs/Tests_v8_locked__58___rsync_remote__58___FAIL.mdwn
+++ b/doc/bugs/Tests_v8_locked__58___rsync_remote__58___FAIL.mdwn
@@ -39,3 +39,5 @@ cron-20220420/build-ubuntu.yaml-667-0346631d-failed/2_test-annex (crippled-tmp,
 cron-20220420/build-ubuntu.yaml-667-0346631d-failed/4_test-annex (nfs-home, ubuntu-latest).txt:1511:2022-04-20T03:34:57.9524586Z     rsync remote:                                         FAIL (1.57s)
 ...
 ```
+
+> [[fixed|done]] --[[Joey]]

diff --git a/doc/bugs/Tests_v8_locked__58___rsync_remote__58___FAIL/comment_2_0327663314d2a8b2f0cab7536fdaa6bd._comment b/doc/bugs/Tests_v8_locked__58___rsync_remote__58___FAIL/comment_2_0327663314d2a8b2f0cab7536fdaa6bd._comment
new file mode 100644 (file)
index 0000000..c6aa04f
--- /dev/null
+++ b/doc/bugs/Tests_v8_locked__58___rsync_remote__58___FAIL/comment_2_0327663314d2a8b2f0cab7536fdaa6bd._comment
@@ -0,0 +1,32 @@
+[[!comment format=mdwn
+ username="joey"
+ subject="""comment 2"""
+ date="2022-05-02T17:08:15Z"
+ content="""
+Here is the bug in action:
+
+       -r--r--r-- 1 joey joey 30 May  2 12:42 .git/annex/objects/Gj/8J/SHA256E-s30--3d65cafd9435fde3867a527d75ff8aea05a3632cb60574d45e0fc277f06c8a64/SHA256E-s30--3d65cafd9435fde3867a527d75ff8aea05a3632cb60574d45e0fc277f06c8a64
+       joey@darkstar:/tmp/x>git-annex copy --to foo
+       copy x (to foo...) 
+       ok
+       joey@darkstar:/tmp/x>ls -l .git/annex/objects/Gj/8J/SHA256E-s30--3d65cafd9435fde3867a527d75ff8aea05a3632cb60574d45e0fc277f06c8a64/SHA256E-s30--3d65cafd9435fde3867a527d75ff8aea05a3632cb60574d45e0fc277f06c8a64
+       -rwxr--r-- 1 joey joey 30 May  2 12:42 .git/annex/objects/Gj/8J/SHA256E-s30--3d65cafd9435fde3867a527d75ff8aea05a3632cb60574d45e0fc277f06c8a64/SHA256E-s30--3d65cafd9435fde3867a527d75ff8aea05a3632cb60574d45e0fc277f06c8a64*
+
+At first I thought this was rsync modifying the permissions of the source file.
+
+But no... [[!commit 17b20a24502aee3bfc5683146c3899a233295aea]]
+changed how temp directories get cleaned up. removePathForcibly
+is actually changing the permissions of the object file hard link
+in the rsynctmp directory when deleting that directory. Which also 
+changes the permissions of the object file.
+
+Filed a bug on removePathForcibly. <https://github.com/haskell/directory/issues/135>
+
+I think this makes removePathForcibly unsuitable for general purpose
+use in git-annex, because there are just too many ways for a hard link
+to enter the picture. (Eg annex.thin, or even a user making their own
+hard link that git-annex does not know about.) 
+
+So, I've reverted that commit, and put in a more
+targeted fix for the problem it was addressing.
+"""]]