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

[Jeff King <peff@peff.net>]

On Fri, Oct 18, 2013 at 05:41:54PM +0200, Johan Herland wrote:

>  (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