[PATCH v4] remote-curl: fix large pushes with GSSAPI

Due to an interaction between the way libcurl handles GSSAPI authentication over
HTTP and the way git uses libcurl, large pushes (those over http.postBuffer
bytes) would fail due to an authentication failure requiring a rewind of the
curl buffer.  Such a rewind was not possible because the data did not fit into
the entire buffer.

Enable the use of the Expect: 100-continue header for large requests where the
server offers GSSAPI authentication to avoid this issue, since the request would
otherwise fail.  This allows git to get the authentication data right before
sending the pack contents.  Existing cases where pushes would succeed, including
small requests using GSSAPI, still disable the use of 100 Continue, as it causes
problems for some remote HTTP implementations (servers and proxies).

Signed-off-by: brian m. carlson <redacted>
---
 remote-curl.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/remote-curl.c b/remote-curl.c
index c9b891a..2c276de 100644
--- a/remote-curl.c
+++ b/remote-curl.c

@@ -449,6 +449,7 @@ static int post_rpc(struct rpc_state *rpc)
 	char *gzip_body = NULL;
 	size_t gzip_size = 0;
 	int err, large_request = 0;
+	int needs_100_continue = 0;
 
 	/* Try to load the entire request, if we can fit it into the
 	 * allocated buffer space we can use HTTP/1.0 and avoid the

@@ -472,6 +473,8 @@ static int post_rpc(struct rpc_state *rpc)
 	}
 
 	if (large_request) {
+		long authtype = 0;
+
 		do {
 			err = probe_rpc(rpc);
 			if (err == HTTP_REAUTH)

@@ -479,11 +482,19 @@ static int post_rpc(struct rpc_state *rpc)
 		} while (err == HTTP_REAUTH);
 		if (err != HTTP_OK)
 			return -1;
+
+#if LIBCURL_VERSION_NUM >= 0x070a08
+		slot = get_active_slot();
+		curl_easy_getinfo(slot->curl, CURLINFO_HTTPAUTH_AVAIL, &authtype);
+		if (authtype & CURLAUTH_GSSNEGOTIATE)
+			needs_100_continue = 1;
+#endif
 	}
 
 	headers = curl_slist_append(headers, rpc->hdr_content_type);
 	headers = curl_slist_append(headers, rpc->hdr_accept);
-	headers = curl_slist_append(headers, "Expect:");
+	headers = curl_slist_append(headers, needs_100_continue ?
+		"Expect: 100-continue" : "Expect:");
 
 retry:
 	slot = get_active_slot();

-- 
1.8.4.1.635.g55556a5

On Tue, Oct 29, 2013 at 02:36:37AM +0000, brian m. carlson wrote:

 	if (large_request) {
+		long authtype = 0;
+

Minor nit, but this will produce an unused variable warning if the code
in the #if below doesn't get compiled. I don't know how much we care.

+#if LIBCURL_VERSION_NUM >= 0x070a08
+		slot = get_active_slot();
+		curl_easy_getinfo(slot->curl, CURLINFO_HTTPAUTH_AVAIL, &authtype);
+		if (authtype & CURLAUTH_GSSNEGOTIATE)
+			needs_100_continue = 1;
+#endif

I didn't notice this in the last round, but this is somewhat of an abuse
of get_active_slot. It is intended to give you a new, pristine curl slot
that you can use to make a request, and it is yours until you call
finish_active_slot. You're not supposed to look at it after that.

However, we do reuse the curl handles. And in the case of rpc case, we
are only doing one request at a time, so the handle you get is
guaranteed to be the last one used.  So it works in practice, but it
would break if the curl handle code breaks any of these assumptions.

I think the clean way to do it would be to teach the slot code to pull
out the available auth methods, and pass them up through the call chain.
Like this on top of your patch:

diff --git a/http.c b/http.c
index 0ddb164..32fa998 100644
--- a/http.c
+++ b/http.c

@@ -761,6 +761,12 @@ void finish_active_slot(struct active_request_slot *slot)
 	if (slot->results != NULL) {
 		slot->results->curl_result = slot->curl_result;
 		slot->results->http_code = slot->http_code;
+#if LIBCURL_VERSION_NUM >= 0x070a08
+		curl_easy_getinfo(slot->curl, CURLINFO_HTTPAUTH_AVAIL,
+				  &slot->results->authtype);
+#else
+		slot->results->authtype = 0;
+#endif
 	}
 
 	/* Run callback if appropriate */

