[RFC/PATCH] daemon, tag, verify-tag: do not pass ignored signals to child (Re: Scripted clone generating an incomplete, unusable .git/config)

[Jonathan Nieder <jrnieder@gmail.com>]

Jonathan Nieder wrote:

> Currently git daemon uses SIG_IGN state on SIGTERM to protect
> children with active connections.  Why isn't that causing the same
> sort of problems as os.popen() causes?

It's late so please do not trust me, but I think the following would
fix that.

-- 8< --
Subject: daemon, tag, verify-tag: do not pass ignored signals to child

It is bad practice to have signals ignored or blocked while creating a
child process, since to do so triggers not-so-well-tested code paths
in many programs.

tag and verify-tag block SIGPIPE to avoid termination from writing
after gpg fails and closes its pipe early.  Ignoring SIGPIPE in the
child is an unintended side-effect; avoid it by narrowing the scope
of the request to ignore SIGPIPE to encompass only the write() (and
in particular, not the fork()).

Connection handling threads in daemon block SIGTERM to avoid
termination of active connections when the number of connections gets
too high.  Use a signal handling function instead of SIG_IGN to
avoid passing the ignored signal to the child.  Ignoring SIGTERM in
the request-handling child is not necessary, since kill_some_child()
never tries to kill those.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Needs tests.

 builtin/tag.c        |   11 +++++++----
 builtin/verify-tag.c |   10 +++++++---
 daemon.c             |    6 +++++-
 3 files changed, 19 insertions(+), 8 deletions(-)

diff --git a/builtin/tag.c b/builtin/tag.c
index d311491..efc9b93 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -173,10 +173,6 @@ static int do_sign(struct strbuf *buffer)
 			bracket[1] = '\0';
 	}
 
-	/* When the username signingkey is bad, program could be terminated
-	 * because gpg exits without reading and then write gets SIGPIPE. */
-	signal(SIGPIPE, SIG_IGN);
-
 	memset(&gpg, 0, sizeof(gpg));
 	gpg.argv = args;
 	gpg.in = -1;
@@ -189,9 +185,14 @@ static int do_sign(struct strbuf *buffer)
 	if (start_command(&gpg))
 		return error("could not run gpg.");
 
+	/* When the username signingkey is bad, program could be terminated
+	 * because gpg exits without reading and then write gets SIGPIPE. */
+	sigchain_push(SIGPIPE, SIG_IGN);
+
 	if (write_in_full(gpg.in, buffer->buf, buffer->len) != buffer->len) {
 		close(gpg.in);
 		close(gpg.out);
+		sigchain_pop(SIGPIPE);
 		finish_command(&gpg);
 		return error("gpg did not accept the tag data");
 	}
@@ -199,6 +200,8 @@ static int do_sign(struct strbuf *buffer)
 	len = strbuf_read(buffer, gpg.out, 1024);
 	close(gpg.out);
 
+	sigchain_pop(SIGPIPE);
+
 	if (finish_command(&gpg) || !len || len < 0)
 		return error("gpg failed to sign the tag");
 
diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c
index 9f482c2..5361017 100644
--- a/builtin/verify-tag.c
+++ b/builtin/verify-tag.c
@@ -54,8 +54,15 @@ static int run_gpg_verify(const char *buf, unsigned long size, int verbose)
 		return error("could not run gpg.");
 	}
 
+	/* sometimes the program was terminated because this signal
+	 * was received in the process of writing the gpg input: */
+	sigchain_push(SIGPIPE, ignore_signal);
+
 	write_in_full(gpg.in, buf, len);
 	close(gpg.in);
+
+	sigchain_pop(SIGPIPE);
+
 	ret = finish_command(&gpg);
 
 	unlink_or_warn(path);
@@ -104,9 +111,6 @@ int cmd_verify_tag(int argc, const char **argv, const char *prefix)
 	if (argc <= i)
 		usage_with_options(verify_tag_usage, verify_tag_options);
 
-	/* sometimes the program was terminated because this signal
-	 * was received in the process of writing the gpg input: */
-	signal(SIGPIPE, SIG_IGN);
 	while (i < argc)
 		if (verify_tag(argv[i++], verbose))
 			had_error = 1;
diff --git a/daemon.c b/daemon.c
index 7719f33..ccc560b 100644
--- a/daemon.c
+++ b/daemon.c
@@ -243,6 +243,10 @@ static int git_daemon_config(const char *var, const char *value, void *cb)
 	return 0;
 }
 
+static void ignore_termination_signal(int sig)
+{
+}
+
 static int run_service(char *dir, struct daemon_service *service)
 {
 	const char *path;
@@ -294,7 +298,7 @@ static int run_service(char *dir, struct daemon_service *service)
 	 * We'll ignore SIGTERM from now on, we have a
 	 * good client.
 	 */
-	signal(SIGTERM, SIG_IGN);
+	signal(SIGTERM, ignore_termination_signal);
 
 	return service->fn();
 }
-- 
1.7.2.3.557.gab647.dirty