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

From: Stefan Beller <sbeller@google.com>
Date: May 22, 2018, 00:02 UTC
Message-ID: <CAGZ79kZiyi_1nxvfLttD6HPyV66Wz3pLnuAe=L7FB9ak05dGAQ@mail.gmail.com>
In-Reply-To: <20180521234004.142548-1-bmwill@google.com>

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?

Show 39 quoted lines

> 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?

Show 27 quoted lines

>
>         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
>