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

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

[Johan Herland]

There are cases (e.g. when running concurrent fetches in a repo) where
multiple Git processes concurrently attempt to create loose objects
within the same objects/XX/ dir. The creation of the loose object files
is (AFAICS) safe from races, but the creation of the objects/XX/ dir in
which the loose objects reside is unsafe, for example:

Two concurrent fetches - A and B. As part of its fetch, A needs to store
12aaaaa as a loose object. B, on the other hand, needs to store 12bbbbb
as a loose object. The objects/12 directory does not already exist.
Concurrently, both A and B determine that they need to create the
objects/12 directory (because their first call to git_mkstemp_mode()
within create_tmpfile() fails witn ENOENT). One of them - let's say A -
executes the following mkdir() call before the other. This first call
returns success, and A moves on. When B gets around to calling mkdir(),
it fails with EEXIST, because A won the race. The mkdir() error causes B
to return -1 from create_tmpfile(), which propagates all the way,
resulting in the fetch failing with:

  error: unable to create temporary file: File exists
  fatal: failed to write object
  fatal: unpack-objects failed

Although it's hard to add a testcase reproducing this issue, it's easy
to reproduce if we insert a sleep after the

  if (mkdir(buffer, 0777) || adjust_shared_perm(buffer))
      return -1;

block, and then run two concurrent "git fetch"es against the same repo.

The fix is to simply handle mkdir() setting errno = EEXIST as success.

Signed-off-by: Johan Herland <johan@herland.net>
---
 sha1_file.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

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;
 
 		/* Try again */
-- 
1.8.4.653.g2df02b3

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

[Eric Sunshine]

On Fri, Oct 18, 2013 at 9:17 AM, Johan Herland <johan@herland.net> wrote:
> There are cases (e.g. when running concurrent fetches in a repo) where
> multiple Git processes concurrently attempt to create loose objects
> within the same objects/XX/ dir. The creation of the loose object files
> is (AFAICS) safe from races, but the creation of the objects/XX/ dir in
> which the loose objects reside is unsafe, for example:
>
> Two concurrent fetches - A and B. As part of its fetch, A needs to store
> 12aaaaa as a loose object. B, on the other hand, needs to store 12bbbbb
> as a loose object. The objects/12 directory does not already exist.
> Concurrently, both A and B determine that they need to create the
> objects/12 directory (because their first call to git_mkstemp_mode()
> within create_tmpfile() fails witn ENOENT). One of them - let's say A -
> executes the following mkdir() call before the other. This first call
> returns success, and A moves on. When B gets around to calling mkdir(),
> it fails with EEXIST, because A won the race. The mkdir() error causes B
> to return -1 from create_tmpfile(), which propagates all the way,
> resulting in the fetch failing with:
>
>   error: unable to create temporary file: File exists
>   fatal: failed to write object
>   fatal: unpack-objects failed
>
> Although it's hard to add a testcase reproducing this issue, it's easy
> to reproduce if we insert a sleep after the
>
>   if (mkdir(buffer, 0777) || adjust_shared_perm(buffer))
>       return -1;
>
> block, and then run two concurrent "git fetch"es against the same repo.
>
> The fix is to simply handle mkdir() setting errno = EEXIST as success.

Would it make sense for the commit message to explain what happens if
EEXIST is returned but the pathname is not a directory? (For instance,
in the same race window, another process had created a plain file or
pipe there.)

> Signed-off-by: Johan Herland <johan@herland.net>
> ---
>  sha1_file.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> 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;
>
>                 /* Try again */
> --
> 1.8.4.653.g2df02b3
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Johan Herland]

On Fri, Oct 18, 2013 at 3:53 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Fri, Oct 18, 2013 at 9:17 AM, Johan Herland <johan@herland.net> wrote:
>> There are cases (e.g. when running concurrent fetches in a repo) where
>> multiple Git processes concurrently attempt to create loose objects
>> within the same objects/XX/ dir. The creation of the loose object files
>> is (AFAICS) safe from races, but the creation of the objects/XX/ dir in
>> which the loose objects reside is unsafe, for example:

[...]

>> The fix is to simply handle mkdir() setting errno = EEXIST as success.
>
> Would it make sense for the commit message to explain what happens if
> EEXIST is returned but the pathname is not a directory? (For instance,
> in the same race window, another process had created a plain file or
> pipe there.)

I'm pretty sure in that case, the following adjust_shared_perm() or
git_mkstemp_mode() would fail, and we'd end up returning error from
create_tmpfile() in any case.

I can add the above to the commit message if you insist. :)


...Johan

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

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

[Eric Sunshine]