diff --git a/http.h b/http.h
index d77c1b5..4b32b9b 100644
--- a/http.h
+++ b/http.h

@@ -54,6 +54,7 @@
 struct slot_results {
 	CURLcode curl_result;
 	long http_code;
+	long authtype;
 };
 
 struct active_request_slot {

diff --git a/remote-curl.c b/remote-curl.c
index eaa286c..d026f05 100644
--- a/remote-curl.c
+++ b/remote-curl.c

@@ -383,25 +383,29 @@ static size_t rpc_in(char *ptr, size_t eltsize,
 	return size;
 }
 
-static int run_slot(struct active_request_slot *slot)
+static int run_slot(struct active_request_slot *slot,
+		    struct slot_results *results)
 {
 	int err;
-	struct slot_results results;
+	struct slot_results results_buf;
 
-	slot->results = &results;
+	if (!results)
+		results = &results_buf;
+
+	slot->results = results;
 	slot->curl_result = curl_easy_perform(slot->curl);
 	finish_active_slot(slot);
 
-	err = handle_curl_result(&results);
+	err = handle_curl_result(results);
 	if (err != HTTP_OK && err != HTTP_REAUTH) {
 		error("RPC failed; result=%d, HTTP code = %ld",
-		      results.curl_result, results.http_code);
+		      results->curl_result, results->http_code);
 	}
 
 	return err;
 }
 
-static int probe_rpc(struct rpc_state *rpc)
+static int probe_rpc(struct rpc_state *rpc, struct slot_results *results)
 {
 	struct active_request_slot *slot;
 	struct curl_slist *headers = NULL;

@@ -423,7 +427,7 @@ static int probe_rpc(struct rpc_state *rpc)
 	curl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, fwrite_buffer);
 	curl_easy_setopt(slot->curl, CURLOPT_FILE, &buf);
 
-	err = run_slot(slot);
+	err = run_slot(slot, results);
 
 	curl_slist_free_all(headers);
 	strbuf_release(&buf);

@@ -462,20 +466,16 @@ static int post_rpc(struct rpc_state *rpc)
 	}
 
 	if (large_request) {
-		long authtype = 0;
+		struct slot_results results;
 
 		do {
-			err = probe_rpc(rpc);
+			err = probe_rpc(rpc, &results);
 		} while (err == HTTP_REAUTH);
 		if (err != HTTP_OK)
 			return -1;
 
-#if LIBCURL_VERSION_NUM >= 0x070a08
-		slot = get_active_slot();
-		curl_easy_getinfo(slot->curl, CURLINFO_HTTPAUTH_AVAIL, &authtype);
-		if (authtype & CURLAUTH_GSSNEGOTIATE)
+		if (results.authtype & CURLAUTH_GSSNEGOTIATE)
 			needs_100_continue = 1;
-#endif
 	}
 
 	headers = curl_slist_append(headers, rpc->hdr_content_type);

@@ -572,7 +572,7 @@ retry:
 	curl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, rpc_in);
 	curl_easy_setopt(slot->curl, CURLOPT_FILE, rpc);
 
-	err = run_slot(slot);
+	err = run_slot(slot, NULL);
 	if (err == HTTP_REAUTH && !large_request)
 		goto retry;
 	if (err != HTTP_OK)

That's note tested beyond compiling, but I think it should work. Feel
free to squash it into your patch, or if you'd like, I can split out the
refactoring steps with a commit message for you.

-Peff

On Wed, Oct 30, 2013 at 04:45:10AM -0400, Jeff King wrote:

If you would split it out, that would be great.  Then I'll simply rebase
my patch on top of yours and go from there.

-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187

On Wed, Oct 30, 2013 at 10:40:30PM +0000, brian m. carlson wrote:

If you would split it out, that would be great.  Then I'll simply rebase
my patch on top of yours and go from there.

I just included your patch on top, since it was the residue left over
after committing my refactoring. Please read over the result to make
sure I am not defaming you. :)

I noticed while committing the first patch that we do not actually
follow the "do not look at curl after finish_active_slot" rule for the
content-type. Again, we get away with it because we are not running
multiple slots at the time (we only check content-type during the
initial discovery).

