{"thread":{"id":"35163","subject":"[PATCH] sha1_file.c:create_tmpfile(): Fix race when creating loose object dirs","startedAt":"2013-10-18T13:17:47Z","lastAt":"2013-10-22T18:09:47Z","messageCount":12,"participants":["Johan Herland","Eric Sunshine","Duy Nguyen","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"229168","messageId":"1382102267-18151-1-git-send-email-johan@herland.net","threadId":"35163","inReplyTo":null,"subject":"[PATCH] sha1_file.c:create_tmpfile(): Fix race when creating loose object dirs","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2013-10-18T13:17:47Z","receivedAt":"2013-10-18T13:17:47Z","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 reproduce 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() setting errno = EEXIST as success.\n\nSigned-off-by: Johan Herland \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":"229170","messageId":"CAPig+cRg0ovyutXmTggr5P++dWuVP3izf5JfcJEeEVZhHuVJiA@mail.gmail.com","threadId":"35163","inReplyTo":"1382102267-18151-1-git-send-email-johan@herland.net","subject":"Re: [PATCH] sha1_file.c:create_tmpfile(): Fix race when creating loose object dirs","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2013-10-18T13:53:41Z","receivedAt":"2013-10-18T13:53:41Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Oct 18, 2013 at 9:17 AM, Johan Herland wrote:\n> There are cases (e.g. when running concurrent fetches in a repo) where\n> multiple Git processes concurrently attempt to create loose objects\n> within the same objects/XX/ dir. The creation of the loose object files\n> is (AFAICS) safe from races, but the creation of the objects/XX/ dir in\n> which the loose objects reside is unsafe, for example:\n>\n> Two concurrent fetches - A and B. As part of its fetch, A needs to store\n> 12aaaaa as a loose object. B, on the other hand, needs to store 12bbbbb\n> as a loose object. The objects/12 directory does not already exist.\n> Concurrently, both A and B determine that they need to create the\n> objects/12 directory (because their first call to git_mkstemp_mode()\n> within create_tmpfile() fails witn ENOENT). One of them - let's say A -\n> executes the following mkdir() call before the other. This first call\n> returns success, and A moves on. When B gets around to calling mkdir(),\n> it fails with EEXIST, because A won the race. The mkdir() error causes B\n> to return -1 from create_tmpfile(), which propagates all the way,\n> resulting 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>\n> Although it's hard to add a testcase reproducing this issue, it's easy\n> to reproduce if we insert a sleep after the\n>\n> if (mkdir(buffer, 0777) || adjust_shared_perm(buffer))\n> return -1;\n>\n> block, and then run two concurrent \"git fetch\"es against the same repo.\n>\n> The fix is to simply handle mkdir() setting errno = EEXIST as success.\n\nWould it make sense for the commit message to explain what happens if\nEEXIST is returned but the pathname is not a directory? (For instance,\nin the same race window, another process had created a plain file or\npipe there.)\n\n> Signed-off-by: Johan Herland \n> ---\n> sha1_file.c | 4 +++-\n> 1 file changed, 3 insertions(+), 1 deletion(-)\n>\n> diff --git a/sha1_file.c b/sha1_file.c\n> index 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> /* Make sure the directory exists */\n> memcpy(buffer, filename, dirlen);\n> buffer[dirlen-1] = 0;\n> - if (mkdir(buffer, 0777) || adjust_shared_perm(buffer))\n> + if (mkdir(buffer, 0777) && errno != EEXIST)\n> + return -1;\n> + if (adjust_shared_perm(buffer))\n> return -1;\n>\n> /* Try again */\n> --\n> 1.8.4.653.g2df02b3\n>\n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at http://vger.kernel.org/majordomo-info.html\n"},{"id":"229171","messageId":"CACsJy8C=kaa2OgdxOrefP+Ck_sSA00tHs9UqMXwkh9v4K3Mdvg@mail.gmail.com","threadId":"35163","inReplyTo":"1382102267-18151-1-git-send-email-johan@herland.net","subject":"Re: [PATCH] sha1_file.c:create_tmpfile(): Fix race when creating loose object dirs","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2013-10-18T13:55:06Z","receivedAt":"2013-10-18T13:55:06Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Fri, Oct 18, 2013 at 8:17 PM, Johan Herland wrote:\n> diff --git a/sha1_file.c b/sha1_file.c\n> index 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> /* Make sure the directory exists */\n> memcpy(buffer, filename, dirlen);\n> buffer[dirlen-1] = 0;\n> - if (mkdir(buffer, 0777) || adjust_shared_perm(buffer))\n> + if (mkdir(buffer, 0777) && errno != EEXIST)\n> + return -1;\n> + if (adjust_shared_perm(buffer))\n> return -1;\n\nI was going to ask what if the created directory does not have\npermission 0777. But adjust_shared_perm follows immediately after, so\nwe're good. It also answers the question why \"mkdir(buffer, mode) &&\nerrno != EEXIST\" should not be wrapped in xmkdir() or something to be\nused in other places as well. Thanks.\n-- \nDuy\n"},{"id":"229172","messageId":"CACsJy8ChkKQJJfV1G6DUQgq1Pwy7Hki1qkD8V4FsLC0bN362PA@mail.gmail.com","threadId":"35163","inReplyTo":"CACsJy8C=kaa2OgdxOrefP+Ck_sSA00tHs9UqMXwkh9v4K3Mdvg@mail.gmail.com","subject":"Re: [PATCH] sha1_file.c:create_tmpfile(): Fix race when creating loose object dirs","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2013-10-18T14:00:41Z","receivedAt":"2013-10-18T14:00:41Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Fri, Oct 18, 2013 at 8:55 PM, Duy Nguyen wrote:\n> On Fri, Oct 18, 2013 at 8:17 PM, Johan Herland wrote:\n>> diff --git a/sha1_file.c b/sha1_file.c\n>> index 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>> /* Make sure the directory exists */\n>> memcpy(buffer, filename, dirlen);\n>> buffer[dirlen-1] = 0;\n>> - if (mkdir(buffer, 0777) || adjust_shared_perm(buffer))\n>> + if (mkdir(buffer, 0777) && errno != EEXIST)\n>> + return -1;\n>> + if (adjust_shared_perm(buffer))\n>> return -1;\n>\n> I was going to ask what if the created directory does not have\n> permission 0777. But adjust_shared_perm follows immediately after, so\n> we're good.\n\nAnd I sent too early. adjust_shared_perm() does rely on current dir's\npermission. So if the creator does \"mkdir(buffer, 0)\", we still have a\nbad permission in the end. And adjust_shared_perm() is only active\nwhen the repository is configured to be shared.\n-- \nDuy\n"},{"id":"229173","messageId":"CALKQrgdMagUQMTGWX7RwBfBkA6+Q65aPfpk1W3m1BKMUNmjKFg@mail.gmail.com","threadId":"35163","inReplyTo":"CAPig+cRg0ovyutXmTggr5P++dWuVP3izf5JfcJEeEVZhHuVJiA@mail.gmail.com","subject":"Re: [PATCH] sha1_file.c:create_tmpfile(): Fix race when creating loose object dirs","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2013-10-18T14:52:49Z","receivedAt":"2013-10-18T14:52:49Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"On Fri, Oct 18, 2013 at 3:53 PM, Eric Sunshine wrote:\n> On Fri, Oct 18, 2013 at 9:17 AM, Johan Herland wrote:\n>> There are cases (e.g. when running concurrent fetches in a repo) where\n>> multiple Git processes concurrently attempt to create loose objects\n>> within the same objects/XX/ dir. The creation of the loose object files\n>> is (AFAICS) safe from races, but the creation of the objects/XX/ dir in\n>> which the loose objects reside is unsafe, for example:\n\n[...]\n\n>> The fix is to simply handle mkdir() setting errno = EEXIST as success.\n>\n> Would it make sense for the commit message to explain what happens if\n> EEXIST is returned but the pathname is not a directory? (For instance,\n> in the same race window, another process had created a plain file or\n> pipe there.)\n\nI'm pretty sure in that case, the following adjust_shared_perm() or\ngit_mkstemp_mode() would fail, and we'd end up returning error from\ncreate_tmpfile() in any case.\n\nI can add the above to the commit message if you insist. :)\n\n\n...Johan\n\n-- \nJohan Herland, \nwww.herland.net\n"},{"id":"229175","messageId":"CALKQrgcc6gXcthgZgrGS6YA0b8KmnaG=WO=Eu_YQc9pgjRRj6Q@mail.gmail.com","threadId":"35163","inReplyTo":"CACsJy8ChkKQJJfV1G6DUQgq1Pwy7Hki1qkD8V4FsLC0bN362PA@mail.gmail.com","subject":"Re: [PATCH] sha1_file.c:create_tmpfile(): Fix race when creating loose object dirs","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2013-10-18T15:41:54Z","receivedAt":"2013-10-18T15:41:54Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"On Fri, Oct 18, 2013 at 4:00 PM, Duy Nguyen wrote:\n> On Fri, Oct 18, 2013 at 8:55 PM, Duy Nguyen wrote:\n>> On Fri, Oct 18, 2013 at 8:17 PM, Johan Herland wrote:\n>>> diff --git a/sha1_file.c b/sha1_file.c\n>>> index 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>>> /* Make sure the directory exists */\n>>> memcpy(buffer, filename, dirlen);\n>>> buffer[dirlen-1] = 0;\n>>> - if (mkdir(buffer, 0777) || adjust_shared_perm(buffer))\n>>> + if (mkdir(buffer, 0777) && errno != EEXIST)\n>>> + return -1;\n>>> + if (adjust_shared_perm(buffer))\n>>> return -1;\n>>\n>> I was going to ask what if the created directory does not have\n>> permission 0777. But adjust_shared_perm follows immediately after, so\n>> we're good.\n>\n> And I sent too early. adjust_shared_perm() does rely on current dir's\n> permission. So if the creator does \"mkdir(buffer, 0)\", we still have a\n> bad permission in the end. And adjust_shared_perm() is only active\n> when the repository is configured to be shared.\n\nI'm not sure where you're going with this... Whatever happens in\nanother process between the first call git_mkstemp_mode() and our\nmkdir() call, one of three things will happen in this code:\n\n (a) mkdir() succeeds, i.e. we won the race. This case is unchanged by my patch.\n\n (b) mkdir() fails with errno != EEXIST. This results in us returning\n-1 to our caller. This case is also unchanged by my patch.\n\n (c) mkdir() fails with EEXIST, i.e. we lost the race. We do the\nadjust_shared_perms() which might fail (-> abort) or succeed, and we\nthen _retry_ the git_mkstemp_mode(). There is no case where the\n\"whatever\" that happened between the first git_mkstemp_mode() and\nmkdir() will have a different outcome (create_tmpfile() failing or\nsuceeding) than if it had happened _before_ the first\ngit_mkstemp_mode(). And it is not our responsibility to control what\nother/unrelated processes might end up doing with directories inside\n.git/objects/...\n\nWhat am I missing?\n\n...Johan\n\n-- \nJohan Herland, \nwww.herland.net\n"},{"id":"229177","messageId":"CAPig+cTLMKZ4MnG6YmKmqyFAZ0OgNLf83nOher_MqhPfDTDpLQ@mail.gmail.com","threadId":"35163","inReplyTo":"CALKQrgdMagUQMTGWX7RwBfBkA6+Q65aPfpk1W3m1BKMUNmjKFg@mail.gmail.com","subject":"Re: [PATCH] sha1_file.c:create_tmpfile(): Fix race when creating loose object dirs","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2013-10-18T15:58:46Z","receivedAt":"2013-10-18T15:58:46Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Oct 18, 2013 at 10:52 AM, Johan Herland wrote:\n> On Fri, Oct 18, 2013 at 3:53 PM, Eric Sunshine wrote:\n>> On Fri, Oct 18, 2013 at 9:17 AM, Johan Herland wrote:\n>>> There are cases (e.g. when running concurrent fetches in a repo) where\n>>> multiple Git processes concurrently attempt to create loose objects\n>>> within the same objects/XX/ dir. The creation of the loose object files\n>>> is (AFAICS) safe from races, but the creation of the objects/XX/ dir in\n>>> which the loose objects reside is unsafe, for example:\n>\n> [...]\n>\n>>> The fix is to simply handle mkdir() setting errno = EEXIST as success.\n>>\n>> Would it make sense for the commit message to explain what happens if\n>> EEXIST is returned but the pathname is not a directory? (For instance,\n>> in the same race window, another process had created a plain file or\n>> pipe there.)\n>\n> I'm pretty sure in that case, the following adjust_shared_perm() or\n> git_mkstemp_mode() would fail, and we'd end up returning error from\n> create_tmpfile() in any case.\n\n>From a quick glance at the code, that was my impression as well.\n\n> I can add the above to the commit message if you insist. :)\n\nIt was the only unanswered question which popped into my mind upon\nreading the commit message, and required code consultation to answer.\n\nA benefit of mentioning it in the commit message is that future\nreaders see that these other cases were taken into consideration (but\nI don't insist upon a re-roll).\n"},{"id":"229183","messageId":"20131018190537.GB11040@sigill.intra.peff.net","threadId":"35163","inReplyTo":"CALKQrgcc6gXcthgZgrGS6YA0b8KmnaG=WO=Eu_YQc9pgjRRj6Q@mail.gmail.com","subject":"Re: [PATCH] sha1_file.c:create_tmpfile(): Fix race when creating loose object dirs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-10-18T19:05:37Z","receivedAt":"2013-10-18T19:05:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Oct 18, 2013 at 05:41:54PM +0200, Johan Herland wrote:\n\n> (c) mkdir() fails with EEXIST, i.e. we lost the race. We do the\n> adjust_shared_perms() which might fail (-> abort) or succeed, and we\n> then _retry_ the git_mkstemp_mode(). There is no case where the\n> \"whatever\" that happened between the first git_mkstemp_mode() and\n> mkdir() will have a different outcome (create_tmpfile() failing or\n> suceeding) than if it had happened _before_ the first\n> git_mkstemp_mode().\n\nI think your (c) is fine as long as adjust_shared_perms() is idempotent,\nas we will run it twice (one for each side of the race). But from my\nreading, I think it is.\n\nI was almost tempted to say \"we do not even need to run\nadjust_shared_perm twice\", but we do, or we risk another race: one side\nloses the mkdir race, but wins the open() race, and writes to a\nwrong-permission directory. Of course, that should not matter unless the\nracers are two different users (in a shared repo), and in that case, we\nwins the adjust_shared_perm race, but does not have permission to change\nthe mode.\n\n> And it is not our responsibility to control what\n> other/unrelated processes might end up doing with directories inside\n> .git/objects/...\n\nAgreed. We already leave a wrong-permission directory in place if it\nexisted before we started create_tmpfile. The code before your patch,\nwhen racing with such a wrong-directory creator, would abort the\ntmpfile. Now it will correct the permissions. Either behavior seems fine\nto me (yours actually seems better, but the point is that it does not\nmatter because they are dwarfed by the non-race cases where the\ndirectory is already sitting there).\n\n-Peff\n"},{"id":"229186","messageId":"xmqq7gdawfeg.fsf@gitster.dls.corp.google.com","threadId":"35163","inReplyTo":"20131018190537.GB11040@sigill.intra.peff.net","subject":"Re: [PATCH] sha1_file.c:create_tmpfile(): Fix race when creating loose object dirs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-10-18T19:24:55Z","receivedAt":"2013-10-18T19:24:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I was almost tempted to say \"we do not even need to run\n> adjust_shared_perm twice\", but we do, or we risk another race: one side\n> loses the mkdir race, but wins the open() race, and writes to a\n> wrong-permission directory. Of course, that should not matter unless the\n> racers are two different users (in a shared repo), and in that case, we\n> wins the adjust_shared_perm race, but does not have permission to change\n> the mode.\n\nInteresting.\n\n> Agreed. We already leave a wrong-permission directory in place if it\n> existed before we started create_tmpfile. The code before your patch,\n> when racing with such a wrong-directory creator, would abort the\n> tmpfile. Now it will correct the permissions. Either behavior seems fine\n> to me (yours actually seems better, but the point is that it does not\n> matter because they are dwarfed by the non-race cases where the\n> directory is already sitting there).\n\nAgreed. We may notice the failure to correct the permissions in the\nnew code, where the old code left existing directories incorrect\npermissions as they were.\n"},{"id":"229200","messageId":"CALKQrgdAhLZStLUquizkW_t6wVQsWs0hpGauEm8NjB6tZ67JFw@mail.gmail.com","threadId":"35163","inReplyTo":"xmqq7gdawfeg.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] sha1_file.c:create_tmpfile(): Fix race when creating loose object dirs","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2013-10-18T23:45:03Z","receivedAt":"2013-10-18T23:45:03Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"On Fri, Oct 18, 2013 at 9:24 PM, Junio C Hamano wrote:\n> Jeff King writes:\n>> Agreed. We already leave a wrong-permission directory in place if it\n>> existed before we started create_tmpfile. The code before your patch,\n>> when racing with such a wrong-directory creator, would abort the\n>> tmpfile. Now it will correct the permissions. Either behavior seems fine\n>> to me (yours actually seems better, but the point is that it does not\n>> matter because they are dwarfed by the non-race cases where the\n>> directory is already sitting there).\n>\n> Agreed. We may notice the failure to correct the permissions in the\n> new code, where the old code left existing directories incorrect\n> permissions as they were.\n\nI'm trying to mentally construct a scenario where two writers with\ndifferent configuration contexts - one with shared_repository and one\nwithout - compete in this race, and we somehow end up with one of them\nnot being able to write its object. But the best/worst case I manage\nto construct is this:\n\n1. core.sharedRepository = 0640 (group-read-only)\n2. Fetch A starts running\n3. core.sharedRepository = 0660 (group-writable)\n4. Fetch B starts running as a non-owner (i.e. depends on group-writability)\n5. One of them (doesn't matter which) wins the mkdir() race\n6. A and B next enter the adjust_shared_perm() race\n7. B first sets dir mode to 0660\n8. A then sets dir mode to 0640\n9. B now tries to write its object into the dir, but fails because\nit's not group-writable\n\nThis case is unfortunate, but no different than if steps 3 and 4 are\nswapped, i.e. the case where fetch B fails because the repo is not yet\ngroup-writable. Also, remember that as part of changing\ncore.sharedRepository like this (step 3), we also require the admin to\nmanually alter the permission bits of existing object dirs, to make\nsure they are group-writable (call this step 3.5). All of this has to\nhappen _while_ fetch A is running.\n\nTrying to code around this (frankly insane) case in the\ncreate_tmpfile()/adjust_shared_perm() code is IMHO silly. Instead, a\nbetter solution is for the admin to ensure that no fetch (or\nreceive-pack, or other object-creating process) is running while the\nmodification of core.sharedRepository and associated resetting of\npermission bits on object dirs takes place (in any case, the admin\nmust ensure this, to resolve the possible race between an\nobject-creating process started before the core.sharedRepository\nchange, and the manual permission update of object dirs).\n\n...Johan\n\n-- \nJohan Herland, \nwww.herland.net\n"},{"id":"229204","messageId":"20131019025317.GB13895@sigill.intra.peff.net","threadId":"35163","inReplyTo":"CALKQrgdAhLZStLUquizkW_t6wVQsWs0hpGauEm8NjB6tZ67JFw@mail.gmail.com","subject":"Re: [PATCH] sha1_file.c:create_tmpfile(): Fix race when creating loose object dirs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-10-19T02:53:17Z","receivedAt":"2013-10-19T02:53:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Oct 19, 2013 at 01:45:03AM +0200, Johan Herland wrote:\n\n> I'm trying to mentally construct a scenario where two writers with\n> different configuration contexts - one with shared_repository and one\n> without - compete in this race, and we somehow end up with one of them\n> not being able to write its object. But the best/worst case I manage\n> to construct is this:\n> \n> 1. core.sharedRepository = 0640 (group-read-only)\n> 2. Fetch A starts running\n> 3. core.sharedRepository = 0660 (group-writable)\n> 4. Fetch B starts running as a non-owner (i.e. depends on group-writability)\n> 5. One of them (doesn't matter which) wins the mkdir() race\n> 6. A and B next enter the adjust_shared_perm() race\n> 7. B first sets dir mode to 0660\n> 8. A then sets dir mode to 0640\n> 9. B now tries to write its object into the dir, but fails because\n> it's not group-writable\n> [...]\n> Trying to code around this (frankly insane) case in the\n> create_tmpfile()/adjust_shared_perm() code is IMHO silly.\n\nYeah, I'd agree. You cannot just willy-nilly set core.sharedRepository\nper-process and expect things to work. I do not think your patch makes\nanything worse there.\n\nI was wondering more about the chmod call itself in adjust_shared_perms,\nthough, even if both processes have the same settings. If we have lost\nthe mkdir race, then the other process created the directory, and it may\nhave a different owner, causing our chmod to fail.\n\nIf we call adjust_shared_perm after an EEXIST, we are subject to this\nrace:\n\n 1. A calls mkdir, creates directory\n 2. B calls mkdir, gets EEXIST\n 3. B calls adjust_shared_perm, which wants to set g+w. It calls\n chmod(), which fails (it's A's directory)\n 4. A calls adjust_shared_perm, which sets g+w; all is well if B now\n creates the object, but it already barfed after failing step 3.\n\nBut if we do not call adjust_shared_perm, we are subject to this race:\n\n 1. A calls mkdir, creates directory\n 2. B calls mkdir, gets EEXIST\n 3. B tries to create object in the directory, but fails (no\n permission)\n 4. A calls adjust_shared_perm, but B has already barfed due to failure\n in step 3\n\nI do not see an easy way around it. Only A can set the permissions, and\nB cannot continue until A has done so. So we would need some kind of\nsynchronization (B busy-waits checking for A's chmod? Yech), or we would\nneed to atomically create the directory with the right permissions (and\ngroup).\n\nI do not think of any of this is introduced or made worse by your patch,\nthough. Without your patch, we do not even get as far as wondering\nabout permissions. :)\n\nYour patch successfully handles the single-user case, but stops short of\nhandling the multi-user case. I am not sure if it is worth trying to\nfollow-on to fix the multi-user race, but even if we do, it would want\nto build on top of your patch anyway.\n\n-Peff\n"},{"id":"229297","messageId":"xmqqhac9tbx0.fsf@gitster.dls.corp.google.com","threadId":"35163","inReplyTo":"CALKQrgdAhLZStLUquizkW_t6wVQsWs0hpGauEm8NjB6tZ67JFw@mail.gmail.com","subject":"Re: [PATCH] sha1_file.c:create_tmpfile(): Fix race when creating loose object dirs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-10-22T18:09:47Z","receivedAt":"2013-10-22T18:09:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johan Herland writes:\n\n>> Agreed. We may notice the failure to correct the permissions in the\n>> new code, where the old code left existing directories incorrect\n>> permissions as they were.\n>\n> I'm trying to mentally construct a scenario where two writers with\n> different configuration contexts ...\n> This case is unfortunate, but no different than if steps 3 and 4 are\n> swapped, i.e. the case where fetch B fails because the repo is not yet\n> group-writable. Also, remember that as part of changing\n> core.sharedRepository like this (step 3), we also require the admin to\n> manually alter the permission bits of existing object dirs, to make\n> sure they are group-writable (call this step 3.5). All of this has to\n> happen _while_ fetch A is running.\n>\n> Trying to code around this (frankly insane) case in the\n> create_tmpfile()/adjust_shared_perm() code is IMHO silly.\n\nI agree. Note that \"We may notice\" above were not meant as \"we\nwould fail unnecessarily in some cases where we did not, which is a\nregression\". Both the old and the new code will have problems when\ntwo different users race with each other while having umask that is\nunsuitable for working together.\n"}]}