[PATCH 0/5] fix deadlock in git-push

[PATCH 0/5] fix deadlock in git-push

[Jeff King]

I ran across a deadlock today while pushing from a corrupted repository
where pack-objects fails. Obviously I don't expect this to succeed, but
it should not hang indefinitely.

The first patch below fixes the deadlock. Unfortunately, it turns it
into a likely SIGPIPE death. Which is an improvement, but not ideal.

Patches 2 and 3 address that by fixing the way we handle SIGPIPE in
async threads.

Patches 4 and 5 are cleanups to earlier topics that are enabled by the
new SIGPIPE handling.

  [1/5]: send-pack: close demux pipe before finishing async process
  [2/5]: run-command: teach async threads to ignore SIGPIPE
  [3/5]: send-pack: isolate sigpipe in demuxer thread
  [4/5]: fetch-pack: isolate sigpipe in demuxer thread
  [5/5]: t5504: drop sigpipe=ok from push tests

I confirmed that after this series we still pass the stress test I
outlined in

  http://article.gmane.org/gmane.comp.version-control.git/287176

So hopefully I haven't made anything worse (and that the tightening in
5/5 isn't just bringing back some test flakiness).

-Peff

[PATCH 1/5] send-pack: close demux pipe before finishing async process

[Jeff King]

This fixes a deadlock on the client side when pushing a
large number of refs from a corrupted repo.  There's a
reproduction script below, but let's start with a
human-readable explanation.

The client side of a push goes something like this:

  1. Start an async process to demux sideband coming from
     the server.

  2. Run pack-objects to send the actual pack, and wait for
     its status via finish_command().

  3. If pack-objects failed, abort immediately.

  4. If pack-objects succeeded, read the per-ref status from
     the server, which is actually coming over a pipe from
     the demux process started in step 1.

We run finish_async() to wait for and clean up the demux
process in two places. In step 3, if we see an error, we
want it to end early. And after step 4, it should be done
writing any data and we are just cleaning it up.

Let's focus on the error case first. We hand the output
descriptor to the server over to pack-objects. So by the
time it has returned an error to us, it has closed the
descriptor and the server has gotten EOF. The server will
mark all refs as failed with "unpacker error" and send us
back the status for each (followed by EOF).

This status goes to the demuxer thread, which relays it over
a pipe to the main thread. But the main thread never even
tries reading the status. It's trying to bail because of the
pack-objects error, and is waiting for the demuxer thread to
finish. If there are a small number of refs, that's OK; the
demuxer thread writes into the pipe buffer, sees EOF from
the server, and quits. But if there are a large number of
refs, it may block on write() back to the main thread,
leading to a deadlock (the main thread is waiting for the
demuxer to finish, the demuxer is waiting for the main
thread to read).

We can break this deadlock by closing the pipe between the
demuxer and the main thread before calling finish_async().
Then the demuxer gets a write() error and exits.

The non-error case usually just works, because we will have
read all of the data from the other side. We do close
demux.out already, but we only do so _after_ calling
finish_async(). This is OK because there shouldn't be any
more data coming from the server. But technically we've only
read to a flush packet, and a broken or malicious server
could be sending more cruft. In such a case, we would hit
the same deadlock. Closing the pipe first doesn't affect the
normal case, and means that for a cruft-sending server,
we'll notice a write() error rather than deadlocking.

Note that when write() sees this error, we'll actually
deliver SIGPIPE to the thread, which will take down the
whole process (unless we're compiled with NO_PTHREADS). This
isn't ideal, but it's an improvement over the status quo,
which is deadlocking. And SIGPIPE handling in async threads
is a bigger problem that we can deal with separately.

A simple reproduction for the error case is below. It's
technically racy (we could exit the main process and take
down the async thread with us before it even reads the
status), though in practice it seems to fail pretty
consistently.

    git init repo &&
    cd repo &&

    # make some commits; we need two so we can simulate corruption
    # in the history later.
    git commit --allow-empty -m one &&
    one=$(git rev-parse HEAD) &&
    git commit --allow-empty -m two &&
    two=$(git rev-parse HEAD) &&

    # now make a ton of refs; our goal here is to overflow the pipe buffer
    # when reporting the ref status, which will cause the demuxer to block
    # on write()
    for i in $(seq 20000); do
    	echo "create refs/heads/this-is-a-really-long-branch-name-$i $two"
    done |
    git update-ref --stdin &&

    # now make a corruption in the history such that pack-objects will fail
    rm -vf .git/objects/$(echo $one | sed 's}..}&/}') &&

    # and then push the result
    git init --bare dst.git &&
    git push --mirror dst.git

Signed-off-by: Jeff King <peff@peff.net>
---
Even though the SIGPIPE isn't great, it's certainly better
than a deadlock.  So if we had to take just this one for
"maint" (e.g., because the SIGPIPE stuff causes problems on
Windows), I think it's still worth doing (and is why I stuck
this patch at the front of the series).

