{"thread":{"id":"33203","subject":"[PATCH] safe_create_leading_directories: fix race that could give a false negative","startedAt":"2013-03-16T19:30:56Z","lastAt":"2013-03-17T19:45:45Z","messageCount":4,"participants":["Steven Walter","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"211470","messageId":"1363462256-5823-1-git-send-email-stevenrwalter@gmail.com","threadId":"33203","inReplyTo":null,"subject":"[PATCH] safe_create_leading_directories: fix race that could give a false negative","fromName":"Steven Walter","fromEmail":"stevenrwalter@gmail.com","sentAt":"2013-03-16T19:30:56Z","receivedAt":"2013-03-16T19:30:56Z","isPatch":true,"sender":{"key":"stevenrwalter@gmail.com","avatar":"https://avatars.githubusercontent.com/u/79127?v=4"},"body":"If two processes are racing to create the same directory tree, they will\nboth see that the directory doesn't exist, both try to mkdir(), and one\nof them will fail.  This is okay, as we only care that the directory\ngets created.  So, we add a check for EEXIST from mkdir, and continue if\nthe directory now exists.\n---\n sha1_file.c |    7 +++++++\n 1 file changed, 7 insertions(+)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 40b2329..c7b7fec 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -123,6 +123,13 @@ int safe_create_leading_directories(char *path)\n \t\t\t}\n \t\t}\n \t\telse if (mkdir(path, 0777)) {\n+\t\t\tif (errno == EEXIST) {\n+\t\t\t\t/* We could be racing with another process to\n+\t\t\t\t * create the directory.  As long as the\n+\t\t\t\t * directory gets created, we don't care. */\n+\t\t\t\tif (stat(path, &st) && S_ISDIR(st.st_mode))\n+\t\t\t\t\tcontinue;\n+\t\t\t}\n \t\t\t*pos = '/';\n \t\t\treturn -1;\n \t\t}\n-- \n1.7.10.4\n"},{"id":"211498","messageId":"7v7gl6sfsg.fsf@alter.siamese.dyndns.org","threadId":"33203","inReplyTo":"1363462256-5823-1-git-send-email-stevenrwalter@gmail.com","subject":"Re: [PATCH] safe_create_leading_directories: fix race that could give a false negative","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-03-17T06:26:23Z","receivedAt":"2013-03-17T06:26:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Steven Walter <stevenrwalter@gmail.com> writes:\n\n> If two processes are racing to create the same directory tree, they will\n> both see that the directory doesn't exist, both try to mkdir(), and one\n> of them will fail.  This is okay, as we only care that the directory\n> gets created.  So, we add a check for EEXIST from mkdir, and continue if\n> the directory now exists.\n> ---\n\nThanks.  Please sign-off your patch.\n\n>  sha1_file.c |    7 +++++++\n>  1 file changed, 7 insertions(+)\n>\n> diff --git a/sha1_file.c b/sha1_file.c\n> index 40b2329..c7b7fec 100644\n> --- a/sha1_file.c\n> +++ b/sha1_file.c\n> @@ -123,6 +123,13 @@ int safe_create_leading_directories(char *path)\n>  \t\t\t}\n>  \t\t}\n>  \t\telse if (mkdir(path, 0777)) {\n> +\t\t\tif (errno == EEXIST) {\n> +\t\t\t\t/* We could be racing with another process to\n> +\t\t\t\t * create the directory.  As long as the\n> +\t\t\t\t * directory gets created, we don't care. */\n> +\t\t\t\tif (stat(path, &st) && S_ISDIR(st.st_mode))\n> +\t\t\t\t\tcontinue;\n\n\t/*\n         * Nice explanation, but we try to format our\n         * multi-line comments like this, slash-asterisk\n         * and nothing else on the opening line, and\n         * asterisk-slash and nothing else on the closing\n         * line.\n         */\n\nThanks.\n\n> +\t\t\t}\n>  \t\t\t*pos = '/';\n>  \t\t\treturn -1;\n>  \t\t}\n"},{"id":"211520","messageId":"1363529367-5919-1-git-send-email-stevenrwalter@gmail.com","threadId":"33203","inReplyTo":"7v7gl6sfsg.fsf@alter.siamese.dyndns.org","subject":"[PATCH] safe_create_leading_directories: fix race that could give a false negative","fromName":"Steven Walter","fromEmail":"stevenrwalter@gmail.com","sentAt":"2013-03-17T14:09:27Z","receivedAt":"2013-03-17T14:09:27Z","isPatch":true,"sender":{"key":"stevenrwalter@gmail.com","avatar":"https://avatars.githubusercontent.com/u/79127?v=4"},"body":"If two processes are racing to create the same directory tree, they will\nboth see that the directory doesn't exist, both try to mkdir(), and one\nof them will fail.  This is okay, as we only care that the directory\ngets created.  So, we add a check for EEXIST from mkdir, and continue if\nthe directory now exists.\n\nSigned-off-by: Steven Walter <stevenrwalter@gmail.com>\n---\n sha1_file.c |    9 +++++++++\n 1 file changed, 9 insertions(+)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 40b2329..5668ecc 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -123,6 +123,15 @@ int safe_create_leading_directories(char *path)\n \t\t\t}\n \t\t}\n \t\telse if (mkdir(path, 0777)) {\n+\t\t\tif (errno == EEXIST) {\n+\t\t\t\t/*\n+\t\t\t\t * We could be racing with another process to\n+\t\t\t\t * create the directory.  As long as the\n+\t\t\t\t * directory gets created, we don't care.\n+\t\t\t\t */\n+\t\t\t\tif (stat(path, &st) && S_ISDIR(st.st_mode))\n+\t\t\t\t\tcontinue;\n+\t\t\t}\n \t\t\t*pos = '/';\n \t\t\treturn -1;\n \t\t}\n-- \n1.7.10.4\n"},{"id":"211523","messageId":"7vppyxres6.fsf@alter.siamese.dyndns.org","threadId":"33203","inReplyTo":"1363529367-5919-1-git-send-email-stevenrwalter@gmail.com","subject":"Re: [PATCH] safe_create_leading_directories: fix race that could give a false negative","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-03-17T19:45:45Z","receivedAt":"2013-03-17T19:45:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Steven Walter <stevenrwalter@gmail.com> writes:\n\n> If two processes are racing to create the same directory tree, they will\n> both see that the directory doesn't exist, both try to mkdir(), and one\n> of them will fail.  This is okay, as we only care that the directory\n> gets created.  So, we add a check for EEXIST from mkdir, and continue if\n> the directory now exists.\n>\n> Signed-off-by: Steven Walter <stevenrwalter@gmail.com>\n> ---\n>  sha1_file.c |    9 +++++++++\n>  1 file changed, 9 insertions(+)\n>\n> diff --git a/sha1_file.c b/sha1_file.c\n> index 40b2329..5668ecc 100644\n> --- a/sha1_file.c\n> +++ b/sha1_file.c\n> @@ -123,6 +123,15 @@ int safe_create_leading_directories(char *path)\n>  \t\t\t}\n>  \t\t}\n>  \t\telse if (mkdir(path, 0777)) {\n> +\t\t\tif (errno == EEXIST) {\n> +\t\t\t\t/*\n> +\t\t\t\t * We could be racing with another process to\n> +\t\t\t\t * create the directory.  As long as the\n> +\t\t\t\t * directory gets created, we don't care.\n> +\t\t\t\t */\n> +\t\t\t\tif (stat(path, &st) && S_ISDIR(st.st_mode))\n> +\t\t\t\t\tcontinue;\n\nYou probably meant !stat() here, \"we can successfully stat() and it\nturns out that we already have a directory there, so let's not do\nthe error thing\".\n\nDon't you need to restore (*pos = '/') before doing anything else,\nlike \"continue\", by the way?  We were given \"a/b/c\", and in order to\nmake sure \"a\" exists, we made it to \"a\\0b/c\", did a stat() and found\nit was missing, did a mkdir() and now we got EEXIST.  pos points at\nthat NUL, so I would imagine that in order to continue you need to\n\n * restore the string to be \"a/b/c\"; and\n * make pos to point at \"b\" in the string.\n\nPerhaps something like this instead?\n\n sha1_file.c | 9 +++++++--\n 1 file changed, 7 insertions(+), 2 deletions(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 9152974..964c4d4 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -122,8 +122,13 @@ int safe_create_leading_directories(char *path)\n \t\t\t}\n \t\t}\n \t\telse if (mkdir(path, 0777)) {\n-\t\t\t*pos = '/';\n-\t\t\treturn -1;\n+\t\t\tif (errno == EEXIST &&\n+\t\t\t    !stat(path, &st) && S_ISDIR(st.st_mode)) {\n+\t\t\t\t; /* somebody created it since we checked */\n+\t\t\t} else {\n+\t\t\t\t*pos = '/';\n+\t\t\t\treturn -1;\n+\t\t\t}\n \t\t}\n \t\telse if (adjust_shared_perm(path)) {\n \t\t\t*pos = '/';\n"}]}