Keep clone files and oversized responses out of the cache

The dumb transport reads files that already sit on the disk, so a pack
copied into a slot cost that disk twice and the request a second write
of every byte. A snapshot took a slot whatever its size, so a visitor
naming distinct refs and ids could fill the cache root with archives.
`cache-max-slot-size`, 64 MB unless set, now serves a larger response
from the lock file and drops it, along with any expired copy it would
have replaced.

9 files changed, 124 insertions, 15 deletions

diff --git a/MANUAL.txt b/MANUAL.txt
index d1150ef..0ea60ff 100644
--- a/MANUAL.txt
+++ b/MANUAL.txt
@@ -59,6 +59,12 @@ cache-index-ttl::
 	version of the repository index page. See also: "The cache". Default
 	value: "5".
 
+cache-max-slot-size::
+	Number which specifies the largest response, in kilobytes, that a cache
+	slot may keep. A larger response is still served but not kept, so one
+	archive cannot take a slot's worth of disk. Set to "0" to remove the
+	limit. See also: "The cache". Default value: "65536" (64 MB).
+
 cache-root::
 	Path used to store the cgit cache entries. Default value:
 	"/var/cache/cgit". See also: "Macro expansion" and the note on ownership
@@ -217,7 +223,8 @@ enable-html-serving::
 enable-http-clone::
 	If set to "1", cgit acts as a dumb HTTP endpoint for git clones. Adding
 	"http://$HTTP_HOST$SCRIPT_NAME/$CGIT_REPO_URL" to clone-url exposes it.
-	A site that serves git repositories another way can turn this off.
+	The files it serves come straight off the disk and are never cached. A
+	site that serves git repositories another way can turn this off.
 	Default value: "1".
 
 enable-index-links::
@@ -910,7 +917,9 @@ All cache ttl values are in minutes. Negative ttl values indicate that a page
 type will never expire, and thus the first time a URL is accessed, the result
 will be cached indefinitely, even if the underlying git repository changes.
 Conversely, when a ttl value is zero, the cache is disabled for that particular
-page type.
+page type. The files of the dumb transport are never cached, since they already
+sit on the disk, and a response larger than "cache-max-slot-size" is served but
+not kept.
 
 The cache directory holds one file per slot plus transient lock files, all
 created by cgit itself. Create the directory ahead of time, owned by the account
diff --git a/custom/cgitrc b/custom/cgitrc
index 06106c1..2dd738b 100644
--- a/custom/cgitrc
+++ b/custom/cgitrc
@@ -55,6 +55,11 @@ cache-about-ttl=15
 # 5.
 cache-snapshot-ttl=5
 
+# Largest response, in kilobytes, that a cache slot keeps. A larger one is
+# served but not kept. Value is an integer, and 0 removes the limit. Default is
+# 65536.
+cache-max-slot-size=65536
+
 # Expose the ls_cache page, which lists cache paths and the urls other visitors
 # requested. Values are 0 or 1. Default is 0.
 enable-cache-list=0
diff --git a/source/cache.c b/source/cache.c
index 091c078..8c81f5c 100644
--- a/source/cache.c
+++ b/source/cache.c
@@ -60,6 +60,11 @@ struct cache_slot {
 	// page has already reached the visitor and nothing more may be served
 	// after it, not the lock file and not the stale copy still open.
 	int abandoned;
+	// The largest page a slot may keep, with zero for no bound, and whether
+	// the fill went past it, in which case the lock file is served and then
+	// dropped instead of published.
+	size_t max_bytes;
+	int oversized;
 	// The slot as it was when it was opened, or the lock file once
 	// fill_slot has written a page into it.
 	struct stat st;
@@ -374,6 +379,7 @@ static int fill_slot(struct cache_slot *slot)
 	// it copies after a fill is the lock file rather than the old slot.
 	if (fstat(slot->lock_fd, &slot->st))
 		return errno;
+	slot->oversized = slot->max_bytes && (size_t)slot->st.st_size > slot->max_bytes;
 
 	return 0;
 }
