Re: [PATCH] safe_create_leading_directories: fix race that could give a false negative

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 17, 2013, 19:45 UTC
Message-ID: <7vppyxres6.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1363529367-5919-1-git-send-email-stevenrwalter@gmail.com>

Steven Walter <stevenrwalter@gmail.com> writes:

Show 27 quoted lines

> If two processes are racing to create the same directory tree, they will
> both see that the directory doesn't exist, both try to mkdir(), and one
> of them will fail.  This is okay, as we only care that the directory
> gets created.  So, we add a check for EEXIST from mkdir, and continue if
> the directory now exists.
>
> Signed-off-by: Steven Walter <stevenrwalter@gmail.com>
> ---
>  sha1_file.c |    9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/sha1_file.c b/sha1_file.c
> index 40b2329..5668ecc 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -123,6 +123,15 @@ int safe_create_leading_directories(char *path)
>  			}
>  		}
>  		else if (mkdir(path, 0777)) {
> +			if (errno == EEXIST) {
> +				/*
> +				 * We could be racing with another process to
> +				 * create the directory.  As long as the
> +				 * directory gets created, we don't care.
> +				 */
> +				if (stat(path, &st) && S_ISDIR(st.st_mode))
> +					continue;

You probably meant !stat() here, "we can successfully stat() and it turns out that we already have a directory there, so let's not do the error thing".

Don't you need to restore (*pos = '/') before doing anything else, like "continue", by the way? We were given "a/b/c", and in order to make sure "a" exists, we made it to "a\0b/c", did a stat() and found it was missing, did a mkdir() and now we got EEXIST. pos points at that NUL, so I would imagine that in order to continue you need to

 * restore the string to be "a/b/c"; and
 * make pos to point at "b" in the string.

Perhaps something like this instead?

 sha1_file.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/sha1_file.c b/sha1_file.c
index 9152974..964c4d4 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -122,8 +122,13 @@ int safe_create_leading_directories(char *path)
 			}
 		}
 		else if (mkdir(path, 0777)) {
-			*pos = '/';
-			return -1;
+			if (errno == EEXIST &&
+			    !stat(path, &st) && S_ISDIR(st.st_mode)) {
+				; /* somebody created it since we checked */
+			} else {
+				*pos = '/';
+				return -1;
+			}
 		}
 		else if (adjust_shared_perm(path)) {
 			*pos = '/';