threads / patch / 35163

patchsha1_file.c:create_tmpfile(): Fix race when creating loose object dirs

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

## tl;dr

12 messages between Oct 18, 2013 and Oct 22, 2013. Diffs are folded; open one to read it.

replies: 11people: 5as markdown or json

Johan Herland· Oct 18, 2013, 13:17 UTC · lore

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(-)
Show changes to sha1_file.c +3 −1
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
Eric Sunshine· Oct 18, 2013, 13:53 UTC · re: Johan Herland · lore

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

On Fri, Oct 18, 2013 at 9:17 AM, Johan Herland <johan@herland.net> wrote:
Show 31 quoted lines
> 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.)

Show 27 quoted lines
> 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
Johan Herland· Oct 18, 2013, 14:52 UTC · re: Eric Sunshine · lore

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

On Fri, Oct 18, 2013 at 3:53 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
Show 6 quoted lines
> 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:
[...]
Show 6 quoted lines
>> 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
Eric Sunshine· Oct 18, 2013, 15:58 UTC · re: Johan Herland · lore

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

On Fri, Oct 18, 2013 at 10:52 AM, Johan Herland <johan@herland.net> wrote:
Show 20 quoted lines
> 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).

Duy Nguyen· Oct 18, 2013, 13:55 UTC · re: Johan Herland · lore

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

On Fri, Oct 18, 2013 at 8:17 PM, Johan Herland <johan@herland.net> wrote:
Show 13 quoted lines
> 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
Duy Nguyen· Oct 18, 2013, 14:00 UTC · re: Duy Nguyen · lore

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

On Fri, Oct 18, 2013 at 8:55 PM, Duy Nguyen <pclouds@gmail.com> wrote:
Show 18 quoted lines
> 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
Johan Herland· Oct 18, 2013, 15:41 UTC · re: Duy Nguyen · lore

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

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
Jeff King· Oct 18, 2013, 19:05 UTC · re: Johan Herland · lore

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

On Fri, Oct 18, 2013 at 05:41:54PM +0200, Johan Herland wrote:
Show 7 quoted lines
>  (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
Junio C Hamano· Oct 18, 2013, 19:24 UTC · re: Jeff King · lore

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

Jeff King <peff@peff.net> writes:
Show 7 quoted lines
> 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.
Show 7 quoted lines
> 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.

Johan Herland· Oct 18, 2013, 23:45 UTC · re: Junio C Hamano · lore

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

On Fri, Oct 18, 2013 at 9:24 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 12 quoted lines
> 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
Jeff King· Oct 19, 2013, 02:53 UTC · re: Johan Herland · lore

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

On Sat, Oct 19, 2013 at 01:45:03AM +0200, Johan Herland wrote:
Show 19 quoted lines
> 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
Junio C Hamano· Oct 22, 2013, 18:09 UTC · re: Johan Herland · lore

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

Johan Herland <johan@herland.net> writes:
Show 16 quoted lines
>> 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.

← back to recent threads