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

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

[Steven Walter]

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.
---
 sha1_file.c |    7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/sha1_file.c b/sha1_file.c
index 40b2329..c7b7fec 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -123,6 +123,13 @@ 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;
+			}
 			*pos = '/';
 			return -1;
 		}
-- 
1.7.10.4

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

[Junio C Hamano]

Steven Walter <stevenrwalter@gmail.com> writes:

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

Thanks.  Please sign-off your patch.

>  sha1_file.c |    7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/sha1_file.c b/sha1_file.c
> index 40b2329..c7b7fec 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -123,6 +123,13 @@ 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;

	/*
         * Nice explanation, but we try to format our
         * multi-line comments like this, slash-asterisk
         * and nothing else on the opening line, and
         * asterisk-slash and nothing else on the closing
         * line.
         */

Thanks.

> +			}
>  			*pos = '/';
>  			return -1;
>  		}

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

[Steven Walter]

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;
+			}
 			*pos = '/';
 			return -1;
 		}
-- 
1.7.10.4

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

[Junio C Hamano]

Steven Walter <stevenrwalter@gmail.com> writes:

> 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 = '/';