[PATCH 1/2] remote-curl: accept all encoding supported by curl

[PATCH 1/2] remote-curl: accept all encoding supported by curl

[Brandon Williams]

Configure curl to accept all encoding which curl supports instead of
only accepting gzip responses.

This is necessary to fix a bug when using an installation of curl which
doesn't support gzip.  Since curl doesn't do any checking to verify that
it supports the encoding set when calling 'curl_easy_setopt()', curl can
end up sending an "Accept-Encoding" header indicating that it supports
a particular encoding when in fact it doesn't.  Instead when the empty
string "" is used when setting `CURLOPT_ENCODING`, curl will send an
"Accept-Encoding" header containing only the encoding methods curl
supports.

Signed-off-by: Brandon Williams <bmwill@google.com>
---
 http.c                      | 2 +-
 remote-curl.c               | 2 +-
 t/t5551-http-fetch-smart.sh | 4 ++--
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/http.c b/http.c
index fed13b216..709150fc7 100644
--- a/http.c
+++ b/http.c
@@ -1788,7 +1788,7 @@ static int http_request(const char *url,
 
 	curl_easy_setopt(slot->curl, CURLOPT_URL, url);
 	curl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, headers);
-	curl_easy_setopt(slot->curl, CURLOPT_ENCODING, "gzip");
+	curl_easy_setopt(slot->curl, CURLOPT_ENCODING, "");
 
 	ret = run_one_slot(slot, &results);
 
diff --git a/remote-curl.c b/remote-curl.c
index ceb05347b..565bba104 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -684,7 +684,7 @@ static int post_rpc(struct rpc_state *rpc)
 	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, "gzip");
