Re: [PATCH 5/5] rename_ref(): fix a mkdir()/rmdir() race

[Jonathan Nieder]

Michael Haggerty wrote:

  refs.c | 10 +-
  1 file changed, 9 insertions(+), 1 deletion(-)

A test or example reproduction recipe would be nice.  (But I can
understand not having one --- races are hard to test.)

[...]
 --- a/refs.c
 +++ b/refs.c
[...]
 @@ -2574,6 +2575,13 @@ int rename_ref(const char *oldrefname, const char 
 *newrefname, const char *logms
   }
   goto retry;
   } else {
 + if (errno == ENOENT  --attempts)
 + /*
 +  * Perhaps somebody just pruned the empty
 +  * directory into which we wanted to move the
 +  * file.
 +  */
 + goto retry;

Style nit: it's easier to read a test of errno when the 'else's
cascade (i.e., using 'else if' here).

This patch doesn't depend on any of the others from the series.  For
what it's worth, with or without the following squashed in,

Reviewed-by: Jonathan Nieder jrnie...@gmail.com

Thanks.

diff --git i/refs.c w/refs.c
index 3ab1491..ea62395 100644
--- i/refs.c
+++ w/refs.c
@@ -2574,14 +2574,14 @@ int rename_ref(const char *oldrefname, const char 
*newrefname, const char *logms
goto rollback;
}
goto retry;
+   } else if (errno == ENOENT  --attempts)
+   /*
+* Perhaps somebody just pruned the empty
+* directory into which we wanted to move the
+* file.
+*/
+   goto retry;
} else {
-   if (errno == ENOENT  --attempts)
-   /*
-* Perhaps somebody just pruned the empty
-* directory into which we wanted to move the
-* file.
-*/
-   goto retry;
error(unable to move logfile TMP_RENAMED_LOG to 
logs/%s: %s,
newrefname, strerror(errno));
goto rollback;
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 5/5] rename_ref(): fix a mkdir()/rmdir() race

[Michael Haggerty]

When renaming a reflog file, it was possible that an empty directory
that we just created using safe_create_leading_directories() might get
deleted by another process before we have a chance to move the new
file into it.

So if the rename fails with ENOENT, then retry from the beginning.
Make up to three attempts before giving up.

It could theoretically happen that the ENOENT comes from the
disappearance of TMP_RENAMED_LOG.  In that case three pointless
attempts will be made to move the nonexistent file, but no other harm
should come of it.

Signed-off-by: Michael Haggerty mhag...@alum.mit.edu
---
 refs.c | 10 +-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/refs.c b/refs.c
index 3926136..3ab1491 100644
--- a/refs.c
+++ b/refs.c
@@ -2516,6 +2516,7 @@ int rename_ref(const char *oldrefname, const char 
*newrefname, const char *logms
struct stat loginfo;
int log = !lstat(git_path(logs/%s, oldrefname), loginfo);
const char *symref = NULL;
+   int attempts = 3;
 
if (log  S_ISLNK(loginfo.st_mode))
return error(reflog for %s is a symlink, oldrefname);
@@ -2555,12 +2556,12 @@ int rename_ref(const char *oldrefname, const char 
*newrefname, const char *logms
}
}
 
+ retry:
if (log  safe_create_leading_directories(git_path(logs/%s, 
newrefname))) {
error(unable to create directory for %s, newrefname);
goto rollback;
}
 
- retry:
if (log  rename(git_path(TMP_RENAMED_LOG), git_path(logs/%s, 
newrefname))) {
if (errno==EISDIR || errno==ENOTDIR) {
/*
@@ -2574,6 +2575,13 @@ int rename_ref(const char *oldrefname, const char 
*newrefname, const char *logms
}
goto retry;
} else {
+   if (errno == ENOENT  --attempts)
+   /*
+* Perhaps somebody just pruned the empty
+* directory into which we wanted to move the
+* file.
+*/
+   goto retry;
error(unable to move logfile TMP_RENAMED_LOG to 
logs/%s: %s,
newrefname, strerror(errno));
goto rollback;
-- 
1.8.5.1

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html