Re: [PATCH] sha1_file.c:create_tmpfile(): Fix race when creating loose object dirs

From: Johan Herland <johan@herland.net>
Date: Oct 18, 2013, 15:41 UTC
Message-ID: <CALKQrgcc6gXcthgZgrGS6YA0b8KmnaG=WO=Eu_YQc9pgjRRj6Q@mail.gmail.com>
In-Reply-To: <CACsJy8ChkKQJJfV1G6DUQgq1Pwy7Hki1qkD8V4FsLC0bN362PA@mail.gmail.com>

On Fri, Oct 18, 2013 at 4:00 PM, Duy Nguyen <pclouds@gmail.com> wrote:

Show 24 quoted lines

> On Fri, Oct 18, 2013 at 8:55 PM, Duy Nguyen <pclouds@gmail.com> wrote:
>> On Fri, Oct 18, 2013 at 8:17 PM, Johan Herland <johan@herland.net> wrote:
>>> diff --git a/sha1_file.c b/sha1_file.c
>>> index f80bbe4..00ffffe 100644
>>> --- a/sha1_file.c
>>> +++ b/sha1_file.c
>>> @@ -2857,7 +2857,9 @@ static int create_tmpfile(char *buffer, size_t bufsiz, const char *filename)
>>>                 /* Make sure the directory exists */
>>>                 memcpy(buffer, filename, dirlen);
>>>                 buffer[dirlen-1] = 0;
>>> -               if (mkdir(buffer, 0777) || adjust_shared_perm(buffer))
>>> +               if (mkdir(buffer, 0777) && errno != EEXIST)
>>> +                       return -1;
>>> +               if (adjust_shared_perm(buffer))
>>>                         return -1;
>>
>> I was going to ask what if the created directory does not have
>> permission 0777. But adjust_shared_perm follows immediately after, so
>> we're good.
>
> And I sent too early. adjust_shared_perm() does rely on current dir's
> permission. So if the creator does "mkdir(buffer, 0)", we still have a
> bad permission in the end. And adjust_shared_perm() is only active
> when the repository is configured to be shared.

I'm not sure where you're going with this... Whatever happens in another process between the first call git_mkstemp_mode() and our mkdir() call, one of three things will happen in this code:

 (a) mkdir() succeeds, i.e. we won the race. This case is unchanged by my patch.

 (b) mkdir() fails with errno != EEXIST. This results in us returning
-1 to our caller. This case is also unchanged by my patch.

 (c) mkdir() fails with EEXIST, i.e. we lost the race. We do the
adjust_shared_perms() which might fail (-> abort) or succeed, and we
then _retry_ the git_mkstemp_mode(). There is no case where the
"whatever" that happened between the first git_mkstemp_mode() and
mkdir() will have a different outcome (create_tmpfile() failing or
suceeding) than if it had happened _before_ the first
git_mkstemp_mode(). And it is not our responsibility to control what
other/unrelated processes might end up doing with directories inside
.git/objects/...

What am I missing?

...Johan

-- 
Johan Herland, <johan@herland.net>
www.herland.net