+	curl_easy_setopt(slot->curl, CURLOPT_ENCODING, "");
 
 	if (large_request) {
 		/* The request body is large and the size cannot be predicted.
diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
index f5721b4a5..39c65482c 100755
--- a/t/t5551-http-fetch-smart.sh
+++ b/t/t5551-http-fetch-smart.sh
@@ -26,14 +26,14 @@ setup_askpass_helper
 cat >exp <<EOF
 > GET /smart/repo.git/info/refs?service=git-upload-pack HTTP/1.1
 > Accept: */*
-> Accept-Encoding: gzip
+> Accept-Encoding: deflate, gzip
 > Pragma: no-cache
 < HTTP/1.1 200 OK
 < Pragma: no-cache
 < Cache-Control: no-cache, max-age=0, must-revalidate
 < Content-Type: application/x-git-upload-pack-advertisement
 > POST /smart/repo.git/git-upload-pack HTTP/1.1
-> Accept-Encoding: gzip
+> Accept-Encoding: deflate, gzip
 > Content-Type: application/x-git-upload-pack-request
 > Accept: application/x-git-upload-pack-result
 > Content-Length: xxx
-- 
2.17.0.441.gb46fe60e1d-goog

[PATCH 2/2] remote-curl: accept compressed responses with protocol v2

[Brandon Williams]

Configure curl to accept compressed responses when using protocol v2 by
setting `CURLOPT_ENCODING` to "", which indicates that curl should send
an "Accept-Encoding" header with all supported compression encodings.

Signed-off-by: Brandon Williams <bmwill@google.com>
---
 remote-curl.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/remote-curl.c b/remote-curl.c
index 565bba104..99b0bedc6 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -1259,6 +1259,7 @@ static int proxy_request(struct proxy_state *p)
 
 	slot = get_active_slot();
 
+	curl_easy_setopt(slot->curl, CURLOPT_ENCODING, "");
 	curl_easy_setopt(slot->curl, CURLOPT_NOBODY, 0);
 	curl_easy_setopt(slot->curl, CURLOPT_POST, 1);
 	curl_easy_setopt(slot->curl, CURLOPT_URL, p->service_url);
-- 
2.17.0.441.gb46fe60e1d-goog

Re: [PATCH 1/2] remote-curl: accept all encoding supported by curl

[Jonathan Nieder]

Hi,

Brandon Williams wrote:

> Subject: remote-curl: accept all encoding supported by curl

nit: s/encoding/encodings

> Configure curl to accept all encoding which curl supports instead of
> only accepting gzip responses.

Likewise.

> This is necessary to fix a bug when using an installation of curl which
> doesn't support gzip.  Since curl doesn't do any checking to verify that
> it supports the encoding set when calling 'curl_easy_setopt()', curl can
> end up sending an "Accept-Encoding" header indicating that it supports
> a particular encoding when in fact it doesn't.  Instead when the empty
> string "" is used when setting `CURLOPT_ENCODING`, curl will send an
> "Accept-Encoding" header containing only the encoding methods curl
> supports.
>
> Signed-off-by: Brandon Williams <bmwill@google.com>

Thanks for the analysis and fix.

Reported-by: Anton Golubev <anton.golubev@gmail.com>

Also ccing the reporter so we can hopefully get a tested-by.  Anton,
can you test this patch and let us know how it goes?  You can apply it
as follows:

  curl \
    https://public-inbox.org/git/20180521234004.142548-1-bmwill@google.com/raw \
    >patch.txt
  git am -3 patch.txt

Brandon, can the commit message also say a little more about the
motivating context and symptoms?

  $ curl --version
  curl 7.52.1 (arm-openwrt-linux-gnu) libcurl/7.52.1 mbedTLS/2.6.0
  Protocols: file ftp ftps http https
  Features: IPv6 Largefile SSL

The issue is that when curl is built without the "zlib" feature, since
v1.8.0-rc0~14^2 (Enable info/refs gzip decompression in HTTP client,
2012-09-19) we end up requesting "gzip" encoding anyway despite
libcurl not being able to decode it.  Worse, instead of getting a
clear error message indicating so, we end up falling back to "dumb"
http, producing a confusing and difficult to debug result.

> ---
>  http.c                      | 2 +-
>  remote-curl.c               | 2 +-
>  t/t5551-http-fetch-smart.sh | 4 ++--
>  3 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/http.c b/http.c
> index fed13b216..709150fc7 100644
> --- a/http.c
> +++ b/http.c
> @@ -1788,7 +1788,7 @@ static int http_request(const char *url,
>  
>  	curl_easy_setopt(slot->curl, CURLOPT_URL, url);
>  	curl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, headers);
> -	curl_easy_setopt(slot->curl, CURLOPT_ENCODING, "gzip");
> +	curl_easy_setopt(slot->curl, CURLOPT_ENCODING, "");
>  
>  	ret = run_one_slot(slot, &results);
>  
> diff --git a/remote-curl.c b/remote-curl.c
> index ceb05347b..565bba104 100644
> --- a/remote-curl.c
> +++ b/remote-curl.c
> @@ -684,7 +684,7 @@ static int post_rpc(struct rpc_state *rpc)
>  	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, "gzip");
> +	curl_easy_setopt(slot->curl, CURLOPT_ENCODING, "");
>  
>  	if (large_request) {
>  		/* The request body is large and the size cannot be predicted.

Makes sense.

> diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
> index f5721b4a5..39c65482c 100755
> --- a/t/t5551-http-fetch-smart.sh
> +++ b/t/t5551-http-fetch-smart.sh
> @@ -26,14 +26,14 @@ setup_askpass_helper
>  cat >exp <<EOF
>  > GET /smart/repo.git/info/refs?service=git-upload-pack HTTP/1.1
>  > Accept: */*
> -> Accept-Encoding: gzip
> +> Accept-Encoding: deflate, gzip
>  > Pragma: no-cache
>  < HTTP/1.1 200 OK
>  < Pragma: no-cache
>  < Cache-Control: no-cache, max-age=0, must-revalidate
>  < Content-Type: application/x-git-upload-pack-advertisement
>  > POST /smart/repo.git/git-upload-pack HTTP/1.1
> -> Accept-Encoding: gzip
> +> Accept-Encoding: deflate, gzip
>  > Content-Type: application/x-git-upload-pack-request
>  > Accept: application/x-git-upload-pack-result
>  > Content-Length: xxx

If libcurl gains support for another encoding in the future, this test
would start failing.  Can we make the matching less strict?  For
example, how about something like the following for squashing in?

Thanks,
Jonathan

diff --git i/t/t5551-http-fetch-smart.sh w/t/t5551-http-fetch-smart.sh
index 39c65482ce..913089b144 100755
--- i/t/t5551-http-fetch-smart.sh
+++ w/t/t5551-http-fetch-smart.sh
@@ -26,14 +26,14 @@ setup_askpass_helper
 cat >exp <<EOF
 > GET /smart/repo.git/info/refs?service=git-upload-pack HTTP/1.1
 > Accept: */*
-> Accept-Encoding: deflate, gzip
+> Accept-Encoding: ENCODINGS
 > Pragma: no-cache
 < HTTP/1.1 200 OK
 < Pragma: no-cache
 < Cache-Control: no-cache, max-age=0, must-revalidate
 < Content-Type: application/x-git-upload-pack-advertisement
 > POST /smart/repo.git/git-upload-pack HTTP/1.1
-> Accept-Encoding: deflate, gzip
+> Accept-Encoding: ENCODINGS
 > Content-Type: application/x-git-upload-pack-request
 > Accept: application/x-git-upload-pack-result
 > Content-Length: xxx
@@ -79,8 +79,13 @@ test_expect_success 'clone http repository' '
 		/^< Date: /d
 		/^< Content-Length: /d
 		/^< Transfer-Encoding: /d
-	" >act &&
-	test_cmp exp act
+	" >actual &&
+	sed -e "s/^> Accept-Encoding: .*/> Accept-Encoding: ENCODINGS/" \
+			actual >actual.smudged &&
+	test_cmp exp actual.smudged &&
+
+	grep "Accept-Encoding:.*gzip" actual >actual.gzip &&
+	test_line_count = 2 actual.gzip
 '
 
 test_expect_success 'fetch changes via http' '

RE: [PATCH 1/2] remote-curl: accept all encoding supported by curl

[anton.golubev]

Hi Jonathan,

I'd like to confirm, that your patch fixes my problem: I can sync with
google repository now using git and curl version without gzip support. 
Do you know how this patch is going to be released? Just HEAD now and GA in
the next planned release?

Communication looks like follow now:

root@bsb:~# GIT_TRACE_PACKET=1 GIT_CURL_VERBOSE=1 git ls-remote
https://source.developers.google.co m/p/wired-balm-187912/r/dotfiles 2>&1 |
sed -e 's/\(git-[^=]*\)=.*/\1=REDACTED/' -e 's/Authorizatio n:
.*/Authorization: REDACTED/'
> GET /p/wired-balm-187912/r/dotfiles/info/refs?service=git-upload-pack
HTTP/1.1
Host: source.developers.google.com
User-Agent: git/2.17.0
Accept: */*
Accept-Encoding: identity
Cookie: o=git-anton.golubev.gmail.com=REDACTED
Pragma: no-cache

< HTTP/1.1 200 OK
< Cache-Control: no-cache, max-age=0, must-revalidate
< Content-Length: 374
< Content-Type: application/x-git-upload-pack-advertisement
< Expires: Fri, 01 Jan 1980 00:00:00 GMT
< Pragma: no-cache
< X-Content-Type-Options: nosniff
< X-Frame-Options: SAMEORIGIN
< X-Xss-Protection: 1; mode=block
< Date: Sat, 26 May 2018 11:04:41 GMT
< Alt-Svc: hq=":443"; ma=2592000; quic=51303433; quic=51303432;
quic=51303431; quic=51303339; quic=51303335,quic=":443"; ma=2592000;
v="43,42,41,39,35"
<
13:04:41.274561 pkt-line.c:80           packet:          git< #
service=git-upload-pack
13:04:41.274634 pkt-line.c:80           packet:          git< 0000
13:04:41.274693 pkt-line.c:80           packet:          git<
45e2c99dd1790529cc4b7e029b1e9dfcc817d18e HEAD\0 include-tag
multi_ack_detailed multi_ack ofs-delta side-band side-band-64k thin-pack
no-progress shallow no-done allow-tip-sha1-in-want
allow-reachable-sha1-in-want agent=JGit/4-google filter
symref=HEAD:refs/heads/master
13:04:41.274739 pkt-line.c:80           packet:          git<
45e2c99dd1790529cc4b7e029b1e9dfcc817d18e refs/heads/master
13:04:41.274777 pkt-line.c:80           packet:          git< 0000
45e2c99dd1790529cc4b7e029b1e9dfcc817d18e        HEAD
45e2c99dd1790529cc4b7e029b1e9dfcc817d18e        refs/heads/master

Kind regards,
Anton Golubev



-----Original Message-----
From: Jonathan Nieder [mailto:jrnieder@gmail.com] 
Sent: Dienstag, 22. Mai 2018 02:01
To: Brandon Williams <bmwill@google.com>
Cc: git@vger.kernel.org; Anton Golubev <anton.golubev@gmail.com>
Subject: Re: [PATCH 1/2] remote-curl: accept all encoding supported by curl

Hi,

Brandon Williams wrote:

> Subject: remote-curl: accept all encoding supported by curl

nit: s/encoding/encodings

> Configure curl to accept all encoding which curl supports instead of 
> only accepting gzip responses.

Likewise.

> This is necessary to fix a bug when using an installation of curl 
> which doesn't support gzip.  Since curl doesn't do any checking to 
> verify that it supports the encoding set when calling 
> 'curl_easy_setopt()', curl can end up sending an "Accept-Encoding" 
> header indicating that it supports a particular encoding when in fact 
> it doesn't.  Instead when the empty string "" is used when setting 
> `CURLOPT_ENCODING`, curl will send an "Accept-Encoding" header 
> containing only the encoding methods curl supports.
>
> Signed-off-by: Brandon Williams <bmwill@google.com>

Thanks for the analysis and fix.

Reported-by: Anton Golubev <anton.golubev@gmail.com>

Also ccing the reporter so we can hopefully get a tested-by.  Anton, can you
test this patch and let us know how it goes?  You can apply it as follows:

  curl \
 
https://public-inbox.org/git/20180521234004.142548-1-bmwill@google.com/raw \
    >patch.txt
  git am -3 patch.txt

Brandon, can the commit message also say a little more about the motivating
context and symptoms?

  $ curl --version
  curl 7.52.1 (arm-openwrt-linux-gnu) libcurl/7.52.1 mbedTLS/2.6.0
  Protocols: file ftp ftps http https
  Features: IPv6 Largefile SSL

The issue is that when curl is built without the "zlib" feature, since
v1.8.0-rc0~14^2 (Enable info/refs gzip decompression in HTTP client,
2012-09-19) we end up requesting "gzip" encoding anyway despite libcurl not
being able to decode it.  Worse, instead of getting a clear error message
indicating so, we end up falling back to "dumb"
http, producing a confusing and difficult to debug result.

> ---
>  http.c                      | 2 +-
>  remote-curl.c               | 2 +-
>  t/t5551-http-fetch-smart.sh | 4 ++--
>  3 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/http.c b/http.c
> index fed13b216..709150fc7 100644
> --- a/http.c
> +++ b/http.c
> @@ -1788,7 +1788,7 @@ static int http_request(const char *url,
>  
>  	curl_easy_setopt(slot->curl, CURLOPT_URL, url);
>  	curl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, headers);
> -	curl_easy_setopt(slot->curl, CURLOPT_ENCODING, "gzip");
> +	curl_easy_setopt(slot->curl, CURLOPT_ENCODING, "");
>  
>  	ret = run_one_slot(slot, &results);
>  
> diff --git a/remote-curl.c b/remote-curl.c index ceb05347b..565bba104 
> 100644
> --- a/remote-curl.c
> +++ b/remote-curl.c
> @@ -684,7 +684,7 @@ static int post_rpc(struct rpc_state *rpc)
>  	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, "gzip");
> +	curl_easy_setopt(slot->curl, CURLOPT_ENCODING, "");
>  
>  	if (large_request) {
>  		/* The request body is large and the size cannot be
predicted.

Makes sense.

> diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh 
> index f5721b4a5..39c65482c 100755
> --- a/t/t5551-http-fetch-smart.sh
> +++ b/t/t5551-http-fetch-smart.sh
> @@ -26,14 +26,14 @@ setup_askpass_helper  cat >exp <<EOF  > GET 
> /smart/repo.git/info/refs?service=git-upload-pack HTTP/1.1  > Accept: 
> */*
> -> Accept-Encoding: gzip
> +> Accept-Encoding: deflate, gzip
>  > Pragma: no-cache
>  < HTTP/1.1 200 OK
>  < Pragma: no-cache
>  < Cache-Control: no-cache, max-age=0, must-revalidate  < 
> Content-Type: application/x-git-upload-pack-advertisement
>  > POST /smart/repo.git/git-upload-pack HTTP/1.1
> -> Accept-Encoding: gzip
> +> Accept-Encoding: deflate, gzip
>  > Content-Type: application/x-git-upload-pack-request
>  > Accept: application/x-git-upload-pack-result
>  > Content-Length: xxx

If libcurl gains support for another encoding in the future, this test would
start failing.  Can we make the matching less strict?  For example, how
about something like the following for squashing in?

Thanks,
Jonathan

diff --git i/t/t5551-http-fetch-smart.sh w/t/t5551-http-fetch-smart.sh index
39c65482ce..913089b144 100755
--- i/t/t5551-http-fetch-smart.sh
+++ w/t/t5551-http-fetch-smart.sh
@@ -26,14 +26,14 @@ setup_askpass_helper  cat >exp <<EOF  > GET
/smart/repo.git/info/refs?service=git-upload-pack HTTP/1.1  > Accept: */*
-> Accept-Encoding: deflate, gzip
+> Accept-Encoding: ENCODINGS
 > Pragma: no-cache
 < HTTP/1.1 200 OK
 < Pragma: no-cache
 < Cache-Control: no-cache, max-age=0, must-revalidate  < Content-Type:
application/x-git-upload-pack-advertisement
 > POST /smart/repo.git/git-upload-pack HTTP/1.1
-> Accept-Encoding: deflate, gzip
+> Accept-Encoding: ENCODINGS
 > Content-Type: application/x-git-upload-pack-request
 > Accept: application/x-git-upload-pack-result
 > Content-Length: xxx
@@ -79,8 +79,13 @@ test_expect_success 'clone http repository' '
 		/^< Date: /d
 		/^< Content-Length: /d
 		/^< Transfer-Encoding: /d
-	" >act &&
-	test_cmp exp act
+	" >actual &&
+	sed -e "s/^> Accept-Encoding: .*/> Accept-Encoding: ENCODINGS/" \
+			actual >actual.smudged &&
+	test_cmp exp actual.smudged &&
+
+	grep "Accept-Encoding:.*gzip" actual >actual.gzip &&
+	test_line_count = 2 actual.gzip
 '
 
 test_expect_success 'fetch changes via http' '

Re: [PATCH 1/2] remote-curl: accept all encoding supported by curl

[Stefan Beller]

On Mon, May 21, 2018 at 4:40 PM, Brandon Williams <bmwill@google.com> wrote:
> Configure curl to accept all encoding which curl supports instead of
> only accepting gzip responses.

This partially reverts aa90b9697f9 (Enable info/refs gzip decompression
in HTTP client, 2012-09-19), as that specifically called out deflate not being
a good option. Is that worth mentioning in the commit message?

> This is necessary to fix a bug when using an installation of curl which
> doesn't support gzip.  Since curl doesn't do any checking to verify that
> it supports the encoding set when calling 'curl_easy_setopt()', curl can
> end up sending an "Accept-Encoding" header indicating that it supports
> a particular encoding when in fact it doesn't.  Instead when the empty
> string "" is used when setting `CURLOPT_ENCODING`, curl will send an
> "Accept-Encoding" header containing only the encoding methods curl
> supports.
>
> Signed-off-by: Brandon Williams <bmwill@google.com>
> ---
>  http.c                      | 2 +-
>  remote-curl.c               | 2 +-
>  t/t5551-http-fetch-smart.sh | 4 ++--
>  3 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/http.c b/http.c
> index fed13b216..709150fc7 100644
> --- a/http.c
> +++ b/http.c
> @@ -1788,7 +1788,7 @@ static int http_request(const char *url,
>
>         curl_easy_setopt(slot->curl, CURLOPT_URL, url);
>         curl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, headers);
> -       curl_easy_setopt(slot->curl, CURLOPT_ENCODING, "gzip");
> +       curl_easy_setopt(slot->curl, CURLOPT_ENCODING, "");
>
>         ret = run_one_slot(slot, &results);
>
> diff --git a/remote-curl.c b/remote-curl.c
> index ceb05347b..565bba104 100644
> --- a/remote-curl.c
> +++ b/remote-curl.c
> @@ -684,7 +684,7 @@ static int post_rpc(struct rpc_state *rpc)
>         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, "gzip");
> +       curl_easy_setopt(slot->curl, CURLOPT_ENCODING, "");

