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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, Oct 18, 2013 at 4:00 PM, Duy Nguyen <pclouds@xxxxxxxxx> wrote:
> On Fri, Oct 18, 2013 at 8:55 PM, Duy Nguyen <pclouds@xxxxxxxxx> wrote:
>> On Fri, Oct 18, 2013 at 8:17 PM, Johan Herland <johan@xxxxxxxxxxx> 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@xxxxxxxxxxx>
www.herland.net
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]