fix overwrite race with small file that got large

When adding a small file, it does not get locked down, so can be modified
after git-annex checks that it's small. The use of queued git add made the
race window nice and wide too.

Fixed by checking if the file has changed, and by not using git add.
Instead, have to recapitulate git add's handling of things like symlinks
and executable files.

Sponsored-by: Jochen Bartl on Patreon

diff --git a/CHANGELOG b/CHANGELOG
index 3e39bbc3a92d97bcfc97589e66a8818a16c9d8ab..02a9696d00fb4896b6715e3e2f66eea61b6f933c 100644 (file)
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -17,6 +17,9 @@ git-annex (10.20220526) UNRELEASED; urgency=medium
     replacing an annex symlink of a file that was already processed
     with a new large file could sometimes cause that large file to be
     added to git. These races have been fixed.
+  * add: Also fix a similar race that could cause a large file be added
+    to git when a small file was modified or overwritten while it was
+    being added.
   * add --batch: Fix handling of a file that is skipped due to being
     gitignored.
 

diff --git a/Command/Add.hs b/Command/Add.hs
index 31c560e4b09529084f3a25628db9fbd0f98b7c3c..671f16799ec5bffbc4dce8d4bbaf256efe8c339b 100644 (file)
--- a/Command/Add.hs
+++ b/Command/Add.hs
@@ -1,6 +1,6 @@
 {- git-annex command
  -
- - Copyright 2010-2021 Joey Hess <id@joeyh.name>
+ - Copyright 2010-2022 Joey Hess <id@joeyh.name>
  -
  - Licensed under the GNU AGPL version 3 or higher.
  -}
@@ -17,16 +17,20 @@ import qualified Database.Keys
 import Annex.FileMatcher
 import Annex.Link
 import Annex.Tmp
+import Annex.HashObject
 import Messages.Progress
 import Git.FilePath
+import Git.Types
+import Git.UpdateIndex
 import Config.GitConfig
-import Config.Smudge
 import Utility.OptParse
 import Utility.InodeCache
 import Annex.InodeSentinal
 import Annex.CheckIgnore
 import qualified Utility.RawFilePath as R
 
+import System.PosixCompat.Files
+
 cmd :: Command
 cmd = notBareRepo $ 
        withGlobalOptions opts $
@@ -80,20 +84,21 @@ seek o = startConcurrency commandStages $ do
        addunlockedmatcher <- addUnlockedMatcher
        annexdotfiles <- getGitConfigVal annexDotFiles 
        let gofile includingsmall (si, file) = case largeFilesOverride o of
-               Nothing -> 
+               Nothing -> do
+                       s <- liftIO $ R.getSymbolicLinkStatus file
                        ifM (pure (annexdotfiles || not (dotfile file))
                                <&&> (checkFileMatcher largematcher file 
                                <||> Annex.getState Annex.force))
