{"thread":{"id":"35216","subject":"[PATCH v2] sha1_file.c:create_tmpfile(): Fix race when creating loose object dirs","startedAt":"2013-10-27T11:35:43Z","lastAt":"2013-10-30T09:30:17Z","messageCount":3,"participants":["Johan Herland","Jeff King"],"isPatch":true,"patchVersion":2,"patchTotal":null},"messages":[{"id":"229611","messageId":"1382873743-4648-1-git-send-email-johan@herland.net","threadId":"35216","inReplyTo":null,"subject":"[PATCH v2] sha1_file.c:create_tmpfile(): Fix race when creating loose object dirs","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2013-10-27T11:35:43Z","receivedAt":"2013-10-27T11:35:43Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"There are cases (e.g. when running concurrent fetches in a repo) where\nmultiple Git processes concurrently attempt to create loose objects\nwithin the same objects/XX/ dir. The creation of the loose object files\nis (AFAICS) safe from races, but the creation of the objects/XX/ dir in\nwhich the loose objects reside is unsafe, for example:\n\nTwo concurrent fetches - A and B. As part of its fetch, A needs to store\n12aaaaa as a loose object. B, on the other hand, needs to store 12bbbbb\nas a loose object. The objects/12 directory does not already exist.\nConcurrently, both A and B determine that they need to create the\nobjects/12 directory (because their first call to git_mkstemp_mode()\nwithin create_tmpfile() fails witn ENOENT). One of them - let's say A -\nexecutes the following mkdir() call before the other. This first call\nreturns success, and A moves on. When B gets around to calling mkdir(),\nit fails with EEXIST, because A won the race. The mkdir() error causes B\nto return -1 from create_tmpfile(), which propagates all the way,\nresulting in the fetch failing with:\n\n  error: unable to create temporary file: File exists\n  fatal: failed to write object\n  fatal: unpack-objects failed\n\nAlthough it's hard to add a testcase reproducing this issue, it's easy\nto provoke if we insert a sleep after the\n\n  if (mkdir(buffer, 0777) || adjust_shared_perm(buffer))\n      return -1;\n\nblock, and then run two concurrent \"git fetch\"es against the same repo.\n\nThe fix is to simply handle mkdir() failing with EEXIST as a success.\nIf EEXIST is somehow returned for the wrong reasons (because the relevant\nobjects/XX is not a directory, or is otherwise unsuitable for object\nstorage), the following call to adjust_shared_perm(), or ultimately the\nretried call to git_mkstemp_mode() will fail, and we end up returning\nerror from create_tmpfile() in any case.\n\nNote that there are still cases where two users with unsuitable umasks\nin a shared repo can end up in two races where one user first wins the\nmkdir() race to create an objects/XX/ directory, and then the other user\nwins the adjust_shared_perms() race to chmod() that directory, but fails\nbecause it is (transiently, until the first users completes its chmod())\nunwriteable to the other user. However, (an equivalent of) this race also\nexists before this patch, and is made no worse by this patch.\n\nSigned-off-by: Johan Herland <johan@herland.net>\n---\n\nI didn't see this in the latest \"What's cooking\", so here's a resend, with\nan expanded commit message to reflect our discussion. The patch itself is\nunchanged.\n\nIn order to fix the remaining race, I assume we have to ensure the dir\ncreation obeys the same rules as the object creation, i.e. that there are\nonly two possible states at any time:\n\n - The directory does not exist\n\n - The directory exists with the correct permissons\n\nTo achieve this, I guess we have to follow the same procedure we do for\nloose object creation:\n\n 1. Create a temporary directory with a unique name (mkdtemp?)\n\n 2. Adjust permissions\n\n 3. Rename into place\n\nCan this be done sufficiently atomically across all platforms?\n\n...Johan\n\n\n sha1_file.c | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex f80bbe4..00ffffe 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -2857,7 +2857,9 @@ static int create_tmpfile(char *buffer, size_t bufsiz, const char *filename)\n \t\t/* Make sure the directory exists */\n \t\tmemcpy(buffer, filename, dirlen);\n \t\tbuffer[dirlen-1] = 0;\n-\t\tif (mkdir(buffer, 0777) || adjust_shared_perm(buffer))\n+\t\tif (mkdir(buffer, 0777) && errno != EEXIST)\n+\t\t\treturn -1;\n+\t\tif (adjust_shared_perm(buffer))\n \t\t\treturn -1;\n \n \t\t/* Try again */\n-- \n1.8.4.653.g2df02b3\n"},{"id":"229801","messageId":"20131030091927.GQ11317@sigill.intra.peff.net","threadId":"35216","inReplyTo":"1382873743-4648-1-git-send-email-johan@herland.net","subject":"Re: [PATCH v2] sha1_file.c:create_tmpfile(): Fix race when creating loose object dirs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-10-30T09:19:27Z","receivedAt":"2013-10-30T09:19:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Oct 27, 2013 at 12:35:43PM +0100, Johan Herland wrote:\n\n> I didn't see this in the latest \"What's cooking\", so here's a resend, with\n> an expanded commit message to reflect our discussion. The patch itself is\n> unchanged.\n\nThanks, your expanded description looks correct to me.\n\n> In order to fix the remaining race, I assume we have to ensure the dir\n> creation obeys the same rules as the object creation, i.e. that there are\n> only two possible states at any time:\n> \n>  - The directory does not exist\n> \n>  - The directory exists with the correct permissons\n> \n> To achieve this, I guess we have to follow the same procedure we do for\n> loose object creation:\n> \n>  1. Create a temporary directory with a unique name (mkdtemp?)\n> \n>  2. Adjust permissions\n> \n>  3. Rename into place\n> \n> Can this be done sufficiently atomically across all platforms?\n\nYeah, I think that is the only way to do it. I do not know offhand of\nany platforms that have problems with atomic directory renames, though I\nwould not be surprised if some network filesystems don't handle it well.\n\nI'd also be fine if you want to simply leave it at your patch for now\nand let somebody who cares more about the other race worry about it\nlater.\n\n-Peff\n"},{"id":"229802","messageId":"20131030093017.GA12125@sigill.intra.peff.net","threadId":"35216","inReplyTo":"20131030091927.GQ11317@sigill.intra.peff.net","subject":"Re: [PATCH v2] sha1_file.c:create_tmpfile(): Fix race when creating loose object dirs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-10-30T09:30:17Z","receivedAt":"2013-10-30T09:30:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Oct 30, 2013 at 05:19:27AM -0400, Jeff King wrote:\n\n> > To achieve this, I guess we have to follow the same procedure we do for\n> > loose object creation:\n> > \n> >  1. Create a temporary directory with a unique name (mkdtemp?)\n> > \n> >  2. Adjust permissions\n> > \n> >  3. Rename into place\n> > \n> > Can this be done sufficiently atomically across all platforms?\n> \n> Yeah, I think that is the only way to do it. I do not know offhand of\n> any platforms that have problems with atomic directory renames, though I\n> would not be surprised if some network filesystems don't handle it well.\n\nActually, thinking on this more, we do not want \"rename into place\"\nsemantics, as that usually implies overwrite. We would prefer a\nhard-link solution, where only one linker \"wins\", and the other gets\nEEXIST. But you cannot hard-link directories.\n\nPOSIX does specify that rename() should return EEXIST rather than\noverwriting if the destination directory exists and is not empty. So\nin theory that would work, as we would then just be racing against the\nother process creating the actual object (and either we replace the\ndirectory with ours before they write the object, or they write the\nobject first and we get EEXIST).\n\nBut I don't know how well that is followed in the real world.\n\n-Peff\n"}]}