Looking at the code here, this succeeds if enough memory is available.
There is no check if the given parameter is part of
Curl_all_content_encodings();
https://github.com/curl/curl/blob/e66cca046cef20d00fba89260dfa6b4a3997233d/lib/setopt.c#L429
https://github.com/curl/curl/blob/c675c40295045d4988eeb6291c54eb48f138822f/lib/content_encoding.c#L686

which may be worth checking first?

>
>         if (large_request) {
>                 /* The request body is large and the size cannot be predicted.
> diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
> index f5721b4a5..39c65482c 100755
> --- a/t/t5551-http-fetch-smart.sh
> +++ b/t/t5551-http-fetch-smart.sh
> @@ -26,14 +26,14 @@ setup_askpass_helper
>  cat >exp <<EOF
>  > GET /smart/repo.git/info/refs?service=git-upload-pack HTTP/1.1
>  > Accept: */*
> -> Accept-Encoding: gzip
> +> Accept-Encoding: deflate, gzip
>  > Pragma: no-cache
>  < HTTP/1.1 200 OK
>  < Pragma: no-cache
>  < Cache-Control: no-cache, max-age=0, must-revalidate
>  < Content-Type: application/x-git-upload-pack-advertisement
>  > POST /smart/repo.git/git-upload-pack HTTP/1.1
> -> Accept-Encoding: gzip
> +> Accept-Encoding: deflate, gzip
>  > Content-Type: application/x-git-upload-pack-request
>  > Accept: application/x-git-upload-pack-result
>  > Content-Length: xxx
> --
> 2.17.0.441.gb46fe60e1d-goog
>

Re: [PATCH 1/2] remote-curl: accept all encoding supported by curl

[Jonathan Nieder]

Hi,

Stefan Beller wrote:
> On Mon, May 21, 2018 at 4:40 PM, Brandon Williams <bmwill@google.com> wrote:

>> Configure curl to accept all encoding which curl supports instead of
>> only accepting gzip responses.
>
> This partially reverts aa90b9697f9 (Enable info/refs gzip decompression
> in HTTP client, 2012-09-19), as that specifically called out deflate not being
> a good option. Is that worth mentioning in the commit message?

More specifically, it mentions the wasted 9 extra bytes from including
"deflate, " on the Accept-Encoding line.  I think the extra bandwidth
usage will be okay. :)

