[PATCH 1/2] cache: don't fill a slot we no longer own
Konstantin Ryabitsev
konstantin at linuxfoundation.org
Tue Aug 25 03:34:42 UTC 2026
lock_slot() opens the lock file and only then acquires the fcntl lock.
Those two steps are not atomic, so between them the process that held
the lock can rename that same file over the cache slot and exit. The
lock we subsequently acquire is then held on the live cache file rather
than on a lock file, and lock_name no longer refers to it at all.
Filling that descriptor rewrites a cache slot that other processes are
streaming, and the closing rename() fails with ENOENT because lock_name
is gone -- a failure no caller checks. Until "cache: truncate lock file
before filling" the slot was not truncated first, so a short response
written over a long one left the tail of the previous occupant in place;
fstat() in fill_slot() reported the combined size and print_slot() sent
all of it. With snapshot tarballs sharing the slot space, that tail can
be hundreds of megabytes of binary data appended to an HTML page, and it
persists until the slot expires.
Verify after taking the lock that our descriptor still refers to the
file lock_name points at, and give up with EAGAIN if it does not. The
caller then serves stale content or generates uncached, both of which
are safe.
While here, stop print_slot() from spinning forever when sendfile()
reports EOF before cache_st.st_size -- reachable once a slot can be
truncated underneath a reader -- report a failed publish instead of
discarding it, and release the lock file on the ftruncate()/xwrite()
error paths instead of leaking it.
Add a test that plants a stale, oversized slot and checks that neither
the served page nor the slot left behind inherits any of it.
Assisted-by: LLM [analysis, codegen, tests]
Signed-off-by: Konstantin Ryabitsev <konstantin at linuxfoundation.org>
---
cache.c | 47 +++++++++++++++++++++++++++++++++++++----
tests/t0021-cache-slot-reuse.sh | 46 ++++++++++++++++++++++++++++++++++++++++
2 files changed, 89 insertions(+), 4 deletions(-)
diff --git a/cache.c b/cache.c
index e70af13..ebc7de2 100644
--- a/cache.c
+++ b/cache.c
@@ -108,6 +108,13 @@ static int print_slot(struct cache_slot *slot)
}
if (off == size)
return 0;
+ if (ret == 0) {
+ /* EOF before cache_st.st_size: the slot was truncated
+ * while we were streaming it. Stop instead of asking
+ * for the same missing bytes forever.
+ */
+ return EIO;
+ }
} while (1);
#endif
@@ -174,6 +181,8 @@ static int lock_slot(struct cache_slot *slot)
.l_start = 0,
.l_len = 0,
};
+ struct stat st_fd, st_name;
+ int err;
slot->lock_fd = open(slot->lock_name, O_RDWR | O_CREAT,
S_IRUSR | S_IWUSR);
@@ -185,11 +194,36 @@ static int lock_slot(struct cache_slot *slot)
slot->lock_fd = -1;
return saved_errno;
}
+
+ /* Between our open() and our F_SETLK, the process that held the lock
+ * may have renamed this very file over the cache slot and exited. In
+ * that case the lock we just took is on the *live cache file*, not on
+ * a lock file, and filling it would rewrite a slot that concurrent
+ * readers are streaming. Only proceed if our descriptor still refers
+ * to the file that lock_name points at.
+ */
+ if (fstat(slot->lock_fd, &st_fd) || stat(slot->lock_name, &st_name) ||
+ st_fd.st_dev != st_name.st_dev || st_fd.st_ino != st_name.st_ino) {
+ close(slot->lock_fd);
+ slot->lock_fd = -1;
+ return EAGAIN;
+ }
+
if (ftruncate(slot->lock_fd, 0) < 0)
- return errno;
+ goto out_err;
if (xwrite(slot->lock_fd, slot->key, slot->keylen + 1) < 0)
- return errno;
+ goto out_err;
return 0;
+
+out_err:
+ /* We own lock_name, so don't leave a half-written lock file (and a
+ * leaked descriptor) behind for the next process to trip over.
+ */
+ err = errno;
+ unlink(slot->lock_name);
+ close(slot->lock_fd);
+ slot->lock_fd = -1;
+ return err;
}
/* Release the current lockfile. If `replace_old_slot` is set the
@@ -297,7 +331,10 @@ static int process_slot(struct cache_slot *slot)
close_lock(slot);
} else {
close_slot(slot);
- unlock_slot(slot, 1);
+ if ((err = unlock_slot(slot, 1)) != 0)
+ cache_log("[cgit] Unable to publish slot %s: %s (%d)\n",
+ slot->lock_name,
+ strerror(err), err);
slot->cache_fd = slot->lock_fd;
}
}
@@ -343,7 +380,9 @@ static int process_slot(struct cache_slot *slot)
// Lets avoid such a race by just printing the content of
// the lock file.
slot->cache_fd = slot->lock_fd;
- unlock_slot(slot, 1);
+ if ((err = unlock_slot(slot, 1)) != 0)
+ cache_log("[cgit] Unable to publish slot %s: %s (%d)\n",
+ slot->lock_name, strerror(err), err);
if ((err = print_slot(slot)) != 0) {
cache_log("[cgit] error printing cache %s: %s (%d)\n",
slot->cache_name,
diff --git a/tests/t0021-cache-slot-reuse.sh b/tests/t0021-cache-slot-reuse.sh
new file mode 100755
index 0000000..e6dd374
--- /dev/null
+++ b/tests/t0021-cache-slot-reuse.sh
@@ -0,0 +1,46 @@
+#!/bin/sh
+
+test_description='Verify that a cache slot never serves stale content'
+. ./setup.sh
+
+# With cache-size=1 every key hashes onto the same slot, which makes the
+# slot filename deterministic and lets us plant content in it.
+slot=cache/00000000
+lock=$slot.lock
+junk=JUNKJUNKJUNKJUNKJUNKJUNKJUNK
+
+# Simulate a slot left behind by a process that was killed while filling
+# it: a key that does not match, followed by far more content than the
+# page we are about to generate.
+plant_junk() {
+ printf 'stale-key\0' >"$1" &&
+ for i in $(test_seq 1 2000)
+ do
+ echo "$junk"
+ done >>"$1"
+}
+
+test_expect_success 'setup' '
+ rm -f cache/* &&
+ sed -e "s/^cache-size=.*/cache-size=1/" cgitrc >cgitrc.tmp &&
+ mv -f cgitrc.tmp cgitrc &&
+ cgit_url "foo/refs" >/dev/null &&
+ test -f "$slot"
+'
+
+test_expect_success 'a stale lock file is not inherited by the next page' '
+ rm -f cache/* &&
+ plant_junk "$lock" &&
+ cgit_url "foo/refs" >output &&
+ ! grep -q "$junk" output &&
+ test "$(tail -n 1 output)" = "</html>"
+'
+
+test_expect_success 'no stale content is left behind in the slot' '
+ ! grep -q "$junk" "$slot" &&
+ cgit_url "foo/refs" >output.cached &&
+ ! grep -q "$junk" output.cached &&
+ test "$(tail -n 1 output.cached)" = "</html>"
+'
+
+test_done
--
2.55.0
More information about the CGit
mailing list