{"thread":{"id":"36373","subject":"Race condition with git-status and git-rebase","startedAt":"2014-04-08T18:44:21Z","lastAt":"2014-04-08T22:56:19Z","messageCount":3,"participants":["Yiannis Marangos","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"238553","messageId":"20140408184421.GD5208@abyss.hitronhub.home","threadId":"36373","inReplyTo":null,"subject":"Race condition with git-status and git-rebase","fromName":"Yiannis Marangos","fromEmail":"yiannis.marangos@gmail.com","sentAt":"2014-04-08T18:44:21Z","receivedAt":"2014-04-08T18:44:21Z","isPatch":false,"sender":{"key":"yiannis.marangos@gmail.com","avatar":"https://avatars.githubusercontent.com/u/194040?v=4"},"body":"Hi,\n\nSince I don't know how to fix it, this is my bug report:\n\ngit-status reads the index file then holds the `index.lock', it writes\nthe updated index in `index.lock' and renames the `index.lock' to\n`index', but if it used with git-rebase then there is a (rare) race\ncondition.\n\nI reproduce this at my work under the following conditions:\n1) The repository is very big\n2) I can reproduce it only in Windows. I can not reproduce it in Linux\n   (my Linux machine has faster CPU and SSD drive.)\n\nI saw this race condition at my coworkers when they use two instances\n(in the same repository) of Git-Extensions GUI, and when they use one\ninstance of Git-Extensions and at the same time they rebase from\nterminal. This race condition happens because Git-Extensions monitors\nthe directory of the repository and when a file is accessed it calls\ngit-status.\n\nSince I can not share the repository, I decided to trace down the\nproblem. I came up with the following:\n\nprocess A calls git-rebase\nprocess A applies the 1st commit\nprocess B calls git-status\nprocess B calls read_cache() (status_init_config() -> gitmodules_config() -> read_cache())\nprocess A applies the 2nd commit\nprocess B holds the index.lock\nprocess B writes back the old index (the one that was read from read_cache())\nprocess A applies the 3rd commit (now the 3rd commit contains also a revert of the 2nd)\n\nThe content of the files of the working directory is correct (i.e. as\nit should be if the race condition didn't happen). But git-status\nshows that the files of the 2nd commit are modified, the git-diff show\nthe exact patch of the 2nd commit.\n\nSo, in this case process B must avoid calling write_index(). Maybe\nsomething is missing from update_index_if_able()? Maybe git should\ncheck the state before proceeding to write_index() (for example that\nwe are in rebase)?\n\nSomething else that I noticed is that when the race condition happens\nthe istate->cache_changed is 0 and has_racy_timestamp(istate) is 1, so\nthe if statement in update_index_if_able() continues to write_index().\n\nI didn't check if there are other similar race conditions.\n\nRegards,\nYiannis.\n\nPS: I tried to move hold_locked_index() above status_init_config() but\nthis cause other problems.\n"},{"id":"238557","messageId":"20140408191914.GA7630@abyss.hitronhub.home","threadId":"36373","inReplyTo":"20140408184421.GD5208@abyss.hitronhub.home","subject":"Re: Race condition with git-status and git-rebase","fromName":"Yiannis Marangos","fromEmail":"yiannis.marangos@gmail.com","sentAt":"2014-04-08T19:20:12Z","receivedAt":"2014-04-08T19:20:12Z","isPatch":false,"sender":{"key":"yiannis.marangos@gmail.com","avatar":"https://avatars.githubusercontent.com/u/194040?v=4"},"body":"Maybe you will find it useful: this is one way that I use to manually\ndebug the race condition (and it works in Linux):\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex d9550c5..c0511f6 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -1283,6 +1283,8 @@ int cmd_status(int argc, const char **argv, const char *prefix)\n \t\tusage_with_options(builtin_status_usage, builtin_status_options);\n \n \tstatus_init_config(&s, git_status_config);\n+\tprintf(\"read_cache() finished, press enter to continue.\\n\");\n+\tgetchar();\n \targc = parse_options(argc, argv, prefix,\n \t\t\t     builtin_status_options,\n \t\t\t     builtin_status_usage, 0);\ndiff --git a/git-rebase--am.sh b/git-rebase--am.sh\nindex df46f4c..6ffd986 100644\n--- a/git-rebase--am.sh\n+++ b/git-rebase--am.sh\n@@ -61,8 +61,8 @@ else\n \t\treturn $?\n \tfi\n \n-\tgit am $git_am_opt --rebasing --resolvemsg=\"$resolvemsg\" \\\n-\t\t${gpg_sign_opt:+\"$gpg_sign_opt\"} <\"$GIT_DIR/rebased-patches\"\n+\tgit am $git_am_opt -i --rebasing --resolvemsg=\"$resolvemsg\" \\\n+\t\t${gpg_sign_opt:+\"$gpg_sign_opt\"} \"$GIT_DIR/rebased-patches\"\n \tret=$?\n \n \trm -f \"$GIT_DIR/rebased-patches\"\ndiff --git a/read-cache.c b/read-cache.c\nindex ba13353..a6f73a7 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -1778,6 +1778,8 @@ static int has_racy_timestamp(struct index_state *istate)\n  */\n void update_index_if_able(struct index_state *istate, struct lock_file *lockfile)\n {\n+\tprintf(\"cache_changed: %d  has_racy_timestamp: %d\\n\",\n+\t       istate->cache_changed, has_racy_timestamp(istate));\n \tif ((istate->cache_changed || has_racy_timestamp(istate)) &&\n \t    !write_index(istate, lockfile->fd))\n \t\tcommit_locked_index(lockfile);\n"},{"id":"238572","messageId":"xmqq1tx72zxo.fsf@gitster.dls.corp.google.com","threadId":"36373","inReplyTo":"20140408184421.GD5208@abyss.hitronhub.home","subject":"Re: Race condition with git-status and git-rebase","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-04-08T22:56:19Z","receivedAt":"2014-04-08T22:56:19Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Yiannis Marangos <yiannis.marangos@gmail.com> writes:\n\n> process A calls git-rebase\n> process A applies the 1st commit\n> process B calls git-status\n> process B calls read_cache()\n> process A applies the 2nd commit\n> process B holds the index.lock\n> process B writes back the old index (the one that was read from read_cache())\n> process A applies the 3rd commit (now the 3rd commit contains also a revert of the 2nd)\n\nThe above sequence shows we are clearly doing it wrong.  B's taking\nof the lock is not protecting anything from anybody.\n\nWe try minimizing the lock contention by delaying the taking of the\nlock by B, but even if we leave it at the current location in the\nabove sequence, at least B must verify that the contents of the\nindex it just took the lock on still matches what it read with its\nearlier call to read_cache(), which would make it pretend as if it\ntook the lock before it did read_cache(), before writing the\nrefreshed results back.\n\nI think we have a checksum for the entire index file at the end, so\none fix might be to teach read_index_from() to read that and store\nit in a new field in \"struct index_state\", and then make the\n\"opportunistic update\" codepath to verify that the index on the\nfilesystem still has the same checksum.\n"}]}