On Fri, Oct 18, 2013 at 9:24 PM, Junio C Hamano <gits...@pobox.com> wrote:
> Jeff King <p...@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 Herland, <jo...@herland.net>
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to