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.gitSigned-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(-)
Jeff King
