On Fri, Oct 18, 2013 at 9:24 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote: > Jeff King <peff@xxxxxxxx> 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@xxxxxxxxxxx> www.herland.net -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html