{"thread":{"id":"31871","subject":"Fix potential hang in https handshake (v3)","startedAt":"2012-10-19T21:04:20Z","lastAt":"2012-10-19T21:21:59Z","messageCount":3,"participants":["szager@google.com","Jeff King","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"201573","messageId":"5081c054.eD2hEWR8K8zW5vdM%szager@google.com","threadId":"31871","inReplyTo":null,"subject":"Fix potential hang in https handshake (v3)","fromName":"","fromEmail":"szager@google.com","sentAt":"2012-10-19T21:04:20Z","receivedAt":"2012-10-19T21:04:20Z","isPatch":false,"sender":{"key":"szager@google.com","avatar":null},"body":">From 32e06128dbc97ceb0d060c88ec8db204fa51be5c Mon Sep 17 00:00:00 2001\nFrom: Stefan Zager <szager@google.com>\nDate: Thu, 18 Oct 2012 16:23:53 -0700\nSubject: [PATCH] Fix potential hang in https handshake.\n\nIt has been observed that curl_multi_timeout may return a very long\ntimeout value (e.g., 294 seconds and some usec) just before\ncurl_multi_fdset returns no file descriptors for reading.  The\nupshot is that select() will hang for a long time -- long enough for\nan https handshake to be dropped.  The observed behavior is that\nthe git command will hang at the terminal and never transfer any\ndata.\n\nThis patch is a workaround for a probable bug in libcurl.  The bug\nonly seems to manifest around a very specific set of circumstances:\n\n- curl version (from curl/curlver.h):\n\n #define LIBCURL_VERSION_NUM 0x071307\n\n- git-remote-https running on an ubuntu-lucid VM.\n- Connecting through squid proxy running on another VM.\n\nInterestingly, the problem doesn't manifest if a host connects\nthrough squid proxy running on localhost; only if the proxy is on\na separate VM (not sure if the squid host needs to be on a separate\nphysical machine).  That would seem to suggest that this issue\nis timing-sensitive.\n\nThis patch is more or less in line with a recommendation in the\ncurl docs about how to behave when curl_multi_fdset doesn't return\nand file descriptors:\n\nhttp://curl.haxx.se/libcurl/c/curl_multi_fdset.html\n\nSigned-off-by: Stefan Zager <szager@google.com>\n---\n http.c |   11 +++++++++++\n 1 files changed, 11 insertions(+), 0 deletions(-)\n\ndiff --git a/http.c b/http.c\nindex df9bb71..51eef02 100644\n--- a/http.c\n+++ b/http.c\n@@ -631,6 +631,17 @@ void run_active_slot(struct active_request_slot *slot)\n \t\t\tFD_ZERO(&excfds);\n \t\t\tcurl_multi_fdset(curlm, &readfds, &writefds, &excfds, &max_fd);\n \n+\t\t\t/* It can happen that curl_multi_timeout returns a pathologically\n+\t\t\t * long timeout when curl_multi_fdset returns no file descriptors\n+\t\t\t * to read.  See commit message for more details.\n+\t\t\t */\n+\t\t\tif (max_fd < 0 &&\n+\t\t\t    select_timeout.tv_sec > 0 ||\n+\t\t\t    select_timeout.tv_usec > 50000) {\n+\t\t\t\tselect_timeout.tv_sec  = 0;\n+\t\t\t\tselect_timeout.tv_usec = 50000;\n+\t\t\t}\n+\n \t\t\tselect(max_fd+1, &readfds, &writefds, &excfds, &select_timeout);\n \t\t}\n \t}\n-- \n1.7.7.3\n"},{"id":"201574","messageId":"20121019210851.GD24184@sigill.intra.peff.net","threadId":"31871","inReplyTo":"5081c054.eD2hEWR8K8zW5vdM%szager@google.com","subject":"Re: Fix potential hang in https handshake (v3)","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-10-19T21:08:51Z","receivedAt":"2012-10-19T21:08:51Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Oct 19, 2012 at 02:04:20PM -0700, szager@google.com wrote:\n\n> From 32e06128dbc97ceb0d060c88ec8db204fa51be5c Mon Sep 17 00:00:00 2001\n> From: Stefan Zager <szager@google.com>\n> Date: Thu, 18 Oct 2012 16:23:53 -0700\n\nDrop these lines.\n\n> Subject: [PATCH] Fix potential hang in https handshake.\n\nAnd make this one your actual email subject.\n\n> It has been observed that curl_multi_timeout may return a very long\n> timeout value (e.g., 294 seconds and some usec) just before\n> curl_multi_fdset returns no file descriptors for reading.  The\n> upshot is that select() will hang for a long time -- long enough for\n> an https handshake to be dropped.  The observed behavior is that\n> the git command will hang at the terminal and never transfer any\n> data.\n> \n> This patch is a workaround for a probable bug in libcurl.  The bug\n> only seems to manifest around a very specific set of circumstances:\n> \n> - curl version (from curl/curlver.h):\n> \n>  #define LIBCURL_VERSION_NUM 0x071307\n> \n> - git-remote-https running on an ubuntu-lucid VM.\n> - Connecting through squid proxy running on another VM.\n> \n> Interestingly, the problem doesn't manifest if a host connects\n> through squid proxy running on localhost; only if the proxy is on\n> a separate VM (not sure if the squid host needs to be on a separate\n> physical machine).  That would seem to suggest that this issue\n> is timing-sensitive.\n\nThanks, that explanation makes much more sense.\n\n> diff --git a/http.c b/http.c\n> index df9bb71..51eef02 100644\n> --- a/http.c\n> +++ b/http.c\n> @@ -631,6 +631,17 @@ void run_active_slot(struct active_request_slot *slot)\n>  \t\t\tFD_ZERO(&excfds);\n>  \t\t\tcurl_multi_fdset(curlm, &readfds, &writefds, &excfds, &max_fd);\n>  \n> +\t\t\t/* It can happen that curl_multi_timeout returns a pathologically\n> +\t\t\t * long timeout when curl_multi_fdset returns no file descriptors\n> +\t\t\t * to read.  See commit message for more details.\n> +\t\t\t */\n\nMinor nit, but our multi-line comment style is:\n\n  /*\n   * blah blah blah\n   */\n\n> +\t\t\tif (max_fd < 0 &&\n> +\t\t\t    select_timeout.tv_sec > 0 ||\n> +\t\t\t    select_timeout.tv_usec > 50000) {\n> +\t\t\t\tselect_timeout.tv_sec  = 0;\n> +\t\t\t\tselect_timeout.tv_usec = 50000;\n> +\t\t\t}\n\nShould there be parentheses separating the || bit from the &&?\n\n-Peff\n"},{"id":"201575","messageId":"7vipa6i2dk.fsf@alter.siamese.dyndns.org","threadId":"31871","inReplyTo":"20121019210851.GD24184@sigill.intra.peff.net","subject":"Re: Fix potential hang in https handshake (v3)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-10-19T21:21:59Z","receivedAt":"2012-10-19T21:21:59Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n>> +\t\t\tif (max_fd < 0 &&\n>> +\t\t\t    select_timeout.tv_sec > 0 ||\n>> +\t\t\t    select_timeout.tv_usec > 50000) {\n>> +\t\t\t\tselect_timeout.tv_sec  = 0;\n>> +\t\t\t\tselect_timeout.tv_usec = 50000;\n>> +\t\t\t}\n>\n> Should there be parentheses separating the || bit from the &&?\n\nYeah, I think there should be.  Thanks for sharp eyes.\n"}]}