patch, 6 partsreceive-pack: fix deadlock when we cannot create tmpdir

On Tue, Mar 07, 2017 at 12:14:06PM +0100, Horst Schirmeier wrote:

Thanks, I was able to reproduce easily with:

  git init --bare foo.git
  chown -R nobody foo.git
  git push foo.git HEAD

Here's a series to fix it. The first patch addresses the deadlock. The rest try to improve the output on the client side. With v2.10.0, this case looks like:

  $ git push ~/tmp/foo.git HEAD
  Counting objects: 209837, done.
  Delta compression using up to 8 threads.
  Compressing objects: 100% (52180/52180), done.
  remote: fatal: Unable to create temporary file '/home/peff/tmp/foo.git/./objects/pack/tmp_pack_XXXXXX': Permission denied
  error: failed to push some refs to '/home/peff/tmp/foo.git'

With just the first patch applied, you get just:

  $ git push ~/tmp/foo.git HEAD
  Counting objects: 210973, done.
  Delta compression using up to 8 threads.
  Compressing objects: 100% (52799/52799), done.
  error: failed to push some refs to '/home/peff/tmp/foo.git'

Yuck. In the original, the error was generated by the child index-pack, and we relayed it over the sideband. But we don't even get as far as running index-pack in the newer version; we fail trying to make the tmpdir. The error ends up in the "unpack" protocol field, but the client side does a bad job of showing that. With the rest of the patches, it looks like:

  $ git push ~/tmp/foo.git HEAD
  Counting objects: 210973, done.
  Delta compression using up to 8 threads.
  Compressing objects: 100% (52799/52799), done.
  error: remote unpack failed: unable to create temporary object directory
  error: failed to push some refs to '/home/peff/tmp/foo.git'

Compared to v2.10.0, that omits the name of the directory and the errno value (because receive-pack does not send them). That potentially makes debugging harder, but arguably it's the right thing to do. On a remote site, those details aren't really the client's business. But if people feel strongly, we could add them back into this error code path.

So the first patch is a no-brainer and should go to maint. The rest can be applied separately if need be.

  [1/6]: receive-pack: fix deadlock when we cannot create tmpdir
  [2/6]: send-pack: extract parsing of "unpack" response
  [3/6]: send-pack: use skip_prefix for parsing unpack status
  [4/6]: send-pack: improve unpack-status error messages
  [5/6]: send-pack: read "unpack" status even on pack-objects failure
  [6/6]: send-pack: report signal death of pack-objects

 builtin/receive-pack.c |  5 ++++-
 send-pack.c            | 46 ++++++++++++++++++++++++++++++++++++----------
 2 files changed, 40 insertions(+), 11 deletions(-)

-Peff

The err_fd descriptor passed to the unpack() function is intended to be handed off to the child index-pack, and our async muxer will read until it gets EOF. However, if we encounter an error before handing off the descriptor, we must manually close(err_fd). Otherwise we will be waiting for our muxer to finish, while the muxer is waiting for EOF on err_fd.

We fixed an identical deadlock already in 49ecfa13f (receive-pack: close sideband fd on early pack errors, 2013-04-19). But since then, the function grew a new early-return in 722ff7f87 (receive-pack: quarantine objects until pre-receive accepts, 2016-10-03), when we fail to create a temporary directory. This return needs the same treatment.

Reported-by: Horst Schirmeier <horst@schirmeier.com>
Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/receive-pack.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

After sending the pack, we call receive_status() which gets both the "unpack" line and the ref status. Let's break these into two functions so we can call the first part independently.

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

If our pack-objects sub-process dies of a signal, then it likely didn't have a chance to write anything useful to stderr. The user may be left scratching their head why the push failed. Let's detect this situation and write something to stderr.

Signed-off-by: Jeff King <peff@peff.net>
---
We could drop the SIGPIPE special-case, but I think it's just noise
after the unpack-status fix in the previous commit.

 send-pack.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

If the local pack-objects of a push fails, we'll tell the user about it. But one likely cause is that the remote index-pack stopped reading for some reason (because it didn't like our input, or encountered another error). In that case we'd expect the remote to report more details to us via the "unpack ..." status line. However, the current code just hangs up completely, and the user never sees it.

