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

From: Jeff King <peff@peff.net>
Date: Oct 18, 2013, 19:05 UTC
Message-ID: <20131018190537.GB11040@sigill.intra.peff.net>
In-Reply-To: <CALKQrgcc6gXcthgZgrGS6YA0b8KmnaG=WO=Eu_YQc9pgjRRj6Q@mail.gmail.com>

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

Show 7 quoted lines

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