There 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@xxxxxxxxxxx>
---
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(-)
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
--
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