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 <[email protected]> --- 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 <[email protected]>
