From 6cdad1f133475f2bff0dfe2fbe90c351df3ea61d Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 7 Mar 2017 08:35:34 -0500 Subject: [PATCH 1/6] receive-pack: fix deadlock when we cannot create tmpdir 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 Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- 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 e6b3879a5b..eb6d25a149 100644 --- a/builtin/receive-pack.c +++ b/builtin/receive-pack.c @@ -1664,8 +1664,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); /* From 7c39df2979733e0041db7aff09c3f3a53b980ef2 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 7 Mar 2017 08:35:57 -0500 Subject: [PATCH 2/6] send-pack: extract parsing of "unpack" response 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 Signed-off-by: Junio C Hamano --- send-pack.c | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-) diff --git a/send-pack.c b/send-pack.c index 6195b43e9a..12e229e447 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_unpack_status(int in) +{ + 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")) + return error("unpack failed: %s", line + 7); + return 0; +} + static int receive_status(int in, struct ref *refs) { struct ref *hint; - int ret = 0; - 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; - } + 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 ")) { From f7cd74d19d3e2a194760024046534adf20f9efde Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 7 Mar 2017 08:36:19 -0500 Subject: [PATCH 3/6] send-pack: use skip_prefix for parsing unpack status This avoids repeating ourselves, and the use of magic numbers. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- send-pack.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/send-pack.c b/send-pack.c index 12e229e447..243633da17 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; } From 40d05d04dd338f80b5392e8dea3a5c854798351e Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 7 Mar 2017 08:37:36 -0500 Subject: [PATCH 4/6] send-pack: improve unpack-status error messages 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 Signed-off-by: Junio C Hamano --- send-pack.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/send-pack.c b/send-pack.c index 243633da17..83c23aef6a 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; } From ba69f92db6033d6187414c57547e8f79d6aa7f1b Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 7 Mar 2017 08:38:51 -0500 Subject: [PATCH 5/6] send-pack: read "unpack" status even on pack-objects failure 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 Signed-off-by: Junio C Hamano --- send-pack.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/send-pack.c b/send-pack.c index 83c23aef6a..e152327394 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); From d1a13d3fcb252631361a961cb5e2bf10ed467cba Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 7 Mar 2017 08:39:48 -0500 Subject: [PATCH 6/6] send-pack: report signal death of pack-objects 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 Signed-off-by: Junio C Hamano --- send-pack.c | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/send-pack.c b/send-pack.c index e152327394..d2d2a49a02 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; }