{"thread":{"id":"33548","subject":"[PATCH] receive-pack: close sideband fd on early pack errors","startedAt":"2013-04-19T21:24:29Z","lastAt":"2013-04-19T22:30:07Z","messageCount":2,"participants":["Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"214890","messageId":"20130419212429.GA20873@sigill.intra.peff.net","threadId":"33548","inReplyTo":null,"subject":"[PATCH] receive-pack: close sideband fd on early pack errors","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-04-19T21:24:29Z","receivedAt":"2013-04-19T21:24:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Since commit a22e6f8 (receive-pack: send pack-processing\nstderr over sideband, 2012-09-21), receive-pack will start\nan async sideband thread to copy the stderr from our\nindex-pack or unpack-objects child to the client. We hand\nthe thread's input descriptor to unpack(), which puts it in\nthe \"err\" member of the \"struct child_process\".\n\nAfter unpack() returns, we use finish_async() to reap the\nsideband thread. The thread is only ready to die when it\ngets EOF on its pipe, which is connected to the err\ndescriptor. So we expect all of the write ends of that pipe\nto be closed as part of unpack().\n\nNormally, this works fine. After start_command forks, it\ncloses the parent copy of the descriptor. Then once the\nchild exits (whether it was successful or not), that closes\nthe only remaining writer.\n\nHowever, there is one code-path in unpack() that does not\nhandle this. Before we decide which of unpack-objects or\nindex-pack to use, we read the pack header ourselves to see\nhow many objects it contains. If there is an error here, we\nexit without running either sub-command, the pipe descriptor\nremains open, and we are in a deadlock, waiting for the\nsideband thread to die (which is in turn waiting for us to\nclose the pipe).\n\nWe can fix this by making sure that unpack() always closes\nthe pipe before returning.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nThis was triggered in the real world by attempting to push a ref from\na corrupted repository. pack-objects dies on the local end, we get an\neof on the receive-pack end without any data, notice that it's a bogus\npackfile, and hit the deadlock.\n\nThe bug was introduced by a22e6f8, which is in v1.7.12.3, so it should\nbe maint-worthy.\n\n builtin/receive-pack.c | 5 ++++-\n 1 file changed, 4 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex ccebd74..e3eb5fc 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -826,8 +826,11 @@ static const char *unpack(int err_fd)\n \t\t\t    : 0);\n \n \thdr_err = parse_pack_header(&hdr);\n-\tif (hdr_err)\n+\tif (hdr_err) {\n+\t\tif (err_fd > 0)\n+\t\t\tclose(err_fd);\n \t\treturn hdr_err;\n+\t}\n \tsnprintf(hdr_arg, sizeof(hdr_arg),\n \t\t\t\"--pack_header=%\"PRIu32\",%\"PRIu32,\n \t\t\tntohl(hdr.hdr_version), ntohl(hdr.hdr_entries));\n-- \n1.8.2.11.g379c3d8\n"},{"id":"214894","messageId":"7vr4i6xifk.fsf@alter.siamese.dyndns.org","threadId":"33548","inReplyTo":"20130419212429.GA20873@sigill.intra.peff.net","subject":"Re: [PATCH] receive-pack: close sideband fd on early pack errors","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-19T22:30:07Z","receivedAt":"2013-04-19T22:30:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> This was triggered in the real world by attempting to push a ref from\n> a corrupted repository. pack-objects dies on the local end, we get an\n> eof on the receive-pack end without any data, notice that it's a bogus\n> packfile, and hit the deadlock.\n>\n> The bug was introduced by a22e6f8, which is in v1.7.12.3, so it should\n> be maint-worthy.\n\nHmm, well spotted.  Will queue.  Thanks.\n"}]}