One alternative would be to add async_cancel() which would
run pthread_cancel() for the threaded case, and kill() for
the non-threaded. That helps the error case, but makes the
non-error case a bit weird (if the thread _isn't_ blocking
on write to us, we want to get the real status code, rather
than canceling it. But we don't have any way of knowing
which of _cancel() or finish() to call).

 send-pack.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/send-pack.c b/send-pack.c
index 047bd18..fc27db6 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -531,8 +531,10 @@ int send_pack(struct send_pack_args *args,
 				close(out);
 			if (git_connection_is_socket(conn))
 				shutdown(fd[0], SHUT_WR);
-			if (use_sideband)
+			if (use_sideband) {
+				close(demux.out);
 				finish_async(&demux);
+			}
 			fd[1] = -1;
 			return -1;
 		}
@@ -551,11 +553,11 @@ int send_pack(struct send_pack_args *args,
 		packet_flush(out);
 
 	if (use_sideband && cmds_sent) {
+		close(demux.out);
 		if (finish_async(&demux)) {
 			error("error in sideband demultiplexer");
 			ret = -1;
 		}
-		close(demux.out);
 	}
 
 	if (ret < 0)
-- 
2.8.1.512.g4e0a533

[PATCH 2/5] run-command: teach async threads to ignore SIGPIPE

[Jeff King]

Async processes can be implemented as separate forked
processes, or as threads (depending on the NO_PTHREADS
setting). In the latter case, if an async thread gets
SIGPIPE, it takes down the whole process. This is obviously
bad if the main process was not otherwise going to die, but
even if we were going to die, it means the main process does
not have a chance to report a useful error message.

There's also the small matter that forked async processes
will not take the main process down on a signal, meaning git
will behave differently depending on the NO_PTHREADS
setting.

This patch fixes it by adding a new flag to "struct async"
to block SIGPIPE just in the async thread. In theory, this
should always be on (which makes async threads behave more
like async processes), but we would first want to make sure
that each async process we spawn is careful about checking
return codes from write() and would not spew endlessly into
a dead pipe. So let's start with it as optional, and we can
enable it for specific sites in future patches.

The natural name for this option would be "ignore_sigpipe",
since that's what it does for the threaded case. But since
that name might imply that we are ignoring it in all cases
(including the separate-process one), let's call it
"isolate_sigpipe". What we are really asking for is
isolation. I.e., not to have our main process taken down by
signals spawned by the async process. How that is
implemented is up to the run-command code.

Signed-off-by: Jeff King <peff@peff.net>
---
This is our first use of pthread_sigmask, and I think Windows will have
to come up with something for this in compat/. I don't know how SIGPIPE
works there at all, so it's possible that we can just turn this into a
noop. Worst case it could probably block SIGPIPE for the whole process.

Note also that by blocking, we may end up with a pending SIGPIPE signal
(so if were ever to unblock, it would get delivered). We can unqueue it
with sigwaitinfo(), but I _think_ it shouldn't matter. We never unblock,
but just kill the thread, and the pending signal should be killed with
it.

At least that's my understanding. This is the first time I've worked
with signals and threads.

 run-command.c | 10 ++++++++++
 run-command.h |  1 +
 2 files changed, 11 insertions(+)

diff --git a/run-command.c b/run-command.c
index 8c7115a..e4593cd 100644
--- a/run-command.c
+++ b/run-command.c
@@ -590,6 +590,16 @@ static void *run_thread(void *data)
 	struct async *async = data;
 	intptr_t ret;
 
+	if (async->isolate_sigpipe) {
+		sigset_t mask;
+		sigemptyset(&mask);
+		sigaddset(&mask, SIGPIPE);
+		if (pthread_sigmask(SIG_BLOCK, &mask, NULL) < 0) {
+			ret = error("unable to block SIGPIPE in async thread");
+			return (void *)ret;
+		}
+	}
+
 	pthread_setspecific(async_key, async);
 	ret = async->proc(async->proc_in, async->proc_out, async->data);
 	return (void *)ret;
diff --git a/run-command.h b/run-command.h
index de1727e..11f76b0 100644
--- a/run-command.h
+++ b/run-command.h
@@ -116,6 +116,7 @@ struct async {
 	int proc_in;
 	int proc_out;
 #endif
+	int isolate_sigpipe;
 };
 
 int start_async(struct async *async);
-- 
2.8.1.512.g4e0a533

Re: [PATCH 2/5] run-command: teach async threads to ignore SIGPIPE

[Johannes Sixt]

Am 20.04.2016 um 00:49 schrieb Jeff King:
> This is our first use of pthread_sigmask, and I think Windows will have
> to come up with something for this in compat/. I don't know how SIGPIPE
> works there at all, so it's possible that we can just turn this into a
> noop. Worst case it could probably block SIGPIPE for the whole process.

There is no SIGPIPE on Windows. write() always returns EPIPE as if 
SIGPIPE was ignored.

We'll have to make pthread_sigmask() a no-op.

-- Hannes

Re: [PATCH 2/5] run-command: teach async threads to ignore SIGPIPE

[Jeff King]

On Thu, Apr 21, 2016 at 07:15:26AM +0200, Johannes Sixt wrote:

> Am 20.04.2016 um 00:49 schrieb Jeff King:
> >This is our first use of pthread_sigmask, and I think Windows will have
> >to come up with something for this in compat/. I don't know how SIGPIPE
> >works there at all, so it's possible that we can just turn this into a
> >noop. Worst case it could probably block SIGPIPE for the whole process.
> 
> There is no SIGPIPE on Windows. write() always returns EPIPE as if SIGPIPE
> was ignored.
> 
> We'll have to make pthread_sigmask() a no-op.

Great, thanks for clarifying. We can also #ifdef out the whole block
there if it's easier, but it looks like you already have noop
implementations for sigset, et al. So adding a noop pthread_sigmask()
should be enough.

-Peff

[PATCH 3/5] send-pack: isolate sigpipe in demuxer thread

[Jeff King]

If we get an error from pack-objects, we may exit
send_pack() early, before reading the server's status
response. In such a case, we may racily see SIGPIPE from our
async demuxer (which is trying to write that status back to
us), and we'd prefer to continue pushing the error up the
call stack, rather than taking down the whole process with
signal death.

This is safe to do because our demuxer just calls
recv_sideband, whose data writes are all done with
write_or_die(), which will notice SIGPIPE.

We do also write sideband 2 to stderr, and we would no
longer die on SIGPIPE there (if it were piped in the first
place, and if the piped program went away). But that's
probably a good thing, as it likewise should not abort the
push process at all (neither immediately by signal, nor
eventually by reporting failure back to the main thread).

Signed-off-by: Jeff King <peff@peff.net>
---
 send-pack.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/send-pack.c b/send-pack.c
index fc27db6..37ee04e 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -518,6 +518,7 @@ int send_pack(struct send_pack_args *args,
 		demux.proc = sideband_demux;
 		demux.data = fd;
 		demux.out = -1;
+		demux.isolate_sigpipe = 1;
 		if (start_async(&demux))
 			die("send-pack: unable to fork off sideband demultiplexer");
 		in = demux.out;
-- 
2.8.1.512.g4e0a533

[PATCH 4/5] fetch-pack: isolate sigpipe in demuxer thread

[Jeff King]

In commit 9ff18fa (fetch-pack: ignore SIGPIPE in sideband
demuxer, 2016-02-24), we started using sigchain_push() to
ignore SIGPIPE in the async demuxer thread. However, this is
rather clumsy, as it ignores SIGPIPE for the entire process,
including the main thread. At the time we didn't have any
per-thread signal support, but we now we do. Let's use it.

Signed-off-by: Jeff King <peff@peff.net>
---
 fetch-pack.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/fetch-pack.c b/fetch-pack.c
index f96f6df..b501d5c 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -15,7 +15,6 @@
 #include "version.h"
 #include "prio-queue.h"
 #include "sha1-array.h"
-#include "sigchain.h"
 
 static int transfer_unpack_limit = -1;
 static int fetch_unpack_limit = -1;
@@ -674,10 +673,8 @@ static int sideband_demux(int in, int out, void *data)
 	int *xd = data;
 	int ret;
 
-	sigchain_push(SIGPIPE, SIG_IGN);
 	ret = recv_sideband("fetch-pack", xd[0], out);
 	close(out);
-	sigchain_pop(SIGPIPE);
 	return ret;
 }
 
@@ -701,6 +698,7 @@ static int get_pack(struct fetch_pack_args *args,
 		demux.proc = sideband_demux;
 		demux.data = xd;
 		demux.out = -1;
+		demux.isolate_sigpipe = 1;
 		if (start_async(&demux))
 			die("fetch-pack: unable to fork off sideband"
 			    " demultiplexer");
-- 
2.8.1.512.g4e0a533

[PATCH 5/5] t5504: drop sigpipe=ok from push tests

[Jeff King]

These were added by 8bf4bec (add "ok=sigpipe" to
test_must_fail and use it to fix flaky tests, 2015-11-27)
because we would racily die via SIGPIPE when the pack was
rejected by the other side.

But since we have recently de-flaked send-pack, we should be
able to tighten up these tests (including re-adding the
expected output checks).

Signed-off-by: Jeff King <peff@peff.net>
---
Would be nice to reference HEAD^2 by name here, but of
course I don't know its final commit sha1 yet.

 t/t5504-fetch-receive-strict.sh | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/t/t5504-fetch-receive-strict.sh b/t/t5504-fetch-receive-strict.sh
index a3e12d2..44f3d5f 100755
--- a/t/t5504-fetch-receive-strict.sh
+++ b/t/t5504-fetch-receive-strict.sh
@@ -100,11 +100,8 @@ test_expect_success 'push with receive.fsckobjects' '
 		git config receive.fsckobjects true &&
 		git config transfer.fsckobjects false
 	) &&
-	test_must_fail ok=sigpipe git push --porcelain dst master:refs/heads/test >act &&
-	{
-		test_cmp exp act ||
-		! test -s act
-	}
+	test_must_fail git push --porcelain dst master:refs/heads/test >act &&
+	test_cmp exp act
 '
 
 test_expect_success 'push with transfer.fsckobjects' '
@@ -114,7 +111,8 @@ test_expect_success 'push with transfer.fsckobjects' '
 		cd dst &&
 		git config transfer.fsckobjects true
 	) &&
