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

From: Jeff King <peff@peff.net>
Date: Oct 19, 2013, 02:53 UTC
Message-ID: <20131019025317.GB13895@sigill.intra.peff.net>
In-Reply-To: <CALKQrgdAhLZStLUquizkW_t6wVQsWs0hpGauEm8NjB6tZ67JFw@mail.gmail.com>

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