4 files changed, 83 insertions, 64 deletions

diff --git a/source/cache.c b/source/cache.cindex 6317a12..0a3166d 100644--- a/source/cache.c+++ b/source/cache.c@@ -198,55 +198,71 @@ static int is_modified(struct cache_slot *slot)  * already being rebuilt, so it returns an errno instead of waiting. Every  * other failure is logged here, for both callers.  */-static int lock_slot(struct cache_slot *slot)+int cache_lock_file(const char *path, int *fd) { 	struct stat held, named; 	int err; -	// Close-on-exec, so that a program the fill runs gets the file only-	// as its stdout, and holds the lock no longer than it can write there.-	slot->lock_fd = open(slot->lock_path, O_RDWR | O_CREAT | O_CLOEXEC, S_IRUSR | S_IWUSR);-	if (slot->lock_fd == -1) {+	// Close-on-exec, so that a program a cache fill runs gets the file+	// only as its stdout, and holds the lock no longer than it can write+	// there.+	*fd = open(path, O_RDWR | O_CREAT | O_CLOEXEC, S_IRUSR | S_IWUSR);+	if (*fd == -1) { 		err = errno;-		fprintf(stderr, "[cgit] Unable to open %s: %s (%d)\n", slot->lock_path, strerror(err), err);+		fprintf(stderr, "[cgit] Unable to open %s: %s (%d)\n", path, strerror(err), err); 		return err; 	}-	// An flock rather than a POSIX record lock. The descriptor is put on-	// stdout for the fill, and a record lock is dropped as soon as any+	// An flock rather than a POSIX record lock. A slot's descriptor is put+	// on stdout for the fill, and a record lock is dropped as soon as any 	// descriptor on the file closes, which an exec filter does to stdout,-	// while an flock holds until the last descriptor closes.-	if (flock(slot->lock_fd, LOCK_EX | LOCK_NB) < 0) {+	// while an flock holds until the last descriptor closes. Either way a+	// holder that dies takes its lock with it.+	if (flock(*fd, LOCK_EX | LOCK_NB) < 0) { 		// A lock held elsewhere only means concurrent requests, which 		// is not worth a line in the server log. Any other failure, a-		// filesystem without lock support say, silently keeps the slot-		// from ever being filled or refreshed and does deserve one.+		// filesystem without lock support say, keeps the cache from+		// ever being written and does deserve one. 		err = errno; 		if (err != EWOULDBLOCK)-			fprintf(stderr, "[cgit] Unable to lock %s: %s (%d)\n", slot->lock_path, strerror(err), err);-		close(slot->lock_fd);-		slot->lock_fd = -1;-		return err;+			fprintf(stderr, "[cgit] Unable to lock %s: %s (%d)\n", path, strerror(err), err);+		goto fail; 	} 	// The lock landed on whatever inode the path named at open. A holder-	// finishing in between renames that inode into place as the live-	// slot, and once the path is confirmed to still name this file that-	// rename can no longer happen, because it takes the lock held here.+	// finishing in between renames that inode into place, and once the+	// path is confirmed to still name this file that rename can no longer+	// happen, because it takes the lock held here. 	if (-		fstat(slot->lock_fd, &held)   ||-		stat(slot->lock_path, &named) ||-		held.st_ino != named.st_ino   ||+		fstat(*fd, &held)           ||+		stat(path, &named)          ||+		held.st_ino != named.st_ino || 		held.st_dev != named.st_dev 	) {-		close(slot->lock_fd);-		slot->lock_fd = -1;-		return EAGAIN;+		err = EAGAIN;+		goto fail; 	} 	// A run that died before its rename leaves the lock file behind, so 	// start from empty now that nobody else can be writing it.-	if (-		ftruncate(slot->lock_fd, 0) < 0 ||-		write_in_full(slot->lock_fd, slot->key, slot->keylen + 1) < 0-	) {+	if (ftruncate(*fd, 0) < 0) {+		err = errno;+		fprintf(stderr, "[cgit] Unable to write %s: %s (%d)\n", path, strerror(err), err);+		unlink(path);+		goto fail;+	}+	return 0;++fail:+	close(*fd);+	*fd = -1;+	return err;+}++static int lock_slot(struct cache_slot *slot)+{+	int err = cache_lock_file(slot->lock_path, &slot->lock_fd);++	if (err)+		return err;+	if (write_in_full(slot->lock_fd, slot->key, slot->keylen + 1) < 0) { 		err = errno; 		fprintf(stderr, "[cgit] Unable to write %s: %s (%d)\n", slot->lock_path, strerror(err), err); 		unlink(slot->lock_path);
diff --git a/source/cache.c b/source/cache.c
index 6317a12..0a3166d 100644
--- a/source/cache.c
+++ b/source/cache.c
@@ -198,55 +198,71 @@ static int is_modified(struct cache_slot *slot)
  * already being rebuilt, so it returns an errno instead of waiting. Every
  * other failure is logged here, for both callers.
  */
-static int lock_slot(struct cache_slot *slot)
+int cache_lock_file(const char *path, int *fd)
 {
 	struct stat held, named;
 	int err;
 
-	// Close-on-exec, so that a program the fill runs gets the file only
-	// as its stdout, and holds the lock no longer than it can write there.
-	slot->lock_fd = open(slot->lock_path, O_RDWR | O_CREAT | O_CLOEXEC, S_IRUSR | S_IWUSR);
-	if (slot->lock_fd == -1) {
+	// Close-on-exec, so that a program a cache fill runs gets the file
+	// only as its stdout, and holds the lock no longer than it can write
+	// there.
+	*fd = open(path, O_RDWR | O_CREAT | O_CLOEXEC, S_IRUSR | S_IWUSR);
+	if (*fd == -1) {
 		err = errno;
-		fprintf(stderr, "[cgit] Unable to open %s: %s (%d)\n", slot->lock_path, strerror(err), err);
+		fprintf(stderr, "[cgit] Unable to open %s: %s (%d)\n", path, strerror(err), err);
 		return err;
 	}
-	// An flock rather than a POSIX record lock. The descriptor is put on
-	// stdout for the fill, and a record lock is dropped as soon as any
+	// An flock rather than a POSIX record lock. A slot's descriptor is put
+	// on stdout for the fill, and a record lock is dropped as soon as any
 	// descriptor on the file closes, which an exec filter does to stdout,
-	// while an flock holds until the last descriptor closes.
-	if (flock(slot->lock_fd, LOCK_EX | LOCK_NB) < 0) {
+	// while an flock holds until the last descriptor closes. Either way a
+	// holder that dies takes its lock with it.
+	if (flock(*fd, LOCK_EX | LOCK_NB) < 0) {
 		// A lock held elsewhere only means concurrent requests, which
 		// is not worth a line in the server log. Any other failure, a
-		// filesystem without lock support say, silently keeps the slot
-		// from ever being filled or refreshed and does deserve one.
+		// filesystem without lock support say, keeps the cache from
+		// ever being written and does deserve one.
 		err = errno;
 		if (err != EWOULDBLOCK)
-			fprintf(stderr, "[cgit] Unable to lock %s: %s (%d)\n", slot->lock_path, strerror(err), err);
-		close(slot->lock_fd);
-		slot->lock_fd = -1;
-		return err;
+			fprintf(stderr, "[cgit] Unable to lock %s: %s (%d)\n", path, strerror(err), err);
+		goto fail;
 	}
 	// The lock landed on whatever inode the path named at open. A holder
-	// finishing in between renames that inode into place as the live
-	// slot, and once the path is confirmed to still name this file that
-	// rename can no longer happen, because it takes the lock held here.
+	// finishing in between renames that inode into place, and once the
+	// path is confirmed to still name this file that rename can no longer
+	// happen, because it takes the lock held here.
 	if (
-		fstat(slot->lock_fd, &held)   ||
-		stat(slot->lock_path, &named) ||
-		held.st_ino != named.st_ino   ||
+		fstat(*fd, &held)           ||
+		stat(path, &named)          ||
+		held.st_ino != named.st_ino ||
 		held.st_dev != named.st_dev
 	) {
-		close(slot->lock_fd);
-		slot->lock_fd = -1;
-		return EAGAIN;
+		err = EAGAIN;
+		goto fail;
 	}
 	// A run that died before its rename leaves the lock file behind, so
 	// start from empty now that nobody else can be writing it.
-	if (
-		ftruncate(slot->lock_fd, 0) < 0 ||
-		write_in_full(slot->lock_fd, slot->key, slot->keylen + 1) < 0
-	) {
+	if (ftruncate(*fd, 0) < 0) {
+		err = errno;
+		fprintf(stderr, "[cgit] Unable to write %s: %s (%d)\n", path, strerror(err), err);
+		unlink(path);
+		goto fail;
+	}
+	return 0;
+
+fail:
+	close(*fd);
+	*fd = -1;
+	return err;
+}
+
+static int lock_slot(struct cache_slot *slot)
+{
+	int err = cache_lock_file(slot->lock_path, &slot->lock_fd);
+
+	if (err)
+		return err;
+	if (write_in_full(slot->lock_fd, slot->key, slot->keylen + 1) < 0) {
 		err = errno;
 		fprintf(stderr, "[cgit] Unable to write %s: %s (%d)\n", slot->lock_path, strerror(err), err);
 		unlink(slot->lock_path);
diff --git a/source/cache.h b/source/cache.hindex b73a6f4..cfe1fbf 100644--- a/source/cache.h+++ b/source/cache.h@@ -40,4 +40,14 @@ extern int cache_abandon_fill(void);  extern unsigned long cache_hash_str(const char *str); +/*+ * Open the lock file at path, creating it if need be, then lock it without+ * waiting and empty it, leaving the descriptor in *fd. The lock holds until+ * the last descriptor on the file closes, and the caller renames or removes+ * the file before that. Returns 0, EAGAIN or EWOULDBLOCK when another process+ * holds the lock or has renamed the file away, or the errno of the step that+ * failed, which has been logged. *fd is -1 on failure.+ */+extern int cache_lock_file(const char *path, int *fd);+ #endif // CGIT_CACHE_H
diff --git a/source/cache.h b/source/cache.h
index b73a6f4..cfe1fbf 100644
--- a/source/cache.h
+++ b/source/cache.h
@@ -40,4 +40,14 @@ extern int cache_abandon_fill(void);
 
 extern unsigned long cache_hash_str(const char *str);
 
+/*
+ * Open the lock file at path, creating it if need be, then lock it without
+ * waiting and empty it, leaving the descriptor in *fd. The lock holds until
+ * the last descriptor on the file closes, and the caller renames or removes
+ * the file before that. Returns 0, EAGAIN or EWOULDBLOCK when another process
+ * holds the lock or has renamed the file away, or the errno of the step that
+ * failed, which has been logged. *fd is -1 on failure.
+ */
+extern int cache_lock_file(const char *path, int *fd);
+
 #endif // CGIT_CACHE_H
diff --git a/source/cgit.c b/source/cgit.cindex bccc58f..245e77d 100644--- a/source/cgit.c+++ b/source/cgit.c@@ -27,7 +27,6 @@ #include "ui-shared.h" #include "ui-snapshot.h" #include "ui-stats.h"-#include <sys/file.h>  // Deliberately small. The whole body is read at once and handed to the auth // filter, so a filter never has to defend itself against a huge or a@@ -329,47 +328,16 @@ static void parse_args(int argc, const char **argv) static int generate_cached_repolist(const char *path, const char *cached_rc) { 	struct strbuf locked_rc = STRBUF_INIT;-	struct stat held, named; 	int err = 0; 	int fd; 	int first; 	FILE *f;  	strbuf_addf(&locked_rc, "%s.lock", cached_rc);-	fd = open(locked_rc.buf, O_RDWR | O_CREAT | O_CLOEXEC, S_IRUSR | S_IWUSR);-	if (fd == -1) {-		err = errno;-		fprintf(stderr, "[cgit] Unable to open %s: %s (%d)\n", locked_rc.buf, strerror(err), err);-		goto out;-	}-	if (flock(fd, LOCK_EX | LOCK_NB) < 0) {-		// A lock held elsewhere only means concurrent requests, which-		// is not worth a line in the server log. Any other failure, a-		// filesystem without lock support say, silently costs a scan-		// on every request and does deserve one.-		err = errno;-		if (err != EWOULDBLOCK)-			fprintf(stderr, "[cgit] Unable to lock %s: %s (%d)\n", locked_rc.buf, strerror(err), err);-		close(fd);+	err = cache_lock_file(locked_rc.buf, &fd);+	if (err) 		goto out;-	}-	// The lock landed on whatever inode the path named at open. A holder-	// finishing in between renames that inode into place as the live-	// list, and once the path is confirmed to still name this file that-	// rename can no longer happen, because it takes the lock held here.-	if (-		fstat(fd, &held)            ||-		stat(locked_rc.buf, &named) ||-		held.st_ino != named.st_ino ||-		held.st_dev != named.st_dev-	) {-		err = EAGAIN;-		close(fd);-		goto out;-	}-	// A run that died before its rename leaves the lock file behind, so-	// start from empty now that nobody else can be writing it.-	if (ftruncate(fd, 0) < 0 || !(f = fdopen(fd, "w"))) {+	if (!(f = fdopen(fd, "w"))) { 		err = errno; 		fprintf(stderr, "[cgit] Unable to write %s: %s (%d)\n", locked_rc.buf, strerror(err), err); 		unlink(locked_rc.buf);
diff --git a/source/cgit.c b/source/cgit.c
index bccc58f..245e77d 100644
--- a/source/cgit.c
+++ b/source/cgit.c
@@ -27,7 +27,6 @@
 #include "ui-shared.h"
 #include "ui-snapshot.h"
 #include "ui-stats.h"
-#include <sys/file.h>
 
 // Deliberately small. The whole body is read at once and handed to the auth
 // filter, so a filter never has to defend itself against a huge or a
@@ -329,47 +328,16 @@ static void parse_args(int argc, const char **argv)
 static int generate_cached_repolist(const char *path, const char *cached_rc)
 {
 	struct strbuf locked_rc = STRBUF_INIT;
-	struct stat held, named;
 	int err = 0;
 	int fd;
 	int first;
 	FILE *f;
 
 	strbuf_addf(&locked_rc, "%s.lock", cached_rc);
-	fd = open(locked_rc.buf, O_RDWR | O_CREAT | O_CLOEXEC, S_IRUSR | S_IWUSR);
-	if (fd == -1) {
-		err = errno;
-		fprintf(stderr, "[cgit] Unable to open %s: %s (%d)\n", locked_rc.buf, strerror(err), err);
-		goto out;
-	}
-	if (flock(fd, LOCK_EX | LOCK_NB) < 0) {
-		// A lock held elsewhere only means concurrent requests, which
-		// is not worth a line in the server log. Any other failure, a
-		// filesystem without lock support say, silently costs a scan
-		// on every request and does deserve one.
-		err = errno;
-		if (err != EWOULDBLOCK)
-			fprintf(stderr, "[cgit] Unable to lock %s: %s (%d)\n", locked_rc.buf, strerror(err), err);
-		close(fd);
+	err = cache_lock_file(locked_rc.buf, &fd);
+	if (err)
 		goto out;
-	}
-	// The lock landed on whatever inode the path named at open. A holder
-	// finishing in between renames that inode into place as the live
-	// list, and once the path is confirmed to still name this file that
-	// rename can no longer happen, because it takes the lock held here.
-	if (
-		fstat(fd, &held)            ||
-		stat(locked_rc.buf, &named) ||
-		held.st_ino != named.st_ino ||
-		held.st_dev != named.st_dev
-	) {
-		err = EAGAIN;
-		close(fd);
-		goto out;
-	}
-	// A run that died before its rename leaves the lock file behind, so
-	// start from empty now that nobody else can be writing it.
-	if (ftruncate(fd, 0) < 0 || !(f = fdopen(fd, "w"))) {
+	if (!(f = fdopen(fd, "w"))) {
 		err = errno;
 		fprintf(stderr, "[cgit] Unable to write %s: %s (%d)\n", locked_rc.buf, strerror(err), err);
 		unlink(locked_rc.buf);
diff --git a/tests/t0003-cache.sh b/tests/t0003-cache.shindex f4fa3cb..17816c3 100755--- a/tests/t0003-cache.sh+++ b/tests/t0003-cache.sh@@ -365,4 +365,29 @@ test_expect_success PERL 'a scan finding its lock held runs uncached' ' 	test_must_be_empty held.err ' +# A run that died before its rename leaves a lock file behind, here longer+# than anything the next run writes and ending in a repository and a marker.+# Emptying the file before reuse keeps that tail out of both caches.+test_expect_success 'a stale lock file reaches neither the scan nor a slot' '+	{+		printf "%01024d\n" 0 0 0 0 &&+		echo "repo.url=ghost" &&+		echo "repo.path=$PWD/scanned/one/.git"+	} >stale.lock &&+	rm -f "cache5/$scanned_rc" &&+	cp stale.lock "cache5/$scanned_rc.lock" &&+	CGIT_CONFIG="$PWD/scanrc" QUERY_STRING="url=" cgit >scan.out &&+	grep ">one</a>" scan.out &&+	! grep ">ghost</a>" scan.out &&+	rm -rf cache3 && mkdir cache3 &&+	CGIT_CONFIG="$PWD/bigkeyrc" QUERY_STRING="url=bigpage/" cgit >page.out &&+	slot=$(ls cache3) &&+	rm "cache3/$slot" &&+	{ cat page.out page.out && echo "STALE-TAIL"; } >"cache3/$slot.lock" &&+	CGIT_CONFIG="$PWD/bigkeyrc" QUERY_STRING="url=bigpage/" cgit >/dev/null &&+	CGIT_CONFIG="$PWD/bigkeyrc" QUERY_STRING="url=bigpage/" cgit >replay.out &&+	grep "</html>" replay.out &&+	! grep "STALE-TAIL" replay.out+'+ test_done
diff --git a/tests/t0003-cache.sh b/tests/t0003-cache.sh
index f4fa3cb..17816c3 100755
--- a/tests/t0003-cache.sh
+++ b/tests/t0003-cache.sh
@@ -365,4 +365,29 @@ test_expect_success PERL 'a scan finding its lock held runs uncached' '
 	test_must_be_empty held.err
 '
 
+# A run that died before its rename leaves a lock file behind, here longer
+# than anything the next run writes and ending in a repository and a marker.
+# Emptying the file before reuse keeps that tail out of both caches.
+test_expect_success 'a stale lock file reaches neither the scan nor a slot' '
+	{
+		printf "%01024d\n" 0 0 0 0 &&
+		echo "repo.url=ghost" &&
+		echo "repo.path=$PWD/scanned/one/.git"
+	} >stale.lock &&
+	rm -f "cache5/$scanned_rc" &&
+	cp stale.lock "cache5/$scanned_rc.lock" &&
+	CGIT_CONFIG="$PWD/scanrc" QUERY_STRING="url=" cgit >scan.out &&
+	grep ">one</a>" scan.out &&
+	! grep ">ghost</a>" scan.out &&
+	rm -rf cache3 && mkdir cache3 &&
+	CGIT_CONFIG="$PWD/bigkeyrc" QUERY_STRING="url=bigpage/" cgit >page.out &&
+	slot=$(ls cache3) &&
+	rm "cache3/$slot" &&
+	{ cat page.out page.out && echo "STALE-TAIL"; } >"cache3/$slot.lock" &&
+	CGIT_CONFIG="$PWD/bigkeyrc" QUERY_STRING="url=bigpage/" cgit >/dev/null &&
+	CGIT_CONFIG="$PWD/bigkeyrc" QUERY_STRING="url=bigpage/" cgit >replay.out &&
+	grep "</html>" replay.out &&
+	! grep "STALE-TAIL" replay.out
+'
+
 test_done