[PATCH] receive-pack: interrupt pre-receive when client disconnects

When hitting ctrl-c on the client while a remote pre-receive hook is
running, receive-pack is not killed by SIGPIPE because the signal is
ignored. This is a side effect of commit ec7dbd145bd8 ("receive-pack:
allow hooks to ignore its standard input stream").

The pre-receive hook itself is not interrupted and does not receive any
error since its stdout is a pipe which is read in an async thread and
output back to the client socket in a side band channel.

After the pre-receive has exited the SIGPIPE default handler is restored
and if the hook did not report any error, objects are migrated from
temporary to permanent storage.

This can be confusing for most people and may even be considered a bug.
When receive-pack cannot forward pre-receive output to the client, do
not ignore the error and kill the hook process so that the push does not
complete.

Signed-off-by: Robin Jarry <redacted>
---
Note that if a pre-receive hook does not produce any output, any
disconnection of the client will not cause the hook to be killed. This
is not ideal but as far as I can see, there is no way to check if the
client is alive without writing in the side band channel.

 builtin/receive-pack.c | 55 ++++++++++++++++++++++++++++++++++++------
 sideband.c             | 31 +++++++++++++++++++++---
 sideband.h             |  4 +++
 3 files changed, 79 insertions(+), 11 deletions(-)

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 9f4a0b816cf9..0f41fe8c6a85 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c

@@ -469,6 +469,7 @@ static int copy_to_sideband(int in, int out, void *arg)
 {
 	char data[128];
 	int keepalive_active = 0;
+	struct child_process *proc = arg;
 
 	if (keepalive_in_sec <= 0)
 		use_keepalive = KEEPALIVE_NEVER;

@@ -494,7 +495,11 @@ static int copy_to_sideband(int in, int out, void *arg)
 			} else if (ret == 0) {
 				/* no data; send a keepalive packet */
 				static const char buf[] = "0005\1";
-				write_or_die(1, buf, sizeof(buf) - 1);
+				if (proc && proc->pid > 0) {
+					if (write_in_full(1, buf, sizeof(buf) - 1) < 0)
+						goto error;
+				} else
+					write_or_die(1, buf, sizeof(buf) - 1);
 				continue;
 			} /* else there is actual data to read */
 		}

@@ -512,8 +517,21 @@ static int copy_to_sideband(int in, int out, void *arg)
 				 * with it.
 				 */
 				keepalive_active = 1;
-				send_sideband(1, 2, data, p - data, use_sideband);
-				send_sideband(1, 2, p + 1, sz - (p - data + 1), use_sideband);
+				if (proc && proc->pid > 0) {
+					if (send_sideband2(1, 2, data, p - data,
+							   use_sideband) < 0)
+						goto error;
+					if (send_sideband2(1, 2, p + 1,
+							   sz - (p - data + 1),
+							   use_sideband) < 0)
+						goto error;
+				} else {
+					send_sideband(1, 2, data, p - data,
+						      use_sideband);
+					send_sideband(1, 2, p + 1,
+						      sz - (p - data + 1),
+						      use_sideband);
+				}
 				continue;
 			}
 		}

@@ -522,10 +540,24 @@ static int copy_to_sideband(int in, int out, void *arg)
 		 * Either we're not looking for a NUL signal, or we didn't see
 		 * it yet; just pass along the data.
 		 */
-		send_sideband(1, 2, data, sz, use_sideband);
+		if (proc && proc->pid > 0) {
+			if (send_sideband2(1, 2, data, sz, use_sideband) < 0)
+				goto error;
+		} else
+			send_sideband(1, 2, data, sz, use_sideband);
 	}
 	close(in);
 	return 0;
