On Fri, Oct 18, 2013 at 9:17 AM, Johan Herland <johan@xxxxxxxxxxx> wrote: > 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 reproduce 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() setting errno = EEXIST as success. Would it make sense for the commit message to explain what happens if EEXIST is returned but the pathname is not a directory? (For instance, in the same race window, another process had created a plain file or pipe there.) > Signed-off-by: Johan Herland <johan@xxxxxxxxxxx> > --- > 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 -- 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