[PATCH 0/6] deadlock regression in v2.11.0 with failed mkdtemp

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

On Tue, 07 Mar 2017, Horst Schirmeier wrote:

quoted

I observe a regression that seems to have been introduced between
v2.10.0 and v2.11.0.  When I try to push into a repository on the local
filesystem that belongs to another user and has not explicitly been
prepared for shared use, v2.11.0 shows some of the usual diagnostic
output and then freezes instead of announcing why it failed to push.

Bisecting points to v2.10.1-373-g722ff7f:

722ff7f876c8a2ad99c42434f58af098e61b96e8 is the first bad commit
commit 722ff7f876c8a2ad99c42434f58af098e61b96e8
Author: Jeff King [off-list ref]
Date:   Mon Oct 3 16:49:14 2016 -0400

    receive-pack: quarantine objects until pre-receive accepts

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