threads / patch / 26509

patchsmart-http: Don't use Expect: 100-Continue

Subject: [PATCH] smart-http: Don't use Expect: 100-Continue

## tl;dr

5 messages between Feb 15, 2011 and Feb 16, 2011. Diffs are folded; open one to read it.

replies: 4people: 3as markdown or json

Shawn O. Pearce· Feb 15, 2011, 16:57 UTC · lore

Some HTTP/1.1 servers or proxies don't correctly implement the 100-Continue feature of HTTP/1.1. Its a difficult feature to implement right, and isn't commonly used by browsers, so many developers may not even be aware that their server (or proxy) doesn't honor it.

Within the smart HTTP protocol for Git we only use this newer "Expect: 100-Continue" feature to probe for missing authentication before uploading a large payload like a pack file during push. If authentication is necessary, we expect the server to send the 401 Not Authorized response before the bulk data transfer starts, thus saving the client bandwidth during the retry.

A different method to probe for working authentication is to send an empty command list (that is just "0000") to $URL/git-receive-pack. or $URL/git-upload-pack. All versions of both receive-pack and upload-pack since the introduction of smart HTTP in Git 1.6.6 cleanly accept just a flush-pkt under --stateless-rpc mode, and exit with success.

If HTTP level authentication is successful, the backend will return an empty response, but with HTTP status code 200. This enables the client to continue with the transfer.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 remote-curl.c |   66 +++++++++++++++++++++++++++++++++++++++++++++++---------
 1 files changed, 55 insertions(+), 11 deletions(-)
Show changes to remote-curl.c +55 −11
diff --git a/remote-curl.c b/remote-curl.c
index 04d4813..3d82dc2 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -356,14 +356,59 @@ static size_t rpc_in(const void *ptr, size_t eltsize,
 	return size;
 }
 
+static int run_slot(struct active_request_slot *slot)
+{
+	int err = 0;
+	struct slot_results results;
+
+	slot->results = &results;
+	slot->curl_result = curl_easy_perform(slot->curl);
+	finish_active_slot(slot);
+
+	if (results.curl_result != CURLE_OK) {
+		err |= error("RPC failed; result=%d, HTTP code = %ld",
+			results.curl_result, results.http_code);
+	}
+
+	return err;
+}
+
+static int probe_rpc(struct rpc_state *rpc)
+{
+	struct active_request_slot *slot;
+	struct curl_slist *headers = NULL;
+	struct strbuf buf = STRBUF_INIT;
+	int err;
+
+	slot = get_active_slot();
+
+	headers = curl_slist_append(headers, rpc->hdr_content_type);
+	headers = curl_slist_append(headers, rpc->hdr_accept);
+
+	curl_easy_setopt(slot->curl, CURLOPT_NOBODY, 0);
+	curl_easy_setopt(slot->curl, CURLOPT_POST, 1);
+	curl_easy_setopt(slot->curl, CURLOPT_URL, rpc->service_url);
+	curl_easy_setopt(slot->curl, CURLOPT_ENCODING, "");
+	curl_easy_setopt(slot->curl, CURLOPT_POSTFIELDS, "0000");
+	curl_easy_setopt(slot->curl, CURLOPT_POSTFIELDSIZE, 4);
+	curl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, headers);
+	curl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, fwrite_buffer);
+	curl_easy_setopt(slot->curl, CURLOPT_FILE, buf);
+
+	err = run_slot(slot);
+
+	curl_slist_free_all(headers);
+	strbuf_release(&buf);
+	return err;
+}
+
 static int post_rpc(struct rpc_state *rpc)
 {
 	struct active_request_slot *slot;
-	struct slot_results results;
 	struct curl_slist *headers = NULL;
 	int use_gzip = rpc->gzip_request;
 	char *gzip_body = NULL;
-	int err = 0, large_request = 0;
+	int err, large_request = 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
@@ -386,8 +431,13 @@ static int post_rpc(struct rpc_state *rpc)
 		rpc->len += n;
 	}
 
+	if (large_request) {
+		err = probe_rpc(rpc);
+		if (err)
+			return err;
+	}
+
 	slot = get_active_slot();
-	slot->results = &results;
 
 	curl_easy_setopt(slot->curl, CURLOPT_NOBODY, 0);
 	curl_easy_setopt(slot->curl, CURLOPT_POST, 1);
@@ -401,7 +451,7 @@ static int post_rpc(struct rpc_state *rpc)
 		/* The request body is large and the size cannot be predicted.
 		 * We must use chunked encoding to send it.
 		 */
-		headers = curl_slist_append(headers, "Expect: 100-continue");
+		headers = curl_slist_append(headers, "Expect:");
 		headers = curl_slist_append(headers, "Transfer-Encoding: chunked");
 		rpc->initial_buffer = 1;
 		curl_easy_setopt(slot->curl, CURLOPT_READFUNCTION, rpc_out);
