From 1fb0c809859252f037286d32f56432444d65bd38 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Fri, 6 Jan 2017 17:22:33 +0100 Subject: [PATCH] log_ref_setup(): improve robustness against races Change log_ref_setup() to use raceproof_create_file() to create the new logfile. This makes it more robust against a race against another process that might be trying to clean up empty directories while we are trying to create a new logfile. This also means that it will only call create_leading_directories() if open() fails, which should be a net win. Even in the cases where we are willing to create a new logfile, it will usually be the case that the logfile already exists, or if not then that the directory containing the logfile already exists. In such cases, we will save some work that was previously done unconditionally. Signed-off-by: Michael Haggerty Reviewed-by: Jeff King Signed-off-by: Junio C Hamano --- refs/files-backend.c | 41 ++++++++++++++++++----------------------- 1 file changed, 18 insertions(+), 23 deletions(-) diff --git a/refs/files-backend.c b/refs/files-backend.c index c8f6d82e6d..27d4fd3f96 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -2710,6 +2710,14 @@ static int commit_ref(struct ref_lock *lock) return 0; } +static int open_or_create_logfile(const char *path, void *cb) +{ + int *fd = cb; + + *fd = open(path, O_APPEND | O_WRONLY | O_CREAT, 0666); + return (*fd < 0) ? -1 : 0; +} + /* * Create a reflog for a ref. If force_create = 0, the reflog will * only be created for certain refs (those for which @@ -2723,31 +2731,18 @@ static int log_ref_setup(const char *refname, struct strbuf *logfile, struct str strbuf_git_path(logfile, "logs/%s", refname); if (force_create || should_autocreate_reflog(refname)) { - if (safe_create_leading_directories(logfile->buf) < 0) { - strbuf_addf(err, "unable to create directory for '%s': " - "%s", logfile->buf, strerror(errno)); - return -1; - } - logfd = open(logfile->buf, O_APPEND | O_WRONLY | O_CREAT, 0666); - if (logfd < 0) { - if (errno == EISDIR) { - /* - * The directory that is in the way might be - * empty. Try to remove it. - */ - if (remove_empty_directories(logfile)) { - strbuf_addf(err, "there are still logs under " - "'%s'", logfile->buf); - return -1; - } - logfd = open(logfile->buf, O_APPEND | O_WRONLY | O_CREAT, 0666); - } - - if (logfd < 0) { + if (raceproof_create_file(logfile->buf, open_or_create_logfile, &logfd)) { + if (errno == ENOENT) + strbuf_addf(err, "unable to create directory for '%s': " + "%s", logfile->buf, strerror(errno)); + else if (errno == EISDIR) + strbuf_addf(err, "there are still logs under '%s'", + logfile->buf); + else strbuf_addf(err, "unable to append to '%s': %s", logfile->buf, strerror(errno)); - return -1; - } + + return -1; } } else { logfd = open(logfile->buf, O_APPEND | O_WRONLY, 0666);