Lock a cache slot with flock

The slot lock was a POSIX record lock, which a process loses the moment
any descriptor on the file closes, and the fill puts the lock file on
stdout, which every exec filter and snapshot compressor closes with its
dup2. A second request on the slot could then rebuild it under the first
and splice the two pages. An flock holds until the last descriptor goes.

2 files changed, 44 insertions, 7 deletions

diff --git a/source/cache.c b/source/cache.cindex 82b3aad..9594fd6 100644--- a/source/cache.c+++ b/source/cache.c@@ -13,6 +13,7 @@ #include "cgit.h" #include "html.h" #include "shared.h"+#include <sys/file.h> #ifdef HAVE_LINUX_SENDFILE #include <sys/sendfile.h> #endif@@ -232,18 +233,16 @@ static int close_lock(struct cache_slot *slot)  */ static int lock_slot(struct cache_slot *slot) {-	struct flock lock = {-		.l_type = F_WRLCK,-		.l_whence = SEEK_SET,-		.l_start = 0,-		.l_len = 0,-	}; 	struct stat held, named;  	slot->lock_fd = open(slot->lock_path, O_RDWR | O_CREAT, S_IRUSR | S_IWUSR); 	if (slot->lock_fd == -1) 		return errno;-	if (fcntl(slot->lock_fd, F_SETLK, &lock) < 0) {+	// 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+	// 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) { 		int saved_errno = errno; 		close(slot->lock_fd); 		slot->lock_fd = -1;
diff --git a/source/cache.c b/source/cache.c
index 82b3aad..9594fd6 100644
--- a/source/cache.c
+++ b/source/cache.c
@@ -13,6 +13,7 @@
 #include "cgit.h"
 #include "html.h"
 #include "shared.h"
+#include <sys/file.h>
 #ifdef HAVE_LINUX_SENDFILE
 #include <sys/sendfile.h>
 #endif
@@ -232,18 +233,16 @@ static int close_lock(struct cache_slot *slot)
  */
 static int lock_slot(struct cache_slot *slot)
 {
-	struct flock lock = {
-		.l_type = F_WRLCK,
-		.l_whence = SEEK_SET,
-		.l_start = 0,
-		.l_len = 0,
-	};
 	struct stat held, named;
 
 	slot->lock_fd = open(slot->lock_path, O_RDWR | O_CREAT, S_IRUSR | S_IWUSR);
 	if (slot->lock_fd == -1)
 		return errno;
-	if (fcntl(slot->lock_fd, F_SETLK, &lock) < 0) {
+	// 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
+	// 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) {
 		int saved_errno = errno;
 		close(slot->lock_fd);
 		slot->lock_fd = -1;
diff --git a/tests/t0003-cache.sh b/tests/t0003-cache.shindex bd186e7..06cdd99 100755--- a/tests/t0003-cache.sh+++ b/tests/t0003-cache.sh@@ -79,6 +79,21 @@ test_expect_success 'set up a repo with a page larger than the output buffer' ' 	rm -rf cache2 && mkdir cache2 ' +# The second request lands while the first is inside its filter, which says+# so through a marker file, since a fresh binary can take a moment to start.+overlap_requests() {+	n=0+	rm -f filter-running+	CGIT_CONFIG="$PWD/lockrc" QUERY_STRING="url=bigpage/about/" cgit >first.out 2>first.err &+	while ! test -f filter-running && test $n -lt 10+	do+		sleep 1+		n=$((n + 1))+	done+	CGIT_CONFIG="$PWD/lockrc" QUERY_STRING="url=bigpage/log/" cgit >second.out 2>second.err+	wait+}+ bigpage_query() { 	CGIT_CONFIG="$PWD/bigrc" QUERY_STRING="$1" cgit }@@ -243,4 +258,27 @@ test_expect_success 'a failed final write still delivers the page' ' 	! grep "called many times" flush.err ' +# The fill writes the page into the lock file through stdout, and an exec+# filter puts its own pipe on stdout while it runs. The slot lock has to+# outlast that, or a second request on the slot rebuilds it under the first+# and the first visitor gets the two pages spliced together.+test_expect_success 'a slot stays locked while an exec filter runs' '+	rm -rf cache3 && mkdir cache3 &&+	write_script slow-filter.sh <<-EOF &&+	touch "$PWD/filter-running"+	sleep 3+	cat+	EOF+	{+		echo "cache-size=1" &&+		sed "/^cache-size=/d" bigkeyrc &&+		echo "repo.readme=:file-1" &&+		echo "repo.about-filter=exec:$PWD/slow-filter.sh"+	} >lockrc &&+	overlap_requests &&+	test $(grep -c "^Status:" first.out) = 1 &&+	grep "id=.about." first.out &&+	! grep "Unable to publish" first.err+'+ test_done
diff --git a/tests/t0003-cache.sh b/tests/t0003-cache.sh
index bd186e7..06cdd99 100755
--- a/tests/t0003-cache.sh
+++ b/tests/t0003-cache.sh
@@ -79,6 +79,21 @@ test_expect_success 'set up a repo with a page larger than the output buffer' '
 	rm -rf cache2 && mkdir cache2
 '
 
+# The second request lands while the first is inside its filter, which says
+# so through a marker file, since a fresh binary can take a moment to start.
+overlap_requests() {
+	n=0
+	rm -f filter-running
+	CGIT_CONFIG="$PWD/lockrc" QUERY_STRING="url=bigpage/about/" cgit >first.out 2>first.err &
+	while ! test -f filter-running && test $n -lt 10
+	do
+		sleep 1
+		n=$((n + 1))
+	done
+	CGIT_CONFIG="$PWD/lockrc" QUERY_STRING="url=bigpage/log/" cgit >second.out 2>second.err
+	wait
+}
+
 bigpage_query() {
 	CGIT_CONFIG="$PWD/bigrc" QUERY_STRING="$1" cgit
 }
@@ -243,4 +258,27 @@ test_expect_success 'a failed final write still delivers the page' '
 	! grep "called many times" flush.err
 '
 
+# The fill writes the page into the lock file through stdout, and an exec
+# filter puts its own pipe on stdout while it runs. The slot lock has to
+# outlast that, or a second request on the slot rebuilds it under the first
+# and the first visitor gets the two pages spliced together.
+test_expect_success 'a slot stays locked while an exec filter runs' '
+	rm -rf cache3 && mkdir cache3 &&
+	write_script slow-filter.sh <<-EOF &&
+	touch "$PWD/filter-running"
+	sleep 3
+	cat
+	EOF
+	{
+		echo "cache-size=1" &&
+		sed "/^cache-size=/d" bigkeyrc &&
+		echo "repo.readme=:file-1" &&
+		echo "repo.about-filter=exec:$PWD/slow-filter.sh"
+	} >lockrc &&
+	overlap_requests &&
+	test $(grep -c "^Status:" first.out) = 1 &&
+	grep "id=.about." first.out &&
+	! grep "Unable to publish" first.err
+'
+
 test_done