On Fri, Oct 18, 2013 at 10:52 AM, Johan Herland <johan@herland.net> wrote:
> On Fri, Oct 18, 2013 at 3:53 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> On Fri, Oct 18, 2013 at 9:17 AM, Johan Herland <johan@herland.net> wrote:
>>> There are cases (e.g. when running concurrent fetches in a repo) where
>>> multiple Git processes concurrently attempt to create loose objects
>>> within the same objects/XX/ dir. The creation of the loose object files
>>> is (AFAICS) safe from races, but the creation of the objects/XX/ dir in
>>> which the loose objects reside is unsafe, for example:
>
> [...]
>
>>> The fix is to simply handle mkdir() setting errno = EEXIST as success.
>>
>> Would it make sense for the commit message to explain what happens if
>> EEXIST is returned but the pathname is not a directory? (For instance,
>> in the same race window, another process had created a plain file or
>> pipe there.)
>
> I'm pretty sure in that case, the following adjust_shared_perm() or
> git_mkstemp_mode() would fail, and we'd end up returning error from
> create_tmpfile() in any case.

>From a quick glance at the code, that was my impression as well.

> I can add the above to the commit message if you insist. :)

It was the only unanswered question which popped into my mind upon
reading the commit message, and required code consultation to answer.

A benefit of mentioning it in the commit message is that future
readers see that these other cases were taken into consideration (but
I don't insist upon a re-roll).

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

[Duy Nguyen]

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. It also answers the question why "mkdir(buffer, mode) &&
errno != EEXIST" should not be wrapped in xmkdir() or something to be
used in other places as well. Thanks.
-- 
Duy

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

[Duy Nguyen]

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.
-- 
Duy

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

[Johan Herland]

On Fri, Oct 18, 2013 at 4:00 PM, Duy Nguyen <pclouds@gmail.com> wrote:
> 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

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

[Jeff King]

On Fri, Oct 18, 2013 at 05:41:54PM +0200, Johan Herland wrote:

>  (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().

I think your (c) is fine as long as adjust_shared_perms() is idempotent,
as we will run it twice (one for each side of the race). But from my
reading, I think it is.

I was almost tempted to say "we do not even need to run
adjust_shared_perm twice", but we do, or we risk another race: one side
loses the mkdir race, but wins the open() race, and writes to a
wrong-permission directory. Of course, that should not matter unless the
racers are two different users (in a shared repo), and in that case, we
wins the adjust_shared_perm race, but does not have permission to change
the mode.

> And it is not our responsibility to control what
> other/unrelated processes might end up doing with directories inside
> .git/objects/...

Agreed. We already leave a wrong-permission directory in place if it
existed before we started create_tmpfile. The code before your patch,
when racing with such a wrong-directory creator, would abort the
tmpfile. Now it will correct the permissions. Either behavior seems fine
to me (yours actually seems better, but the point is that it does not
matter because they are dwarfed by the non-race cases where the
directory is already sitting there).

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I was almost tempted to say "we do not even need to run
> adjust_shared_perm twice", but we do, or we risk another race: one side
> loses the mkdir race, but wins the open() race, and writes to a
> wrong-permission directory. Of course, that should not matter unless the
> racers are two different users (in a shared repo), and in that case, we
> wins the adjust_shared_perm race, but does not have permission to change
> the mode.

Interesting.

> Agreed. We already leave a wrong-permission directory in place if it
> existed before we started create_tmpfile. The code before your patch,
> when racing with such a wrong-directory creator, would abort the
> tmpfile. Now it will correct the permissions. Either behavior seems fine
> to me (yours actually seems better, but the point is that it does not
> matter because they are dwarfed by the non-race cases where the
> directory is already sitting there).

Agreed.  We may notice the failure to correct the permissions in the
new code, where the old code left existing directories incorrect
permissions as they were.

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

[Johan Herland]

On Fri, Oct 18, 2013 at 9:24 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Jeff King <peff@peff.net> writes:
>> Agreed. We already leave a wrong-permission directory in place if it
>> existed before we started create_tmpfile. The code before your patch,
>> when racing with such a wrong-directory creator, would abort the
>> tmpfile. Now it will correct the permissions. Either behavior seems fine
>> to me (yours actually seems better, but the point is that it does not
>> matter because they are dwarfed by the non-race cases where the
>> directory is already sitting there).
>
> Agreed.  We may notice the failure to correct the permissions in the
> new code, where the old code left existing directories incorrect
> permissions as they were.

I'm trying to mentally construct a scenario where two writers with
different configuration contexts - one with shared_repository and one
without - compete in this race, and we somehow end up with one of them
not being able to write its object. But the best/worst case I manage
to construct is this:

1. core.sharedRepository = 0640 (group-read-only)
2. Fetch A starts running
3. core.sharedRepository = 0660 (group-writable)
4. Fetch B starts running as a non-owner (i.e. depends on group-writability)
5. One of them (doesn't matter which) wins the mkdir() race
6. A and B next enter the adjust_shared_perm() race
7. B first sets dir mode to 0660
8. A then sets dir mode to 0640
9. B now tries to write its object into the dir, but fails because
it's not group-writable

This case is unfortunate, but no different than if steps 3 and 4 are
swapped, i.e. the case where fetch B fails because the repo is not yet
group-writable. Also, remember that as part of changing
core.sharedRepository like this (step 3), we also require the admin to
manually alter the permission bits of existing object dirs, to make
sure they are group-writable (call this step 3.5). All of this has to
happen _while_ fetch A is running.

Trying to code around this (frankly insane) case in the
create_tmpfile()/adjust_shared_perm() code is IMHO silly. Instead, a
better solution is for the admin to ensure that no fetch (or
receive-pack, or other object-creating process) is running while the
modification of core.sharedRepository and associated resetting of
permission bits on object dirs takes place (in any case, the admin
must ensure this, to resolve the possible race between an
object-creating process started before the core.sharedRepository
change, and the manual permission update of object dirs).

...Johan

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

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

[Jeff King]

On Sat, Oct 19, 2013 at 01:45:03AM +0200, Johan Herland wrote:

> I'm trying to mentally construct a scenario where two writers with
> different configuration contexts - one with shared_repository and one
> without - compete in this race, and we somehow end up with one of them
> not being able to write its object. But the best/worst case I manage
> to construct is this:
> 
> 1. core.sharedRepository = 0640 (group-read-only)
> 2. Fetch A starts running
> 3. core.sharedRepository = 0660 (group-writable)
> 4. Fetch B starts running as a non-owner (i.e. depends on group-writability)
> 5. One of them (doesn't matter which) wins the mkdir() race
> 6. A and B next enter the adjust_shared_perm() race
> 7. B first sets dir mode to 0660
> 8. A then sets dir mode to 0640
> 9. B now tries to write its object into the dir, but fails because
> it's not group-writable
> [...]
> Trying to code around this (frankly insane) case in the
> create_tmpfile()/adjust_shared_perm() code is IMHO silly.

Yeah, I'd agree. You cannot just willy-nilly set core.sharedRepository
per-process and expect things to work. I do not think your patch makes
anything worse there.

I was wondering more about the chmod call itself in adjust_shared_perms,
though, even if both processes have the same settings.  If we have lost
the mkdir race, then the other process created the directory, and it may
have a different owner, causing our chmod to fail.

If we call adjust_shared_perm after an EEXIST, we are subject to this
race:

  1. A calls mkdir, creates directory
  2. B calls mkdir, gets EEXIST
  3. B calls adjust_shared_perm, which wants to set g+w. It calls
     chmod(), which fails (it's A's directory)
  4. A calls adjust_shared_perm, which sets g+w; all is well if B now
     creates the object, but it already barfed after failing step 3.

But if we do not call adjust_shared_perm, we are subject to this race:

  1. A calls mkdir, creates directory
  2. B calls mkdir, gets EEXIST
  3. B tries to create object in the directory, but fails (no
     permission)
  4. A calls adjust_shared_perm, but B has already barfed due to failure
     in step 3

I do not see an easy way around it. Only A can set the permissions, and
B cannot continue until A has done so. So we would need some kind of
synchronization (B busy-waits checking for A's chmod? Yech), or we would
need to atomically create the directory with the right permissions (and
group).

I do not think of any of this is introduced or made worse by your patch,
though.  Without your patch, we do not even get as far as wondering
about permissions. :)

Your patch successfully handles the single-user case, but stops short of
handling the multi-user case. I am not sure if it is worth trying to
follow-on to fix the multi-user race, but even if we do, it would want
to build on top of your patch anyway.

-Peff

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

[Junio C Hamano]

Johan Herland <johan@herland.net> writes:

>> Agreed.  We may notice the failure to correct the permissions in the
>> new code, where the old code left existing directories incorrect
>> permissions as they were.
>
> I'm trying to mentally construct a scenario where two writers with
> different configuration contexts ...
> This case is unfortunate, but no different than if steps 3 and 4 are
> swapped, i.e. the case where fetch B fails because the repo is not yet
> group-writable. Also, remember that as part of changing
> core.sharedRepository like this (step 3), we also require the admin to
> manually alter the permission bits of existing object dirs, to make
> sure they are group-writable (call this step 3.5). All of this has to
> happen _while_ fetch A is running.
>
> Trying to code around this (frankly insane) case in the
> create_tmpfile()/adjust_shared_perm() code is IMHO silly.

I agree.  Note that "We may notice" above were not meant as "we
would fail unnecessarily in some cases where we did not, which is a
regression".  Both the old and the new code will have problems when
two different users race with each other while having umask that is
unsuitable for working together.