[PATCH 0/2] cache: fix corrupted pages served from the on-disk cache

[Konstantin Ryabitsev]

Every so often cgit serves a page from its file cache with unrelated
data appended after the closing </html> -- anywhere from a few stray
links and a footer of somebody else's source code to hundreds of
megabytes of binary junk that pins the browser at 100% CPU. It clears up
on its own once the slot expires, which is a large part of why it has
been hard to catch in the act. Reports of this go back years.

The cause is a TOCTOU in lock_slot(). It opens the lock file and only
then takes the fcntl lock, and those two steps are not atomic. In
between, the process holding the lock can rename that same file over the
cache slot and exit. The lock subsequently acquired is then held on the
live cache file, lock_name no longer refers to it, and filling it
rewrites a slot that other processes are streaming. The closing rename()
fails with ENOENT, which no caller checks.

Patch 1 has lock_slot() confirm, after taking the lock, that its
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. It also fixes three smaller
things noticed along the way: print_slot() spins forever if sendfile()
reports EOF before the size fstat() promised, a failed publish is
discarded silently, and the ftruncate()/xwrite() error paths leak the
lock file and its descriptor.

Patch 2 adds a CGIT_TEST_LOCK_DELAY hook so the suite can drive the race
directly -- the interleaving happens inside cgit's own execution and
cannot be arranged from the outside. It is split out so the fix can go
in on its own if a test hook in lock_slot() is unwelcome, and its commit
message notes the two seconds of wall clock it costs.

On what is and is not established here: the race is real, it
reproduces standalone in C, and the fix demonstrably closes it. That
this particular race is what produced the reports is inference rather
than confirmation -- no corrupted slot was ever captured while it was
still live. The circumstantial fit is good: the sizes involved, the
binary tail, snapshot tarballs sharing the slot space as the obvious
donor, and the self-healing after roughly a TTL. But if it recurs
after this lands, the thing to grab is the slot file itself before it
expires; the key stored at the front of it will say immediately
whether it is this or something else.

Note, this was largely written by an LLM because I'm not great at
writing C, but I've carefully reviewed the results and they look sane to
me. I'm not sure the solution in the 2nd commit is that great, so feel
free to drop it if you think carrying an env-triggered delay in the
built binary is a no-go.

Signed-off-by: Konstantin Ryabitsev <konstantin at linuxfoundation.org>
---
Konstantin Ryabitsev (2):
      cache: don't fill a slot we no longer own
      tests: exercise the lock_slot() rename race directly

 cache.c                         | 53 ++++++++++++++++++++++++++++++---
 tests/t0021-cache-slot-reuse.sh | 66 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 115 insertions(+), 4 deletions(-)
---
base-commit: 044821677c774cd24f25f1818ea51d09cc64b006
change-id: 20260825-fix-cache-slot-race-e9dc27e02f20

Best regards,
--  
Konstantin Ryabitsev <konstantin at linuxfoundation.org>