-	test_must_fail ok=sigpipe git push --porcelain dst master:refs/heads/test >act
+	test_must_fail git push --porcelain dst master:refs/heads/test >act &&
+	test_cmp exp act
 '
 
 cat >bogus-commit <<\EOF
-- 
2.8.1.512.g4e0a533

Re: [PATCH 0/5] fix deadlock in git-push

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> The first patch below fixes the deadlock. Unfortunately, it turns it
> into a likely SIGPIPE death. Which is an improvement, but not ideal.
>
> Patches 2 and 3 address that by fixing the way we handle SIGPIPE in
> async threads.
>
> Patches 4 and 5 are cleanups to earlier topics that are enabled by the
> new SIGPIPE handling.
>
>   [1/5]: send-pack: close demux pipe before finishing async process
>   [2/5]: run-command: teach async threads to ignore SIGPIPE
>   [3/5]: send-pack: isolate sigpipe in demuxer thread
>   [4/5]: fetch-pack: isolate sigpipe in demuxer thread
>   [5/5]: t5504: drop sigpipe=ok from push tests

Thanks for a very well explained series.

We do not call finish_async (rather, we do not use async) from that
many places, and from a cursory look this codepath is the only case
where we may encounter this kind of deadlock (the ones in
receive-pack is about relaying the error messages back to the other
end over sideband multiplexing)?