+error:
+	close(in);
+	if (proc && proc->pid > 0) {
+		/*
+		 * SIGPIPE would be more relevant but we want to make sure that
+		 * the hook does not ignore the signal.
+		 */
+		kill(proc->pid, SIGKILL);
+	}
+	return -1;
 }
 
 static void hmac_hash(unsigned char *out,

@@ -809,7 +841,8 @@ struct receive_hook_feed_state {
 };
 
 typedef int (*feed_fn)(void *, const char **, size_t *);
-static int run_and_feed_hook(const char *hook_name, feed_fn feed,
+static int run_and_feed_hook(const char *hook_name,
+			     int isolate_sigpipe, feed_fn feed,
 			     struct receive_hook_feed_state *feed_state)
 {
 	struct child_process proc = CHILD_PROCESS_INIT;

@@ -842,6 +875,10 @@ static int run_and_feed_hook(const char *hook_name, feed_fn feed,
 	if (use_sideband) {
 		memset(&muxer, 0, sizeof(muxer));
 		muxer.proc = copy_to_sideband;
+		if (isolate_sigpipe)
+			muxer.data = NULL;
+		else
+			muxer.data = &proc;
 		muxer.in = -1;
 		code = start_async(&muxer);
 		if (code)

@@ -922,6 +959,7 @@ static int feed_receive_hook(void *state_, const char **bufp, size_t *sizep)
 static int run_receive_hook(struct command *commands,
 			    const char *hook_name,
 			    int skip_broken,
+			    int isolate_sigpipe,
 			    const struct string_list *push_options)
 {
 	struct receive_hook_feed_state state;

@@ -935,7 +973,8 @@ static int run_receive_hook(struct command *commands,
 		return 0;
 	state.cmd = commands;
 	state.push_options = push_options;
-	status = run_and_feed_hook(hook_name, feed_receive_hook, &state);
+	status = run_and_feed_hook(hook_name, isolate_sigpipe,
+				   feed_receive_hook, &state);
 	strbuf_release(&state.buf);
 	return status;
 }

@@ -1963,7 +2002,7 @@ static void execute_commands(struct command *commands,
 		}
 	}
 
-	if (run_receive_hook(commands, "pre-receive", 0, push_options)) {
+	if (run_receive_hook(commands, "pre-receive", 0, 0, push_options)) {
 		for (cmd = commands; cmd; cmd = cmd->next) {
 			if (!cmd->error_string)
 				cmd->error_string = "pre-receive hook declined";

@@ -2566,7 +2605,7 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)
 		else if (report_status)
 			report(commands, unpack_status);
 		sigchain_pop(SIGPIPE);
-		run_receive_hook(commands, "post-receive", 1,
+		run_receive_hook(commands, "post-receive", 1, 1,
 				 &push_options);
 		run_update_post_hook(commands);
 		string_list_clear(&push_options, 0);

diff --git a/sideband.c b/sideband.c
index 85bddfdcd4f5..27f8d653eb24 100644
--- a/sideband.c
+++ b/sideband.c

@@ -247,11 +247,25 @@ int demultiplex_sideband(const char *me, int status,
 	return 1;
 }
 
+static int send_sideband_priv(int fd, int band, const char *data, ssize_t sz,
+			      int packet_max, int ignore_errors);
+
 /*
  * fd is connected to the remote side; send the sideband data
  * over multiplexed packet stream.
  */
 void send_sideband(int fd, int band, const char *data, ssize_t sz, int packet_max)
+{
+	(void)send_sideband_priv(fd, band, data, sz, packet_max, 1);
+}
+
+int send_sideband2(int fd, int band, const char *data, ssize_t sz, int packet_max)
+{
+	return send_sideband_priv(fd, band, data, sz, packet_max, 0);
+}
+
+static int send_sideband_priv(int fd, int band, const char *data, ssize_t sz,
+			      int packet_max, int ignore_errors)
 {
 	const char *p = data;
 

@@ -265,13 +279,24 @@ void send_sideband(int fd, int band, const char *data, ssize_t sz, int packet_ma
 		if (0 <= band) {
 			xsnprintf(hdr, sizeof(hdr), "%04x", n + 5);
 			hdr[4] = band;
-			write_or_die(fd, hdr, 5);
+			if (ignore_errors)
+				write_or_die(fd, hdr, 5);
+			else if (write_in_full(fd, hdr, 5) < 0)
+				return -1;
 		} else {
 			xsnprintf(hdr, sizeof(hdr), "%04x", n + 4);
-			write_or_die(fd, hdr, 4);
+			if (ignore_errors)
+				write_or_die(fd, hdr, 4);
+			else if (write_in_full(fd, hdr, 4) < 0)
+				return -1;
 		}
-		write_or_die(fd, p, n);
+		if (ignore_errors)
+			write_or_die(fd, p, n);
+		else if (write_in_full(fd, p, n) < 0)
+			return -1;
 		p += n;
 		sz -= n;
 	}
+
+	return 0;
 }

diff --git a/sideband.h b/sideband.h
index 5a25331be55d..cb92777418e1 100644
--- a/sideband.h
+++ b/sideband.h

@@ -29,5 +29,9 @@ int demultiplex_sideband(const char *me, int status,
 			 enum sideband_type *sideband_type);
 
 void send_sideband(int fd, int band, const char *data, ssize_t sz, int packet_max);
+/*
+ * Do not die on write errors, return -1 instead.
+ */
+int send_sideband2(int fd, int band, const char *data, ssize_t sz, int packet_max);
 
 #endif

-- 
2.34.1

On Wed, Jan 26, 2022 at 12:09 AM Robin Jarry [off-list ref] wrote:

When hitting ctrl-c on the client while a remote pre-receive hook is
running, receive-pack is not killed by SIGPIPE because the signal is
ignored. This is a side effect of commit ec7dbd145bd8 ("receive-pack:
allow hooks to ignore its standard input stream").

The pre-receive hook itself is not interrupted and does not receive any
error since its stdout is a pipe which is read in an async thread and
output back to the client socket in a side band channel.

After the pre-receive has exited the SIGPIPE default handler is restored
and if the hook did not report any error, objects are migrated from
temporary to permanent storage.

We used to ignore the SIGPIPE signal when calling "pre-receive" hook,
so we could tolerant a buggy "pre-receive" implementation which didn't
consume all the input from "receive-pack". On the other side, "ctrl-c"
from the client side will terminate "receive-pack", only if we do not
ignore the SIGPIPE signal when running "pre-receive".

Wouldn't this be much simpler: add a new configuration variable
"receive.loosePreReceiveImplementation", and only ignore SIGPIPE when
"receive-pack" turns off the config variable?

Kill the "pre-receive" process, so the calling of
"finish_command(&proc)" at the end of "run_and_feed_hook()" will
terminate "receive-pack".

"ignore_errors" or "die_on_errors"?

+                               write_or_die(fd, hdr, 5);
+                       else if (write_in_full(fd, hdr, 5) < 0)
+                               return -1;

--
Jiang Xin

Jiang Xin, Jan 26, 2022 at 08:17:

We used to ignore the SIGPIPE signal when calling "pre-receive" hook,
so we could tolerant a buggy "pre-receive" implementation which didn't
consume all the input from "receive-pack". On the other side, "ctrl-c"
from the client side will terminate "receive-pack", only if we do not
ignore the SIGPIPE signal when running "pre-receive".

Wouldn't this be much simpler: add a new configuration variable
"receive.loosePreReceiveImplementation", and only ignore SIGPIPE when
"receive-pack" turns off the config variable?

I had not thought of this. Yes it would be much simpler. I'll prepare
another patch with this approach.

Thanks!

When hitting ctrl-c on the client while a remote pre-receive hook is
running, receive-pack is not killed by SIGPIPE because the signal is
ignored. This is a side effect of commit ec7dbd145bd8 (receive-pack:
allow hooks to ignore its standard input stream).

The pre-receive hook itself is not interrupted and does not receive any
error since its stdout is a pipe which is read in an async thread and
output back to the client socket in a side band channel.

After the pre-receive has exited the SIGPIPE default handler is restored
and if the hook did not report any error, objects are migrated from
temporary to permanent storage.

This can be confusing for most people and may even be considered a bug.

Add a new receive.strictPreReceiveImpl config option to *not* ignore
SIGPIPE when running pre-receive. If set to true, and the hook output
cannot be forwarded to the client, receive-pack will be killed via
SIGPIPE and the push will be aborted. Add a signal handler to kill and
reap the hook process before exiting. This option only affects
pre-receive.

This does not guarantee that all client disconnections will abort
a push. If there is no pre-receive hook or if it does not produce any
output, receive-pack will not be killed via SIGPIPE and the push will
complete.

Signed-off-by: Robin Jarry <redacted>
---
v1 -> v2:
  Changed approach following Jiang Xin advice. Adding an option makes
  more sense and also makes a much simpler patch.

 Documentation/config/receive.txt | 13 +++++++++++++
 builtin/receive-pack.c           | 26 +++++++++++++++++++++++++-
 2 files changed, 38 insertions(+), 1 deletion(-)

diff --git a/Documentation/config/receive.txt b/Documentation/config/receive.txt
index 85d5b5a3d2d8..7174168541dc 100644
--- a/Documentation/config/receive.txt
+++ b/Documentation/config/receive.txt

@@ -143,3 +143,16 @@ receive.updateServerInfo::
 receive.shallowUpdate::
 	If set to true, .git/shallow can be updated when new refs
 	require new shallow roots. Otherwise those refs are rejected.
+
+receive.strictPreReceiveImpl::
+	If a pre-receive hook does not consume its standard input fully, it may
+	kill receive-pack via SIGPIPE. This can lead to obscure push failures.
+	To avoid potential death-by-SIGPIPE due to poorly written hooks,
+	receive-pack ignores SIGPIPE while running the pre-receive hook.
++
+If this option is set to true, SIGPIPE will `not` be ignored by receive-pack
+while running the "pre-receive" hook. This has a side-effect: If the hook
+outputs something and the client has disconnected, receive-pack will be killed
+and the push will be aborted.
++
+SIGPIPE is always ignored while running "post-receive".

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 9f4a0b816cf9..8718a6dd91b4 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c

@@ -74,6 +74,7 @@ static const char *head_name;
 static void *head_name_to_free;
 static int sent_capabilities;
 static int shallow_update;
+static int strict_pre_receive_impl;
 static const char *alt_shallow_file;
 static struct strbuf push_cert = STRBUF_INIT;
 static struct object_id push_cert_oid;

@@ -219,6 +220,11 @@ static int receive_pack_config(const char *var, const char *value, void *cb)
 		return 0;
 	}
 
+	if (strcmp(var, "receive.strictprereceiveimpl") == 0) {
+		strict_pre_receive_impl = git_config_bool(var, value);
+		return 0;
+	}
+
 	if (strcmp(var, "receive.certnonceseed") == 0)
 		return git_config_string(&cert_nonce_seed, var, value);
 

@@ -800,6 +806,19 @@ static void prepare_push_cert_sha1(struct child_process *proc)
 	}
 }
 
+static volatile pid_t hook_pid;
+
+static void kill_hook(int signum)
+{
+	if (hook_pid != 0) {
+		kill(hook_pid, signum);
+		waitpid(hook_pid, NULL, 0);
+		hook_pid = 0;
+	}
+	sigchain_pop(signum);
+	raise(signum);
+}
+
 struct receive_hook_feed_state {
 	struct command *cmd;
 	struct ref_push_report *report;

@@ -858,7 +877,11 @@ static int run_and_feed_hook(const char *hook_name, feed_fn feed,
 		return code;
 	}
 
-	sigchain_push(SIGPIPE, SIG_IGN);
+	hook_pid = proc.pid;
+	if (strict_pre_receive_impl && strcmp(hook_name, "pre-receive") == 0)
+		sigchain_push(SIGPIPE, kill_hook);
+	else
+		sigchain_push(SIGPIPE, SIG_IGN);
 
 	while (1) {
 		const char *buf;

@@ -872,6 +895,7 @@ static int run_and_feed_hook(const char *hook_name, feed_fn feed,
 	if (use_sideband)
 		finish_async(&muxer);
 
+	hook_pid = 0;
 	sigchain_pop(SIGPIPE);
 
 	return finish_command(&proc);

-- 
2.35.0.1.g8273a50afc47

On Thu, Jan 27, 2022 at 10:03 AM Robin Jarry [off-list ref] wrote:

Can we use a flag instead of hook_pid to distinguish the source of the
SIGPIPE signal?
1. "pre-receive" hook exits early without consuming stdin.
2. "pre-receive" hook hangs after receiving commands from stdin, until
client quits by receiving a "ctrl-c".

+static void kill_hook(int signum)
+{
+       if (hook_pid != 0) {
+               kill(hook_pid, signum);
+               waitpid(hook_pid, NULL, 0);
+               hook_pid = 0;

Can we let the signal handler in "pre-receive" to do it job? And we
can show some user friendly error message here. E.g.:

    die("broken pipe: seems like the pre-receive hook exits early
without consuming its stdin");

--
Jiang Xin

Robin Jarry [off-list ref] writes:

When hitting ctrl-c on the client while a remote pre-receive hook is
running, receive-pack is not killed by SIGPIPE because the signal is
ignored. This is a side effect of commit ec7dbd145bd8 (receive-pack:
allow hooks to ignore its standard input stream).

I somehow feel that it is unrealistic to expect the command to be
killed via SIGPIPE because there is no guarantee that the command
has that many bytes to send out to to get the signal in the first
place.  Such an expectation is simply wrong, isn't it?

This can be confusing for most people and may even be considered a bug.

So, there is not much I see is confusing, and I expect "most people"
would not get confused or consider it a bug.  Killing a local
process may or may not have any immediate effect on what happens on
the other side of the connection.

On the other hand, the SIGPIPE death by a poorly written pre-receive
hook was a source of real confusion.  The pushing end cannot do
anything about it to fix if the hook disconnected before reading all
of the proposed updates.

Add a new receive.strictPreReceiveImpl config option to *not* ignore

I guess that the receiving end must know if its hook is loosely written
or not, so having a knob to revert to the older mode of operation
may probably be OK.

Do not abbreviate "Implementation" in the name of a configuration
variable, if that is the word you meant, by the way.  We try to
spell things out for clarity.

Also, "strict implementation" is way too vague.  What you want to
say here is that the hook will not stop reading its input in the
middle, causing the feeder to be killed by SIGPIPE, and from other
aspects its implementation may not be strict at all.

A name that goes well with a statement "This hook reads all of its
input" would work much better.

Should this cover only one hook, or should we introduce just one
configuration to say "all hooks that read from their standard input
stream are clean and will read their input to the end"?  Or do we
need to have N different variables for each of N hooks that may stop
reading from their standard input in the middle (not necessarily
limited to the receive-pack command)?  I think there are a handful
other hooks that take input from their standard input stream and I
am not sure if pre-receive should be singled out like this.

If this Boolean "This hook reads all of its input to the end" is to
be added per hook, I suspect that the namespace of the configuration
variable should be coordinated with the other effort to "define" hooks
in the configuration file(s) in the first place.  Emily, do you have
a suggestion?

+static volatile pid_t hook_pid;
+
+static void kill_hook(int signum)
+{
+	if (hook_pid != 0) {
+		kill(hook_pid, signum);
+		waitpid(hook_pid, NULL, 0);
+		hook_pid = 0;

Is it safe to kill(2) from within a signal handler?

Why does this patch do anything more than a partial reversion of
ec7dbd14 (receive-pack: allow hooks to ignore its standard input
stream, 2014-09-12), i.e. "if the configuration says do not be
lenient to hooks that do not consume their input, do not ignore
sigpipe at all".

Jiang Xin, Jan 27, 2022 at 04:21:

Can we use a flag instead of hook_pid to distinguish the source of the
SIGPIPE signal?
1. "pre-receive" hook exits early without consuming stdin.
2. "pre-receive" hook hangs after receiving commands from stdin, until
client quits by receiving a "ctrl-c".

Also there is:

3. the client has exited and receive-pack got SIGPIPE while forwarding
   pre-receive output in the socket.

I don't think we can differentiate from these three situations from the
receive-pack point of view.

However, using a flag in the signal handler to note that SIGPIPE was
received (for whatever reason) may be better than my current
implementation.

Can we let the signal handler in "pre-receive" to do it job? And we
can show some user friendly error message here. E.g.:

    die("broken pipe: seems like the pre-receive hook exits early
without consuming its stdin");

If that flag is set after pre-receive has exited, we can indeed:

    die("broken pipe: ...").

Of course, if 3. the error message will never reach the client.

Junio C Hamano, Jan 27, 2022 at 05:36:

I somehow feel that it is unrealistic to expect the command to be
killed via SIGPIPE because there is no guarantee that the command
has that many bytes to send out to to get the signal in the first
place.  Such an expectation is simply wrong, isn't it?

Maybe I did not word that properly. Indeed, this only applies if
pre-receive has bytes to send out in the first place. This is what
I referred to with the last paragraph:

quoted

This does not guarantee that all client disconnections will abort
a push. If there is no pre-receive hook or if it does not produce
any output, receive-pack will not be killed via SIGPIPE and the push
will complete.

It would be much better not to rely on pre-receive to have bytes to send
and to expect that receive-pack will receive SIGPIPE when forwarding
them after the client has disconnected.

I thought of sending a "keepalive packet" in the socket *after* the
pre-receive hook has completed. I do not know the protocol details.
Would something like this be suitable:

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 8718a6dd91b4..2e0ddd1a59fe 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c

@@ -1990,16 +1990,28 @@ static void execute_commands(struct command *commands,
 	if (run_receive_hook(commands, "pre-receive", 0, push_options)) {
 		for (cmd = commands; cmd; cmd = cmd->next) {
 			if (!cmd->error_string)
 				cmd->error_string = "pre-receive hook declined";
 		}
 		return;
 	}
 
+	/*
+	 * Send a keepalive packet to ensure that the client has not
+	 * disconnected while pre-receive was running.
+	 */
+	{
+		static const char buf[] = "0001";
+		if (use_sideband)
+			send_sideband(1, 1, buf, sizeof(buf) - 1, use_sideband);
+		else
+			write_or_die(1, buf, sizeof(buf) - 1);
+	}
+
 	/*
 	 * Now we'll start writing out refs, which means the objects need
 	 * to be in their final positions so that other processes can see them.
 	 */
 	if (tmp_objdir_migrate(tmp_objdir) < 0) {
 		for (cmd = commands; cmd; cmd = cmd->next) {
 			if (!cmd->error_string)
 				cmd->error_string = "unable to migrate objects to permanent storage";

In that situation, if the client has exited, receive-pack should be
killed via SIGPIPE before completing the push.

Is it safe to kill(2) from within a signal handler?

Even if it is, it is probably not a good idea. I did that to avoid
leaving a zombie after receive-pack has died. Maybe setting a flag in
the signal handler and checking the flag after the process has exited
would have been better.

Why does this patch do anything more than a partial reversion of
ec7dbd14 (receive-pack: allow hooks to ignore its standard input
stream, 2014-09-12), i.e. "if the configuration says do not be
lenient to hooks that do not consume their input, do not ignore
sigpipe at all".

Indeed it is a partial reversion of that commit. Maybe the "keepalive
before migrating to permanent storage" solution is better.

What do you think?