From 739cf49161db01855c03c283f6aa3f5341a1ab36 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 19 Apr 2016 18:45:17 -0400 Subject: [PATCH 1/5] send-pack: close demux pipe before finishing async process 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 Signed-off-by: Junio C Hamano --- send-pack.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/send-pack.c b/send-pack.c index 047bd18fde..fc27db6055 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) From c792d7b6cebe302d6e0377d9d983608309bcd775 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 19 Apr 2016 18:49:41 -0400 Subject: [PATCH 2/5] run-command: teach async threads to ignore SIGPIPE 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 Signed-off-by: Junio C Hamano --- run-command.c | 10 ++++++++++ run-command.h | 1 + 2 files changed, 11 insertions(+) diff --git a/run-command.c b/run-command.c index 2392b1efe8..11ac060490 100644 --- a/run-command.c +++ b/run-command.c @@ -588,6 +588,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 c0969c7695..a3043b16df 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); From 3e8b06d09c48a46ad7fee761a58040a7b006a642 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 19 Apr 2016 18:50:17 -0400 Subject: [PATCH 3/5] send-pack: isolate sigpipe in demuxer thread 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 Signed-off-by: Junio C Hamano --- send-pack.c | 1 + 1 file changed, 1 insertion(+) diff --git a/send-pack.c b/send-pack.c index fc27db6055..37ee04ea3b 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; From df85757244c47394d42de2a45bd39522ee0c3b1d Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 19 Apr 2016 18:50:29 -0400 Subject: [PATCH 4/5] fetch-pack: isolate sigpipe in demuxer thread 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 Signed-off-by: Junio C Hamano --- fetch-pack.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/fetch-pack.c b/fetch-pack.c index f96f6dfb35..b501d5c320 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"); From c4b27511ab0e3ec464e3fd3d4711251a17b1f733 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 19 Apr 2016 18:51:25 -0400 Subject: [PATCH 5/5] t5504: drop sigpipe=ok from push tests 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 Signed-off-by: Junio C Hamano --- 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 a3e12d295a..44f3d5fb28 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