[...]
>> -       curl_easy_setopt(slot->curl, CURLOPT_ENCODING, "gzip");
>> +       curl_easy_setopt(slot->curl, CURLOPT_ENCODING, "");
>
> Looking at the code here, this succeeds if enough memory is available.
> There is no check if the given parameter is part of
> Curl_all_content_encodings();
> https://github.com/curl/curl/blob/e66cca046cef20d00fba89260dfa6b4a3997233d/lib/setopt.c#L429
> https://github.com/curl/curl/blob/c675c40295045d4988eeb6291c54eb48f138822f/lib/content_encoding.c#L686
>
> which may be worth checking first?

By "this" are you referring to the preimage or the postimage?  Are you
suggesting a change in git or in libcurl?

Curl_all_content_encodings() is an internal function in libcurl, so
I'm assuming the latter.

Thanks,
Jonathan

Re: [PATCH 1/2] remote-curl: accept all encoding supported by curl

[Daniel Stenberg]

On Mon, 21 May 2018, Jonathan Nieder wrote:

>> Looking at the code here, this succeeds if enough memory is available.
>> There is no check if the given parameter is part of
>> Curl_all_content_encodings();
>
> By "this" are you referring to the preimage or the postimage?  Are you 
> suggesting a change in git or in libcurl?
>
> Curl_all_content_encodings() is an internal function in libcurl, so I'm 
> assuming the latter.