Instead, let's call receive_unpack_status(), which will complain on stderr with whatever reason the remote told us. Note that if our pack-objects fails because the connection was severed or the remote just crashed entirely, then our packet_read_line() call may fail with "the remote end hung up unexpectedly". That's OK. It's a more accurate description than what we get now (which is just "some refs failed to push").

This should be safe from any deadlocks. At the point we make this call we'll have closed the writing end of the connection to the server (either by handing it off to a pack-objects which exited, explicitly in the stateless_rpc case, or by doing a half-duplex shutdown for a socket). So there should be no chance that the other side is waiting for the rest of our pack-objects input.

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

When the remote tells us that the "unpack" step failed, we show an error message. However, unless you are familiar with the internals of send-pack and receive-pack, it was not clear that this represented an error on the remote side. Let's re-word to make that more obvious.

Likewise, when we got an unexpected packet from the other end, we complained with a vague message but did not actually show the packet. Let's fix that.

And finally, neither message was marked for translation. The message from the remote probably won't be translated, but there's no reason we can't do better for the local half.

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

Jeff King <peff@peff.net> writes:

Both make sense.

Hmm, OK.

On Tue, Mar 07, 2017 at 02:56:27PM -0800, Junio C Hamano wrote:

I'll admit that I don't actually use the translations myself, being a native English speaker. So I am just guessing that somebody for whom English is a second language would rather see the first half in a more intelligible format. That at least tells them what the second half _is_, so they might be able to search for the error with more context.

If my guess is wrong, though, I'm happy to retract that part or bump it out to a separate patch.

-Peff

Jeff King <peff@peff.net> writes:

I was merely undecided between "at least half is in my language" and "both are consistently untranslated" which one is easier to use by my highschool friends who do not grok English. And I still cannot decide.

When responding to a request-for-help that quotes messages that was translated, I would imagine we would need one extra "git grep" to find the message (without understanding that ourselves) from po/ for the original before running "git grep" to find the code that produced the message [*1*].

On the other hand, we get request-for-help from those having issues in a setup where the software running on the other side is not even ours, so I am (slightly) more inclined to agree that "half is in my language" is better than nothing.

Thanks.

[Footnote]

*1* If the translation came from us, that is. If I recall correctly, some distros do their own po/ and core Git developers are not likely to have them around, so "git grep" in our po/ may not see any hit.

On Tue, Mar 07, 2017 at 08:34:37AM -0500, Jeff King wrote:

There are two other options here I should mention.

One is that when pack-objects dies, we suppress the ref-status table entirely. Which is fair, because it doesn't have anything interesting to say. But for a case like this where the other side stops reading our pack early but still produces status reports, we could actually read them all and have a status table like:

  $ git push ~/tmp/foo.git HEAD
  Counting objects: 209843, done.
  Delta compression using up to 8 threads.
  Compressing objects: 100% (52186/52186), done.
  error: remote unpack failed: unable to create temporary object directory
  To /home/peff/tmp/foo.git
   ! [remote rejected]     HEAD -> jk/push-index-pack-deadlock (unpacker error)
  error: failed to push some refs to '/home/peff/tmp/foo.git'

We'd have to take some care to make sure we handle the case when the remote _doesn't_ manage to give us the status (e.g., when there's a complete hangup). I don't know if it's worth the effort. There's no new information there.

The second thing is that I think the design of the "unpack <reason>" report is a bit dated. These days everybody supports the sideband protocol, so it would probably make more sense to just issue the "<reason>" report via the sideband (at which point it gets a nice "remote: " prefix).

We could do something like this:

This avoids repeating ourselves, and the use of magic numbers.

Signed-off-by: Jeff King <peff@peff.net>
---
Obviously not necessary, but just a cleanup while I was here.

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

On Tue, 07 Mar 2017, Jeff King wrote:

I tested your jk/push-deadlock-regression-fix branch in my local clone, your patches fix the problem for me. Thanks!

Horst