regression: git push in non-shared repo stalls (v2.11.0+)

regression: git push in non-shared repo stalls (v2.11.0+)

[Horst Schirmeier]

Hi,

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.

Horst

Steps to reproduce (tested on Debian 8 "Jessie" amd64):
 -  User A creates a bare repository:
    mkdir /tmp/gittest
    git init --bare /tmp/gittest
 -  User B clones it, adds and commits a file:
    git clone /tmp/gittest
    cd gittest
    echo 42 > x
    git add x
    git commit -m test
 -  User B tries to push to user A's bare repo:
    git push

Expected result (git v2.10.0 and earlier):
test@ios:~/gittest$ git push
Counting objects: 3, done.
Writing objects: 100% (3/3), 230 bytes | 0 bytes/s, done.
Total 3 (delta 0), reused 0 (delta 0)
remote: error: insufficient permission for adding an object to repository database objects
remote: fatal: failed to write object
error: unpack failed: unpack-objects abnormal exit
To /tmp/gittest
 ! [remote rejected] master -> master (unpacker error)
error: failed to push some refs to '/tmp/gittest'

Actual result (git v2.11.0, v2.12.0, and 2.12.0.189.g3bc53220c):
test@ios:~/gittest$ git push
Counting objects: 3, done.
Writing objects: 100% (3/3), 230 bytes | 0 bytes/s, done.
Total 3 (delta 0), reused 0 (delta 0)
[... git freezes here ...]

-- 
PGP-Key 0xD40E0E7A

Re: regression: git push in non-shared repo stalls (v2.11.0+)

[Horst Schirmeier]

On Tue, 07 Mar 2017, Horst Schirmeier wrote:
> 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 <peff@peff.net>
Date:   Mon Oct 3 16:49:14 2016 -0400

    receive-pack: quarantine objects until pre-receive accepts

-- 
PGP-Key 0xD40E0E7A

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

[Jeff King]

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

> On Tue, 07 Mar 2017, Horst Schirmeier wrote:
> > 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 <peff@peff.net>
> 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

[PATCH 1/6] receive-pack: fix deadlock when we cannot create tmpdir

[Jeff King]

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(-)

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 9ed8fbbfa..f2c6953a3 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -1667,8 +1667,11 @@ static const char *unpack(int err_fd, struct shallow_info *si)
 	}
 
 	tmp_objdir = tmp_objdir_create();