Ack, that certainly isn't the most wonderful API for selecting a compression 
method. In reality, almost everyone sticks to passing on a "" to that option 
to let libcurl pick and ask for the compression algos it knows since both gzip 
and brotli are present only conditionally depending on build options.

I would agree that the libcurl setopt call should probably be made to fail if 
asked to use a compression method not built-in/supported. Then an application 
could in fact try different algos in order until one works or ask to disable 
compression completely.

In the generic HTTP case, it usually makes sense to ask for more than one 
algorthim though, since this is asking the server for a compressed version and 
typically a HTTP client doesn't know which compression methods the server 
offers. Not sure this is actually true to the same extent for git.

-- 

  / daniel.haxx.se

Re: [PATCH 1/2] remote-curl: accept all encoding supported by curl

[Brandon Williams]

On 05/22, Daniel Stenberg wrote:
> On Mon, 21 May 2018, Jonathan Nieder wrote:
> 
> > > Looking at the code here, this succeeds if enough memory is available.
> > > There is no check if the given parameter is part of
> > > Curl_all_content_encodings();
> > 
> > By "this" are you referring to the preimage or the postimage?  Are you
> > suggesting a change in git or in libcurl?
> > 
> > Curl_all_content_encodings() is an internal function in libcurl, so I'm
> > assuming the latter.
> 
> Ack, that certainly isn't the most wonderful API for selecting a compression
> method. In reality, almost everyone sticks to passing on a "" to that option
> to let libcurl pick and ask for the compression algos it knows since both
> gzip and brotli are present only conditionally depending on build options.

