{"thread":{"id":"60510","subject":"[PATCH] remote-curl: avoid hang if curl asks for more data after eof","startedAt":"2023-11-13T11:26:11Z","lastAt":"2023-11-27T18:26:22Z","messageCount":18,"participants":["Jiří Hruška","Jonathan Tan","Junio C Hamano","Patrick Steinhardt"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"484800","messageId":"CAGE_+C6DJMAO0bj5QHoKBBV3gMEMtZ-ajJ9ZnDGYq6eorr-oig@mail.gmail.com","threadId":"60510","inReplyTo":null,"subject":"[PATCH] remote-curl: avoid hang if curl asks for more data after eof","fromName":"Jiří Hruška","fromEmail":"jirka@fud.cz","sentAt":"2023-11-13T11:25:17Z","receivedAt":"2023-11-13T11:26:11Z","isPatch":true,"sender":{"key":"jirka@fud.cz","avatar":"https://gravatar.com/avatar/759c05595fe2491dc29b147bf9e4c8a95bcf97793f75a589f4e0893d7a172e25?d=mp&s=160"},"body":"It has been observed that under some circumstances, libcurl can call\nour `CURLOPT_READFUNCTION` callback `rpc_out()` again even after\nalready getting a return value of zero (EOF) back once before.\n\nThis results in `rpc_read_from_out()` trying to read more from the\nchild process pipe, because `rpc->flush_read_but_not_sent` is reset to\nfalse already the first time EOF is returned, and then the whole\noperation gets stuck - the child process is already trying to read\na response back and will not write anything to the output pipe anymore,\nwhile the parent/remote process is now blocked waiting to read more too\nand never even finishes sending the request.\n\nThis commit addresses the bug with the following changes:\n\n- Move resetting of the `flush_read_but_not_sent` flag to `post_rpc()`,\n just before the `rpc_out()` function would be potentially used the\n next time. This makes the callback return a zero any number of times\n at the end of a finished upload, thus fixing the bug.\n\n- Rename `flush_read_but_not_sent` to `read_from_out_done`, because the\n flag is not as much about reading and sending the \"flush\" as about\n the reading/sending phase being done and that `read_from_out()` must\n not be called for this particular RPC exchange anymore.\n\n- Simplify `post_rpc()` a bit by removing nested `if` statements, extra\n logic and comments. This revealed a bug in `stateless_connect()`,\n where `rpc.len` is reset to 0 after each request but not `rpc.pos`,\n relying on it being reset elsewhere. This is being fixed as well.\n\n- Update all related comments and add a few new ones to help the next\n person understand how the chunked large request flow works easier.\n\n- Rename the user data/context argument of libcurl streaming callbacks\n to be consistently `userdata` everywhere, just like the official\n documentation calls it. The current naming is confusing especially\n where it is called `buffer_` now.\n\n- Reset also `CURLOPT_POSTFIELDSIZE_LARGE` to its default value of -1\n before reusing a handle in http.c / `get_active_slot()`. It can stay\n set from a previous request, e.g. to 4 from an earlier probe \"0000\"\n packet. Seems mostly harmless in practice, but looks confusing when\n looking at libcurl state while debugging problems like this one.\n\nSigned-off-by: Jiri Hruska \n---\nHi all. I would like to report a bug in Git's integration of libcurl for the\nHTTP transport and propose a patch to fix it.\n\n\nI have seen Git get stuck while fetching a large repo sometimes over the years,\nbut I have never paid too much attention to it, because just trying again made\nit work. But this started to happen much more frequently to me a few months back\nand pushed me to investigate better.\n\nI get to work with a large repository with lots of commits, branches and overall\ntraffic and when I run `git fetch --tags --force` on it, there are a lot of refs\nto synchronize even after just a few days. And Git happens to hang for me, like\n50% of the time. After a lot of activity and network I/O, it just gets stuck.\n\nI have dumped the relevant processes and found that it all hangs because there\nis a deadlock between a parent \"git-remote-https\" (curl) process and a child\nprocess handling a \"fetch-pack\" command. Both sides are blocked on reading more\nfrom _their_ side of the IPC pipe, so neither gets to do anything.\n\nThe call stacks where both sides get stuck are as follows:\n\n Parent: git-remote-https origin https://blah@dev.azure.com/blah/_git/meh\n\n 00 ntdll!ZwReadFile\n 01 KERNELBASE!ReadFile\n 02 msvcrt!_read_nolock\n 03 msvcrt!_read <<<<<<<<<<<<<<<<<<<<<<<< fd = child/client.out\n 04 git_remote_https!xread\n 05 git_remote_https!read_in_full\n 06 git_remote_https!get_packet_data\n 07 git_remote_https!packet_read_with_status\n 08 git_remote_https!rpc_read_from_out\n 09 git_remote_https!rpc_out\n 0a libcurl_4!CURLOPT_READFUNCTION callback\n 0b libcurl_4!...\n 0e git_remote_https!run_active_slot\n 0f git_remote_https!run_one_slot\n 10 git_remote_https!run_slot\n 11 git_remote_https!post_rpc\n 12 git_remote_https!rpc_service\n 13 git_remote_https!fetch_git\n 14 git_remote_https!fetch\n 15 git_remote_https!parse_fetch\n 16 git_remote_https!cmd_main\n ...\n\n Child: git fetch-pack --stateless-rpc --stdin --lock-pack --thin\n --no-progress https://blah@dev.azure.com/blah/_git/meh\n\n 00 ntdll!ZwReadFile\n 01 KERNELBASE!ReadFile\n 02 msvcrt!_read_nolock\n 03 msvcrt!_read <<<<<<<<<<<<<<<<<<<<<<<< fd = stdin (1)\n 04 git!xread\n 05 git!read_in_full\n 06 git!get_packet_data\n 07 git!packet_read_with_status\n 08 git!packet_reader_read\n 09 git!get_ack\n 0a git!find_common\n 0b git!do_fetch_patch\n 0c git!fetch_pack\n 0d git!cmd_fetch_pack\n ...\n\nThat is, the child is already trying to read the _response_ of an incremental\ncommon commits negotiation, but the parent, orchestrating the whole RPC\ninteraction, has not finished sending the _request_ data yet and wants more.\n\nI looked at the packets passed between the two processes, I read through the\nwhole rpc_service() - post_rpc() - rpc_out() flow many times, but I could not\nsee how it could all get into this blocked state. It was clear that everything\nworked fine as long as the request data fit into one packet, but once the \"large\nrequest\" code path was taken, things broke and got stuck. But I could not see\nany weirdness in the implementation on the Git side that could trigger this.\n\n\nWhich was a correct observation, because the weirdness happens in libcurl!\nIt turns out that curl's function readwrite_upload() in lib/transfer.c [1] can\ncall the configured CURLOPT_READFUNCTION callback multiple times even if already\nat EOF, even if the user callback has previously returned a zero to signal the\nend of the data to upload before. It does not happen every time, only if the\nnext component in curl (OS socket, http/2 library, ...) is not able to send all\nthe data right away (and readwrite_upload() then runs again with offset > 0).\n\n[1]: https://github.com/curl/curl/blob/ad051e1cbec68b2456a22661bf0800d49d085294/lib/transfer.c#L810\n\nThis is harmless when using the default fread() implementation, which the whole\nCURLOPT_READFUNCTION is based on. But Git's rpc_out() function is not safe in\nthis regard. rpc->flush_read_but_not_sent is reset to false already before\nreturning a zero (EOF), so the next invocation goes to rpc_read_from_out()\nagain, reads from the silent pipe and blocks the upload from ever finishing.\n\n\nA trivial solution would be to just take the line which resets the flag\n\n /*\n * The line length either does not need to be sent at\n * all or has already been completely sent. Now we can\n * return 0, indicating EOF, meaning that the flush has\n * been fully sent.\n */\n- rpc->flush_read_but_not_sent = 0;\n return 0;\n\nfrom rpc_out() and reset it only in post_rpc(), before the next time a large\nrequest is being sent out and rpc_out() will go into play again:\n\n if (large_request) {\n ...\n rpc->initial_buffer = 1;\n+ rpc->flush_read_but_not_sent = 0;\n curl_easy_setopt(slot->curl, CURLOPT_READFUNCTION, rpc_out);\n\nThis way the CURLOPT_READFUNCTION would be returning zeros at the end of the\nupload as long as needed, just like fread() at the end of a real file.\n\nHence, the bug could be fixed with just that two-lines change.\n\n\nBut while trying to figure out the above, I noticed a few things that prolonged\nthe time I needed to understand what was going on, so I would like to propose\nsome more changes to make the code perhaps a bit easier to read for the next\nperson who comes to hack on it after me.\n\nThe description of the extra modifications is in the commit message. All of\nthese changes are obviously optional and naturally subjective. I think that we\ncan all agree on some points (less indentation = good), but naming is hard,\nand so is balance between unclear and too verbose, or when to split all\nnon-functional changes to a separate commit. So let me know if there are things\nto do differently and I will gladly obey, it is your codebase after all.\n\n\nWhich brings me to the next topic, testing.\n\nValidating the fix would be trivial with a mocked libcurl, but turns out to be\nmuch harder with the integration-level test suite of this project.\n\nI have validated this solution against a patched version of libcurl:\n\n diff --git a/lib/transfer.c b/lib/transfer.c\n index ba0410fc5..82141b648 100644\n --- a/lib/transfer.c\n +++ b/lib/transfer.c\n @@ -915,6 +915,9 @@ static CURLcode readwrite_upload(struct Curl_easy *data,\n k->upload_fromhere += offset;\n result = Curl_fillreadbuffer(data,\ndata->set.upload_buffer_size-offset,\n &fillcount);\n + if(!result && fillcount == 0)\n + result = Curl_fillreadbuffer(data,\ndata->set.upload_buffer_size-offset,\n + &fillcount);\n k->upload_fromhere -= offset;\n if(result)\n return result;\n\nI validated this solution on Windows, in the real-life case of the repo which\ntriggers these problems, managing to always fetch it smoothly after the fix.\n\nBut outside of that, and as far as the bundled test suite goes, I have failed to\nwrite a test that could validate this problem does not ever occur again.\n\nFor all I know, the special case when curl calls the callback again happens only\non some operating systems (Windows), only with some versions of libcurl, only\nwith some dependencies (libnghttp2, OpenSSL) only when talking to some remote\nservers (Microsoft Azure DevOps repos) over some protocols (HTTP/2), and only\nwith some back luck (network speed, TCP/IP window size, moon phase, ...).\n\nActually, I found at least one report from the past (2017) on Linux [2], which\nseems to show exactly the same symptoms. Hard to tell if the root cause was\nreally the same too, though. The Internet seems full of \"git fetch/pull hangs\"\nstories and there are always helpful people who jump in with their proven\nsolution of \"git config --global http.postBuffer 100000000000000\"... If this\nbug fix could stop some of that nonsense, that would be awesome.\n\n[2]: https://public-inbox.org/git/xmqqefzoxeki.fsf@gitster.mtv.corp.google.com/T/\n\nAnyway, when I enabled extra GIT_TRACE_PACKET and GIT_CURL_VERBOSE, I could not\nreproduce it anymore. When I tried to play with some of the t/*http*.sh\ntestcases, I did not manage to reproduce it either. And even then, reproducing\nit _sometimes_ is not the same as reproducing it _always_. A flaky test is\noften worse than no test at all.\n\nSo, I do not know how to write a test for this. Any suggestions are welcome.\n\n\nBased on the guidance from Documentation/SubmittingPatches, as a bug fix, the\npatch is based on the current `maint` (2.42.1). But applies cleanly to `master`,\n`next` and `seen` as of 2023-11-12 too.\n\nI have added Jeff and Jonathan to CC based on some previous commits to similar\nplaces in remote-curl.c. Sorry if not or no longer relevant.\n\nThank you for your kind consideration.\n\nJiri\n\n http.c | 1 +\n remote-curl.c | 93 +++++++++++++++++++++++++++++----------------------\n 2 files changed, 54 insertions(+), 40 deletions(-)\n\ndiff --git a/http.c b/http.c\nindex 8f71bf00d8..14f2fbb82e 100644\n--- a/http.c\n+++ b/http.c\n@@ -1454,6 +1454,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_LARGE, (curl_off_t)-1);\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);\ndiff --git a/remote-curl.c b/remote-curl.c\nindex ef05752ca5..c043836214 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -606,13 +606,14 @@ struct rpc_state {\n \tunsigned write_line_lengths : 1;\n\n \t/*\n-\t * Used by rpc_out; initialize to 0. This is true if a flush has been\n-\t * read, but the corresponding line length (if write_line_lengths is\n-\t * true) and EOF have not been sent to libcurl. Since each flush marks\n-\t * the end of a request, each flush must be completely sent before any\n-\t * further reading occurs.\n+\t * Used by rpc_out; initialize to 0. This is true if a flush packet\n+\t * has been read from the child process, signaling the end of the\n+\t * current data to send. There might be still some bytes pending in\n+\t * 'buf' (e.g. the corresponding line length, if write_line_lengths\n+\t * is true), but no more reads can be performed on the 'out' pipe as\n+\t * part of the current RPC exchange.\n \t */\n-\tunsigned flush_read_but_not_sent : 1;\n+\tunsigned read_from_out_done : 1;\n };\n\n #define RPC_STATE_INIT { 0 }\n@@ -675,48 +676,51 @@ static int rpc_read_from_out(struct rpc_state\n*rpc, int options,\n \treturn 1;\n }\n\n+/*\n+ * CURLOPT_READFUNCTION callback, called by libcurl when it wants more data\n+ * to send out. Used only if the request did not fit into just one buffer and\n+ * data must be streamed as it comes.\n+ * Has the same semantics as fread(), but reads packets from the pipe from\n+ * the child process instead. A return value of 0 (EOF) finishes the upload.\n+ */\n static size_t rpc_out(void *ptr, size_t eltsize,\n-\t\tsize_t nmemb, void *buffer_)\n+\t\tsize_t nmemb, void *userdata)\n {\n \tsize_t max = eltsize * nmemb;\n-\tstruct rpc_state *rpc = buffer_;\n+\tstruct rpc_state *rpc = userdata;\n \tsize_t avail = rpc->len - rpc->pos;\n \tenum packet_read_status status;\n\n-\tif (!avail) {\n+\t/*\n+\t * If there is no more data available in our buffer and the child\n+\t * process is not done sending yet, read the next packet.\n+\t */\n+\tif (!avail && !rpc->read_from_out_done) {\n \t\trpc->initial_buffer = 0;\n \t\trpc->len = 0;\n \t\trpc->pos = 0;\n-\t\tif (!rpc->flush_read_but_not_sent) {\n-\t\t\tif (!rpc_read_from_out(rpc, 0, &avail, &status))\n-\t\t\t\tBUG(\"The entire rpc->buf should be larger than LARGE_PACKET_MAX\");\n-\t\t\tif (status == PACKET_READ_FLUSH)\n-\t\t\t\trpc->flush_read_but_not_sent = 1;\n-\t\t}\n+\t\tif (!rpc_read_from_out(rpc, 0, &avail, &status))\n+\t\t\tBUG(\"The entire rpc->buf should be larger than LARGE_PACKET_MAX\");\n+\n \t\t/*\n-\t\t * If flush_read_but_not_sent is true, we have already read one\n-\t\t * full request but have not fully sent it + EOF, which is why\n-\t\t * we need to refrain from reading.\n-\t\t */\n-\t}\n-\tif (rpc->flush_read_but_not_sent) {\n-\t\tif (!avail) {\n-\t\t\t/*\n-\t\t\t * The line length either does not need to be sent at\n-\t\t\t * all or has already been completely sent. Now we can\n-\t\t\t * return 0, indicating EOF, meaning that the flush has\n-\t\t\t * been fully sent.\n-\t\t\t */\n-\t\t\trpc->flush_read_but_not_sent = 0;\n-\t\t\treturn 0;\n-\t\t}\n-\t\t/*\n-\t\t * If avail is non-zero, the line length for the flush still\n-\t\t * hasn't been fully sent. Proceed with sending the line\n-\t\t * length.\n+\t\t * If a flush packet was read, it means the child process is\n+\t\t * done sending this request. The buffer might be fully empty\n+\t\t * at this point or contain a flush packet too, depending on\n+\t\t * rpc->write_line_lengths.\n+\t\t * In any case, we must refrain from reading any more, because\n+\t\t * the child process already expects to receive a response back\n+\t\t * instead. If both sides would try to read at once, they would\n+\t\t * just hang waiting for each other.\n \t\t */\n+\t\trpc->read_from_out_done = (status == PACKET_READ_FLUSH);\n \t}\n\n+\t/*\n+\t * Copy data to the provided buffer. If there is nothing more to send,\n+\t * nothing gets written and the return value is 0 (EOF).\n+\t * It is important to keep returning 0 as long as needed in that case,\n+\t * as libcurl invokes the callback multiple times at EOF sometimes.\n+\t */\n \tif (max < avail)\n \t\tavail = max;\n \tmemcpy(ptr, rpc->buf + rpc->pos, avail);\n@@ -724,9 +728,16 @@ static size_t rpc_out(void *ptr, size_t eltsize,\n \treturn avail;\n }\n\n-static int rpc_seek(void *clientp, curl_off_t offset, int origin)\n+/*\n+ * CURLOPT_SEEKFUNCTION callback, called by libcurl when it wants to seek in\n+ * the data being sent out. Used only if the request did not fit into just\n+ * one buffer and data must be streamed as it comes.\n+ * Has the same semantics as fseek(), but seeks in the buffered packet read\n+ * from the pipe from the child process instead.\n+ */\n+static int rpc_seek(void *userdata, curl_off_t offset, int origin)\n {\n-\tstruct rpc_state *rpc = clientp;\n+\tstruct rpc_state *rpc = userdata;\n\n \tif (origin != SEEK_SET)\n \t\tBUG(\"rpc_seek only handles SEEK_SET, not %d\", origin);\n@@ -800,10 +811,10 @@ struct rpc_in_data {\n * from ptr.\n */\n static size_t rpc_in(char *ptr, size_t eltsize,\n-\t\tsize_t nmemb, void *buffer_)\n+\t\tsize_t nmemb, void *userdata)\n {\n \tsize_t size = eltsize * nmemb;\n-\tstruct rpc_in_data *data = buffer_;\n+\tstruct rpc_in_data *data = userdata;\n \tlong response_code;\n\n \tif (curl_easy_getinfo(data->slot->curl, CURLINFO_RESPONSE_CODE,\n@@ -963,6 +974,7 @@ static int post_rpc(struct rpc_state *rpc, int\nstateless_connect, int flush_rece\n \t\t */\n \t\theaders = curl_slist_append(headers, \"Transfer-Encoding: chunked\");\n \t\trpc->initial_buffer = 1;\n+\t\trpc->read_from_out_done = 0;\n \t\tcurl_easy_setopt(slot->curl, CURLOPT_READFUNCTION, rpc_out);\n \t\tcurl_easy_setopt(slot->curl, CURLOPT_INFILE, rpc);\n \t\tcurl_easy_setopt(slot->curl, CURLOPT_SEEKFUNCTION, rpc_seek);\n@@ -1482,7 +1494,7 @@ static int stateless_connect(const char *service_name)\n \trpc.gzip_request = 1;\n \trpc.initial_buffer = 0;\n \trpc.write_line_lengths = 1;\n-\trpc.flush_read_but_not_sent = 0;\n+\trpc.read_from_out_done = 0;\n\n \t/*\n \t * Dump the capability listing that we got from the server earlier\n@@ -1505,6 +1517,7 @@ static int stateless_connect(const char *service_name)\n \t\t\tbreak;\n \t\t/* Reset the buffer for next request */\n \t\trpc.len = 0;\n+\t\trpc.pos = 0;\n \t}\n\n \tfree(rpc.service_url);\n-- \n2.42.1.1.g18afbdf3b0\n"},{"id":"484817","messageId":"20231113212243.1495815-1-jonathantanmy@google.com","threadId":"60510","inReplyTo":"CAGE_+C6DJMAO0bj5QHoKBBV3gMEMtZ-ajJ9ZnDGYq6eorr-oig@mail.gmail.com","subject":"Re: [PATCH] remote-curl: avoid hang if curl asks for more data after eof","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2023-11-13T21:22:42Z","receivedAt":"2023-11-13T21:22:49Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Jiří Hruška writes:\n> It has been observed that under some circumstances, libcurl can call\n> our `CURLOPT_READFUNCTION` callback `rpc_out()` again even after\n> already getting a return value of zero (EOF) back once before.\n> \n> This results in `rpc_read_from_out()` trying to read more from the\n> child process pipe, because `rpc->flush_read_but_not_sent` is reset to\n> false already the first time EOF is returned, and then the whole\n> operation gets stuck - the child process is already trying to read\n> a response back and will not write anything to the output pipe anymore,\n> while the parent/remote process is now blocked waiting to read more too\n> and never even finishes sending the request.\n\nAh, thanks for finding this bug. It sounds worthwhile to make Git more\nresilient in this situation.\n\nI'll just make some preliminary comments.\n\n> This commit addresses the bug with the following changes:\n\n[snip]\n\nThis seems like a long list of changes, when from the description of the\nbug above, I would have assumed it sufficient to record somewhere when\nthe CURLOPT_READFUNCTION callback returns zero, and then always return\nzero after that if the callback is ever re-invoked. If this is indeed\nnot sufficient, we should describe why.\n\nAlso, if multiple changes are needed, please split them into several\ncommits.\n\n> A trivial solution would be to just take the line which resets the flag\n> \n> /*\n> * The line length either does not need to be sent at\n> * all or has already been completely sent. Now we can\n> * return 0, indicating EOF, meaning that the flush has\n> * been fully sent.\n> */\n> - rpc->flush_read_but_not_sent = 0;\n> return 0;\n> \n> from rpc_out() and reset it only in post_rpc(), before the next time a large\n> request is being sent out and rpc_out() will go into play again:\n> \n> if (large_request) {\n> ...\n> rpc->initial_buffer = 1;\n> + rpc->flush_read_but_not_sent = 0;\n> curl_easy_setopt(slot->curl, CURLOPT_READFUNCTION, rpc_out);\n> \n> This way the CURLOPT_READFUNCTION would be returning zeros at the end of the\n> upload as long as needed, just like fread() at the end of a real file.\n> \n> Hence, the bug could be fixed with just that two-lines change.\n\nAh, you describe the straightforward fix here. I haven't looked at this\nclosely enough to see if this would work, though.\n\n> But while trying to figure out the above, I noticed a few things that prolonged\n> the time I needed to understand what was going on, so I would like to propose\n> some more changes to make the code perhaps a bit easier to read for the next\n> person who comes to hack on it after me.\n> \n> The description of the extra modifications is in the commit message. All of\n> these changes are obviously optional and naturally subjective. I think that we\n> can all agree on some points (less indentation = good), but naming is hard,\n> and so is balance between unclear and too verbose, or when to split all\n> non-functional changes to a separate commit. So let me know if there are things\n> to do differently and I will gladly obey, it is your codebase after all.\n\nYes, please split non-functional changes into a separate commit\n(preferably one for each concern). I do envision reviewers saying \"let's\nput patches X, Y, and Z in, but not patches A, B, and C\", so splitting\nwould make it easier to decide what's worthwhile to have.\n\n> Which brings me to the next topic, testing.\n> \n> Validating the fix would be trivial with a mocked libcurl, but turns out to be\n> much harder with the integration-level test suite of this project.\n\n[snip]\n\n> But outside of that, and as far as the bundled test suite goes, I have failed to\n> write a test that could validate this problem does not ever occur again.\n\nDue to the nature of the bug and the fix, I do agree that it's hard to\ntest and I would be OK with including the fix without associated tests.\n\n \n"},{"id":"484835","messageId":"xmqqv8a515ge.fsf@gitster.g","threadId":"60510","inReplyTo":"20231113212243.1495815-1-jonathantanmy@google.com","subject":"Re: [PATCH] remote-curl: avoid hang if curl asks for more data after eof","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-11-14T01:36:33Z","receivedAt":"2023-11-14T01:36:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n> Due to the nature of the bug and the fix, I do agree that it's hard to\n> test and I would be OK with including the fix without associated tests.\n\nIs this a bug on our side, or cURL library calling us when it should not?\nEven if the latter, we should be prepared and the two liner fix you\nsuggested would be worth doing, but at the same time we should let\nthe cURL maintainer know if the latter is the case, and that is why\nI am asking.\n\nThanks, both. Especially thanks, Jonathan, for excellent review\ncomments.\n\n\n"},{"id":"484900","messageId":"CAGE_+C5PrqgMiyrJCSzb12ukihad5QHpwtUSpTXVbsiNWBgYuA@mail.gmail.com","threadId":"60510","inReplyTo":"20231113212243.1495815-1-jonathantanmy@google.com","subject":"Re: [PATCH] remote-curl: avoid hang if curl asks for more data after eof","fromName":"Jiří Hruška","fromEmail":"jirka@fud.cz","sentAt":"2023-11-14T23:16:45Z","receivedAt":"2023-11-14T23:16:59Z","isPatch":true,"sender":{"key":"jirka@fud.cz","avatar":"https://gravatar.com/avatar/759c05595fe2491dc29b147bf9e4c8a95bcf97793f75a589f4e0893d7a172e25?d=mp&s=160"},"body":"Thank you for the comments,\n\n> Yes, please split non-functional changes into a separate commit\n> (preferably one for each concern). I do envision reviewers saying \"let's\n> put patches X, Y, and Z in, but not patches A, B, and C\", so splitting\n> would make it easier to decide what's worthwhile to have.\n\nSure, will do\n"},{"id":"484901","messageId":"CAGE_+C7h6HOh+ptdhwJ3MNn5HWFoc1WF-foLRrew6FJEo_yupg@mail.gmail.com","threadId":"60510","inReplyTo":"xmqqv8a515ge.fsf@gitster.g","subject":"Re: [PATCH] remote-curl: avoid hang if curl asks for more data after eof","fromName":"Jiří Hruška","fromEmail":"jirka@fud.cz","sentAt":"2023-11-14T23:33:19Z","receivedAt":"2023-11-14T23:33:31Z","isPatch":true,"sender":{"key":"jirka@fud.cz","avatar":"https://gravatar.com/avatar/759c05595fe2491dc29b147bf9e4c8a95bcf97793f75a589f4e0893d7a172e25?d=mp&s=160"},"body":"> Is this a bug on our side, or cURL library calling us when it should not?\n\nI thought the same and yes, I suppose it _might_ be potentially considered\na bug on cURL side in the first place. But then also\n\n1/ it is not mandated anywhere in the API that the callback will never be\n called again after already getting an EOF once,\n\n2/ I looked at libcurl code and it was not entirely clear to me that the\n behavior would be accidental, that it could be clearly called as a bug,\n\n3/ anything that follows how fread() works would never be affected,\n but git-remote-curl is, because it does something differently,\n\n4/ even if it gets fixed in libcurl today, people might be building Git with\n whatever old versions of the library for years to come,\n\nso worth fixing here in any case (imho).\n\nBut I'll reach out to curl-library and get their opinion, so that we have\na full picture here. Thanks\n"},{"id":"484906","messageId":"CAGE_+C4mUw8U6YK0m6hRvcqriv4pWdsELpyRJBCY-LrdHjWwgw@mail.gmail.com","threadId":"60510","inReplyTo":"CAGE_+C6DJMAO0bj5QHoKBBV3gMEMtZ-ajJ9ZnDGYq6eorr-oig@mail.gmail.com","subject":"[PATCH v2 0/5] Avoid hang if curl needs eof twice + minor related improvements","fromName":"Jiří Hruška","fromEmail":"jirka@fud.cz","sentAt":"2023-11-15T03:34:51Z","receivedAt":"2023-11-15T03:34:56Z","isPatch":true,"sender":{"key":"jirka@fud.cz","avatar":"https://gravatar.com/avatar/759c05595fe2491dc29b147bf9e4c8a95bcf97793f75a589f4e0893d7a172e25?d=mp&s=160"},"body":"Proposed changes split into several commits for clarity\n\nJiri Hruska (5):\n remote-curl: avoid hang if curl asks for more data after eof\n remote-curl: improve readability of curl callbacks\n remote-curl: simplify rpc_out() - remove superfluous ifs\n remote-curl: simplify rpc_out() - less nesting and rename\n http: reset CURLOPT_POSTFIELDSIZE_LARGE between requests\n\n http.c | 1 +\n remote-curl.c | 99 +++++++++++++++++++++++++++++----------------------\n 2 files changed, 58 insertions(+), 42 deletions(-)\n\n-- \n2.42.1.5.g2f21867bd5\n"},{"id":"484907","messageId":"CAGE_+C6ukSe-XTS2x8j3QDJ96yMib7kOtMbcrXes4zNbix0Oyg@mail.gmail.com","threadId":"60510","inReplyTo":"20231115033121.939-1-jirka@fud.cz","subject":"[PATCH v2 2/5] remote-curl: improve readability of curl callbacks","fromName":"Jiří Hruška","fromEmail":"jirka@fud.cz","sentAt":"2023-11-15T03:34:52Z","receivedAt":"2023-11-15T03:34:56Z","isPatch":true,"sender":{"key":"jirka@fud.cz","avatar":"https://gravatar.com/avatar/759c05595fe2491dc29b147bf9e4c8a95bcf97793f75a589f4e0893d7a172e25?d=mp&s=160"},"body":"The \"user data\" or \"context\" argument of libcurl streaming callbacks\nis sometimes called `clientp` and sometimes `buffer_`. The latter is\nespecially confusing, because there is an actual buffer pointer\nargument passed to the same functions.\n\n- Make the argument consistently named `userdata` everywhere, just\n like the official cURL documentation calls it.\n\n- Also add comments to all the callbacks, to make it easier to grasp\n what is the \"in\" and what is the \"out\" direction in this code.\n\nSigned-off-by: Jiri Hruska \n---\n remote-curl.c | 31 +++++++++++++++++++++++--------\n 1 file changed, 23 insertions(+), 8 deletions(-)\n\ndiff --git a/remote-curl.c b/remote-curl.c\nindex 199c4615a5..428dd70aa1 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -675,11 +675,18 @@ static int rpc_read_from_out(struct rpc_state\n*rpc, int options,\n \treturn 1;\n }\n\n+/*\n+ * CURLOPT_READFUNCTION callback, called by libcurl when it wants more data\n+ * to send out. Used only if the request did not fit into just one buffer and\n+ * data must be streamed as it comes.\n+ * Has the same semantics as fread(), but reads packets from the pipe from\n+ * the child process instead. A return value of 0 (EOF) finishes the upload.\n+ */\n static size_t rpc_out(void *ptr, size_t eltsize,\n-\t\tsize_t nmemb, void *buffer_)\n+\t\tsize_t nmemb, void *userdata)\n {\n \tsize_t max = eltsize * nmemb;\n-\tstruct rpc_state *rpc = buffer_;\n+\tstruct rpc_state *rpc = userdata;\n \tsize_t avail = rpc->len - rpc->pos;\n \tenum packet_read_status status;\n\n@@ -725,9 +732,16 @@ static size_t rpc_out(void *ptr, size_t eltsize,\n \treturn avail;\n }\n\n-static int rpc_seek(void *clientp, curl_off_t offset, int origin)\n+/*\n+ * CURLOPT_SEEKFUNCTION callback, called by libcurl when it wants to seek in\n+ * the data being sent out. Used only if the request did not fit into just\n+ * one buffer and data must be streamed as it comes.\n+ * Has the same semantics as fseek(), but seeks in the buffered packet read\n+ * from the pipe from the child process instead.\n+ */\n+static int rpc_seek(void *userdata, curl_off_t offset, int origin)\n {\n-\tstruct rpc_state *rpc = clientp;\n+\tstruct rpc_state *rpc = userdata;\n\n \tif (origin != SEEK_SET)\n \t\tBUG(\"rpc_seek only handles SEEK_SET, not %d\", origin);\n@@ -797,14 +811,15 @@ struct rpc_in_data {\n };\n\n /*\n- * A callback for CURLOPT_WRITEFUNCTION. The return value is the bytes consumed\n- * from ptr.\n+ * CURLOPT_WRITEFUNCTION callback, called when more received data has come in.\n+ * Has the same semantics as fwrite(), but writes packets to the pipe to the\n+ * child process instead. The return value is the bytes consumed from ptr.\n */\n static size_t rpc_in(char *ptr, size_t eltsize,\n-\t\tsize_t nmemb, void *buffer_)\n+\t\tsize_t nmemb, void *userdata)\n {\n \tsize_t size = eltsize * nmemb;\n-\tstruct rpc_in_data *data = buffer_;\n+\tstruct rpc_in_data *data = userdata;\n \tlong response_code;\n\n \tif (curl_easy_getinfo(data->slot->curl, CURLINFO_RESPONSE_CODE,\n-- \n2.42.1.5.g2f21867bd5\n"},{"id":"484909","messageId":"CAGE_+C4JWA0DrcV4rT7J6hXsbYPL2Po31wFPvLESe_sKq_16FQ@mail.gmail.com","threadId":"60510","inReplyTo":"20231115033121.939-1-jirka@fud.cz","subject":"[PATCH v2 1/5] remote-curl: avoid hang if curl asks for more data after eof","fromName":"Jiří Hruška","fromEmail":"jirka@fud.cz","sentAt":"2023-11-15T03:34:51Z","receivedAt":"2023-11-15T03:34:56Z","isPatch":true,"sender":{"key":"jirka@fud.cz","avatar":"https://gravatar.com/avatar/759c05595fe2491dc29b147bf9e4c8a95bcf97793f75a589f4e0893d7a172e25?d=mp&s=160"},"body":"It has been observed that under some circumstances, libcurl can call\nour `CURLOPT_READFUNCTION` callback `rpc_out()` again even after\nalready getting a return value of zero (EOF) back once before.\n\nBecause `rpc->flush_read_but_not_sent` is reset to false immediately\nthe first time an EOF is returned, the repeated call goes again to\n`rpc_read_from_out()`, which tries to read more from the child process\npipe and the whole operation gets stuck - the child process is already\ntrying to read a response back and will not write anything to the\noutput pipe anymore, while the parent/remote process is now blocked\nwaiting to read more too and never even finishes sending the request.\n\nThe bug is fixed by moving the reset of the `flush_read_but_not_sent`\nflag to `post_rpc()`, only before `rpc_out()` would be potentially used\nthe next time. This makes the callback behave like fread() and return\na zero any number of times at the end of a finished upload.\n\nSigned-off-by: Jiri Hruska \n---\n remote-curl.c | 6 ++++--\n 1 file changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/remote-curl.c b/remote-curl.c\nindex ef05752ca5..199c4615a5 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -705,9 +705,10 @@ static size_t rpc_out(void *ptr, size_t eltsize,\n \t\t\t * The line length either does not need to be sent at\n \t\t\t * all or has already been completely sent. Now we can\n \t\t\t * return 0, indicating EOF, meaning that the flush has\n-\t\t\t * been fully sent.\n+\t\t\t * been fully sent. It is important to keep returning 0\n+\t\t\t * as long as needed in that case, as libcurl invokes\n+\t\t\t * the callback multiple times at EOF sometimes.\n \t\t\t */\n-\t\t\trpc->flush_read_but_not_sent = 0;\n \t\t\treturn 0;\n \t\t}\n \t\t/*\n@@ -963,6 +964,7 @@ static int post_rpc(struct rpc_state *rpc, int\nstateless_connect, int flush_rece\n \t\t */\n \t\theaders = curl_slist_append(headers, \"Transfer-Encoding: chunked\");\n \t\trpc->initial_buffer = 1;\n+\t\trpc->flush_read_but_not_sent = 0;\n \t\tcurl_easy_setopt(slot->curl, CURLOPT_READFUNCTION, rpc_out);\n \t\tcurl_easy_setopt(slot->curl, CURLOPT_INFILE, rpc);\n \t\tcurl_easy_setopt(slot->curl, CURLOPT_SEEKFUNCTION, rpc_seek);\n-- \n2.42.1.5.g2f21867bd5\n"},{"id":"484908","messageId":"CAGE_+C58LWwF5YAm+S3Ynev+mTnm_ZSVuir9sZsHmeB+56T1kA@mail.gmail.com","threadId":"60510","inReplyTo":"20231115033121.939-1-jirka@fud.cz","subject":"[PATCH v2 3/5] remote-curl: simplify rpc_out() - remove superfluous ifs","fromName":"Jiří Hruška","fromEmail":"jirka@fud.cz","sentAt":"2023-11-15T03:34:53Z","receivedAt":"2023-11-15T03:34:57Z","isPatch":true,"sender":{"key":"jirka@fud.cz","avatar":"https://gravatar.com/avatar/759c05595fe2491dc29b147bf9e4c8a95bcf97793f75a589f4e0893d7a172e25?d=mp&s=160"},"body":"Remove the second set of nested `if` statements and an early `return`\nin `rpc_out()`, because they accomplish nothing. The rest of the\nfunction would behave the same without any branching.\n\nSigned-off-by: Jiri Hruska \n---\n remote-curl.c | 24 ++++++------------------\n 1 file changed, 6 insertions(+), 18 deletions(-)\n\ndiff --git a/remote-curl.c b/remote-curl.c\nindex 428dd70aa1..690df2a43e 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -706,25 +706,13 @@ static size_t rpc_out(void *ptr, size_t eltsize,\n \t\t * we need to refrain from reading.\n \t\t */\n \t}\n-\tif (rpc->flush_read_but_not_sent) {\n-\t\tif (!avail) {\n-\t\t\t/*\n-\t\t\t * The line length either does not need to be sent at\n-\t\t\t * all or has already been completely sent. Now we can\n-\t\t\t * return 0, indicating EOF, meaning that the flush has\n-\t\t\t * been fully sent. It is important to keep returning 0\n-\t\t\t * as long as needed in that case, as libcurl invokes\n-\t\t\t * the callback multiple times at EOF sometimes.\n-\t\t\t */\n-\t\t\treturn 0;\n-\t\t}\n-\t\t/*\n-\t\t * If avail is non-zero, the line length for the flush still\n-\t\t * hasn't been fully sent. Proceed with sending the line\n-\t\t * length.\n-\t\t */\n-\t}\n\n+\t/*\n+\t * Copy data to the provided buffer. If there is nothing more to send,\n+\t * nothing gets written and the return value is 0 (EOF).\n+\t * It is important to keep returning 0 as long as needed in that case,\n+\t * as libcurl invokes the callback multiple times at EOF sometimes.\n+\t */\n \tif (max < avail)\n \t\tavail = max;\n \tmemcpy(ptr, rpc->buf + rpc->pos, avail);\n-- \n2.42.1.5.g2f21867bd5\n"},{"id":"484910","messageId":"CAGE_+C7CEh63ThpJx2BvMQLb==v=h79pMKHC14t_wYeRowMfwQ@mail.gmail.com","threadId":"60510","inReplyTo":"20231115033121.939-1-jirka@fud.cz","subject":"[PATCH v2 4/5] remote-curl: simplify rpc_out() - less nesting and rename","fromName":"Jiří Hruška","fromEmail":"jirka@fud.cz","sentAt":"2023-11-15T03:34:54Z","receivedAt":"2023-11-15T03:34:58Z","isPatch":true,"sender":{"key":"jirka@fud.cz","avatar":"https://gravatar.com/avatar/759c05595fe2491dc29b147bf9e4c8a95bcf97793f75a589f4e0893d7a172e25?d=mp&s=160"},"body":"- Remove one indentation level in `rpc_out()`, as the conditions can be\n combined into just one `if` statement without losing readability.\n\n Skipping the resetting of `initial_buffer`/`len`/`pos` revealed a bug\n in `stateless_connect()`, where `rpc.len` is reset to 0 after each\n request, but not `rpc.pos`. Relying on `rpc_out()` always doing this\n before has never been safe (it might have not finished cleanly, for\n example). So better reset it there, just like `rpc.len`.\n\n- Rename `flush_read_but_not_sent` to `read_from_out_done`. The name is\n slightly misleading, because the \"flush\" might never be really \"sent\"\n (depends on `write_line_lengths`), and this is not the most important\n part anyway. The primary role of the flag is rather to signal that\n `read_from_out()` is \"done\" and must not be called for this particular\n RPC exchange anymore.\n\n- Update/add some related comments.\n\nSigned-off-by: Jiri Hruska \n---\n remote-curl.c | 46 ++++++++++++++++++++++++++++------------------\n 1 file changed, 28 insertions(+), 18 deletions(-)\n\ndiff --git a/remote-curl.c b/remote-curl.c\nindex 690df2a43e..d5aa66a44c 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -606,13 +606,14 @@ struct rpc_state {\n \tunsigned write_line_lengths : 1;\n\n \t/*\n-\t * Used by rpc_out; initialize to 0. This is true if a flush has been\n-\t * read, but the corresponding line length (if write_line_lengths is\n-\t * true) and EOF have not been sent to libcurl. Since each flush marks\n-\t * the end of a request, each flush must be completely sent before any\n-\t * further reading occurs.\n+\t * Used by rpc_out; initialize to 0. This is true if a flush packet\n+\t * has been read from the child process, signaling the end of the\n+\t * current data to send. There might be still some bytes pending in\n+\t * 'buf' (e.g. the corresponding line length, if write_line_lengths\n+\t * is true), but no more reads can be performed on the 'out' pipe as\n+\t * part of the current RPC exchange.\n \t */\n-\tunsigned flush_read_but_not_sent : 1;\n+\tunsigned read_from_out_done : 1;\n };\n\n #define RPC_STATE_INIT { 0 }\n@@ -690,21 +691,29 @@ static size_t rpc_out(void *ptr, size_t eltsize,\n \tsize_t avail = rpc->len - rpc->pos;\n \tenum packet_read_status status;\n\n-\tif (!avail) {\n+\t/*\n+\t * If there is no more data available in our buffer and the child\n+\t * process is not done sending yet, read the next packet.\n+\t */\n+\tif (!avail && !rpc->read_from_out_done) {\n \t\trpc->initial_buffer = 0;\n \t\trpc->len = 0;\n \t\trpc->pos = 0;\n-\t\tif (!rpc->flush_read_but_not_sent) {\n-\t\t\tif (!rpc_read_from_out(rpc, 0, &avail, &status))\n-\t\t\t\tBUG(\"The entire rpc->buf should be larger than LARGE_PACKET_MAX\");\n-\t\t\tif (status == PACKET_READ_FLUSH)\n-\t\t\t\trpc->flush_read_but_not_sent = 1;\n-\t\t}\n+\t\tif (!rpc_read_from_out(rpc, 0, &avail, &status))\n+\t\t\tBUG(\"The entire rpc->buf should be larger than LARGE_PACKET_MAX\");\n+\n \t\t/*\n-\t\t * If flush_read_but_not_sent is true, we have already read one\n-\t\t * full request but have not fully sent it + EOF, which is why\n-\t\t * we need to refrain from reading.\n+\t\t * If a flush packet was read, it means the child process is\n+\t\t * done sending this request. The buffer might be fully empty\n+\t\t * at this point or contain a flush packet too, depending on\n+\t\t * rpc->write_line_lengths.\n+\t\t * In any case, we must refrain from reading any more, because\n+\t\t * the child process already expects to receive a response back\n+\t\t * instead. If both sides would try to read at once, they would\n+\t\t * just hang waiting for each other.\n \t\t */\n+\t\tif (status == PACKET_READ_FLUSH)\n+\t\t\trpc->read_from_out_done = 1;\n \t}\n\n \t/*\n@@ -967,7 +976,7 @@ static int post_rpc(struct rpc_state *rpc, int\nstateless_connect, int flush_rece\n \t\t */\n \t\theaders = curl_slist_append(headers, \"Transfer-Encoding: chunked\");\n \t\trpc->initial_buffer = 1;\n-\t\trpc->flush_read_but_not_sent = 0;\n+\t\trpc->read_from_out_done = 0;\n \t\tcurl_easy_setopt(slot->curl, CURLOPT_READFUNCTION, rpc_out);\n \t\tcurl_easy_setopt(slot->curl, CURLOPT_INFILE, rpc);\n \t\tcurl_easy_setopt(slot->curl, CURLOPT_SEEKFUNCTION, rpc_seek);\n@@ -1487,7 +1496,7 @@ static int stateless_connect(const char *service_name)\n \trpc.gzip_request = 1;\n \trpc.initial_buffer = 0;\n \trpc.write_line_lengths = 1;\n-\trpc.flush_read_but_not_sent = 0;\n+\trpc.read_from_out_done = 0;\n\n \t/*\n \t * Dump the capability listing that we got from the server earlier\n@@ -1510,6 +1519,7 @@ static int stateless_connect(const char *service_name)\n \t\t\tbreak;\n \t\t/* Reset the buffer for next request */\n \t\trpc.len = 0;\n+\t\trpc.pos = 0;\n \t}\n\n \tfree(rpc.service_url);\n-- \n2.42.1.5.g2f21867bd5\n"},{"id":"484911","messageId":"CAGE_+C5pnASOsrDr4ehNj-deYbSTr=pRgPcWqq5VSQs-Y08ttQ@mail.gmail.com","threadId":"60510","inReplyTo":"20231115033121.939-1-jirka@fud.cz","subject":"[PATCH v2 5/5] http: reset CURLOPT_POSTFIELDSIZE_LARGE between requests","fromName":"Jiří Hruška","fromEmail":"jirka@fud.cz","sentAt":"2023-11-15T03:34:55Z","receivedAt":"2023-11-15T03:35:00Z","isPatch":true,"sender":{"key":"jirka@fud.cz","avatar":"https://gravatar.com/avatar/759c05595fe2491dc29b147bf9e4c8a95bcf97793f75a589f4e0893d7a172e25?d=mp&s=160"},"body":"`get_active_slot()` makes sure that the reused cURL handles it gives\nout are as good as fresh ones, by resetting all options that other code\nmight have set on them back to defaults.\n\nBut this does not apply to `CURLOPT_POSTFIELDSIZE_LARGE` yet, which can\nstay set from a previous request. For example, an earlier probe request\nwith just a flush packet \"0000\" leaves it set to 4.\n\nThe problem seems harmless in practice, but it can be confusing to see\na negative amount of remaining bytes to upload when inspecting libcurl\ninternals while debugging networking-related issues, for example.\n\nSo reset also this option to its default value (which is -1, not 0).\n\nSigned-off-by: Jiri Hruska \n---\n http.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/http.c b/http.c\nindex 8f71bf00d8..14f2fbb82e 100644\n--- a/http.c\n+++ b/http.c\n@@ -1454,6 +1454,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_LARGE, (curl_off_t)-1);\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.42.1.5.g2f21867bd5\n"},{"id":"484912","messageId":"ZVRovA9OSfY5odhy@tanuki","threadId":"60510","inReplyTo":"CAGE_+C5pnASOsrDr4ehNj-deYbSTr=pRgPcWqq5VSQs-Y08ttQ@mail.gmail.com","subject":"Re: [PATCH v2 5/5] http: reset CURLOPT_POSTFIELDSIZE_LARGE between requests","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-11-15T06:44:12Z","receivedAt":"2023-11-15T06:44:24Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Nov 14, 2023 at 07:34:55PM -0800, Jiří Hruška wrote:\n> `get_active_slot()` makes sure that the reused cURL handles it gives\n> out are as good as fresh ones, by resetting all options that other code\n> might have set on them back to defaults.\n> \n> But this does not apply to `CURLOPT_POSTFIELDSIZE_LARGE` yet, which can\n> stay set from a previous request. For example, an earlier probe request\n> with just a flush packet \"0000\" leaves it set to 4.\n> \n> The problem seems harmless in practice, but it can be confusing to see\n> a negative amount of remaining bytes to upload when inspecting libcurl\n> internals while debugging networking-related issues, for example.\n> \n> So reset also this option to its default value (which is -1, not 0).\n> \n> Signed-off-by: Jiri Hruska \n> ---\n> http.c | 1 +\n> 1 file changed, 1 insertion(+)\n> \n> diff --git a/http.c b/http.c\n> index 8f71bf00d8..14f2fbb82e 100644\n> --- a/http.c\n> +++ b/http.c\n> @@ -1454,6 +1454,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_LARGE, (curl_off_t)-1);\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\nIt feels quite easy for this list to grow stale whenever we start to set\na new option somewhere else. Is there a specific reason why we can't\ninstead use `curl_easy_reset()` here? Quoting its description:\n\n> Re-initializes all options previously set on a specified CURL handle\n> to the default values. This puts back the handle to the same state as\n> it was in when it was just created with curl_easy_init.\n> \n> It does not change the following information kept in the handle: live\n> connections, the Session ID cache, the DNS cache, the cookies, the\n> shares or the alt-svc cache.\n\nFrom my naive point of view it sounds like exactly what we're after.\nMost of the code in question was introduced in 9094950d73 (http: prevent\nsegfault during curl handle reuse, 2006-05-31), where we used to support\nlibcurl at least back to v7.7. `curl_easy_reset()` on the other hand had\nonly been introduced with v7.12.1 of libcurl, so maybe that's the reason\nwhy it's not used here?\n\nI dunno, might as well be that there is a good reason why we don't use\nit here. But if we can, then I'd argue it would be a great cleanup to\nconvert to `curl_easy_reset()` here instead of piling onto the list of\noptions.\n\nPatrick\n"},{"id":"484947","messageId":"20231115192027.2468887-1-jonathantanmy@google.com","threadId":"60510","inReplyTo":"CAGE_+C4JWA0DrcV4rT7J6hXsbYPL2Po31wFPvLESe_sKq_16FQ@mail.gmail.com","subject":"Re: [PATCH v2 1/5] remote-curl: avoid hang if curl asks for more data after eof","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2023-11-15T19:20:26Z","receivedAt":"2023-11-15T19:20:30Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Jiří Hruška writes:\n> It has been observed that under some circumstances, libcurl can call\n> our `CURLOPT_READFUNCTION` callback `rpc_out()` again even after\n> already getting a return value of zero (EOF) back once before.\n> \n> Because `rpc->flush_read_but_not_sent` is reset to false immediately\n> the first time an EOF is returned, the repeated call goes again to\n> `rpc_read_from_out()`, which tries to read more from the child process\n> pipe and the whole operation gets stuck - the child process is already\n> trying to read a response back and will not write anything to the\n> output pipe anymore, while the parent/remote process is now blocked\n> waiting to read more too and never even finishes sending the request.\n> \n> The bug is fixed by moving the reset of the `flush_read_but_not_sent`\n> flag to `post_rpc()`, only before `rpc_out()` would be potentially used\n> the next time. This makes the callback behave like fread() and return\n> a zero any number of times at the end of a finished upload.\n> \n> Signed-off-by: Jiri Hruska \n\nThanks for splitting this out - this makes it much easier to review. I\ncan see that the patch indeed works, but some things need to be clearer\n(described below).\n\nIn the commit message, I think we should add a historical note,\nsomething like:\n\n\t`flush_read_but_not_sent` was introduced in a97d00799a (remote-curl:\n\tuse post_rpc() for protocol v2 also, 2019-02-21), which needed a way to\n\tindicate that Git should not read from a certain child process anymore\n\tonce \"flush\" has been received from the child process. This field was\n\tscoped to what was believed the minimum necessary - the interval between\n\t\"flush\" being received and EOF being reported to Curl.\n\n\tHowever, as described at the beginning of the commit message, we need\n\tthe scope of this to be greater than that - in case Curl continues to\n\task us for more data, we still need to remember that \"flush\" has been\n\treceived. Therefore, change this field from `flush_read_but_not_sent` to\n\t`flush_read`, which both is simpler and is a fix for this bug.\n\nFeel free to use this verbatim, or adapt it as you wish (or write your\nown).\n\nAs described in the historical note, I also think we need to change the\nname of this field, since we are changing its semantics.\n\nI am trying to be better at indicating when I think a prefix subset of\na patch set is worth merging on its own (so that, if some patches in\na patch set are good and some still need to be discussed, we have the\noption of merging a prefix of a patch set). So, assuming my comments are\naddressed, I think this patch is good to go in on its own, even if we\ndon't want some of the later patches. I'll review the rest of this patch\nset later.\n"},{"id":"484952","messageId":"20231115232824.2507648-1-jonathantanmy@google.com","threadId":"60510","inReplyTo":"CAGE_+C4mUw8U6YK0m6hRvcqriv4pWdsELpyRJBCY-LrdHjWwgw@mail.gmail.com","subject":"Re: [PATCH v2 0/5] Avoid hang if curl needs eof twice + minor related improvements","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2023-11-15T23:28:23Z","receivedAt":"2023-11-15T23:28:29Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Jiří Hruška writes:\n> Proposed changes split into several commits for clarity\n> \n> Jiri Hruska (5):\n> remote-curl: avoid hang if curl asks for more data after eof\n\nI've already reviewed this [1] so I'll summarize what I think of the\nrest.\n\n[1] https://lore.kernel.org/git/20231115192027.2468887-1-jonathantanmy@google.com/\n\n> remote-curl: improve readability of curl callbacks\n> remote-curl: simplify rpc_out() - remove superfluous ifs\n> remote-curl: simplify rpc_out() - less nesting and rename\n> http: reset CURLOPT_POSTFIELDSIZE_LARGE between requests\n\nOverall I can see how all of these make the code clearer, but in a\nlong-lived project like Git where it is very common to look at code\nhistory to try to see why something was written the way it is, I'm a\nbit reluctant to include 2/5 and 4/5. I think 3/5 (removes an \"if\"),\nthe part of 4/5 where we set \"rpc.pos = 0;\", and 5/5 (sets a parameter\nthat one could expect to be set) have significant benefit and should be\nincluded, though. Having said that, I don't have strong opposition to\nincluding all of them.\n"},{"id":"485170","messageId":"CAGE_+C5u9H8m5faK1vXKk6QTyjcHgKHqxOZy5ptzsYbF_0yrCQ@mail.gmail.com","threadId":"60510","inReplyTo":"CAGE_+C7h6HOh+ptdhwJ3MNn5HWFoc1WF-foLRrew6FJEo_yupg@mail.gmail.com","subject":"Re: [PATCH] remote-curl: avoid hang if curl asks for more data after eof","fromName":"Jiří Hruška","fromEmail":"jirka@fud.cz","sentAt":"2023-11-27T13:19:52Z","receivedAt":"2023-11-27T13:20:01Z","isPatch":true,"sender":{"key":"jirka@fud.cz","avatar":"https://gravatar.com/avatar/759c05595fe2491dc29b147bf9e4c8a95bcf97793f75a589f4e0893d7a172e25?d=mp&s=160"},"body":"I have discussed this with Daniel Stenberg himself in this thread:\nhttps://curl.se/mail/lib-2023-11/0017.html\n\nThe summary is:\n\n- It was never intended/expected that CURLOPT_READFUNCTION would be called\n more after already returning EOF once. libcurl has been modified to never\n do this anymore (in master, to be released in what comes after 8.4.0).\n\n- The problem has been tracked down to a particular case that could reproduce\n only if using HTTP/2 and only in cURL versions 7.84.0 – 8.1.2 (inclusive,\n roughly between June 2022 and July 2023). The same misbehavior is still\n technically possible in newer versions, but not happening in practice.\n\nTherefore, I believe nothing strongly _needs_ to be done on the Git side.\n"},{"id":"485171","messageId":"CAGE_+C6ZWVP9iZeK8GLuR2VpHz3NZ8nFkJBEb4cJ+zrXwQiDDQ@mail.gmail.com","threadId":"60510","inReplyTo":"ZVRovA9OSfY5odhy@tanuki","subject":"Re: [PATCH v2 5/5] http: reset CURLOPT_POSTFIELDSIZE_LARGE between requests","fromName":"Jiří Hruška","fromEmail":"jirka@fud.cz","sentAt":"2023-11-27T13:21:17Z","receivedAt":"2023-11-27T13:21:26Z","isPatch":true,"sender":{"key":"jirka@fud.cz","avatar":"https://gravatar.com/avatar/759c05595fe2491dc29b147bf9e4c8a95bcf97793f75a589f4e0893d7a172e25?d=mp&s=160"},"body":"> It feels quite easy for this list to grow stale whenever we start to set\n> a new option somewhere else. Is there a specific reason why we can't\n> instead use `curl_easy_reset()` here?\n\nGood point, makes sense. Thanks, Patrick, I will use that instead.\n"},{"id":"485172","messageId":"CAGE_+C4B=zjPqNV+fGt9gaA0O8qmqmK3CDh-eLBPmXx+ChzXAQ@mail.gmail.com","threadId":"60510","inReplyTo":"20231115232824.2507648-1-jonathantanmy@google.com","subject":"Re: [PATCH v2 0/5] Avoid hang if curl needs eof twice + minor related improvements","fromName":"Jiří Hruška","fromEmail":"jirka@fud.cz","sentAt":"2023-11-27T13:39:00Z","receivedAt":"2023-11-27T13:39:09Z","isPatch":true,"sender":{"key":"jirka@fud.cz","avatar":"https://gravatar.com/avatar/759c05595fe2491dc29b147bf9e4c8a95bcf97793f75a589f4e0893d7a172e25?d=mp&s=160"},"body":"> I've already reviewed 1/5 so I'll summarize what I think of the rest.\nThank you for your time looking at everything, Jonathan.\n\nI'm sorry for the delay in my reply, I was away for a while and also trying\nto get clarity from the cURL side, summarized in [1].\n\n[1] https://lore.kernel.org/git/CAGE_+C5u9H8m5faK1vXKk6QTyjcHgKHqxOZy5ptzsYbF_0yrCQ@mail.gmail.com/\n\nMost importantly, the bug has been apparently in libcurl only for ~1 year and\nit can no longer manifest for a few months. Therefore, the main point of my\nchanges is a bit moot and we might reconsider what is useful and what is not.\n\n> I'm a bit reluctant to include 2/5 and 4/5.\n> I think 3/5 [...], part of 4/5 [...] and 5/5 have significant benefit and\n> should be included, though.\nFair enough. I will update everything based on your and other's comments and\nmy new learnings, then the final decision can be made about each part.\n"},{"id":"485176","messageId":"20231127182618.212901-1-jonathantanmy@google.com","threadId":"60510","inReplyTo":"CAGE_+C4B=zjPqNV+fGt9gaA0O8qmqmK3CDh-eLBPmXx+ChzXAQ@mail.gmail.com","subject":"Re: [PATCH v2 0/5] Avoid hang if curl needs eof twice + minor related improvements","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2023-11-27T18:26:18Z","receivedAt":"2023-11-27T18:26:22Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Jiří Hruška writes:\n> > I've already reviewed 1/5 so I'll summarize what I think of the rest.\n> Thank you for your time looking at everything, Jonathan.\n> \n> I'm sorry for the delay in my reply, I was away for a while and also trying\n> to get clarity from the cURL side, summarized in [1].\n> \n> [1] https://lore.kernel.org/git/CAGE_+C5u9H8m5faK1vXKk6QTyjcHgKHqxOZy5ptzsYbF_0yrCQ@mail.gmail.com/\n> \n> Most importantly, the bug has been apparently in libcurl only for ~1 year and\n> it can no longer manifest for a few months. Therefore, the main point of my\n> changes is a bit moot and we might reconsider what is useful and what is not.\n\nThanks for following up with the Curl side. Even if the Curl bug is\nfixed now, for what it's worth, I still think that it's worth including\nthis mitigation in Git.\n"}]}