{"thread":{"id":"61228","subject":"tests broken with curl-8.7.0","startedAt":"2024-03-30T00:02:14Z","lastAt":"2024-04-05T21:49:25Z","messageCount":15,"participants":["Jeff King","Daniel Stenberg","rsbecker@nexbridge.com","Junio C Hamano","Patrick Steinhardt"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"491867","messageId":"20240330000212.GA1261238@coredump.intra.peff.net","threadId":"61228","inReplyTo":null,"subject":"tests broken with curl-8.7.0","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-03-30T00:02:12Z","receivedAt":"2024-03-30T00:02:14Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"I noticed some http-related failures in the test suite on my Debian\nunstable system, which recently got an upgraded curl package. It looks\nlike it's related to cases where we use the remote-curl helper in\n\"connect\" mode (i.e., protocol v2) and the http buffer is small\n(requiring us to stream the data to curl). Besides just running t5551,\nan easy reproduction is:\n\n  [this works]\n  $ git ls-remote https://git.kernel.org/pub/scm/git/git.git | wc -l\n  1867\n\n  [this doesn't]\n  $ git -c http.postbuffer=65536 ls-remote https://git.kernel.org/pub/scm/git/git.git\n  fatal: expected flush after ref listing\n\nThe error message comes from ls-remote itself, which was expecting a\nFLUSH packet from the remote. Instead it gets the RESPONSE_END from\nremote-curl (remember that in connect mode, remote-curl is just ferrying\nbytes back and forth between ls-remote and the server).\n\nIt works with older versions of libcurl, but not 8.7.0 (or 8.7.1).\nBisecting in libcurl points to 9369c30cd (lib: Curl_read/Curl_write\nclarifications, 2024-02-15).\n\nRunning with GIT_TRACE_CURL=1 shows weirdness on the POST we send to\nissue the ls-refs command. With older curl, I see this:\n\n  => Send header: POST /pub/scm/git/git.git/git-upload-pack HTTP/1.1\n  => Send header: Host: git.kernel.org\n  => Send header: User-Agent: git/2.44.0.789.g252ee96bc5.dirty\n  => Send header: Accept-Encoding: deflate, gzip\n  => Send header: Content-Type: application/x-git-upload-pack-request\n  => Send header: Accept: application/x-git-upload-pack-result\n  => Send header: Git-Protocol: version=2\n  => Send header: Transfer-Encoding: chunked\n  => Send header:\n  => Send data: 14..0014command=ls-refs...\n  => Send data: 2a..002aagent=git/2.44.0.789.g252ee96bc5.dirty..\n  [and so on until...]\n  == Info: Signaling end of chunked upload via terminating chunk.\n\nBut with the broken version, I get:\n\n  => Send header: POST /pub/scm/git/git.git/git-upload-pack HTTP/1.1\n  => Send header: Host: git.kernel.org\n  => Send header: User-Agent: git/2.44.0.789.g252ee96bc5.dirty\n  => Send header: Accept-Encoding: deflate, gzip, br, zstd\n  => Send header: Content-Type: application/x-git-upload-pack-request\n  => Send header: Accept: application/x-git-upload-pack-result\n  => Send header: Git-Protocol: version=2\n  => Send header: Transfer-Encoding: chunked\n  => Send header:\n  => Send data, 0000000014 bytes (0x0000000e)\n  => Send data: 4..0014..0....\n  == Info: upload completely sent off: 14 bytes\n\nSo we only get the first 4 bytes, and then we quit (the double mention\nof 14 is confusing, but I think it is both the size of the pkt-line\n(\"command=ls-refs\\n\") but also the length of the 4-byte string when\nframed with chunked transfer-encoding). Those 4 bytes are the first\nthing returned by rpc_out(), which we use as our CURLOPT_READFUNCTION.\n\nIt's possible that we're doing something wrong with our read/write\nfunction callbacks. But I don't see how; we say \"here's 4 bytes\", but\nthen we never get called again. It's like curl is giving up on trying to\nread the post input early for some reason.\n\nI'm not sure how to dig further. That commit is pretty big and scary. I\ndid check that the tip of master in curl.git is still affected (I'd\nhoped maybe the 0-length write fixes in b30d694a027 would be related,\nbut that's not it).\n\nIdeas?\n\n-Peff\n"},{"id":"491876","messageId":"2n7sn76-p413-5632-4o2s-o5n2p1rqnr5@unkk.fr","threadId":"61228","inReplyTo":"20240330000212.GA1261238@coredump.intra.peff.net","subject":"Re: tests broken with curl-8.7.0","fromName":"Daniel Stenberg","fromEmail":"daniel@haxx.se","sentAt":"2024-03-30T08:54:02Z","receivedAt":"2024-03-30T08:59:45Z","isPatch":false,"sender":{"key":"daniel@haxx.se","avatar":"https://gravatar.com/avatar/69fdca87edd17cee21ca2e79fc2ff671d644603c3dc27167430f3cd3dbab7ba8?d=mp&s=160"},"body":"On Fri, 29 Mar 2024, Jeff King wrote:\n\n> I noticed some http-related failures in the test suite on my Debian unstable \n> system, which recently got an upgraded curl package. It looks like it's \n> related to cases where we use the remote-curl helper in \"connect\" mode \n> (i.e., protocol v2) and the http buffer is small (requiring us to stream the \n> data to curl). Besides just running t5551, an easy reproduction is:\n\nThis smells like a libcurl regression to me. I \"imported\" this into our issue \ntracker here: https://github.com/curl/curl/issues/13229\n\n-- \n\n  / daniel.haxx.se\n"},{"id":"492101","messageId":"20240402200254.GA874754@coredump.intra.peff.net","threadId":"61228","inReplyTo":"2n7sn76-p413-5632-4o2s-o5n2p1rqnr5@unkk.fr","subject":"[PATCH 0/2] git+curl 8.7.0 workaround","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-04-02T20:02:54Z","receivedAt":"2024-04-02T20:03:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Mar 30, 2024 at 09:54:02AM +0100, Daniel Stenberg wrote:\n\n> On Fri, 29 Mar 2024, Jeff King wrote:\n> \n> > I noticed some http-related failures in the test suite on my Debian\n> > unstable system, which recently got an upgraded curl package. It looks\n> > like it's related to cases where we use the remote-curl helper in\n> > \"connect\" mode (i.e., protocol v2) and the http buffer is small\n> > (requiring us to stream the data to curl). Besides just running t5551,\n> > an easy reproduction is:\n> \n> This smells like a libcurl regression to me. I \"imported\" this into our\n> issue tracker here: https://github.com/curl/curl/issues/13229\n\nThis was all resolved in that issue, but just to summarize for the list\nhere: it was a regression in curl and there's a fix already. Thanks\nDaniel for your (as usual) prompt digging into the problem (and likewise\nto Stefan for the actual fix).\n\nUltimately the issue will be fixed by moving to a different version of\nlibcurl, but here's an easy workaround in the meantime, with a small doc\ncleanup I found along the way.\n\n  [1/2]: http: reset POSTFIELDSIZE when clearing curl handle\n  [2/2]: INSTALL: bump libcurl version to 7.21.3\n\n INSTALL | 2 +-\n http.c  | 1 +\n 2 files changed, 2 insertions(+), 1 deletion(-)\n\n-Peff\n"},{"id":"492103","messageId":"20240402200517.GA875182@coredump.intra.peff.net","threadId":"61228","inReplyTo":"20240402200254.GA874754@coredump.intra.peff.net","subject":"[PATCH 1/2] http: reset POSTFIELDSIZE when clearing curl handle","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-04-02T20:05:17Z","receivedAt":"2024-04-02T20:05:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"In get_active_slot(), we return a CURL handle that may have been used\nbefore (reusing them is good because it lets curl reuse the same\nconnection across many requests). We set a few curl options back to\ndefaults that may have been modified by previous requests.\n\nWe reset POSTFIELDS to NULL, but do not reset POSTFIELDSIZE (which\ndefaults to \"-1\"). This usually doesn't matter because most POSTs will\nset both fields together anyway. But there is one exception: when\nhandling a large request in remote-curl's post_rpc(), we don't set\n_either_, and instead set a READFUNCTION to stream data into libcurl.\n\nThis can interact weirdly with a stale POSTFIELDSIZE setting, because\ncurl will assume it should read only some set number of bytes from our\nREADFUNCTION. However, it has worked in practice because we also\nmanually set a \"Transfer-Encoding: chunked\" header, which libcurl uses\nas a clue to set the POSTFIELDSIZE to -1 itself.\n\nSo everything works, but we're better off resetting the size manually\nfor a few reasons:\n\n  - there was a regression in curl 8.7.0 where the chunked header\n    detection didn't kick in, causing any large HTTP requests made by\n    Git to fail. This has since been fixed (but not yet released). In\n    the issue, curl folks recommended setting it explicitly to -1:\n\n      https://github.com/curl/curl/issues/13229#issuecomment-2029826058\n\n    and it indeed works around the regression. So even though it won't\n    be strictly necessary after the fix there, this will help folks who\n    end up using the affected libcurl versions.\n\n  - it's consistent with what a new curl handle would look like. Since\n    get_active_slot() may or may not return a used handle, this reduces\n    the possibility of heisenbugs that only appear with certain request\n    patterns.\n\nNote that the recommendation in the curl issue is to actually drop the\nmanual Transfer-Encoding header. Modern libcurl will add the header\nitself when streaming from a READFUNCTION. However, that code wasn't\nadded until 802aa5ae2 (HTTP: use chunked Transfer-Encoding for HTTP_POST\nif size unknown, 2019-07-22), which is in curl 7.66.0. We claim to\nsupport back to 7.19.5, so those older versions still need the manual\nheader.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n http.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/http.c b/http.c\nindex e73b136e58..3d80bd6116 100644\n--- a/http.c\n+++ b/http.c\n@@ -1452,6 +1452,7 @@ struct active_request_slot *get_active_slot(void)\n \tcurl_easy_setopt(slot->curl, CURLOPT_READFUNCTION, NULL);\n \tcurl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, NULL);\n \tcurl_easy_setopt(slot->curl, CURLOPT_POSTFIELDS, NULL);\n+\tcurl_easy_setopt(slot->curl, CURLOPT_POSTFIELDSIZE, -1L);\n \tcurl_easy_setopt(slot->curl, CURLOPT_UPLOAD, 0);\n \tcurl_easy_setopt(slot->curl, CURLOPT_HTTPGET, 1);\n \tcurl_easy_setopt(slot->curl, CURLOPT_FAILONERROR, 1);\n-- \n2.44.0.789.g5ea01f6724\n\n"},{"id":"492104","messageId":"20240402200600.GB875182@coredump.intra.peff.net","threadId":"61228","inReplyTo":"20240402200254.GA874754@coredump.intra.peff.net","subject":"[PATCH 2/2] INSTALL: bump libcurl version to 7.21.3","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-04-02T20:06:00Z","receivedAt":"2024-04-02T20:06:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Our documentation claims we support curl versions back to 7.19.5. But we\ncan no longer compile with that version since adding an unconditional\nuse of CURLOPT_RESOLVE in 511cfd3bff (http: add custom hostname to IP\naddress resolutions, 2022-05-16). That feature wasn't added to libcurl\nuntil 7.21.3.\n\nWe could add #ifdefs to make this work back to 7.19.5. But given that\nnobody noticed the compilation failure in the intervening two years, it\nmakes more sense to bump the version in the documentation to 7.21.3\n(which is itself over 13 years old).\n\nWe could perhaps go forward even more (which would let us drop some\ncruft from git-curl-compat.h), but this should be an obviously safe\njump, and we can move forward later.\n\nNote that user-visible syntax for CURLOPT_RESOLVE has grown new features\nin subsequent curl versions. Our documentation mentions \"+\" and \"-\"\nentries, which require more recent versions than 7.21.3. We could\nperhaps clarify that in our docs, but it's probably not worth cluttering\nthem with restrictions of ancient curl versions.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n INSTALL | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/INSTALL b/INSTALL\nindex c6fb240c91..2a46d04592 100644\n--- a/INSTALL\n+++ b/INSTALL\n@@ -139,7 +139,7 @@ Issues of note:\n \t  not need that functionality, use NO_CURL to build without\n \t  it.\n \n-\t  Git requires version \"7.19.5\" or later of \"libcurl\" to build\n+\t  Git requires version \"7.21.3\" or later of \"libcurl\" to build\n \t  without NO_CURL. This version requirement may be bumped in\n \t  the future.\n \n-- \n2.44.0.789.g5ea01f6724\n"},{"id":"492105","messageId":"08c401da853b$6c0b8570$44229050$@nexbridge.com","threadId":"61228","inReplyTo":"20240402200254.GA874754@coredump.intra.peff.net","subject":"RE: [PATCH 0/2] git+curl 8.7.0 workaround","fromName":"","fromEmail":"rsbecker@nexbridge.com","sentAt":"2024-04-02T20:21:56Z","receivedAt":"2024-04-02T20:22:41Z","isPatch":true,"sender":{"key":"randall.becker@nexbridge.ca","avatar":"https://avatars.githubusercontent.com/u/28956764?v=4"},"body":"On Tuesday, April 2, 2024 4:03 PM, Peff wrote:\n>To: git@vger.kernel.org\n>Cc: Daniel Stenberg <daniel@haxx.se>\n>Subject: [PATCH 0/2] git+curl 8.7.0 workaround\n>\n>On Sat, Mar 30, 2024 at 09:54:02AM +0100, Daniel Stenberg wrote:\n>\n>> On Fri, 29 Mar 2024, Jeff King wrote:\n>>\n>> > I noticed some http-related failures in the test suite on my Debian\n>> > unstable system, which recently got an upgraded curl package. It\n>> > looks like it's related to cases where we use the remote-curl helper\n>> > in \"connect\" mode (i.e., protocol v2) and the http buffer is small\n>> > (requiring us to stream the data to curl). Besides just running\n>> > t5551, an easy reproduction is:\n>>\n>> This smells like a libcurl regression to me. I \"imported\" this into\n>> our issue tracker here: https://github.com/curl/curl/issues/13229\n>\n>This was all resolved in that issue, but just to summarize for the list\n>here: it was a regression in curl and there's a fix already. Thanks Daniel for your (as\n>usual) prompt digging into the problem (and likewise to Stefan for the actual fix).\n>\n>Ultimately the issue will be fixed by moving to a different version of libcurl, but\n>here's an easy workaround in the meantime, with a small doc cleanup I found along\n>the way.\n>\n>  [1/2]: http: reset POSTFIELDSIZE when clearing curl handle\n>  [2/2]: INSTALL: bump libcurl version to 7.21.3\n>\n> INSTALL | 2 +-\n> http.c  | 1 +\n> 2 files changed, 2 insertions(+), 1 deletion(-)\n\nDo we have an ETA for this fix? That or do we know when curl is planning on resolving this?\n\nThanks,\nRandall\n\n"},{"id":"492108","messageId":"xmqqcyr74ijn.fsf@gitster.g","threadId":"61228","inReplyTo":"20240402200517.GA875182@coredump.intra.peff.net","subject":"Re: [PATCH 1/2] http: reset POSTFIELDSIZE when clearing curl handle","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-04-02T20:27:08Z","receivedAt":"2024-04-02T20:27:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> In get_active_slot(), we return a CURL handle that may have been used\n> before (reusing them is good because it lets curl reuse the same\n> connection across many requests). We set a few curl options back to\n> defaults that may have been modified by previous requests.\n>\n> We reset POSTFIELDS to NULL, but do not reset POSTFIELDSIZE (which\n> defaults to \"-1\"). This usually doesn't matter because most POSTs will\n> set both fields together anyway. But there is one exception: when\n> handling a large request in remote-curl's post_rpc(), we don't set\n> _either_, and instead set a READFUNCTION to stream data into libcurl.\n>\n> This can interact weirdly with a stale POSTFIELDSIZE setting, because\n> curl will assume it should read only some set number of bytes from our\n> READFUNCTION. However, it has worked in practice because we also\n> manually set a \"Transfer-Encoding: chunked\" header, which libcurl uses\n> as a clue to set the POSTFIELDSIZE to -1 itself.\n>\n> So everything works, but we're better off resetting the size manually\n> for a few reasons:\n>\n>   - there was a regression in curl 8.7.0 where the chunked header\n>     detection didn't kick in, causing any large HTTP requests made by\n>     Git to fail. This has since been fixed (but not yet released). In\n>     the issue, curl folks recommended setting it explicitly to -1:\n>\n>       https://github.com/curl/curl/issues/13229#issuecomment-2029826058\n>\n>     and it indeed works around the regression. So even though it won't\n>     be strictly necessary after the fix there, this will help folks who\n>     end up using the affected libcurl versions.\n>\n>   - it's consistent with what a new curl handle would look like. Since\n>     get_active_slot() may or may not return a used handle, this reduces\n>     the possibility of heisenbugs that only appear with certain request\n>     patterns.\n>\n> Note that the recommendation in the curl issue is to actually drop the\n> manual Transfer-Encoding header. Modern libcurl will add the header\n> itself when streaming from a READFUNCTION. However, that code wasn't\n> added until 802aa5ae2 (HTTP: use chunked Transfer-Encoding for HTTP_POST\n> if size unknown, 2019-07-22), which is in curl 7.66.0. We claim to\n> support back to 7.19.5, so those older versions still need the manual\n> header.\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>  http.c | 1 +\n>  1 file changed, 1 insertion(+)\n\nAs always, well articulated.  Thanks.  Will queue.\n\n> diff --git a/http.c b/http.c\n> index e73b136e58..3d80bd6116 100644\n> --- a/http.c\n> +++ b/http.c\n> @@ -1452,6 +1452,7 @@ struct active_request_slot *get_active_slot(void)\n>  \tcurl_easy_setopt(slot->curl, CURLOPT_READFUNCTION, NULL);\n>  \tcurl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, NULL);\n>  \tcurl_easy_setopt(slot->curl, CURLOPT_POSTFIELDS, NULL);\n> +\tcurl_easy_setopt(slot->curl, CURLOPT_POSTFIELDSIZE, -1L);\n>  \tcurl_easy_setopt(slot->curl, CURLOPT_UPLOAD, 0);\n>  \tcurl_easy_setopt(slot->curl, CURLOPT_HTTPGET, 1);\n>  \tcurl_easy_setopt(slot->curl, CURLOPT_FAILONERROR, 1);\n"},{"id":"492109","messageId":"20240402203101.GD875182@coredump.intra.peff.net","threadId":"61228","inReplyTo":"08c401da853b$6c0b8570$44229050$@nexbridge.com","subject":"Re: [PATCH 0/2] git+curl 8.7.0 workaround","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-04-02T20:31:01Z","receivedAt":"2024-04-02T20:31:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Apr 02, 2024 at 04:21:56PM -0400, rsbecker@nexbridge.com wrote:\n\n> Do we have an ETA for this fix? That or do we know when curl is\n> planning on resolving this?\n\nThe fix is in curl's master branch already. According to their\ndocs/RELEASE-PROCEDURE.md, the next release would be May 22. But Daniel\nis the more authoritative word. ;)\n\n-Peff\n"},{"id":"492130","messageId":"20240403032045.GA1559972@coredump.intra.peff.net","threadId":"61228","inReplyTo":"20240402200517.GA875182@coredump.intra.peff.net","subject":"Re: [PATCH 1/2] http: reset POSTFIELDSIZE when clearing curl handle","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-04-03T03:20:45Z","receivedAt":"2024-04-03T03:20:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Apr 02, 2024 at 04:05:17PM -0400, Jeff King wrote:\n\n> So everything works, but we're better off resetting the size manually\n> for a few reasons:\n> \n>   - there was a regression in curl 8.7.0 where the chunked header\n>     detection didn't kick in, causing any large HTTP requests made by\n>     Git to fail. This has since been fixed (but not yet released). In\n>     the issue, curl folks recommended setting it explicitly to -1:\n> \n>       https://github.com/curl/curl/issues/13229#issuecomment-2029826058\n> \n>     and it indeed works around the regression. So even though it won't\n>     be strictly necessary after the fix there, this will help folks who\n>     end up using the affected libcurl versions.\n\nHmph. This isn't quite enough to make things work with 8.7.0, because\nthere are two things wrong there:\n\n  1. curl uses the leftover POSTFIELDSIZE, which this patch fixes. So\n     HTTP/1.1 is fine (including t5551).\n\n  2. In HTTP/2 mode, it sends chunked data, even though HTTP/2 does not\n     use \"Transfer-Encoding: chunked\" (it correctly does not send the\n     header, but it puts unnecessary chunk framing around the data).\n\n     t5559 (which covers HTTP/2) fails, and so does accessing HTTP/2\n     capable hosts like github.com (though score another win for t5559;\n     it has introduced some hassles, but it diagnosed a real problem we\n     would not have otherwise seen in the test suite).\n\nThis second problem can be fixed by eliminating the manual\ntransfer-encoding header, which is what's confusing curl's HTTP/2 code.\nBut as I said earlier...\n\n> Note that the recommendation in the curl issue is to actually drop the\n> manual Transfer-Encoding header. Modern libcurl will add the header\n> itself when streaming from a READFUNCTION. However, that code wasn't\n> added until 802aa5ae2 (HTTP: use chunked Transfer-Encoding for HTTP_POST\n> if size unknown, 2019-07-22), which is in curl 7.66.0. We claim to\n> support back to 7.19.5, so those older versions still need the manual\n> header.\n\n...we can't do so unconditionally. So we'd need something like:\n\ndiff --git a/remote-curl.c b/remote-curl.c\nindex 31b02b8840..215bcc6e10 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -955,7 +955,9 @@ static int post_rpc(struct rpc_state *rpc, int stateless_connect, int flush_rece\n \t\t/* The request body is large and the size cannot be predicted.\n \t\t * We must use chunked encoding to send it.\n \t\t */\n+#if LIBCURL_VERSION_NUM < 0x074200\n \t\theaders = curl_slist_append(headers, \"Transfer-Encoding: chunked\");\n+#endif\n \t\trpc->initial_buffer = 1;\n \t\tcurl_easy_setopt(slot->curl, CURLOPT_READFUNCTION, rpc_out);\n \t\tcurl_easy_setopt(slot->curl, CURLOPT_INFILE, rpc);\n\nI was hoping not to have to carry any version-specific cruft (especially\nsince newer versions of curl should fix the regression). But it's not\ntoo bad, and it actually marks the code as something that we can ditch\nin the future when that version of curl becomes obsolete.\n\nI can try to prepare a cleaner patch for it tomorrow, but comments\nwelcome in the meantime.\n\n-Peff\n"},{"id":"492142","messageId":"Zgz3nvMLg4ts2rRI@tanuki","threadId":"61228","inReplyTo":"20240402200517.GA875182@coredump.intra.peff.net","subject":"Re: [PATCH 1/2] http: reset POSTFIELDSIZE when clearing curl handle","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-04-03T06:30:54Z","receivedAt":"2024-04-03T06:31:01Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Apr 02, 2024 at 04:05:17PM -0400, Jeff King wrote:\n> In get_active_slot(), we return a CURL handle that may have been used\n> before (reusing them is good because it lets curl reuse the same\n> connection across many requests). We set a few curl options back to\n> defaults that may have been modified by previous requests.\n> \n> We reset POSTFIELDS to NULL, but do not reset POSTFIELDSIZE (which\n> defaults to \"-1\"). This usually doesn't matter because most POSTs will\n> set both fields together anyway. But there is one exception: when\n> handling a large request in remote-curl's post_rpc(), we don't set\n> _either_, and instead set a READFUNCTION to stream data into libcurl.\n> \n> This can interact weirdly with a stale POSTFIELDSIZE setting, because\n> curl will assume it should read only some set number of bytes from our\n> READFUNCTION. However, it has worked in practice because we also\n> manually set a \"Transfer-Encoding: chunked\" header, which libcurl uses\n> as a clue to set the POSTFIELDSIZE to -1 itself.\n> \n> So everything works, but we're better off resetting the size manually\n> for a few reasons:\n> \n>   - there was a regression in curl 8.7.0 where the chunked header\n>     detection didn't kick in, causing any large HTTP requests made by\n>     Git to fail. This has since been fixed (but not yet released). In\n>     the issue, curl folks recommended setting it explicitly to -1:\n> \n>       https://github.com/curl/curl/issues/13229#issuecomment-2029826058\n> \n>     and it indeed works around the regression. So even though it won't\n>     be strictly necessary after the fix there, this will help folks who\n>     end up using the affected libcurl versions.\n> \n>   - it's consistent with what a new curl handle would look like. Since\n>     get_active_slot() may or may not return a used handle, this reduces\n>     the possibility of heisenbugs that only appear with certain request\n>     patterns.\n> \n> Note that the recommendation in the curl issue is to actually drop the\n> manual Transfer-Encoding header. Modern libcurl will add the header\n> itself when streaming from a READFUNCTION. However, that code wasn't\n> added until 802aa5ae2 (HTTP: use chunked Transfer-Encoding for HTTP_POST\n> if size unknown, 2019-07-22), which is in curl 7.66.0. We claim to\n> support back to 7.19.5, so those older versions still need the manual\n> header.\n> \n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>  http.c | 1 +\n>  1 file changed, 1 insertion(+)\n> \n> diff --git a/http.c b/http.c\n> index e73b136e58..3d80bd6116 100644\n> --- a/http.c\n> +++ b/http.c\n> @@ -1452,6 +1452,7 @@ struct active_request_slot *get_active_slot(void)\n>  \tcurl_easy_setopt(slot->curl, CURLOPT_READFUNCTION, NULL);\n>  \tcurl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, NULL);\n>  \tcurl_easy_setopt(slot->curl, CURLOPT_POSTFIELDS, NULL);\n> +\tcurl_easy_setopt(slot->curl, CURLOPT_POSTFIELDSIZE, -1L);\n>  \tcurl_easy_setopt(slot->curl, CURLOPT_UPLOAD, 0);\n>  \tcurl_easy_setopt(slot->curl, CURLOPT_HTTPGET, 1);\n>  \tcurl_easy_setopt(slot->curl, CURLOPT_FAILONERROR, 1);\n\nCan't we refactor this code to instead use `curl_easy_reset()`? That\nfunction already resets most of the data we want to reset and would also\nend up setting `POSFIELDSIZE = -1` via `Curl_init_userdefined()`. So\nwouldn't the following be a more sensible fix?\n\ndiff --git a/http.c b/http.c\nindex e73b136e58..e5f5bc23db 100644\n--- a/http.c\n+++ b/http.c\n@@ -1442,20 +1442,14 @@ struct active_request_slot *get_active_slot(void)\n \tslot->finished = NULL;\n \tslot->callback_data = NULL;\n \tslot->callback_func = NULL;\n+\tcurl_easy_reset(slot->curl);\n \tcurl_easy_setopt(slot->curl, CURLOPT_COOKIEFILE, curl_cookie_file);\n \tif (curl_save_cookies)\n \t\tcurl_easy_setopt(slot->curl, CURLOPT_COOKIEJAR, curl_cookie_file);\n \tcurl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, pragma_header);\n \tcurl_easy_setopt(slot->curl, CURLOPT_RESOLVE, host_resolutions);\n \tcurl_easy_setopt(slot->curl, CURLOPT_ERRORBUFFER, curl_errorstr);\n-\tcurl_easy_setopt(slot->curl, CURLOPT_CUSTOMREQUEST, NULL);\n-\tcurl_easy_setopt(slot->curl, CURLOPT_READFUNCTION, NULL);\n-\tcurl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, NULL);\n-\tcurl_easy_setopt(slot->curl, CURLOPT_POSTFIELDS, NULL);\n-\tcurl_easy_setopt(slot->curl, CURLOPT_UPLOAD, 0);\n-\tcurl_easy_setopt(slot->curl, CURLOPT_HTTPGET, 1);\n \tcurl_easy_setopt(slot->curl, CURLOPT_FAILONERROR, 1);\n-\tcurl_easy_setopt(slot->curl, CURLOPT_RANGE, NULL);\n \n \t/*\n \t * Default following to off unless \"ALWAYS\" is configured; this gives\n\nPatrick\n"},{"id":"492143","messageId":"Zgz4fTJg2iL07W_h@tanuki","threadId":"61228","inReplyTo":"Zgz3nvMLg4ts2rRI@tanuki","subject":"Re: [PATCH 1/2] http: reset POSTFIELDSIZE when clearing curl handle","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-04-03T06:34:37Z","receivedAt":"2024-04-03T06:34:41Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Apr 03, 2024 at 08:30:54AM +0200, Patrick Steinhardt wrote:\n> On Tue, Apr 02, 2024 at 04:05:17PM -0400, Jeff King wrote:\n> > In get_active_slot(), we return a CURL handle that may have been used\n> > before (reusing them is good because it lets curl reuse the same\n> > connection across many requests). We set a few curl options back to\n> > defaults that may have been modified by previous requests.\n> > \n> > We reset POSTFIELDS to NULL, but do not reset POSTFIELDSIZE (which\n> > defaults to \"-1\"). This usually doesn't matter because most POSTs will\n> > set both fields together anyway. But there is one exception: when\n> > handling a large request in remote-curl's post_rpc(), we don't set\n> > _either_, and instead set a READFUNCTION to stream data into libcurl.\n> > \n> > This can interact weirdly with a stale POSTFIELDSIZE setting, because\n> > curl will assume it should read only some set number of bytes from our\n> > READFUNCTION. However, it has worked in practice because we also\n> > manually set a \"Transfer-Encoding: chunked\" header, which libcurl uses\n> > as a clue to set the POSTFIELDSIZE to -1 itself.\n> > \n> > So everything works, but we're better off resetting the size manually\n> > for a few reasons:\n> > \n> >   - there was a regression in curl 8.7.0 where the chunked header\n> >     detection didn't kick in, causing any large HTTP requests made by\n> >     Git to fail. This has since been fixed (but not yet released). In\n> >     the issue, curl folks recommended setting it explicitly to -1:\n> > \n> >       https://github.com/curl/curl/issues/13229#issuecomment-2029826058\n> > \n> >     and it indeed works around the regression. So even though it won't\n> >     be strictly necessary after the fix there, this will help folks who\n> >     end up using the affected libcurl versions.\n> > \n> >   - it's consistent with what a new curl handle would look like. Since\n> >     get_active_slot() may or may not return a used handle, this reduces\n> >     the possibility of heisenbugs that only appear with certain request\n> >     patterns.\n> > \n> > Note that the recommendation in the curl issue is to actually drop the\n> > manual Transfer-Encoding header. Modern libcurl will add the header\n> > itself when streaming from a READFUNCTION. However, that code wasn't\n> > added until 802aa5ae2 (HTTP: use chunked Transfer-Encoding for HTTP_POST\n> > if size unknown, 2019-07-22), which is in curl 7.66.0. We claim to\n> > support back to 7.19.5, so those older versions still need the manual\n> > header.\n> > \n> > Signed-off-by: Jeff King <peff@peff.net>\n> > ---\n> >  http.c | 1 +\n> >  1 file changed, 1 insertion(+)\n> > \n> > diff --git a/http.c b/http.c\n> > index e73b136e58..3d80bd6116 100644\n> > --- a/http.c\n> > +++ b/http.c\n> > @@ -1452,6 +1452,7 @@ struct active_request_slot *get_active_slot(void)\n> >  \tcurl_easy_setopt(slot->curl, CURLOPT_READFUNCTION, NULL);\n> >  \tcurl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, NULL);\n> >  \tcurl_easy_setopt(slot->curl, CURLOPT_POSTFIELDS, NULL);\n> > +\tcurl_easy_setopt(slot->curl, CURLOPT_POSTFIELDSIZE, -1L);\n> >  \tcurl_easy_setopt(slot->curl, CURLOPT_UPLOAD, 0);\n> >  \tcurl_easy_setopt(slot->curl, CURLOPT_HTTPGET, 1);\n> >  \tcurl_easy_setopt(slot->curl, CURLOPT_FAILONERROR, 1);\n> \n> Can't we refactor this code to instead use `curl_easy_reset()`? That\n> function already resets most of the data we want to reset and would also\n> end up setting `POSFIELDSIZE = -1` via `Curl_init_userdefined()`. So\n> wouldn't the following be a more sensible fix?\n> \n> diff --git a/http.c b/http.c\n> index e73b136e58..e5f5bc23db 100644\n> --- a/http.c\n> +++ b/http.c\n> @@ -1442,20 +1442,14 @@ struct active_request_slot *get_active_slot(void)\n>  \tslot->finished = NULL;\n>  \tslot->callback_data = NULL;\n>  \tslot->callback_func = NULL;\n> +\tcurl_easy_reset(slot->curl);\n>  \tcurl_easy_setopt(slot->curl, CURLOPT_COOKIEFILE, curl_cookie_file);\n>  \tif (curl_save_cookies)\n>  \t\tcurl_easy_setopt(slot->curl, CURLOPT_COOKIEJAR, curl_cookie_file);\n>  \tcurl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, pragma_header);\n>  \tcurl_easy_setopt(slot->curl, CURLOPT_RESOLVE, host_resolutions);\n>  \tcurl_easy_setopt(slot->curl, CURLOPT_ERRORBUFFER, curl_errorstr);\n> -\tcurl_easy_setopt(slot->curl, CURLOPT_CUSTOMREQUEST, NULL);\n> -\tcurl_easy_setopt(slot->curl, CURLOPT_READFUNCTION, NULL);\n> -\tcurl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, NULL);\n> -\tcurl_easy_setopt(slot->curl, CURLOPT_POSTFIELDS, NULL);\n> -\tcurl_easy_setopt(slot->curl, CURLOPT_UPLOAD, 0);\n> -\tcurl_easy_setopt(slot->curl, CURLOPT_HTTPGET, 1);\n>  \tcurl_easy_setopt(slot->curl, CURLOPT_FAILONERROR, 1);\n> -\tcurl_easy_setopt(slot->curl, CURLOPT_RANGE, NULL);\n>  \n>  \t/*\n>  \t * Default following to off unless \"ALWAYS\" is configured; this gives\n\nOh well, the answer is \"no\", or at least not as easily as this, as the\nfailing tests tell us. I guess it resets more data than we actually want\nit to reset, but I didn't dig any deeper than that.\n\nPatrick\n"},{"id":"492186","messageId":"20240403201858.GA1949464@coredump.intra.peff.net","threadId":"61228","inReplyTo":"Zgz4fTJg2iL07W_h@tanuki","subject":"Re: [PATCH 1/2] http: reset POSTFIELDSIZE when clearing curl handle","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-04-03T20:18:58Z","receivedAt":"2024-04-03T20:19:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Apr 03, 2024 at 08:34:37AM +0200, Patrick Steinhardt wrote:\n\n> > Can't we refactor this code to instead use `curl_easy_reset()`? That\n> > function already resets most of the data we want to reset and would also\n> > end up setting `POSFIELDSIZE = -1` via `Curl_init_userdefined()`. So\n> > wouldn't the following be a more sensible fix?\n> [...]\n> Oh well, the answer is \"no\", or at least not as easily as this, as the\n> failing tests tell us. I guess it resets more data than we actually want\n> it to reset, but I didn't dig any deeper than that.\n\nYeah. The curl setup is really in two parts:\n\n  1. we make a handle in get_curl_handle(), which also sets up a bunch\n     of options. We use that to make a single \"curl_default\" handle, and\n     then when we want a new handle we curl_easy_duphandle() that\n\n  2. when we want to make a request we call get_active_slot(), which\n     will either return an already-used handle or duphandle() a new one.\n     And then reset some options, but also do some more setup.\n\nYour patch touches spot (2), so it's erasing all of the setup done in\n(1). I don't think there's a way to say \"go back to state when we called\nduphandle(), but keep reusing connections, etc\".\n\nPossibly it could be solved by pushing all of the setup from (1) into\n(2). Though that would probably require separating out some config\nhandling, etc, from the actual curl_easy_setopt() calls (we wouldn't\nwant to complain about invalid config for _every_ request, for example,\njust once per program run).\n\nThis is all also a bit more complicated than it needs to be for\nsmart-http. The dumb-http code may want to have several handles going at\nonce to do individual object/pack downloads. Whereas for smart http, we\nreally are only working with one handle, and doing one request at a\ntime. I don't know if we'll ever drop dumb-http support, but there would\nprobably be a lot of cleanup possibilities if we did.\n\n-Peff\n"},{"id":"492321","messageId":"20240405200451.GA3700566@coredump.intra.peff.net","threadId":"61228","inReplyTo":"20240402200254.GA874754@coredump.intra.peff.net","subject":"[PATCH 3/2] remote-curl: add Transfer-Encoding header only for older curl","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-04-05T20:04:51Z","receivedAt":"2024-04-05T20:04:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Apr 02, 2024 at 04:02:54PM -0400, Jeff King wrote:\n\n> Ultimately the issue will be fixed by moving to a different version of\n> libcurl, but here's an easy workaround in the meantime, with a small doc\n> cleanup I found along the way.\n> \n>   [1/2]: http: reset POSTFIELDSIZE when clearing curl handle\n>   [2/2]: INSTALL: bump libcurl version to 7.21.3\n\nHere's a final patch on top that gives us a workaround for the HTTP/2\nhalf of this. It's a cleaned-up version of what I showed earlier.\n\n-- >8 --\nSubject: remote-curl: add Transfer-Encoding header only for older curl\n\nAs of curl 7.66.0, we don't need to manually specify a \"chunked\"\nTransfer-Encoding header. Instead, modern curl deduces the need for it\nin a POST that has a POSTFIELDSIZE of -1 and uses READFUNCTION rather\nthan POSTFIELDS.\n\nThat version is recent enough that we can't just drop the header; we\nneed to do so conditionally. Since it's only a single line, it seems\nlike the simplest thing would just be to keep setting it unconditionally\n(after all, the #ifdefs are much longer than the actual code). But\nthere's another wrinkle: HTTP/2.\n\nCurl may choose to use HTTP/2 under the hood if the server supports it.\nAnd in that protocol, we do not use the chunked encoding for streaming\nat all. Most versions of curl handle this just fine by recognizing and\nremoving the header. But there's a regression in curl 8.7.0 and 8.7.1\nwhere it doesn't, and large requests over HTTP/2 are broken (which t5559\nnotices). That regression has since been fixed upstream, but not yet\nreleased.\n\nMake the setting of this header conditional, which will let Git work\neven with those buggy curl versions. And as a bonus, it serves as a\nreminder that we can eventually clean up the code as we bump the\nsupported curl versions.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nManually tested against curl 8.7.0 (where skipping the header fixes\nt5559), as well as 7.65.0 (which continues to pass t5551, but would fail\nif we didn't set the header).\n\n git-curl-compat.h | 9 +++++++++\n remote-curl.c     | 3 +++\n 2 files changed, 12 insertions(+)\n\ndiff --git a/git-curl-compat.h b/git-curl-compat.h\nindex fd96b3cdff..e1d0bdd273 100644\n--- a/git-curl-compat.h\n+++ b/git-curl-compat.h\n@@ -126,6 +126,15 @@\n #define GIT_CURL_HAVE_CURLSSLSET_NO_BACKENDS\n #endif\n \n+/**\n+ * Versions before curl 7.66.0 (September 2019) required manually setting the\n+ * transfer-encoding for a streaming POST; after that this is handled\n+ * automatically.\n+ */\n+#if LIBCURL_VERSION_NUM < 0x074200\n+#define GIT_CURL_NEED_TRANSFER_ENCODING_HEADER\n+#endif\n+\n /**\n  * CURLOPT_PROTOCOLS_STR and CURLOPT_REDIR_PROTOCOLS_STR were added in 7.85.0,\n  * released in August 2022.\ndiff --git a/remote-curl.c b/remote-curl.c\nindex 31b02b8840..0b6d7815fd 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -1,4 +1,5 @@\n #include \"git-compat-util.h\"\n+#include \"git-curl-compat.h\"\n #include \"config.h\"\n #include \"environment.h\"\n #include \"gettext.h\"\n@@ -955,7 +956,9 @@ static int post_rpc(struct rpc_state *rpc, int stateless_connect, int flush_rece\n \t\t/* The request body is large and the size cannot be predicted.\n \t\t * We must use chunked encoding to send it.\n \t\t */\n+#ifdef GIT_CURL_NEED_TRANSFER_ENCODING_HEADER\n \t\theaders = curl_slist_append(headers, \"Transfer-Encoding: chunked\");\n+#endif\n \t\trpc->initial_buffer = 1;\n \t\tcurl_easy_setopt(slot->curl, CURLOPT_READFUNCTION, rpc_out);\n \t\tcurl_easy_setopt(slot->curl, CURLOPT_INFILE, rpc);\n-- \n2.44.0.872.gbe303efee2\n\n"},{"id":"492323","messageId":"n18qon6-9r33-9224-6rs4-64onqrp4794o@unkk.fr","threadId":"61228","inReplyTo":"20240405200451.GA3700566@coredump.intra.peff.net","subject":"Re: [PATCH 3/2] remote-curl: add Transfer-Encoding header only for older curl","fromName":"Daniel Stenberg","fromEmail":"daniel@haxx.se","sentAt":"2024-04-05T21:30:27Z","receivedAt":"2024-04-05T21:30:34Z","isPatch":true,"sender":{"key":"daniel@haxx.se","avatar":"https://gravatar.com/avatar/69fdca87edd17cee21ca2e79fc2ff671d644603c3dc27167430f3cd3dbab7ba8?d=mp&s=160"},"body":"On Fri, 5 Apr 2024, Jeff King wrote:\n\n> Here's a final patch on top that gives us a workaround for the HTTP/2\n> half of this. It's a cleaned-up version of what I showed earlier.\n\n+1 from me.\n\n(and appologies for this regression, but you know...)\n\n-- \n\n  / daniel.haxx.se\n"},{"id":"492325","messageId":"xmqqle5rtr8d.fsf@gitster.g","threadId":"61228","inReplyTo":"20240405200451.GA3700566@coredump.intra.peff.net","subject":"Re: [PATCH 3/2] remote-curl: add Transfer-Encoding header only for older curl","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-04-05T21:49:22Z","receivedAt":"2024-04-05T21:49:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> ... And as a bonus, it serves as a\n> reminder that we can eventually clean up the code as we bump the\n> supported curl versions.\n\n;-).\n\nThe way git-curl-compat.h lays out various features/workarounds and\nthe lowest version that needs them makes it very easy to do that.\n\nThanks, will queue.\n"}]}