threads / patch / 33203

patchsafe_create_leading_directories: fix race that could give a false negative

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

## tl;dr

4 messages between Mar 16, 2013 and Mar 17, 2013. Diffs are folded; open one to read it.

replies: 3people: 2as markdown or json

Steven Walter· Mar 16, 2013, 19:30 UTC · lore

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(+)

Show changes to sha1_file.c +7 −0

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

Junio C Hamano· Mar 17, 2013, 06:26 UTC · re: Steven Walter · lore

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

Steven Walter <stevenrwalter@gmail.com> writes:

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

Thanks.  Please sign-off your patch.

Show 17 quoted lines

>  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;
>  		}

Steven Walter· Mar 17, 2013, 14:09 UTC · re: Junio C Hamano · lore

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(+)

Show changes to sha1_file.c +9 −0

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

Junio C Hamano· Mar 17, 2013, 19:45 UTC · re: Steven Walter · lore

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

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(-)

Show changes to sha1_file.c +7 −2

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