Re: [PATCH] safe_create_leading_directories: fix race that could give a false negative
From: Junio C Hamano <hidden>
Date: 2016-06-15 22:56:25
Subsystem:
the rest · Maintainer:
Linus Torvalds
Possibly related (same subject, not in this thread)
- 2016-06-15 · [PATCH] safe_create_leading_directories: fix race that could give a false negative · Steven Walter <hidden>
- 2016-06-15 · Re: [PATCH] safe_create_leading_directories: fix race that could give a false negative · Junio C Hamano <hidden>
- 2016-06-15 · [PATCH] safe_create_leading_directories: fix race that could give a false negative · Steven Walter <hidden>
Steven Walter [off-list ref] writes:
quoted hunk
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 <redacted> --- 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 = '/';