Thanks for the clarification.  Sounds like the best option is to
continue with this patch and let curl decide using "".

> 
> I would agree that the libcurl setopt call should probably be made to fail
> if asked to use a compression method not built-in/supported. Then an
> application could in fact try different algos in order until one works or
> ask to disable compression completely.
> 
> In the generic HTTP case, it usually makes sense to ask for more than one
> algorthim though, since this is asking the server for a compressed version
> and typically a HTTP client doesn't know which compression methods the
> server offers. Not sure this is actually true to the same extent for git.
> 
> -- 
> 
>  / daniel.haxx.se

-- 
Brandon Williams

[PATCH v2 1/2] remote-curl: accept all encodings supported by curl

[Brandon Williams]

Configure curl to accept all encodings which curl supports instead of
only accepting gzip responses.

This fixes an issue when using an installation of curl which is built
without the "zlib" feature. Since aa90b9697 (Enable info/refs gzip
decompression in HTTP client, 2012-09-19) we end up requesting "gzip"
encoding anyway despite libcurl not being able to decode it.  Worse,
instead of getting a clear error message indicating so, we end up
falling back to "dumb" http, producing a confusing and difficult to
debug result.

Since curl doesn't do any checking to verify that it supports the a
requested encoding, instead set the curl option `CURLOPT_ENCODING` with
an empty string indicating that curl should send an "Accept-Encoding"
header containing only the encodings supported by curl.

Reported-by: Anton Golubev <anton.golubev@gmail.com>
Signed-off-by: Brandon Williams <bmwill@google.com>
---

Version 2 of this series just tweaks the commit message and the test per
Jonathan's suggestion.

 http.c                      |  2 +-
 remote-curl.c               |  2 +-
 t/t5551-http-fetch-smart.sh | 13 +++++++++----
 3 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/http.c b/http.c
index fed13b216..709150fc7 100644
--- a/http.c
+++ b/http.c
@@ -1788,7 +1788,7 @@ static int http_request(const char *url,
 
 	curl_easy_setopt(slot->curl, CURLOPT_URL, url);
 	curl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, headers);
-	curl_easy_setopt(slot->curl, CURLOPT_ENCODING, "gzip");
+	curl_easy_setopt(slot->curl, CURLOPT_ENCODING, "");
 
 	ret = run_one_slot(slot, &results);
 
diff --git a/remote-curl.c b/remote-curl.c
index ceb05347b..565bba104 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -684,7 +684,7 @@ static int post_rpc(struct rpc_state *rpc)
 	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, "gzip");