-                               ( start o si file addunlockedmatcher
+                               ( start si file addunlockedmatcher
                                , if includingsmall
                                        then ifM (annexAddSmallFiles <$> Annex.getGitConfig)
-                                               ( startSmall o si file
+                                               ( startSmall si file s
                                                , stop
                                                )
                                        else stop
                                )
-               Just True -> start o si file addunlockedmatcher
-               Just False -> startSmallOverridden o si file
+               Just True -> start si file addunlockedmatcher
+               Just False -> startSmallOverridden si file
        case batchOption o of
                Batch fmt
                        | updateOnly o ->
@@ -121,64 +126,84 @@ seek o = startConcurrency commandStages $ do
                        go False withUnmodifiedUnlockedPointers
 
 {- Pass file off to git-add. -}
-startSmall :: AddOptions -> SeekInput -> RawFilePath -> CommandStart
-startSmall o si file =
+startSmall :: SeekInput -> RawFilePath -> FileStatus -> CommandStart
+startSmall si file s =
        starting "add" (ActionItemTreeFile file) si $
-               next $ addSmall (checkGitIgnoreOption o) file
+               next $ addSmall file s
 
-addSmall :: CheckGitIgnore -> RawFilePath -> Annex Bool
-addSmall ci file = do
+addSmall :: RawFilePath -> FileStatus -> Annex Bool
+addSmall file s = do
        showNote "non-large file; adding content to git repository"
-       addFile Small ci file
+       addFile Small file s
 
-startSmallOverridden :: AddOptions -> SeekInput -> RawFilePath -> CommandStart
-startSmallOverridden o si file = 
-       starting "add" (ActionItemTreeFile file) si $ next $ do
-               showNote "adding content to git repository"
-               addFile Small (checkGitIgnoreOption o) file
+startSmallOverridden :: SeekInput -> RawFilePath -> CommandStart
+startSmallOverridden si file = 
+       liftIO (catchMaybeIO $ R.getSymbolicLinkStatus file) >>= \case
+               Just s -> starting "add" (ActionItemTreeFile file) si $ next $ do
+                       
+                       showNote "adding content to git repository"
+                       addFile Small file s
+               Nothing -> stop
 
 data SmallOrLarge = Small | Large
 
-addFile :: SmallOrLarge -> CheckGitIgnore -> RawFilePath -> Annex Bool
-addFile smallorlarge ci file = do
-       ps <- gitAddParams ci
-       cps <- case smallorlarge of
-               -- In case the file is being converted from an annexed file
-               -- to be stored in git, remove the cached inode, so that
-               -- if the smudge clean filter later runs on the file,
-               -- it will not remember it was annexed.
-               --
-               -- The use of bypassSmudgeConfig prevents the smudge
-               -- filter from being run. So the changes to the database
-               -- can be queued up and not flushed to disk immediately.
-               Small -> do
-                       maybe noop Database.Keys.removeInodeCache
-                               =<< withTSDelta (liftIO . genInodeCache file)
-                       return bypassSmudgeConfig
-               Large -> return []
-       Annex.Queue.addCommand cps "add" (ps++[Param "--"])
-               [fromRawFilePath file]
-       return True
-
-start :: AddOptions -> SeekInput -> RawFilePath -> AddUnlockedMatcher -> CommandStart
-start o si file addunlockedmatcher = do
-       mk <- liftIO $ isPointerFile file
-       maybe go fixuppointer mk
+addFile :: SmallOrLarge -> RawFilePath -> FileStatus -> Annex Bool
+addFile smallorlarge file s = do
+       sha <- if isSymbolicLink s
+               then hashBlob =<< liftIO (R.readSymbolicLink file)
+               else if isRegularFile s
+                       then hashFile file
+                       else giveup $ fromRawFilePath file ++ " is not a regular file"
+       let treetype = if isSymbolicLink s
+               then TreeSymlink
+               else if intersectFileModes ownerExecuteMode (fileMode s) /= 0
+                       then TreeExecutable
+                       else TreeFile
+       s' <- liftIO $ catchMaybeIO $ R.getSymbolicLinkStatus file
+       if maybe True (changed s) s'
+               then do
+                       warning $ fromRawFilePath file ++ " changed while it was being added"
+                       return False
+               else do
+                       case smallorlarge of
+                               -- In case the file is being converted from 
+                               -- an annexed file to be stored in git,
+                               -- remove the cached inode, so that if the
+                               -- smudge clean filter later runs on the file,
+                               -- it will not remember it was annexed.
+                               Small -> maybe noop Database.Keys.removeInodeCache
+                                       =<< withTSDelta (liftIO . genInodeCache file)
+                               Large -> noop
+                       Annex.Queue.addUpdateIndex =<<
+                               inRepo (stageFile sha treetype (fromRawFilePath file))
+                       return True
   where
-       go = ifAnnexed file addpresent add
-       add = liftIO (catchMaybeIO $ R.getSymbolicLinkStatus file) >>= \case
+       changed a b =
+               deviceID a /= deviceID b ||
+               fileID a /= fileID b ||
+               fileSize a /= fileSize b ||
+               modificationTime a /= modificationTime b ||
+               isRegularFile a /= isRegularFile b ||
+               isSymbolicLink a /= isSymbolicLink b
+
+start :: SeekInput -> RawFilePath -> AddUnlockedMatcher -> CommandStart
+start si file addunlockedmatcher = 
+       liftIO (catchMaybeIO $ R.getSymbolicLinkStatus file) >>= \case
                Nothing -> stop
-               Just s 
+               Just s
                        | not (isRegularFile s) && not (isSymbolicLink s) -> stop
-                       | otherwise -> 
-                               starting "add" (ActionItemTreeFile file) si $
-                                       if isSymbolicLink s
-                                               then next $ addFile Small (checkGitIgnoreOption o) file
-                                               else perform file addunlockedmatcher
-       addpresent key = 
-               liftIO (catchMaybeIO $ R.getSymbolicLinkStatus file) >>= \case
-                       Just s | isSymbolicLink s -> fixuplink key
-                       _ -> add
+                       | otherwise -> do
+                               mk <- liftIO $ isPointerFile file
+                               maybe (go s) (fixuppointer s) mk
+  where
+       go s = ifAnnexed file (addpresent s) (add s)
+       add s = starting "add" (ActionItemTreeFile file) si $
+               if isSymbolicLink s
+                       then next $ addFile Small file s
+                       else perform file addunlockedmatcher
+       addpresent s key
+               | isSymbolicLink s = fixuplink key
+               | otherwise = add s
        fixuplink key = 
                starting "add" (ActionItemTreeFile file) si $
                        addingExistingLink file key $
@@ -194,11 +219,11 @@ start o si file addunlockedmatcher = do
                                                        liftIO $ moveFile tmpf (fromRawFilePath file)
                                                        next $ return True
                                                )
-       fixuppointer key =
+       fixuppointer s key =
                starting "add" (ActionItemTreeFile file) si $
                        addingExistingLink file key $ do
                                Database.Keys.addAssociatedFile key =<< inRepo (toTopFilePath file)
-                               next $ addFile Large (checkGitIgnoreOption o) file
+                               next $ addFile Large file s
 
 perform :: RawFilePath -> AddUnlockedMatcher -> CommandPerform
 perform file addunlockedmatcher = withOtherTmp $ \tmpdir -> do

diff --git a/Command/AddUrl.hs b/Command/AddUrl.hs
index ae1680a19d71ae60ac3cd1281d53d9e31fdccb5b..1de729d4c5141a5123c865fe89ad12f662d407f9 100644 (file)
--- a/Command/AddUrl.hs
+++ b/Command/AddUrl.hs
@@ -453,6 +453,7 @@ addWorkTree :: CanAddFile -> AddUnlockedMatcher -> UUID -> URLString -> RawFileP
 addWorkTree _ addunlockedmatcher u url file key mtmp = case mtmp of
        Nothing -> go
        Just tmp -> do
+               s <- liftIO $ R.getSymbolicLinkStatus tmp
                -- Move to final location for large file check.
                pruneTmpWorkDirBefore tmp $ \_ -> do
                        createWorkTreeDirectory (P.takeDirectory file)
@@ -470,7 +471,7 @@ addWorkTree _ addunlockedmatcher u url file key mtmp = case mtmp of
                                        (fromRawFilePath file)
                                        (fromRawFilePath tmp)
                                go
-                       else void $ Command.Add.addSmall noci file
+                       else void $ Command.Add.addSmall file s
   where
        go = do
                maybeShowJSON $ JSONChunk [("key", serializeKey key)]
@@ -483,10 +484,6 @@ addWorkTree _ addunlockedmatcher u url file key mtmp = case mtmp of
                        , maybe noop (\tmp -> pruneTmpWorkDirBefore tmp (liftIO . removeWhenExistsWith R.removeLink)) mtmp
                        )
 