-	if (!tmp_objdir)
+	if (!tmp_objdir) {
+		if (err_fd > 0)
+			close(err_fd);
 		return "unable to create temporary object directory";
+	}
 	child.env = tmp_objdir_env(tmp_objdir);
 
 	/*
-- 
2.12.0.429.gde83c8049

[PATCH 2/6] send-pack: extract parsing of "unpack" response

[Jeff King]

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(-)

diff --git a/send-pack.c b/send-pack.c
index 6195b43e9..12e229e44 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -130,22 +130,27 @@ static int pack_objects(int fd, struct ref *refs, struct sha1_array *extra, stru
 	return 0;
 }
 
-static int receive_status(int in, struct ref *refs)
+static int receive_unpack_status(int in)
 {
-	struct ref *hint;
-	int ret = 0;
-	char *line = packet_read_line(in, NULL);
+	const char *line = packet_read_line(in, NULL);
 	if (!starts_with(line, "unpack "))
 		return error("did not receive remote status");
-	if (strcmp(line, "unpack ok")) {
-		error("unpack failed: %s", line + 7);
-		ret = -1;
-	}
+	if (strcmp(line, "unpack ok"))
+		return error("unpack failed: %s", line + 7);
+	return 0;
+}
+
+static int receive_status(int in, struct ref *refs)
+{
+	struct ref *hint;
+	int ret;
+
 	hint = NULL;
+	ret = receive_unpack_status(in);
 	while (1) {
 		char *refname;
 		char *msg;
-		line = packet_read_line(in, NULL);
+		char *line = packet_read_line(in, NULL);
 		if (!line)
 			break;
 		if (!starts_with(line, "ok ") && !starts_with(line, "ng ")) {
-- 
2.12.0.429.gde83c8049

[PATCH 3/6] send-pack: use skip_prefix for parsing unpack status

[Jeff King]

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(-)

diff --git a/send-pack.c b/send-pack.c
index 12e229e44..243633da1 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -133,10 +133,10 @@ static int pack_objects(int fd, struct ref *refs, struct sha1_array *extra, stru
 static int receive_unpack_status(int in)
 {
 	const char *line = packet_read_line(in, NULL);
-	if (!starts_with(line, "unpack "))
+	if (!skip_prefix(line, "unpack ", &line))
 		return error("did not receive remote status");
-	if (strcmp(line, "unpack ok"))
-		return error("unpack failed: %s", line + 7);
+	if (strcmp(line, "ok"))
+		return error("unpack failed: %s", line);
 	return 0;
 }
 
-- 
2.12.0.429.gde83c8049

[PATCH 4/6] send-pack: improve unpack-status error messages

[Jeff King]

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(-)

diff --git a/send-pack.c b/send-pack.c
index 243633da1..83c23aef6 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -134,9 +134,9 @@ static int receive_unpack_status(int in)
 {
 	const char *line = packet_read_line(in, NULL);
 	if (!skip_prefix(line, "unpack ", &line))
-		return error("did not receive remote status");
+		return error(_("unable to parse remote unpack status: %s"), line);
 	if (strcmp(line, "ok"))
-		return error("unpack failed: %s", line);
+		return error(_("remote unpack failed: %s"), line);
 	return 0;
 }
 
-- 
2.12.0.429.gde83c8049

[PATCH 5/6] send-pack: read "unpack" status even on pack-objects failure

[Jeff King]

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(+)

diff --git a/send-pack.c b/send-pack.c
index 83c23aef6..e15232739 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -562,6 +562,14 @@ int send_pack(struct send_pack_args *args,
 				close(out);
 			if (git_connection_is_socket(conn))
 				shutdown(fd[0], SHUT_WR);
+
+			/*
+			 * Do not even bother with the return value; we know we
+			 * are failing, and just want the error() side effects.
+			 */
+			if (status_report)
+				receive_unpack_status(in);
+
 			if (use_sideband) {
 				close(demux.out);
 				finish_async(&demux);
-- 
2.12.0.429.gde83c8049

[PATCH 6/6] send-pack: report signal death of pack-objects

[Jeff King]

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(-)

diff --git a/send-pack.c b/send-pack.c
index e15232739..d2d2a49a0 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -72,6 +72,7 @@ static int pack_objects(int fd, struct ref *refs, struct sha1_array *extra, stru
 	struct child_process po = CHILD_PROCESS_INIT;
 	FILE *po_in;
 	int i;
+	int rc;
 
 	i = 4;
 	if (args->use_thin_pack)
@@ -125,8 +126,20 @@ static int pack_objects(int fd, struct ref *refs, struct sha1_array *extra, stru
 		po.out = -1;
 	}
 
-	if (finish_command(&po))
+	rc = finish_command(&po);
+	if (rc) {
+		/*
+		 * For a normal non-zero exit, we assume pack-objects wrote
+		 * something useful to stderr. For death by signal, though,
+		 * we should mention it to the user. The exception is SIGPIPE
+		 * (141), because that's a normal occurence if the remote end
+		 * hangs up (and we'll report that by trying to read the unpack
+		 * status).
+		 */
+		if (rc > 128 && rc != 141)
+			error("pack-objects died of signal %d", rc - 128);
 		return -1;
+	}
 	return 0;
 }
 
-- 
2.12.0.429.gde83c8049

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

[Jeff King]

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

> 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'

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:

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index f2c6953a3..6204d3d00 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -1670,6 +1670,8 @@ static const char *unpack(int err_fd, struct shallow_info *si)
 	if (!tmp_objdir) {
 		if (err_fd > 0)
 			close(err_fd);
+		rp_error("unable to create temporary object directory: %s",
+			 strerror(errno));
 		return "unable to create temporary object directory";
 	}
 	child.env = tmp_objdir_env(tmp_objdir);

and drop the "try to show the unpack failure" parts of my series, and
you'd end up with:

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

but in cases where pack-objects _doesn't_ fail, existing git versions
do show the "unpack" error. So you'd see it twice.

I don't know if it's worth trying to hack around.

-Peff

Re: [PATCH 4/6] send-pack: improve unpack-status error messages

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> 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.

Both make sense.

> 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.

Hmm, OK.

>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  send-pack.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/send-pack.c b/send-pack.c
> index 243633da1..83c23aef6 100644
> --- a/send-pack.c
> +++ b/send-pack.c
> @@ -134,9 +134,9 @@ static int receive_unpack_status(int in)
>  {
>  	const char *line = packet_read_line(in, NULL);
>  	if (!skip_prefix(line, "unpack ", &line))
> -		return error("did not receive remote status");
> +		return error(_("unable to parse remote unpack status: %s"), line);
>  	if (strcmp(line, "ok"))
> -		return error("unpack failed: %s", line);
> +		return error(_("remote unpack failed: %s"), line);
>  	return 0;
>  }

Re: [PATCH 4/6] send-pack: improve unpack-status error messages

[Jeff King]

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

> Jeff King <peff@peff.net> writes:
> 
> > 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.
> 
> Both make sense.
> 
> > 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.
> 
> Hmm, OK.

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

Re: [PATCH 4/6] send-pack: improve unpack-status error messages

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>> > 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.
>> 
>> Hmm, OK.
>
> 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.

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.

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

[Horst Schirmeier]

On Tue, 07 Mar 2017, Jeff King wrote:
> On Tue, Mar 07, 2017 at 12:14:06PM +0100, Horst Schirmeier wrote:
> > On Tue, 07 Mar 2017, Horst Schirmeier wrote:
> > > 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 <peff@peff.net>
> > 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'

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

Horst

-- 
PGP-Key 0xD40E0E7A