@@ -398,7 +404,14 @@ static void refresh_slot(struct cache_slot *slot)
 		close_lock(slot);
 	} else {
 		close_slot(slot);
-		publish_slot(slot);
+		if (slot->oversized) {
+			// The expired copy goes too, or every later request
+			// would refill it and drop the result again.
+			unlink(slot->path);
+			unlock_slot(slot, 0);
+		} else {
+			publish_slot(slot);
+		}
 		slot->cache_fd = slot->lock_fd;
 	}
 }
@@ -460,7 +473,10 @@ static int process_slot(struct cache_slot *slot)
 	// concurrent writer put there for a different key, so what gets
 	// printed is the descriptor still open on the lock file.
 	slot->cache_fd = slot->lock_fd;
-	publish_slot(slot);
+	if (slot->oversized)
+		unlock_slot(slot, 0);
+	else
+		publish_slot(slot);
 	err = serve_slot(slot);
 	close_slot(slot);
 	return err;
@@ -500,7 +516,8 @@ unsigned long cache_hash_str(const char *str)
 	return h;
 }
 
-int cache_process(int size, const char *path, const char *key, int ttl, cache_fill_fn fn)
+int cache_process(int size, const char *path, const char *key, int ttl, size_t max_bytes,
+	cache_fill_fn fn)
 {
 	unsigned long hash;
 	int i;
@@ -539,6 +556,8 @@ int cache_process(int size, const char *path, const char *key, int ttl, cache_fi
 	slot.ttl = ttl;
 	slot.saved_stdout = -1;
 	slot.abandoned = 0;
+	slot.max_bytes = max_bytes;
+	slot.oversized = 0;
 	slot.path = slot_path.buf;
 	slot.lock_path = lock_path.buf;
 	slot.key = key;
diff --git a/source/cache.h b/source/cache.h
index bae283c..d3a7ec4 100644
--- a/source/cache.h
+++ b/source/cache.h
@@ -7,6 +7,8 @@
 #ifndef CGIT_CACHE_H
 #define CGIT_CACHE_H
 
+#include <stddef.h>
+
 typedef void (*cache_fill_fn)(void);
 
 /*
@@ -14,10 +16,12 @@ typedef void (*cache_fill_fn)(void);
  * slot for that key is there and rendering it through fn when it is not. size
  * is how many slots the cache may use and path is the directory holding them.
  * ttl is how many minutes a slot for this key stays fresh, where a negative
- * ttl never expires and a ttl of zero skips the cache for this request.
+ * ttl never expires and a ttl of zero skips the cache for this request. A
+ * page larger than max_bytes is served but not kept, and zero sets no bound.
  * Returns 0 when the page was written, and an errno value when it was not.
  */
-extern int cache_process(int size, const char *path, const char *key, int ttl, cache_fill_fn fn);
+extern int cache_process(int size, const char *path, const char *key, int ttl,
+	size_t max_bytes, cache_fill_fn fn);
 
 /*
  * Write one line per cache slot to stdout, giving its path, modification time,
diff --git a/source/cgit.c b/source/cgit.c
index 5806293..3e47dbe 100644
--- a/source/cgit.c
+++ b/source/cgit.c
@@ -100,6 +100,9 @@ static void prepare_context(void)
 	ctx.cfg.cache_scan_ttl = 15;
 	ctx.cfg.cache_dynamic_ttl = 5;
 	ctx.cfg.cache_static_ttl = -1;
+	// Counted in kilobytes, so sixty-four megabytes, past which a response
+	// is served but not kept.
+	ctx.cfg.cache_max_slot_size = 64 * 1024;
 	ctx.cfg.case_sensitive_sort = 1;
 	ctx.cfg.branch_sort = 0;
 	ctx.cfg.commit_sort = 0;
@@ -608,6 +611,8 @@ static void apply_config(const char *name, const char *value)
 		ctx.cfg.cache_about_ttl = atoi(value);
 	else if (!strcmp(name, "cache-snapshot-ttl"))
 		ctx.cfg.cache_snapshot_ttl = atoi(value);
+	else if (!strcmp(name, "cache-max-slot-size"))
+		ctx.cfg.cache_max_slot_size = atoi(value);
 	else if (!strcmp(name, "case-sensitive-sort"))
 		ctx.cfg.case_sensitive_sort = atoi(value);
 	else if (!strcmp(name, "about-filter"))
@@ -908,12 +913,20 @@ static int page_is_static(void)
  */
 static int calc_ttl(void)
 {
+	const struct cgit_cmd *cmd;
+
 	if (!ctx.repo)
 		return ctx.cfg.cache_index_ttl;
 
 	if (!ctx.qry.page)
 		return ctx.cfg.cache_summary_ttl;
 
+	// The dumb transport serves files that already sit on the disk, so a
+	// copy in a slot would cost that disk twice and gain nothing.
+	cmd = cgit_find_cmd(ctx.qry.page);
+	if (cmd && cmd->is_clone)
+		return 0;
+
 	if (!strcmp(ctx.qry.page, "about"))
 		return ctx.cfg.cache_about_ttl;
 
@@ -1304,6 +1317,7 @@ int cmd_main(int argc, const char **argv)
 {
 	struct strbuf cache_key = STRBUF_INIT;
 	const char *path;
+	size_t max_bytes = 0;
 	int err, ttl;
 
 	isolate_git_environment();
@@ -1357,8 +1371,12 @@ int cmd_main(int argc, const char **argv)
 	if (!ctx.env.authenticated || (ctx.env.request_method && !strcmp(ctx.env.request_method, "HEAD")))
 		ctx.cfg.cache_size = 0;
 
+	// Written in kilobytes in cgitrc, where zero and below lift the bound.
+	if (ctx.cfg.cache_max_slot_size > 0)
+		max_bytes = (size_t)ctx.cfg.cache_max_slot_size * 1024;
 	build_cache_key(&cache_key);
-	err = cache_process(ctx.cfg.cache_size, ctx.cfg.cache_root, cache_key.buf, ttl, process_request);
+	err = cache_process(ctx.cfg.cache_size, ctx.cfg.cache_root, cache_key.buf, ttl,
+		max_bytes, process_request);
 	strbuf_release(&cache_key);
 
 	cgit_cleanup_filters();
diff --git a/source/cgit.h b/source/cgit.h
index 3d3b154..b863e7b 100644
--- a/source/cgit.h
+++ b/source/cgit.h
@@ -223,6 +223,7 @@ struct cgit_config {
 	int cache_summary_ttl;
 	int cache_about_ttl;
 	int cache_snapshot_ttl;
+	int cache_max_slot_size;
 	int case_sensitive_sort;
 	int embedded;
 	int trust_scan_config;
diff --git a/source/cmd.c b/source/cmd.c
index a19aa50..38fe596 100644
--- a/source/cmd.c
+++ b/source/cmd.c
@@ -208,19 +208,23 @@ static const struct cgit_cmd commands[] = {
 	{ .name = "tree", .fn = tree_fn, .want_repo = 1, .want_vpath = 1 },
 };
 
-const struct cgit_cmd *cgit_get_cmd(void)
+const struct cgit_cmd *cgit_find_cmd(const char *name)
 {
 	size_t i;
 
+	for (i = 0; i < ARRAY_SIZE(commands); i++)
+		if (!strcmp(name, commands[i].name))
+			return &commands[i];
+	return NULL;
+}
+
+const struct cgit_cmd *cgit_get_cmd(void)
+{
 	if (!ctx.qry.page) {
 		if (ctx.repo)
 			ctx.qry.page = "summary";
 		else
 			ctx.qry.page = "repolist";
 	}
-
-	for (i = 0; i < ARRAY_SIZE(commands); i++)
-		if (!strcmp(ctx.qry.page, commands[i].name))
-			return &commands[i];
-	return NULL;
+	return cgit_find_cmd(ctx.qry.page);
 }
diff --git a/source/cmd.h b/source/cmd.h
index f580fa1..f47878b 100644
--- a/source/cmd.h
+++ b/source/cmd.h
@@ -17,7 +17,13 @@ struct cgit_cmd {
 };
 
 /*
- * The entry naming ctx.qry.page, or NULL when no page goes by that name.
+ * The entry for the page named, or NULL when no page goes by that name.
+ */
+extern const struct cgit_cmd *cgit_find_cmd(const char *name);
+
+/*
+ * The entry naming ctx.qry.page, after filling in the default page for the
+ * request, or NULL when no page goes by that name.
  */
 extern const struct cgit_cmd *cgit_get_cmd(void);
 
diff --git a/tests/t0003-cache.sh b/tests/t0003-cache.sh
index d3e448f..3e2a971 100755
--- a/tests/t0003-cache.sh
+++ b/tests/t0003-cache.sh
@@ -169,4 +169,47 @@ test_expect_success 'an error page leaves no slot behind' '
 	test_line_count = 0 error.slots
 '
 
+# The dumb transport serves files that already sit on the disk, so a copy in
+# a slot would only double the disk they take.
+test_expect_success 'a clone file leaves no slot behind' '
+	rm -rf cache3 && mkdir cache3 &&
+	CGIT_CONFIG="$PWD/bigkeyrc" QUERY_STRING="url=bigpage/info/refs" cgit >clone.out &&
+	grep "refs/heads/master" clone.out &&
+	CGIT_CONFIG="$PWD/bigkeyrc" QUERY_STRING="url=bigpage/objects/info/packs" cgit >packs.out &&
+	grep "^Status: 200" packs.out &&
+	ls cache3 >clone.slots &&
+	test_line_count = 0 clone.slots
+'
+
+# A response past cache-max-slot-size is served whole but kept nowhere, both
+# on a first fill and when it would have replaced an expired slot, which is
+# dropped along with it. The summary page stays under the limit set here
+# while the big blob page is well past it.
+test_expect_success 'a response over cache-max-slot-size is served but not kept' '
+	rm -rf cache3 && mkdir cache3 &&
+	{
+		echo "cache-max-slot-size=32" &&
+		cat bigkeyrc
+	} >slotsizerc &&
+	CGIT_CONFIG="$PWD/slotsizerc" QUERY_STRING="url=bigpage/tree/big.txt" cgit >big.unkept &&
+	tail -c 200 big.unkept | grep "</html>" &&
+	ls cache3 >unkept.slots &&
+	test_line_count = 0 unkept.slots &&
+	CGIT_CONFIG="$PWD/slotsizerc" QUERY_STRING="url=bigpage/" cgit >/dev/null &&
+	ls cache3 >kept.slots &&
+	test_line_count = 1 kept.slots
+'
+
+test_expect_success 'an expired slot is dropped when its refill is too large' '
+	rm -rf cache3 && mkdir cache3 &&
+	CGIT_CONFIG="$PWD/bigkeyrc" QUERY_STRING="url=bigpage/tree/big.txt" cgit >/dev/null &&
+	ls cache3 >filled.slots &&
+	test_line_count = 1 filled.slots &&
+	touch -t 200001010000 cache3/* &&
+	CGIT_CONFIG="$PWD/slotsizerc" QUERY_STRING="url=bigpage/tree/big.txt" cgit >big.refill &&
+	tail -c 200 big.refill | grep "</html>" &&
+	ls cache3 >refill.slots &&
+	test_line_count = 0 refill.slots
+'
+
 test_done