+	curl_easy_setopt(slot->curl, CURLOPT_ENCODING, "");
 
 	if (large_request) {
 		/* The request body is large and the size cannot be predicted.
diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
index f5721b4a5..913089b14 100755
--- a/t/t5551-http-fetch-smart.sh
+++ b/t/t5551-http-fetch-smart.sh
@@ -26,14 +26,14 @@ setup_askpass_helper
 cat >exp <<EOF
 > GET /smart/repo.git/info/refs?service=git-upload-pack HTTP/1.1
 > Accept: */*
-> Accept-Encoding: gzip
+> Accept-Encoding: ENCODINGS
 > Pragma: no-cache
 < HTTP/1.1 200 OK
 < Pragma: no-cache
 < Cache-Control: no-cache, max-age=0, must-revalidate
 < Content-Type: application/x-git-upload-pack-advertisement
 > POST /smart/repo.git/git-upload-pack HTTP/1.1
-> Accept-Encoding: gzip
+> Accept-Encoding: ENCODINGS
 > Content-Type: application/x-git-upload-pack-request
 > Accept: application/x-git-upload-pack-result
 > Content-Length: xxx
@@ -79,8 +79,13 @@ test_expect_success 'clone http repository' '
 		/^< Date: /d
 		/^< Content-Length: /d
 		/^< Transfer-Encoding: /d
-	" >act &&
-	test_cmp exp act
+	" >actual &&
+	sed -e "s/^> Accept-Encoding: .*/> Accept-Encoding: ENCODINGS/" \
+			actual >actual.smudged &&
+	test_cmp exp actual.smudged &&
+
+	grep "Accept-Encoding:.*gzip" actual >actual.gzip &&
+	test_line_count = 2 actual.gzip
 '
 
 test_expect_success 'fetch changes via http' '
-- 
2.17.0.441.gb46fe60e1d-goog

[PATCH v2 2/2] remote-curl: accept compressed responses with protocol v2

[Brandon Williams]

Configure curl to accept compressed responses when using protocol v2 by
setting `CURLOPT_ENCODING` to "", which indicates that curl should send
an "Accept-Encoding" header with all supported compression encodings.

Signed-off-by: Brandon Williams <bmwill@google.com>
---
 remote-curl.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/remote-curl.c b/remote-curl.c
index 565bba104..99b0bedc6 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -1259,6 +1259,7 @@ static int proxy_request(struct proxy_state *p)
 
 	slot = get_active_slot();
 
+	curl_easy_setopt(slot->curl, CURLOPT_ENCODING, "");
 	curl_easy_setopt(slot->curl, CURLOPT_NOBODY, 0);
 	curl_easy_setopt(slot->curl, CURLOPT_POST, 1);
 	curl_easy_setopt(slot->curl, CURLOPT_URL, p->service_url);
-- 
2.17.0.441.gb46fe60e1d-goog

Re: [PATCH v2 2/2] remote-curl: accept compressed responses with protocol v2

[Jonathan Nieder]

Brandon Williams wrote:

> Configure curl to accept compressed responses when using protocol v2 by
> setting `CURLOPT_ENCODING` to "", which indicates that curl should send
> an "Accept-Encoding" header with all supported compression encodings.
>
> Signed-off-by: Brandon Williams <bmwill@google.com>
> ---
>  remote-curl.c | 1 +
>  1 file changed, 1 insertion(+)

Yay!

> diff --git a/remote-curl.c b/remote-curl.c
> index 565bba104..99b0bedc6 100644
> --- a/remote-curl.c
> +++ b/remote-curl.c
> @@ -1259,6 +1259,7 @@ static int proxy_request(struct proxy_state *p)
>  
>  	slot = get_active_slot();
>  
> +	curl_easy_setopt(slot->curl, CURLOPT_ENCODING, "");
>  	curl_easy_setopt(slot->curl, CURLOPT_NOBODY, 0);
>  	curl_easy_setopt(slot->curl, CURLOPT_POST, 1);
>  	curl_easy_setopt(slot->curl, CURLOPT_URL, p->service_url);

Can this get a test?

I'm particularly interested in it since it's easy to accidentally
apply this patch to the wrong duplicated place (luckily 'p' is a
different variable name than 'rpc' but it's an easy mistake to make if
applying the patch manually).

With or without such a test,
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Re: [PATCH v2 1/2] remote-curl: accept all encodings supported by curl

[Jonathan Nieder]

Brandon Williams wrote:

> Configure curl to accept all encodings which curl supports instead of
> only accepting gzip responses.
>
> This fixes an issue when using an installation of curl which is built
> without the "zlib" feature. Since aa90b9697 (Enable info/refs gzip
> decompression in HTTP client, 2012-09-19) we end up requesting "gzip"
> encoding anyway despite libcurl not being able to decode it.  Worse,
> instead of getting a clear error message indicating so, we end up
> falling back to "dumb" http, producing a confusing and difficult to
> debug result.
>
> Since curl doesn't do any checking to verify that it supports the a
> requested encoding, instead set the curl option `CURLOPT_ENCODING` with
> an empty string indicating that curl should send an "Accept-Encoding"
> header containing only the encodings supported by curl.

Even better, this means we get the benefit of future of even better
compression algorithms once libcurl learns them.

> Reported-by: Anton Golubev <anton.golubev@gmail.com>
> Signed-off-by: Brandon Williams <bmwill@google.com>
> ---
> Version 2 of this series just tweaks the commit message and the test per
> Jonathan's suggestion.
>
>  http.c                      |  2 +-
>  remote-curl.c               |  2 +-
>  t/t5551-http-fetch-smart.sh | 13 +++++++++----
>  3 files changed, 11 insertions(+), 6 deletions(-)

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Thanks for fixing it.

Patch left unsnipped for reference.

> --- a/http.c
> +++ b/http.c
> @@ -1788,7 +1788,7 @@ static int http_request(const char *url,
>  
>  	curl_easy_setopt(slot->curl, CURLOPT_URL, url);
>  	curl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, headers);
> -	curl_easy_setopt(slot->curl, CURLOPT_ENCODING, "gzip");
> +	curl_easy_setopt(slot->curl, CURLOPT_ENCODING, "");
>  
>  	ret = run_one_slot(slot, &results);
>  
> diff --git a/remote-curl.c b/remote-curl.c
> index ceb05347b..565bba104 100644
> --- a/remote-curl.c
> +++ b/remote-curl.c
> @@ -684,7 +684,7 @@ static int post_rpc(struct rpc_state *rpc)
>  	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, "gzip");
> +	curl_easy_setopt(slot->curl, CURLOPT_ENCODING, "");
>  
>  	if (large_request) {
>  		/* The request body is large and the size cannot be predicted.
> diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
> index f5721b4a5..913089b14 100755
> --- a/t/t5551-http-fetch-smart.sh
> +++ b/t/t5551-http-fetch-smart.sh
> @@ -26,14 +26,14 @@ setup_askpass_helper
>  cat >exp <<EOF
>  > GET /smart/repo.git/info/refs?service=git-upload-pack HTTP/1.1
>  > Accept: */*
> -> Accept-Encoding: gzip
> +> Accept-Encoding: ENCODINGS
>  > Pragma: no-cache
>  < HTTP/1.1 200 OK
>  < Pragma: no-cache
>  < Cache-Control: no-cache, max-age=0, must-revalidate
>  < Content-Type: application/x-git-upload-pack-advertisement
>  > POST /smart/repo.git/git-upload-pack HTTP/1.1
> -> Accept-Encoding: gzip
> +> Accept-Encoding: ENCODINGS
>  > Content-Type: application/x-git-upload-pack-request
>  > Accept: application/x-git-upload-pack-result
>  > Content-Length: xxx
> @@ -79,8 +79,13 @@ test_expect_success 'clone http repository' '
>  		/^< Date: /d
>  		/^< Content-Length: /d
>  		/^< Transfer-Encoding: /d
> -	" >act &&
> -	test_cmp exp act
> +	" >actual &&
> +	sed -e "s/^> Accept-Encoding: .*/> Accept-Encoding: ENCODINGS/" \
> +			actual >actual.smudged &&
> +	test_cmp exp actual.smudged &&
> +
> +	grep "Accept-Encoding:.*gzip" actual >actual.gzip &&
> +	test_line_count = 2 actual.gzip
>  '
>  
>  test_expect_success 'fetch changes via http' '

Re: [PATCH v2 1/2] remote-curl: accept all encodings supported by curl

[Junio C Hamano]

Brandon Williams <bmwill@google.com> writes:

> diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
> index f5721b4a5..913089b14 100755
> --- a/t/t5551-http-fetch-smart.sh
> +++ b/t/t5551-http-fetch-smart.sh
> @@ -26,14 +26,14 @@ setup_askpass_helper
>  cat >exp <<EOF
>  > GET /smart/repo.git/info/refs?service=git-upload-pack HTTP/1.1
>  > Accept: */*
> -> Accept-Encoding: gzip
> +> Accept-Encoding: ENCODINGS
>  > Pragma: no-cache

Is the ordering of these headers determined by the user of cURL
library (i.e. Git), or whatever the version of cURL we happened to
link with happens to produce?

The point is whether the order is expected to be stable, or we are
better off sorting the actual log before comparing.

>  < HTTP/1.1 200 OK
>  < Pragma: no-cache
>  < Cache-Control: no-cache, max-age=0, must-revalidate
>  < Content-Type: application/x-git-upload-pack-advertisement

A similar question for this response.

>  > POST /smart/repo.git/git-upload-pack HTTP/1.1
> -> Accept-Encoding: gzip
> +> Accept-Encoding: ENCODINGS
>  > Content-Type: application/x-git-upload-pack-request
>  > Accept: application/x-git-upload-pack-result
>  > Content-Length: xxx

Ditto for this request.

> @@ -79,8 +79,13 @@ test_expect_success 'clone http repository' '
>  		/^< Date: /d
>  		/^< Content-Length: /d
>  		/^< Transfer-Encoding: /d
> -	" >act &&
> -	test_cmp exp act
> +	" >actual &&
> +	sed -e "s/^> Accept-Encoding: .*/> Accept-Encoding: ENCODINGS/" \
> +			actual >actual.smudged &&
> +	test_cmp exp actual.smudged &&
> +
> +	grep "Accept-Encoding:.*gzip" actual >actual.gzip &&
> +	test_line_count = 2 actual.gzip
>  '

Similarly, how much control do we have to ensure that the test HTTPD
server (1) supports gzip and (2) does not support encoding algos
with confusing names e.g. "funnygzipalgo" that may accidentally
match that pattern?

Thanks.  Not a new issue with this patch, but just being curious if
you or anybody thought about it as a possible issue.

Re: [PATCH v2 1/2] remote-curl: accept all encodings supported by curl

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 1223 bytes --]