-       -- git does not need to check ignores, because that has already
-       -- been done, as witnessed by the CannAddFile.
-       noci = CheckGitIgnore False
-
 nodownloadWeb :: AddUnlockedMatcher -> DownloadOptions -> URLString -> Url.UrlInfo -> RawFilePath -> Annex (Maybe Key)
 nodownloadWeb addunlockedmatcher o url urlinfo file
        | Url.urlExists urlinfo = if rawOption o

diff --git a/Command/Import.hs b/Command/Import.hs
index 716fbcf63aa5dfb2319cededfe994e034e1ae74b..21d847ea50b1f3744920273004381aa082d00ff4 100644 (file)
--- a/Command/Import.hs
+++ b/Command/Import.hs
@@ -229,7 +229,8 @@ startLocal o addunlockedmatcher largematcher mode (srcfile, destfile) =
                -- weakly the same as the originally locked down file's
                -- inode cache. (Since the file may have been copied,
                -- its inodes may not be the same.)
-               newcache <- withTSDelta $ liftIO . genInodeCache destfile
+               s <- liftIO $ R.getSymbolicLinkStatus destfile
+               newcache <- withTSDelta $ \d -> liftIO $ toInodeCache d destfile s
                let unchanged = case (newcache, inodeCache (keySource ld)) of
                        (_, Nothing) -> True
                        (Just newc, Just c) | compareWeak c newc -> True
@@ -250,7 +251,7 @@ startLocal o addunlockedmatcher largematcher mode (srcfile, destfile) =
                                >>= maybe
                                        stop
                                        (\addedk -> next $ Command.Add.cleanup addedk True)
-                       , next $ Command.Add.addSmall (checkGitIgnoreOption o) destfile
+                       , next $ Command.Add.addSmall destfile s
                        )
        notoverwriting why = do
                warning $ "not overwriting existing " ++ fromRawFilePath destfile ++ " " ++ why

diff --git a/doc/bugs/add_overwrite_race.mdwn b/doc/bugs/add_overwrite_race.mdwn
index 026f1e7c3d50825a17097ba94cf57b8afe2fbc4c..65f695d69fb0ebe58103dcc9accbe0c43d310a74 100644 (file)
--- a/doc/bugs/add_overwrite_race.mdwn
+++ b/doc/bugs/add_overwrite_race.mdwn
@@ -50,8 +50,10 @@ added to the annex after all. Test case for this:
        git-annex add
        git diff --cached 1
 
-Unsure how to fix this case yet? Maybe it needs to cache the inode,
+Guess it needs to cache the inode,
 hash the file content, then verifiy the inode did not change during
 hashing, and then also use update-index.
 
+> [[done]]
+
 --[[Joey]]