Re: [PATCH 1/2] http: reset POSTFIELDSIZE when clearing curl handle

From: Jeff King <peff@peff.net>
Date: Apr 3, 2024, 03:20 UTC
Message-ID: <20240403032045.GA1559972@coredump.intra.peff.net>
In-Reply-To: <20240402200517.GA875182@coredump.intra.peff.net>

On Tue, Apr 02, 2024 at 04:05:17PM -0400, Jeff King wrote:

Show 13 quoted lines

> So everything works, but we're better off resetting the size manually
> for a few reasons:
> 
>   - there was a regression in curl 8.7.0 where the chunked header
>     detection didn't kick in, causing any large HTTP requests made by
>     Git to fail. This has since been fixed (but not yet released). In
>     the issue, curl folks recommended setting it explicitly to -1:
> 
>       https://github.com/curl/curl/issues/13229#issuecomment-2029826058
> 
>     and it indeed works around the regression. So even though it won't
>     be strictly necessary after the fix there, this will help folks who
>     end up using the affected libcurl versions.

Hmph. This isn't quite enough to make things work with 8.7.0, because there are two things wrong there:

  1. curl uses the leftover POSTFIELDSIZE, which this patch fixes. So
     HTTP/1.1 is fine (including t5551).

  2. In HTTP/2 mode, it sends chunked data, even though HTTP/2 does not
     use "Transfer-Encoding: chunked" (it correctly does not send the
     header, but it puts unnecessary chunk framing around the data).

     t5559 (which covers HTTP/2) fails, and so does accessing HTTP/2
     capable hosts like github.com (though score another win for t5559;
     it has introduced some hassles, but it diagnosed a real problem we
     would not have otherwise seen in the test suite).

This second problem can be fixed by eliminating the manual transfer-encoding header, which is what's confusing curl's HTTP/2 code. But as I said earlier...

Show 7 quoted lines

> Note that the recommendation in the curl issue is to actually drop the
> manual Transfer-Encoding header. Modern libcurl will add the header
> itself when streaming from a READFUNCTION. However, that code wasn't
> added until 802aa5ae2 (HTTP: use chunked Transfer-Encoding for HTTP_POST
> if size unknown, 2019-07-22), which is in curl 7.66.0. We claim to
> support back to 7.19.5, so those older versions still need the manual
> header.

...we can't do so unconditionally. So we'd need something like:

diff --git a/remote-curl.c b/remote-curl.c
index 31b02b8840..215bcc6e10 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -955,7 +955,9 @@ static int post_rpc(struct rpc_state *rpc, int stateless_connect, int flush_rece
 		/* The request body is large and the size cannot be predicted.
 		 * We must use chunked encoding to send it.
 		 */
+#if LIBCURL_VERSION_NUM < 0x074200
 		headers = curl_slist_append(headers, "Transfer-Encoding: chunked");
+#endif
 		rpc->initial_buffer = 1;
 		curl_easy_setopt(slot->curl, CURLOPT_READFUNCTION, rpc_out);
 		curl_easy_setopt(slot->curl, CURLOPT_INFILE, rpc);

I was hoping not to have to carry any version-specific cruft (especially
since newer versions of curl should fix the regression). But it's not
too bad, and it actually marks the code as something that we can ditch
in the future when that version of curl becomes obsolete.

I can try to prepare a cleaner patch for it tomorrow, but comments
welcome in the meantime.

-Peff