On Wed, May 23, 2018 at 10:23:26AM +0900, Junio C Hamano wrote:
> Similarly, how much control do we have to ensure that the test HTTPD
> server (1) supports gzip and (2) does not support encoding algos
> with confusing names e.g. "funnygzipalgo" that may accidentally
> match that pattern?

I feel it's quite likely indeed that pretty much any Apache instance is
going to have the gzip encoding.  Every distributor I know supports it.

As for whether there are confusing alternate algorithms, I think it's
best to just look at the IANA registration[0] to see what people are
using.  Potential matches include gzip, x-gzip (a deprecated alias that
versions of Apache we can use are not likely to support), and
pack200-gzip (a format for Java archives, which we hope the remote side
will not be sending).

Overall, I think this is not likely to be a problem, but if necessary in
the future, we can add a prerequisite that looks in the module directory
for the appropriate module.  We haven't seen an issue with it yet,
though, TTBOMK.

[0] https://www.iana.org/assignments/http-parameters/http-parameters.xml#content-coding
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 867 bytes --]

Re: [PATCH v2 1/2] remote-curl: accept all encodings supported by curl

[Daniel Stenberg]

On Wed, 23 May 2018, Junio C Hamano wrote:

>> -> Accept-Encoding: gzip
>> +> Accept-Encoding: ENCODINGS
>
> Is the ordering of these headers determined by the user of cURL library 
> (i.e. Git), or whatever the version of cURL we happened to link with happens 
> to produce?
>
> The point is whether the order is expected to be stable, or we are better 
> off sorting the actual log before comparing.

The order is not guaranteed by libcurl to be fixed, but it is likely to remain 
stable since we too have test cases and compare outputs with expected outputs! 
=)

Going forward, brotli (br) is going to become more commonly present in that 
header.

-- 

  / daniel.haxx.se