Johan HerlandThere are cases (e.g. when running concurrent fetches in a repo) where multiple Git processes concurrently attempt to create loose objects within the same objects/XX/ dir. The creation of the loose object files is (AFAICS) safe from races, but the creation of the objects/XX/ dir in which the loose objects reside is unsafe, for example:
Two concurrent fetches - A and B. As part of its fetch, A needs to store 12aaaaa as a loose object. B, on the other hand, needs to store 12bbbbb as a loose object. The objects/12 directory does not already exist. Concurrently, both A and B determine that they need to create the objects/12 directory (because their first call to git_mkstemp_mode() within create_tmpfile() fails witn ENOENT). One of them - let's say A - executes the following mkdir() call before the other. This first call returns success, and A moves on. When B gets around to calling mkdir(), it fails with EEXIST, because A won the race. The mkdir() error causes B to return -1 from create_tmpfile(), which propagates all the way, resulting in the fetch failing with:
error: unable to create temporary file: File exists fatal: failed to write object fatal: unpack-objects failed
Although it's hard to add a testcase reproducing this issue, it's easy to provoke if we insert a sleep after the
if (mkdir(buffer, 0777) || adjust_shared_perm(buffer))
return -1;block, and then run two concurrent "git fetch"es against the same repo.
The fix is to simply handle mkdir() failing with EEXIST as a success. If EEXIST is somehow returned for the wrong reasons (because the relevant objects/XX is not a directory, or is otherwise unsuitable for object storage), the following call to adjust_shared_perm(), or ultimately the retried call to git_mkstemp_mode() will fail, and we end up returning error from create_tmpfile() in any case.
Note that there are still cases where two users with unsuitable umasks in a shared repo can end up in two races where one user first wins the mkdir() race to create an objects/XX/ directory, and then the other user wins the adjust_shared_perms() race to chmod() that directory, but fails because it is (transiently, until the first users completes its chmod()) unwriteable to the other user. However, (an equivalent of) this race also exists before this patch, and is made no worse by this patch.
Signed-off-by: Johan Herland <johan@herland.net> ---
I didn't see this in the latest "What's cooking", so here's a resend, with an expanded commit message to reflect our discussion. The patch itself is unchanged.
In order to fix the remaining race, I assume we have to ensure the dir creation obeys the same rules as the object creation, i.e. that there are only two possible states at any time:
- The directory does not exist
- The directory exists with the correct permissons
To achieve this, I guess we have to follow the same procedure we do for loose object creation:
1. Create a temporary directory with a unique name (mkdtemp?)
2. Adjust permissions
3. Rename into place
Can this be done sufficiently atomically across all platforms?
...Johan
sha1_file.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
Show changes to sha1_file.c +3 −1
diff --git a/sha1_file.c b/sha1_file.c index f80bbe4..00ffffe 100644 --- a/sha1_file.c +++ b/sha1_file.c @@ -2857,7 +2857,9 @@ static int create_tmpfile(char *buffer, size_t bufsiz, const char *filename) /* Make sure the directory exists */ memcpy(buffer, filename, dirlen); buffer[dirlen-1] = 0; - if (mkdir(buffer, 0777) || adjust_shared_perm(buffer)) + if (mkdir(buffer, 0777) && errno != EEXIST) + return -1; + if (adjust_shared_perm(buffer)) return -1; /* Try again */
-- 1.8.4.653.g2df02b3