I think the refactoring here is the cleanest thing by the existing
rules, but I also think we could get away with the somewhat simpler
patch of just teaching probe_rpc to grab the AUTHAVAIL (because it still
has the old slot and does not need to call get_active_slot again, and
because we know we are only using a single slot).

Going through all of this, I can't help but be annoyed at how much http
baggage we are carrying around for the curl_multi code for parallel
fetches, which is only used for dumb http. The smart-http code would be
happy with a single curl handle we used each time. But I imagine there
are still people relying on dumb http, and dropping the parallel fetch
would be a pretty severe regression for them.

  [1/3]: http: return curl's AUTHAVAIL via slot_results
  [2/3]: remote-curl: pass curl slot_results back through run_slot
  [3/3]: remote-curl: fix large pushes with GSSAPI

-Peff

Callers of the http code may want to know which auth types
were available for the previous request. But after finishing
with the curl slot, they are not supposed to look at the
curl handle again. We already handle returning other
information via the slot_results struct; let's add a flag to
check the available auth.

Note that older versions of curl did not support this, so we
simply return 0 (something like "-1" would be worse, as the
value is a bitflag and we might accidentally set a flag).
This is sufficient for the callers planned in this series,
who only trigger some optional behavior if particular bits
are set, and can live with a fake "no bits" answer.

Signed-off-by: Jeff King <redacted>
---
 http.c | 6 ++++++
 http.h | 1 +
 2 files changed, 7 insertions(+)

diff --git a/http.c b/http.c
index 0ddb164..5c51865 100644
--- a/http.c
+++ b/http.c

@@ -761,6 +761,12 @@ void finish_active_slot(struct active_request_slot *slot)
 	if (slot->results != NULL) {
 		slot->results->curl_result = slot->curl_result;
 		slot->results->http_code = slot->http_code;
+#if LIBCURL_VERSION_NUM >= 0x070a08
+		curl_easy_getinfo(slot->curl, CURLINFO_HTTPAUTH_AVAIL,
+				  &slot->results->auth_avail);
+#else
+		slot->results->auth_avail = 0;
+#endif
 	}
 
 	/* Run callback if appropriate */

diff --git a/http.h b/http.h
index d77c1b5..81d4843 100644
--- a/http.h
+++ b/http.h

@@ -54,6 +54,7 @@
 struct slot_results {
 	CURLcode curl_result;
 	long http_code;
+	long auth_avail;
 };
 
 struct active_request_slot {

-- 
1.8.4.1.898.g8bf8a41.dirty

Some callers may want to know more than just the integer
error code we return. Let them optionally pass a
slot_results struct to fill in (or NULL if they do not
care). In either case we continue to return the integer
code.

We can also give probe_rpc the same treatment (since it
builds directly on run_slot).

Signed-off-by: Jeff King <redacted>
---
 remote-curl.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/remote-curl.c b/remote-curl.c
index b5ebe01..79db21e 100644
--- a/remote-curl.c
+++ b/remote-curl.c

@@ -383,25 +383,29 @@ static size_t rpc_in(char *ptr, size_t eltsize,
 	return size;
 }
 
-static int run_slot(struct active_request_slot *slot)
+static int run_slot(struct active_request_slot *slot,
+		    struct slot_results *results)
 {
 	int err;
-	struct slot_results results;
+	struct slot_results results_buf;
 
-	slot->results = &results;
+	if (!results)
+		results = &results_buf;
+
+	slot->results = results;
 	slot->curl_result = curl_easy_perform(slot->curl);
 	finish_active_slot(slot);
 
-	err = handle_curl_result(&results);
+	err = handle_curl_result(results);
 	if (err != HTTP_OK && err != HTTP_REAUTH) {
 		error("RPC failed; result=%d, HTTP code = %ld",
-		      results.curl_result, results.http_code);
+		      results->curl_result, results->http_code);
 	}
 
 	return err;
 }
 
-static int probe_rpc(struct rpc_state *rpc)
+static int probe_rpc(struct rpc_state *rpc, struct slot_results *results)
 {
 	struct active_request_slot *slot;
 	struct curl_slist *headers = NULL;

@@ -423,7 +427,7 @@ static int probe_rpc(struct rpc_state *rpc)
 	curl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, fwrite_buffer);
 	curl_easy_setopt(slot->curl, CURLOPT_FILE, &buf);
 
