Harden the page renderers

1 file changed, 23 insertions, 12 deletions

diff --git a/source/ui-patch.c b/source/ui-patch.c
index baa87c9..1dcdfbe 100644
--- a/source/ui-patch.c
+++ b/source/ui-patch.c
@@ -35,7 +35,7 @@ static int resolve_range(const char *new_rev, const char *old_rev,
 	}
 	commit = lookup_commit_reference(the_repository, new_oid);
 	if (!commit) {
-		cgit_print_error_page(404, "Not Found", "Bad commit reference: %s", new_rev);
+		cgit_print_error_page(404, "Not Found", "Not a commit: %s", new_rev);
 		return -1;
 	}
 
@@ -45,7 +45,7 @@ static int resolve_range(const char *new_rev, const char *old_rev,
 			return -1;
 		}
 		if (!lookup_commit_reference(the_repository, old_oid)) {
-			cgit_print_error_page(404, "Not Found", "Bad commit reference: %s", old_rev);
+			cgit_print_error_page(404, "Not Found", "Not a commit: %s", old_rev);
 			return -1;
 		}
 	} else if (commit->parents && commit->parents->item) {
@@ -75,6 +75,13 @@ void cgit_print_patch(const char *new_rev, const char *old_rev, const char *pref
 	if (!new_rev)
 		new_rev = ctx.qry.head;
 
+	// A leading colon makes git read the path as a pathspec with magic,
+	// which can end the request inside git after the headers are out.
+	if (prefix && prefix[0] == ':') {
+		cgit_print_error_page(400, "Bad Request", "Invalid path");
+		return;
+	}
+
 	if (resolve_range(new_rev, old_rev, &new_oid, &old_oid))
 		return;
 
@@ -84,10 +91,6 @@ void cgit_print_patch(const char *new_rev, const char *old_rev, const char *pref
 		xsnprintf(rev_range, REV_RANGE_LEN, "%s..%s", oid_to_hex(&old_oid), oid_to_hex(&new_oid));
 	}
 
-	ctx.page.mimetype = "text/plain";
-	ctx.page.filename = cgit_fmt("%s.patch", rev_range);
-	cgit_print_http_headers();
-
 	if (!ctx.cfg.enable_plain_email) {
 		rev_argv[FORMAT_ARG] =
 			"--format=format:From %H Mon Sep 17 00:00:00 2001%n"
@@ -101,11 +104,10 @@ void cgit_print_patch(const char *new_rev, const char *old_rev, const char *pref
 	rev.show_root_diff = 1;
 	rev.max_parents = 1;
 	rev.diffopt.output_format |= DIFF_FORMAT_DIFFSTAT | DIFF_FORMAT_PATCH | DIFF_FORMAT_SUMMARY;
+	// Allocated rather than formatted into cgit_fmt's fixed buffer, because
+	// the path comes from the request and a long one would abort the
+	// process.
 	if (prefix)
-		// Allocated rather than formatted into cgit_fmt's fixed
-		// buffer, because the path comes from the request and a long
-		// one would abort the process here, with the headers for a
-		// successful response already on the wire.
 		rev.diffopt.stat_sep = cgit_fmtalloc("(limited to '%s')\n\n", prefix);
 	setup_revisions(rev_argc, rev_argv, &rev, NULL);
 	// A single commit resolves to a range starting at its parent, so this
@@ -113,9 +115,18 @@ void cgit_print_patch(const char *new_rev, const char *old_rev, const char *pref
 	// emitting a patch for the whole history.
 	if (ctx.cfg.max_patch_count > 0)
 		rev.max_count = ctx.cfg.max_patch_count;
-	prepare_revision_walk(&rev);
+	// A failed setup leaves the walk holding freed commits, so it must
+	// not be read from, and the headers wait until it has succeeded.
+	if (prepare_revision_walk(&rev)) {
+		cgit_print_error_page(500, "Internal Server Error", "Unable to read the history");
+		return;
+	}
+
+	ctx.page.mimetype = "text/plain";
+	ctx.page.filename = cgit_fmt("%s.patch", rev_range);
+	cgit_print_http_headers();
 
-	while ((commit = get_revision(&rev)) != NULL) {
+	while ((commit = get_revision(&rev))) {
 		log_tree_commit(&rev, commit);
 		// Two dashes and a space is the mail signature separator, so
 		// git am and mail readers drop the version note below rather