@@ -475,13 +525,7 @@ static int post_rpc(struct rpc_state *rpc)
 	curl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, rpc_in);
 	curl_easy_setopt(slot->curl, CURLOPT_FILE, rpc);
 
-	slot->curl_result = curl_easy_perform(slot->curl);
-	finish_active_slot(slot);
-
-	if (results.curl_result != CURLE_OK) {
-		err |= error("RPC failed; result=%d, HTTP code = %ld",
-			results.curl_result, results.http_code);
-	}
+	err = run_slot(slot);
 
 	curl_slist_free_all(headers);
 	free(gzip_body);
-- 
1.7.4.rc3.268.g2af8b
Junio C Hamano· Feb 15, 2011, 19:42 UTC · re: Shawn O. Pearce · lore

Re: [PATCH] smart-http: Don't use Expect: 100-Continue

"Shawn O. Pearce" <spearce@spearce.org> writes:
Show 18 quoted lines
> diff --git a/remote-curl.c b/remote-curl.c
> index 04d4813..3d82dc2 100644
> --- a/remote-curl.c
> +++ b/remote-curl.c
> @@ -356,14 +356,59 @@ static size_t rpc_in(const void *ptr, size_t eltsize,
> ...
> +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);
> +
> +	curl_slist_free_all(headers);
> +	strbuf_release(&buf);
> +	return err;
> +}
Hmm, I am getting
    remote-curl.c:403: error: call to '_curl_easy_setopt_err_cb_data' declared
    with attribute warning: curl_easy_setopt expects a private data pointer as
    argument for this option
Shouldn't the above be giving a pointer to buf anyway?
 remote-curl.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
Show changes to remote-curl.c +1 −1
diff --git a/remote-curl.c b/remote-curl.c
index 297ecf7..256326a 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -400,7 +400,7 @@ static int probe_rpc(struct rpc_state *rpc)
 	curl_easy_setopt(slot->curl, CURLOPT_POSTFIELDSIZE, 4);
 	curl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, headers);
 	curl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, fwrite_buffer);
-	curl_easy_setopt(slot->curl, CURLOPT_FILE, buf);
+	curl_easy_setopt(slot->curl, CURLOPT_FILE, &buf);
 
 	err = run_slot(slot);
 
Shawn Pearce· Feb 15, 2011, 23:54 UTC · re: Junio C Hamano · lore

Re: [PATCH] smart-http: Don't use Expect: 100-Continue

On Tue, Feb 15, 2011 at 11:42, Junio C Hamano <gitster@pobox.com> wrote:
Show 28 quoted lines
> "Shawn O. Pearce" <spearce@spearce.org> writes:
>
>> diff --git a/remote-curl.c b/remote-curl.c
>> index 04d4813..3d82dc2 100644
>> --- a/remote-curl.c
>> +++ b/remote-curl.c
>> @@ -356,14 +356,59 @@ static size_t rpc_in(const void *ptr, size_t eltsize,
>> ...
>> +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);
>> +
>> +     curl_slist_free_all(headers);
>> +     strbuf_release(&buf);
>> +     return err;
>> +}
>
> Hmm, I am getting
>
>    remote-curl.c:403: error: call to '_curl_easy_setopt_err_cb_data' declared
>    with attribute warning: curl_easy_setopt expects a private data pointer as
>    argument for this option
>
> Shouldn't the above be giving a pointer to buf anyway?

Yes. Please squash your patch into mine. I'm surprised my build doesn't have sufficient warning flags enabled when I built this. :-(

-- 
Shawn.
Daniel Stenberg· Feb 16, 2011, 07:38 UTC · re: Shawn Pearce · lore

Re: [PATCH] smart-http: Don't use Expect: 100-Continue

On Tue, 15 Feb 2011, Shawn Pearce wrote:
Show 6 quoted lines
>>    remote-curl.c:403: error: call to '_curl_easy_setopt_err_cb_data' declared
>>    with attribute warning: curl_easy_setopt expects a private data pointer as
>>    argument for this option
>
> I'm surprised my build doesn't have sufficient warning flags enabled when I 
> built this. :-(

Those are warnings generated by the macro magic in curl's typecheck-gcc.h header file, and they require gcc 4.3 or later so perhaps you used an older compiler?

-- 
  / daniel.haxx.se
Junio C Hamano· Feb 16, 2011, 18:54 UTC · re: Daniel Stenberg · lore

Re: [PATCH] smart-http: Don't use Expect: 100-Continue

Daniel Stenberg <daniel@haxx.se> writes:
Show 6 quoted lines
> On Tue, 15 Feb 2011, Shawn Pearce wrote:
>> I'm surprised my build doesn't have sufficient warning flags enabled
>> when I built this. :-(
>
> Those are warnings generated by the macro magic in curl's
> typecheck-gcc.h header file, and they require gcc 4.3 or later...

Yes, and thanks for putting them in. I wouldn't have caught this myself by a mere code inspection, as I tend to skim over patches from people with skills known to be above a certain threshold.

← back to recent threads