-	err = run_slot(slot);
+	err = run_slot(slot, results);
 
 	curl_slist_free_all(headers);
 	strbuf_release(&buf);

@@ -462,7 +466,7 @@ static int post_rpc(struct rpc_state *rpc)
 
 	if (large_request) {
 		do {
-			err = probe_rpc(rpc);
+			err = probe_rpc(rpc, NULL);
 		} while (err == HTTP_REAUTH);
 		if (err != HTTP_OK)
 			return -1;

@@ -561,7 +565,7 @@ retry:
 	curl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, rpc_in);
 	curl_easy_setopt(slot->curl, CURLOPT_FILE, rpc);
 
-	err = run_slot(slot);
+	err = run_slot(slot, NULL);
 	if (err == HTTP_REAUTH && !large_request)
 		goto retry;
 	if (err != HTTP_OK)

-- 
1.8.4.1.898.g8bf8a41.dirty

From: brian m. carlson <redacted>

Due to an interaction between the way libcurl handles GSSAPI
authentication over HTTP and the way git uses libcurl, large
pushes (those over http.postBuffer bytes) would fail due to
an authentication failure requiring a rewind of the curl
buffer.  Such a rewind was not possible because the data did
not fit into the entire buffer.

Enable the use of the Expect: 100-continue header for large
requests where the server offers GSSAPI authentication to
avoid this issue, since the request would otherwise fail.
This allows git to get the authentication data right before
sending the pack contents.  Existing cases where pushes
would succeed, including small requests using GSSAPI, still
disable the use of 100 Continue, as it causes problems for
some remote HTTP implementations (servers and proxies).

Signed-off-by: brian m. carlson <redacted>
Signed-off-by: Jeff King <redacted>
---
 remote-curl.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/remote-curl.c b/remote-curl.c
index 79db21e..f646b5f 100644
--- a/remote-curl.c
+++ b/remote-curl.c

@@ -442,6 +442,7 @@ static int post_rpc(struct rpc_state *rpc)
 	char *gzip_body = NULL;
 	size_t gzip_size = 0;
 	int err, large_request = 0;
+	int needs_100_continue = 0;
 
 	/* Try to load the entire request, if we can fit it into the
 	 * allocated buffer space we can use HTTP/1.0 and avoid the

@@ -465,16 +466,22 @@ static int post_rpc(struct rpc_state *rpc)
 	}
 
 	if (large_request) {
+		struct slot_results results;
+
 		do {
-			err = probe_rpc(rpc, NULL);
+			err = probe_rpc(rpc, &results);
 		} while (err == HTTP_REAUTH);
 		if (err != HTTP_OK)
 			return -1;
+
+		if (results.auth_avail & CURLAUTH_GSSNEGOTIATE)
+			needs_100_continue = 1;
 	}
 
 	headers = curl_slist_append(headers, rpc->hdr_content_type);
 	headers = curl_slist_append(headers, rpc->hdr_accept);
-	headers = curl_slist_append(headers, "Expect:");
+	headers = curl_slist_append(headers, needs_100_continue ?
+		"Expect: 100-continue" : "Expect:");
 
 retry:
 	slot = get_active_slot();

-- 
1.8.4.1.898.g8bf8a41.dirty

On Thu, Oct 31, 2013 at 02:36:51AM -0400, Jeff King wrote:

From: brian m. carlson <redacted>

Due to an interaction between the way libcurl handles GSSAPI
authentication over HTTP and the way git uses libcurl, large
pushes (those over http.postBuffer bytes) would fail due to
an authentication failure requiring a rewind of the curl
buffer.  Such a rewind was not possible because the data did
not fit into the entire buffer.

Enable the use of the Expect: 100-continue header for large
requests where the server offers GSSAPI authentication to
avoid this issue, since the request would otherwise fail.
This allows git to get the authentication data right before
sending the pack contents.  Existing cases where pushes
would succeed, including small requests using GSSAPI, still
disable the use of 100 Continue, as it causes problems for
some remote HTTP implementations (servers and proxies).

Signed-off-by: brian m. carlson <redacted>
Signed-off-by: Jeff King <redacted>

The entire series looks fine by me.  Thanks for fixing this up.

-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187