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

From: Johan Herland <johan@herland.net>
Date: Oct 18, 2013, 23:45 UTC
Message-ID: <CALKQrgdAhLZStLUquizkW_t6wVQsWs0hpGauEm8NjB6tZ67JFw@mail.gmail.com>
In-Reply-To: <xmqq7gdawfeg.fsf@gitster.dls.corp.google.com>

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