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 17b20a2450
was intending to fix, with a much smaller hammer.

Sponsored-by: Dartmouth College's Datalad project
This commit is contained in:
Joey Hess 2022-05-02 14:06:20 -04:00
parent 1b02cd4715
commit 642703c7e4
No known key found for this signature in database
GPG key ID: DB12DB0FF05F8F38
4 changed files with 40 additions and 8 deletions

View file

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

View file

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

View file

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

View file

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