Re: [PATCH 0/5] fix deadlock in git-push

[Jeff King]

On Wed, Apr 20, 2016 at 02:17:16PM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > The first patch below fixes the deadlock. Unfortunately, it turns it
> > into a likely SIGPIPE death. Which is an improvement, but not ideal.
> >
> > Patches 2 and 3 address that by fixing the way we handle SIGPIPE in
> > async threads.
> >
> > Patches 4 and 5 are cleanups to earlier topics that are enabled by the
> > new SIGPIPE handling.
> >
> >   [1/5]: send-pack: close demux pipe before finishing async process
> >   [2/5]: run-command: teach async threads to ignore SIGPIPE
> >   [3/5]: send-pack: isolate sigpipe in demuxer thread
> >   [4/5]: fetch-pack: isolate sigpipe in demuxer thread
> >   [5/5]: t5504: drop sigpipe=ok from push tests
> 
> Thanks for a very well explained series.
> 
> We do not call finish_async (rather, we do not use async) from that
> many places, and from a cursory look this codepath is the only case
> where we may encounter this kind of deadlock (the ones in
> receive-pack is about relaying the error messages back to the other
> end over sideband multiplexing)?

Yeah, I checked the other demuxer in fetch-pack, but it does not have
any early returns like this (it just dies :) ).

It does not do an explicit close on demux.out, but I think it is
effectively closed when we hand it off to index-pack/unpack-objects via
cmd.in.

Arguably finish_async() should "close(demux.out)" itself, but that felt
like an ownership violation. Yes, that's how "struct async" passes out
the descriptor, but the caller is then expected to handle it, and
correct callers will typically have closed it themselves, handed it off
to a sub-process, etc. Closing it in finish_async() runs the risk that
we just call close() on a descriptor number that is either unattached,
or attached to some random other thing.

-Peff