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

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

> Signed-off-by: Johan Herland <johan@herland.net>
> ---
>  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@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html