On Sat, Oct 19, 2013 at 01:45:03AM +0200, Johan Herland wrote:
> 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
> [...]
> Trying to code around this (frankly insane) case in the
> create_tmpfile()/adjust_shared_perm() code is IMHO silly.
Yeah, I'd agree. You cannot just willy-nilly set core.sharedRepository
per-process and expect things to work. I do not think your patch makes
anything worse there.
I was wondering more about the chmod call itself in adjust_shared_perms,
though, even if both processes have the same settings. If we have lost
the mkdir race, then the other process created the directory, and it may
have a different owner, causing our chmod to fail.
If we call adjust_shared_perm after an EEXIST, we are subject to this
race:
1. A calls mkdir, creates directory
2. B calls mkdir, gets EEXIST
3. B calls adjust_shared_perm, which wants to set g+w. It calls
chmod(), which fails (it's A's directory)
4. A calls adjust_shared_perm, which sets g+w; all is well if B now
creates the object, but it already barfed after failing step 3.
But if we do not call adjust_shared_perm, we are subject to this race:
1. A calls mkdir, creates directory
2. B calls mkdir, gets EEXIST
3. B tries to create object in the directory, but fails (no
permission)
4. A calls adjust_shared_perm, but B has already barfed due to failure
in step 3
I do not see an easy way around it. Only A can set the permissions, and
B cannot continue until A has done so. So we would need some kind of
synchronization (B busy-waits checking for A's chmod? Yech), or we would
need to atomically create the directory with the right permissions (and
group).
I do not think of any of this is introduced or made worse by your patch,
though. Without your patch, we do not even get as far as wondering
about permissions. :)
Your patch successfully handles the single-user case, but stops short of
handling the multi-user case. I am not sure if it is worth trying to
follow-on to fix the multi-user race, but even if we do, it would want
to build on top of your patch anyway.
-Peff
--
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