Harden the request path, scan and error recovery

1 file changed, 43 insertions, 13 deletions

diff --git a/source/filter.c b/source/filter.c
index 1cfdde2..f5fb2d5 100644
--- a/source/filter.c
+++ b/source/filter.c
@@ -22,6 +22,9 @@
 // for a command not found.
 #define EXEC_FAILED 127
 
+// The exec filter holding stdout, for cgit_abort_filters to take it back.
+static struct cgit_exec_filter *running_exec;
+
 static int open_exec_filter(struct cgit_filter *base, va_list ap)
 {
 	struct cgit_exec_filter *filter = (struct cgit_exec_filter *)base;
@@ -31,13 +34,19 @@ static int open_exec_filter(struct cgit_filter *base, va_list ap)
 	for (i = 0; i < filter->base.argument_count; i++)
 		filter->argv[i + 1] = va_arg(ap, char *);
 
-	filter->old_stdout = cgit_die_unless_positive(dup(STDOUT_FILENO), "Unable to duplicate STDOUT");
+	filter->old_stdout = cgit_die_unless_positive(dup(STDOUT_FILENO), "Unable to duplicate stdout");
+	// The child keeps the page's stdout, but not the descriptor the parent
+	// needs it back from.
+	fcntl(filter->old_stdout, F_SETFD, FD_CLOEXEC);
 	cgit_die_unless_zero(pipe(pipefd), "Unable to create pipe to subprocess");
 	filter->pid = cgit_die_unless_non_negative(fork(), "Unable to create subprocess");
 	if (filter->pid == 0) {
 		close(pipefd[1]);
 		if (dup2(pipefd[0], STDIN_FILENO) < 0)
 			_exit(EXEC_FAILED);
+		// cgit ignores SIGPIPE and the disposition survives exec, so a
+		// filter that writes to a closed pipe would otherwise carry on.
+		signal(SIGPIPE, SIG_DFL);
 		execvp(filter->cmd, filter->argv);
 		// The child shares the parent's page buffer, cache lock and exit
 		// handlers, so it must not die through them.
@@ -45,11 +54,9 @@ static int open_exec_filter(struct cgit_filter *base, va_list ap)
 		_exit(EXEC_FAILED);
 	}
 	close(pipefd[0]);
-	// The child keeps the page's stdout, but not the descriptor the parent
-	// needs it back from.
-	fcntl(filter->old_stdout, F_SETFD, FD_CLOEXEC);
-	cgit_die_unless_non_negative(dup2(pipefd[1], STDOUT_FILENO), "Unable to use pipe as STDOUT");
+	cgit_die_unless_non_negative(dup2(pipefd[1], STDOUT_FILENO), "Unable to use pipe as stdout");
 	close(pipefd[1]);
+	running_exec = filter;
 	return 0;
 }
 
@@ -58,7 +65,8 @@ static int close_exec_filter(struct cgit_filter *base)
 	struct cgit_exec_filter *filter = (struct cgit_exec_filter *)base;
 	int i, exit_status = 0;
 
-	cgit_die_unless_non_negative(dup2(filter->old_stdout, STDOUT_FILENO), "Unable to restore STDOUT");
+	running_exec = NULL;
+	cgit_die_unless_non_negative(dup2(filter->old_stdout, STDOUT_FILENO), "Unable to restore stdout");
 	close(filter->old_stdout);
 	if (filter->pid < 0)
 		goto done;
@@ -77,12 +85,14 @@ done:
 static void fprintf_exec_filter(struct cgit_filter *base, FILE *f, const char *prefix)
 {
 	struct cgit_exec_filter *filter = (struct cgit_exec_filter *)base;
+
 	fprintf(f, "%sexec:%s\n", prefix, filter->cmd);
 }
 
 static void cleanup_exec_filter(struct cgit_filter *base)
 {
 	struct cgit_exec_filter *filter = (struct cgit_exec_filter *)base;
+
 	free(filter->argv);
 	filter->argv = NULL;
 	free(filter->cmd);
@@ -141,7 +151,7 @@ void cgit_init_filters(void)
 {
 	libc_write = dlsym(RTLD_NEXT, "write");
 	if (!libc_write)
-		die("Could not locate libc's write function");
+		die("Unable to find libc's write function");
 }
 
 /*
@@ -159,16 +169,16 @@ static inline void hook_write(struct cgit_filter *filter, filter_write_fn write_
 {
 	// Filters cannot nest, because there is one stdout and one hook, so a
 	// second one would strand the first.
-	assert(filter_write == NULL);
-	assert(current_write_filter == NULL);
+	assert(!filter_write);
+	assert(!current_write_filter);
 	current_write_filter = filter;
 	filter_write = write_fn;
 }
 
 static inline void unhook_write(void)
 {
-	assert(filter_write != NULL);
-	assert(current_write_filter != NULL);
+	assert(filter_write);
+	assert(current_write_filter);
 	filter_write = NULL;
 	current_write_filter = NULL;
 }
@@ -322,7 +332,7 @@ static int close_lua_filter(struct cgit_filter *base)
 	lua_getglobal(filter->lua_state, "filter_close");
 	if (lua_pcall(filter->lua_state, 0, 1, 0))
 		die_lua_error(filter);
-	ret = lua_tonumber(filter->lua_state, -1);
+	ret = (int)lua_tointeger(filter->lua_state, -1);
 	lua_pop(filter->lua_state, 1);
 
 	unhook_write();
@@ -368,6 +378,7 @@ int cgit_open_filter(struct cgit_filter *filter, ...)
 {
 	int result;
 	va_list ap;
+
 	if (!filter)
 		return 0;
 	// Whatever is still buffered belongs to the page, not to the filter
@@ -403,6 +414,7 @@ static inline void cleanup_filter(struct cgit_filter *filter)
 void cgit_cleanup_filters(void)
 {
 	int i;
+
 	cleanup_filter(ctx.cfg.about_filter);
 	cleanup_filter(ctx.cfg.commit_filter);
 	cleanup_filter(ctx.cfg.source_filter);
@@ -418,6 +430,24 @@ void cgit_cleanup_filters(void)
 	}
 }
 
+/*
+ * Take stdout back from a filter that holds it when a die lands, so the error
+ * page reaches the visitor and not the filter. What the page had buffered for
+ * the filter is dropped with it, since it was never meant to go out as it is.
+ * An exec child sees the end of its input and exits on its own.
+ */
+void cgit_abort_filters(void)
+{
+	html_discard();
+	if (running_exec) {
+		dup2(running_exec->old_stdout, STDOUT_FILENO);
+		close(running_exec->old_stdout);
+		running_exec = NULL;
+	}
+	if (filter_write)
+		unhook_write();
+}
+
 static const struct {
 	const char *prefix;
 	struct cgit_filter *(*create)(const char *cmd, int argument_count);
@@ -473,5 +503,5 @@ struct cgit_filter *cgit_new_filter(const char *cmd, filter_type filtertype)
 			return filter_specs[i].create(colon + 1, argument_count);
 	}
 
-	die("Invalid filter type: %.*s", (int) len, cmd);
+	die("Invalid filter type: %.*s", (int)len, cmd);
 }