{"thread":{"id":"53462","subject":"[PATCH 0/6] remote-curl: partial fix for a deadlock with stateless rpc","startedAt":"2020-05-13T18:05:10Z","lastAt":"2020-05-22T16:31:25Z","messageCount":62,"participants":["Denton Liu","Eric Sunshine","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":6},"messages":[{"id":"397725","messageId":"cover.1589393036.git.liu.denton@gmail.com","threadId":"53462","inReplyTo":null,"subject":"[PATCH 0/6] remote-curl: partial fix for a deadlock with stateless rpc","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-05-13T18:04:52Z","receivedAt":"2020-05-13T18:05:10Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"The following command hangs forever:\n\n\t$ git -c protocol.version=2 clone https://github.com/git/git.git --shallow-since=20151012\n\tCloning into 'git'...\n\nThis occurs because the --shallow-since arg is incorrect and the server\ndies early. However, remote-curl does not realise that the server\nerrored out and just faithfully forwards the packets to fetch-pack\nbefore waiting on more input from fetch-pack. Meanwhile, fetch-pack\nkeeps reading as it still expects more input. As a result, the processes\ndeadlock. Original analysis by Peff:\nhttps://lore.kernel.org/git/20200328154936.GA1217052@coredump.intra.peff.net/\n\nThis isn't a full fix as it may still be possible to deadlock, as\ndescribed in the last commit message. However, this patch is probably\nbetter than nothing as it fixes the reported bug. I've been working on\nthe proper reframing fix on-and-off for the past while but it seems\nconsiderably more complicated so it'll probably take a while for me to\nget it to a ready state.\n\nDenton Liu (6):\n remote-curl: fix typo\n remote-curl: remove label indentation\n transport: combine common cases with a fallthrough\n pkt-line: extern packet_length()\n remote-curl: error on incomplete packet\n remote-curl: ensure last packet is a flush\n\n pkt-line.c | 2 +-\n pkt-line.h | 5 ++++\n remote-curl.c | 58 +++++++++++++++++++++++++++++++++++++++---\n t/t5702-protocol-v2.sh | 17 +++++++++++++\n transport.c | 10 +++-----\n 5 files changed, 81 insertions(+), 11 deletions(-)\n\n-- \n2.26.2.706.g87896c9627\n\n"},{"id":"397726","messageId":"b390875f87dbf5f3f48909ce40f5a5749b32f5d4.1589393036.git.liu.denton@gmail.com","threadId":"53462","inReplyTo":"cover.1589393036.git.liu.denton@gmail.com","subject":"[PATCH 1/6] remote-curl: fix typo","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-05-13T18:04:53Z","receivedAt":"2020-05-13T18:05:12Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"Signed-off-by: Denton Liu \n---\n remote-curl.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/remote-curl.c b/remote-curl.c\nindex 1c9aa3d0ab..6844708f38 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -643,7 +643,7 @@ static size_t rpc_out(void *ptr, size_t eltsize,\n \t\t\treturn 0;\n \t\t}\n \t\t/*\n-\t\t * If avail is non-zerp, the line length for the flush still\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-- \n2.26.2.706.g87896c9627\n\n"},{"id":"397727","messageId":"a2b28c0b28bb972f164ff0a9e48ae6b623a2a125.1589393036.git.liu.denton@gmail.com","threadId":"53462","inReplyTo":"cover.1589393036.git.liu.denton@gmail.com","subject":"[PATCH 2/6] remote-curl: remove label indentation","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-05-13T18:04:54Z","receivedAt":"2020-05-13T18:05:13Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"In the codebase, labels are aligned to the leftmost column. Remove the\nspace-indentation from `free_specs:` to conform to this.\n\nSigned-off-by: Denton Liu \n---\n remote-curl.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/remote-curl.c b/remote-curl.c\nindex 6844708f38..da3e07184a 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -1276,7 +1276,7 @@ static void parse_push(struct strbuf *buf)\n \tif (ret)\n \t\texit(128); /* error already reported */\n \n- free_specs:\n+free_specs:\n \targv_array_clear(&specs);\n }\n \n-- \n2.26.2.706.g87896c9627\n\n"},{"id":"397728","messageId":"c89c1841008dfc2d111369fb682b946a0c33b7be.1589393036.git.liu.denton@gmail.com","threadId":"53462","inReplyTo":"cover.1589393036.git.liu.denton@gmail.com","subject":"[PATCH 3/6] transport: combine common cases with a fallthrough","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-05-13T18:04:55Z","receivedAt":"2020-05-13T18:05:15Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"In the switch statement, the difference between the `protocol_v2` and\n`protocol_v{1,0}` arms is a prepatory call to die_if_server_options() in\nthe latter. The fetch_pack() call is identical in both arms. However,\nsince this fetch_pack() call has so many parameters, it is not\nimmediately obvious that the call is identical in both cases.\n\nRewrite the switch statement to fallthrough from the v{1,0} case to v2\nso that they share a common fetch_pack() call. This reduces duplication\nand makes the logic more clear for future readers.\n\nSigned-off-by: Denton Liu \n---\n transport.c | 10 +++-------\n 1 file changed, 3 insertions(+), 7 deletions(-)\n\ndiff --git a/transport.c b/transport.c\nindex 15f5ba4e8f..475f94564a 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -370,15 +370,11 @@ static int fetch_refs_via_pack(struct transport *transport,\n \t}\n \n \tswitch (data->version) {\n-\tcase protocol_v2:\n-\t\trefs = fetch_pack(&args, data->fd,\n-\t\t\t\t refs_tmp ? refs_tmp : transport->remote_refs,\n-\t\t\t\t to_fetch, nr_heads, &data->shallow,\n-\t\t\t\t &transport->pack_lockfile, data->version);\n-\t\tbreak;\n-\tcase protocol_v1:\n \tcase protocol_v0:\n+\tcase protocol_v1:\n \t\tdie_if_server_options(transport);\n+\t\t/* fallthrough */\n+\tcase protocol_v2:\n \t\trefs = fetch_pack(&args, data->fd,\n \t\t\t\t refs_tmp ? refs_tmp : transport->remote_refs,\n \t\t\t\t to_fetch, nr_heads, &data->shallow,\n-- \n2.26.2.706.g87896c9627\n\n"},{"id":"397729","messageId":"891a39c853ce3669b6167dc9ad8a2328e4321a9e.1589393036.git.liu.denton@gmail.com","threadId":"53462","inReplyTo":"cover.1589393036.git.liu.denton@gmail.com","subject":"[PATCH 4/6] pkt-line: extern packet_length()","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-05-13T18:04:56Z","receivedAt":"2020-05-13T18:05:17Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"In a future commit, we will be manually processing packets and we will\nneed to access the length header. In order to simplify this, extern\npacket_length() so that the logic can be reused.\n\nSigned-off-by: Denton Liu \n---\n pkt-line.c | 2 +-\n pkt-line.h | 5 +++++\n 2 files changed, 6 insertions(+), 1 deletion(-)\n\ndiff --git a/pkt-line.c b/pkt-line.c\nindex a0e87b1e81..6b60886770 100644\n--- a/pkt-line.c\n+++ b/pkt-line.c\n@@ -306,7 +306,7 @@ static int get_packet_data(int fd, char **src_buf, size_t *src_size,\n \treturn ret;\n }\n \n-static int packet_length(const char *linelen)\n+int packet_length(const char *linelen)\n {\n \tint val = hex2chr(linelen);\n \treturn (val < 0) ? val : (val << 8) | hex2chr(linelen + 2);\ndiff --git a/pkt-line.h b/pkt-line.h\nindex fef3a0d792..f443185f8f 100644\n--- a/pkt-line.h\n+++ b/pkt-line.h\n@@ -74,6 +74,11 @@ int write_packetized_from_buf(const char *src_in, size_t len, int fd_out);\n int packet_read(int fd, char **src_buffer, size_t *src_len, char\n \t\t*buffer, unsigned size, int options);\n \n+/*\n+ * Reads a packetized line and returns the length header of the packet.\n+ */\n+int packet_length(const char *linelen);\n+\n /*\n * Read a packetized line into a buffer like the 'packet_read()' function but\n * returns an 'enum packet_read_status' which indicates the status of the read.\n-- \n2.26.2.706.g87896c9627\n\n"},{"id":"397730","messageId":"3ed7cf87aaa40ee66b20aa929d89d28fefcec312.1589393036.git.liu.denton@gmail.com","threadId":"53462","inReplyTo":"cover.1589393036.git.liu.denton@gmail.com","subject":"[PATCH 5/6] remote-curl: error on incomplete packet","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-05-13T18:04:57Z","receivedAt":"2020-05-13T18:05:18Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"Currently, remote-curl acts as a proxy and blindly forwards packets\nbetween an HTTP server and fetch-pack. In the case of a stateless RPC\nconnection where the connection is terminated with a partially written\npacket, remote-curl will blindly send the partially written packet\nbefore waiting on more input from fetch-pack. Meanwhile, fetch-pack will\nread the partial packet and continue reading, expecting more input. This\nresults in a deadlock between the two processes.\n\nInstead of blindly forwarding packets, inspect each packet to ensure\nthat it is a full packet, erroring out if a partial packet is sent.\n\nHelped-by: Jeff King \nSigned-off-by: Denton Liu \n---\n\nNotes:\n Unfortunately, I'm not really sure how to test this.\n\n remote-curl.c | 45 ++++++++++++++++++++++++++++++++++++++++++++-\n 1 file changed, 44 insertions(+), 1 deletion(-)\n\ndiff --git a/remote-curl.c b/remote-curl.c\nindex da3e07184a..8b740354e5 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -682,6 +682,8 @@ static curlioerr rpc_ioctl(CURL *handle, int cmd, void *clientp)\n struct rpc_in_data {\n \tstruct rpc_state *rpc;\n \tstruct active_request_slot *slot;\n+\tstruct strbuf len_buf;\n+\tint remaining;\n };\n \n /*\n@@ -692,6 +694,7 @@ static size_t rpc_in(char *ptr, size_t eltsize,\n \t\tsize_t nmemb, void *buffer_)\n {\n \tsize_t size = eltsize * nmemb;\n+\tsize_t unwritten = size;\n \tstruct rpc_in_data *data = buffer_;\n \tlong response_code;\n \n@@ -702,7 +705,42 @@ static size_t rpc_in(char *ptr, size_t eltsize,\n \t\treturn size;\n \tif (size)\n \t\tdata->rpc->any_written = 1;\n-\twrite_or_die(data->rpc->in, ptr, size);\n+\n+\twhile (unwritten) {\n+\t\tif (!data->remaining) {\n+\t\t\tint digits_remaining = 4 - data->len_buf.len;\n+\t\t\tif (digits_remaining > unwritten)\n+\t\t\t\tdigits_remaining = unwritten;\n+\t\t\tstrbuf_add(&data->len_buf, ptr, digits_remaining);\n+\t\t\tptr += digits_remaining;\n+\t\t\tunwritten -= digits_remaining;\n+\n+\t\t\tif (data->len_buf.len == 4) {\n+\t\t\t\tdata->remaining = packet_length(data->len_buf.buf);\n+\t\t\t\tif (data->remaining < 0) {\n+\t\t\t\t\tdie(_(\"remote-curl: bad line length character: %.4s\"), data->len_buf.buf);\n+\t\t\t\t} else if (data->remaining <= 1) {\n+\t\t\t\t\tdata->remaining = 0;\n+\t\t\t\t} else if (data->remaining < 4) {\n+\t\t\t\t\tdie(_(\"remote-curl: bad line length %d\"), data->remaining);\n+\t\t\t\t} else {\n+\t\t\t\t\tdata->remaining -= 4;\n+\t\t\t\t}\n+\t\t\t\twrite_or_die(data->rpc->in, data->len_buf.buf, 4);\n+\t\t\t\tstrbuf_reset(&data->len_buf);\n+\t\t\t}\n+\t\t}\n+\n+\t\tif (data->remaining) {\n+\t\t\tint remaining = data->remaining;\n+\t\t\tif (remaining > unwritten)\n+\t\t\t\tremaining = unwritten;\n+\t\t\twrite_or_die(data->rpc->in, ptr, remaining);\n+\t\t\tptr += remaining;\n+\t\t\tunwritten -= remaining;\n+\t\t\tdata->remaining -= remaining;\n+\t\t}\n+\t}\n \treturn size;\n }\n \n@@ -920,6 +958,8 @@ static int post_rpc(struct rpc_state *rpc, int flush_received)\n \tcurl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, rpc_in);\n \trpc_in_data.rpc = rpc;\n \trpc_in_data.slot = slot;\n+\tstrbuf_init(&rpc_in_data.len_buf, 4);\n+\trpc_in_data.remaining = 0;\n \tcurl_easy_setopt(slot->curl, CURLOPT_FILE, &rpc_in_data);\n \tcurl_easy_setopt(slot->curl, CURLOPT_FAILONERROR, 0);\n \n@@ -936,6 +976,9 @@ static int post_rpc(struct rpc_state *rpc, int flush_received)\n \tif (!rpc->any_written)\n \t\terr = -1;\n \n+\tif (rpc_in_data.remaining)\n+\t\terr = error(_(\"%d bytes are still expected\"), rpc_in_data.remaining);\n+\n \tcurl_slist_free_all(headers);\n \tfree(gzip_body);\n \treturn err;\n-- \n2.26.2.706.g87896c9627\n\n"},{"id":"397731","messageId":"7a689da2bb820f70d9e668d656b088af2297d456.1589393036.git.liu.denton@gmail.com","threadId":"53462","inReplyTo":"cover.1589393036.git.liu.denton@gmail.com","subject":"[PATCH 6/6] remote-curl: ensure last packet is a flush","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-05-13T18:04:58Z","receivedAt":"2020-05-13T18:05:20Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"Currently, remote-curl acts as a proxy and blindly forwards packets\nbetween an HTTP server and fetch-pack. In the case of a stateless RPC\nconnection where the connection is terminated before the transaction is\ncomplete, remote-curl will blindly forward the packets before waiting on\nmore input from fetch-pack. Meanwhile, fetch-pack will read the\ntransaction and continue reading, expecting more input to continue the\ntransaction. This results in a deadlock between the two processes.\n\nThis can be seen in the following command which does not terminate:\n\n\t$ git -c protocol.version=2 clone https://github.com/git/git.git --shallow-since=20151012\n\tCloning into 'git'...\n\nwhereas the v1 version does terminate as expected:\n\n\t$ git -c protocol.version=1 clone https://github.com/git/git.git --shallow-since=20151012\n\tCloning into 'git'...\n\tfatal: the remote end hung up unexpectedly\n\nInstead of blindly forwarding packets, raise a flag when a flush packet\nis encountered. Ensure that the last packet sent is a flush packet\notherwise error out, breaking the deadlock.\n\nThis is not a complete solution to the problem, however. It is possible\nthat a flush packet could be sent in the middle of a message and the\nconnection could die immediately after. Then, remote-curl would not\nerror out and fetch-pack would still be in the middle of a transaction\nand they would enter deadlock. A complete solution would involve\nreframing the stateless-connect protocol, possibly by introducing\nanother control packet (\"0002\"?) as a stateless request separator\npacket which is always sent at the end of post_rpc().\n\nAlthough this is not a complete solution, it is better than nothing and\nit resolves the reported issue for now.\n\nReported-by: Force Charlie \nHelped-by: Jeff King \nSigned-off-by: Denton Liu \n---\n\nNotes:\n I wish there were some way to insert a timeout on the test case so that\n we don't block forever in case we regress.\n\n remote-curl.c | 9 +++++++++\n t/t5702-protocol-v2.sh | 17 +++++++++++++++++\n 2 files changed, 26 insertions(+)\n\ndiff --git a/remote-curl.c b/remote-curl.c\nindex 8b740354e5..aab17851be 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -684,6 +684,7 @@ struct rpc_in_data {\n \tstruct active_request_slot *slot;\n \tstruct strbuf len_buf;\n \tint remaining;\n+\tint last_flush;\n };\n \n /*\n@@ -707,6 +708,8 @@ static size_t rpc_in(char *ptr, size_t eltsize,\n \t\tdata->rpc->any_written = 1;\n \n \twhile (unwritten) {\n+\t\tdata->last_flush = 0;\n+\n \t\tif (!data->remaining) {\n \t\t\tint digits_remaining = 4 - data->len_buf.len;\n \t\t\tif (digits_remaining > unwritten)\n@@ -720,6 +723,8 @@ static size_t rpc_in(char *ptr, size_t eltsize,\n \t\t\t\tif (data->remaining < 0) {\n \t\t\t\t\tdie(_(\"remote-curl: bad line length character: %.4s\"), data->len_buf.buf);\n \t\t\t\t} else if (data->remaining <= 1) {\n+\t\t\t\t\tif (!data->remaining)\n+\t\t\t\t\t\tdata->last_flush = 1;\n \t\t\t\t\tdata->remaining = 0;\n \t\t\t\t} else if (data->remaining < 4) {\n \t\t\t\t\tdie(_(\"remote-curl: bad line length %d\"), data->remaining);\n@@ -960,6 +965,7 @@ static int post_rpc(struct rpc_state *rpc, int flush_received)\n \trpc_in_data.slot = slot;\n \tstrbuf_init(&rpc_in_data.len_buf, 4);\n \trpc_in_data.remaining = 0;\n+\trpc_in_data.last_flush = 0;\n \tcurl_easy_setopt(slot->curl, CURLOPT_FILE, &rpc_in_data);\n \tcurl_easy_setopt(slot->curl, CURLOPT_FAILONERROR, 0);\n \n@@ -979,6 +985,9 @@ static int post_rpc(struct rpc_state *rpc, int flush_received)\n \tif (rpc_in_data.remaining)\n \t\terr = error(_(\"%d bytes are still expected\"), rpc_in_data.remaining);\n \n+\tif (!rpc_in_data.last_flush)\n+\t\terr = error(_(\"last packet was not a flush\"));\n+\n \tcurl_slist_free_all(headers);\n \tfree(gzip_body);\n \treturn err;\ndiff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh\nindex 5039e66dc4..4570d0746c 100755\n--- a/t/t5702-protocol-v2.sh\n+++ b/t/t5702-protocol-v2.sh\n@@ -586,6 +586,23 @@ test_expect_success 'clone with http:// using protocol v2' '\n \t! grep \"Send header: Transfer-Encoding: chunked\" log\n '\n \n+test_expect_success 'clone with http:// using protocol v2 and invalid parameters' '\n+\ttest_when_finished \"rm -f log\" &&\n+\n+\ttest_must_fail env GIT_TRACE_PACKET=\"$(pwd)/log\" GIT_TRACE_CURL=\"$(pwd)/log\" \\\n+\t\tgit -c protocol.version=2 \\\n+\t\tclone --shallow-since=20151012 \"$HTTPD_URL/smart/http_parent\" http_child_invalid 2>err &&\n+\n+\t# Client requested to use protocol v2\n+\tgrep \"Git-Protocol: version=2\" log &&\n+\t# Server responded using protocol v2\n+\tgrep \"git< version 2\" log &&\n+\t# Verify that we errored out in the expected way\n+\ttest_i18ngrep \"last packet was not a flush\" err &&\n+\t# Verify that the chunked encoding sending codepath is NOT exercised\n+\t! grep \"Send header: Transfer-Encoding: chunked\" log\n+'\n+\n test_expect_success 'clone big repository with http:// using protocol v2' '\n \ttest_when_finished \"rm -f log\" &&\n \n-- \n2.26.2.706.g87896c9627\n\n"},{"id":"397791","messageId":"CAPig+cQyCSMtSrdk54E6MVYL4prhNyWgBKkkBFHXo5hShqQ+=Q@mail.gmail.com","threadId":"53462","inReplyTo":"c89c1841008dfc2d111369fb682b946a0c33b7be.1589393036.git.liu.denton@gmail.com","subject":"Re: [PATCH 3/6] transport: combine common cases with a fallthrough","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-05-13T23:14:35Z","receivedAt":"2020-05-13T23:14:50Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, May 13, 2020 at 2:07 PM Denton Liu wrote:\n> In the switch statement, the difference between the `protocol_v2` and\n> `protocol_v{1,0}` arms is a prepatory call to die_if_server_options() in\n\nWhat is \"prepatory\"?\n\nMore below...\n\n> the latter. The fetch_pack() call is identical in both arms. However,\n> since this fetch_pack() call has so many parameters, it is not\n> immediately obvious that the call is identical in both cases.\n>\n> Rewrite the switch statement to fallthrough from the v{1,0} case to v2\n> so that they share a common fetch_pack() call. This reduces duplication\n> and makes the logic more clear for future readers.\n>\n> Signed-off-by: Denton Liu \n> ---\n> diff --git a/transport.c b/transport.c\n> @@ -370,15 +370,11 @@ static int fetch_refs_via_pack(struct transport *transport,\n> switch (data->version) {\n> - case protocol_v2:\n> - refs = fetch_pack(&args, data->fd,\n> - refs_tmp ? refs_tmp : transport->remote_refs,\n> - to_fetch, nr_heads, &data->shallow,\n> - &transport->pack_lockfile, data->version);\n> - break;\n> - case protocol_v1:\n> case protocol_v0:\n> + case protocol_v1:\n> die_if_server_options(transport);\n> + /* fallthrough */\n> + case protocol_v2:\n> refs = fetch_pack(&args, data->fd,\n> refs_tmp ? refs_tmp : transport->remote_refs,\n> to_fetch, nr_heads, &data->shallow,\n\nI can't say that I'm a fan of this change. While it may make it clear\nthat the two calls are identical, it makes it more difficult to reason\nabout the distinct v0, v1, and v2 cases. It also makes it more\ndifficult to extend or make changes to one or another case, should\nthat become necessary in the future.\n"},{"id":"397793","messageId":"CAPig+cQpQsoCg4ujASek3RP7HhHVpgprXbekGQJKseX=tbYYfw@mail.gmail.com","threadId":"53462","inReplyTo":"891a39c853ce3669b6167dc9ad8a2328e4321a9e.1589393036.git.liu.denton@gmail.com","subject":"Re: [PATCH 4/6] pkt-line: extern packet_length()","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-05-13T23:23:52Z","receivedAt":"2020-05-13T23:24:07Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, May 13, 2020 at 2:07 PM Denton Liu wrote:\n> In a future commit, we will be manually processing packets and we will\n> need to access the length header. In order to simplify this, extern\n> packet_length() so that the logic can be reused.\n>\n> Signed-off-by: Denton Liu \n> ---\n> diff --git a/pkt-line.c b/pkt-line.c\n> @@ -306,7 +306,7 @@ static int get_packet_data(int fd, char **src_buf, size_t *src_size,\n> +/*\n> + * Reads a packetized line and returns the length header of the packet.\n> + */\n> +int packet_length(const char *linelen);\n\nThe function comment seems rather gobbledy-gooky to me. Perhaps it\ncould be clearer by saying something along the lines of the input\nbeing a hexadecimal 2-digit representation of a packet length and that\nthe function converts it to a numeric value (between 0 and 255).\n"},{"id":"397934","messageId":"20200515205632.GB115445@coredump.intra.peff.net","threadId":"53462","inReplyTo":"891a39c853ce3669b6167dc9ad8a2328e4321a9e.1589393036.git.liu.denton@gmail.com","subject":"Re: [PATCH 4/6] pkt-line: extern packet_length()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-05-15T20:56:32Z","receivedAt":"2020-05-15T20:56:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, May 13, 2020 at 02:04:56PM -0400, Denton Liu wrote:\n\n> +/*\n> + * Reads a packetized line and returns the length header of the packet.\n> + */\n> +int packet_length(const char *linelen);\n\nIf this is becoming public, I think we need to talk a bit more about:\n\n - what is linelen; it's not a NUL-terminated string, which I would\n have expected from the prototype. It must be at least 4 chars, and\n doesn't need terminated.\n\n - what happens on error; it looks like we return -1\n\nI think the prototype would be more clear to me as:\n\n int packet_length(const char *line, size_t len)\n {\n\tif (len < 4)\n\t\treturn -1;\n }\n\nwhich makes it clear that this is a sized buffer. But since nobody\nshould ever be passing anything except \"4\", it may be overkill. I'd be\nhappy enough with a comment.\n\n-Peff\n"},{"id":"397935","messageId":"20200515205717.GC115445@coredump.intra.peff.net","threadId":"53462","inReplyTo":"20200515205632.GB115445@coredump.intra.peff.net","subject":"Re: [PATCH 4/6] pkt-line: extern packet_length()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-05-15T20:57:17Z","receivedAt":"2020-05-15T20:57:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, May 15, 2020 at 04:56:32PM -0400, Jeff King wrote:\n\n> On Wed, May 13, 2020 at 02:04:56PM -0400, Denton Liu wrote:\n> \n> > +/*\n> > + * Reads a packetized line and returns the length header of the packet.\n> > + */\n> > +int packet_length(const char *linelen);\n> \n> If this is becoming public, I think we need to talk a bit more about:\n> \n> - what is linelen; it's not a NUL-terminated string, which I would\n> have expected from the prototype. It must be at least 4 chars, and\n> doesn't need terminated.\n> \n> - what happens on error; it looks like we return -1\n> \n> I think the prototype would be more clear to me as:\n> \n> int packet_length(const char *line, size_t len)\n> {\n> \tif (len < 4)\n> \t\treturn -1;\n> }\n> \n> which makes it clear that this is a sized buffer. But since nobody\n> should ever be passing anything except \"4\", it may be overkill. I'd be\n> happy enough with a comment.\n\nOh, and obviously:\n\n int packet_length(const char linelen[4]);\n\nwould be descriptive, but I think falls afoul of C pointer/array\nweirdness.\n\n-Peff\n"},{"id":"397936","messageId":"20200515210245.GA3278654@generichostname","threadId":"53462","inReplyTo":"7a689da2bb820f70d9e668d656b088af2297d456.1589393036.git.liu.denton@gmail.com","subject":"Re: [PATCH 6/6] remote-curl: ensure last packet is a flush","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-05-15T21:02:45Z","receivedAt":"2020-05-15T21:02:51Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"On Wed, May 13, 2020 at 02:04:58PM -0400, Denton Liu wrote:\n> This is not a complete solution to the problem, however. It is possible\n> that a flush packet could be sent in the middle of a message and the\n> connection could die immediately after. Then, remote-curl would not\n> error out and fetch-pack would still be in the middle of a transaction\n> and they would enter deadlock. A complete solution would involve\n> reframing the stateless-connect protocol, possibly by introducing\n> another control packet (\"0002\"?) as a stateless request separator\n> packet which is always sent at the end of post_rpc().\n> \n> Although this is not a complete solution, it is better than nothing and\n> it resolves the reported issue for now.\n\nI managed to get the implementation of the control packet working. As a\nresult, I will be dropping this patch in the next reroll and replacing\nit with the more complete solution. For anyone reviewing, feel free to\nskip this patch.\n"},{"id":"397940","messageId":"20200515213844.GD115445@coredump.intra.peff.net","threadId":"53462","inReplyTo":"3ed7cf87aaa40ee66b20aa929d89d28fefcec312.1589393036.git.liu.denton@gmail.com","subject":"Re: [PATCH 5/6] remote-curl: error on incomplete packet","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-05-15T21:38:44Z","receivedAt":"2020-05-15T21:38:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, May 13, 2020 at 02:04:57PM -0400, Denton Liu wrote:\n\n> Currently, remote-curl acts as a proxy and blindly forwards packets\n> between an HTTP server and fetch-pack. In the case of a stateless RPC\n> connection where the connection is terminated with a partially written\n> packet, remote-curl will blindly send the partially written packet\n> before waiting on more input from fetch-pack. Meanwhile, fetch-pack will\n> read the partial packet and continue reading, expecting more input. This\n> results in a deadlock between the two processes.\n> \n> Instead of blindly forwarding packets, inspect each packet to ensure\n> that it is a full packet, erroring out if a partial packet is sent.\n\nHmm. Right now there's no assumption in rpc_in that we're writing\npktlines. Will this always be the case?\n\nI think the answer is probably yes. If there's a case where it isn't, it\nmight be something like v0 git-over-http against a server which doesn't\nhave the sideband capability.\n\n> diff --git a/remote-curl.c b/remote-curl.c\n> index da3e07184a..8b740354e5 100644\n> --- a/remote-curl.c\n> +++ b/remote-curl.c\n> @@ -682,6 +682,8 @@ static curlioerr rpc_ioctl(CURL *handle, int cmd, void *clientp)\n> struct rpc_in_data {\n> \tstruct rpc_state *rpc;\n> \tstruct active_request_slot *slot;\n> +\tstruct strbuf len_buf;\n> +\tint remaining;\n\nThis remaining is \"remaining in the current packet\", I assume? An \"int\"\nshould be OK for that.\n\nUsing a strbuf feels a bit like overkill, because we have to remember to\nfree it (which I think doesn't actually happen in your patch). Could we\njust use a \"char len_buf[4]\" (you'd need an extra int to count how many\nbytes you've received there).\n\n> @@ -702,7 +705,42 @@ static size_t rpc_in(char *ptr, size_t eltsize,\n> \t\treturn size;\n> \tif (size)\n> \t\tdata->rpc->any_written = 1;\n> -\twrite_or_die(data->rpc->in, ptr, size);\n> +\n> +\twhile (unwritten) {\n> +\t\tif (!data->remaining) {\n> +\t\t\tint digits_remaining = 4 - data->len_buf.len;\n> +\t\t\tif (digits_remaining > unwritten)\n> +\t\t\t\tdigits_remaining = unwritten;\n> +\t\t\tstrbuf_add(&data->len_buf, ptr, digits_remaining);\n> +\t\t\tptr += digits_remaining;\n> +\t\t\tunwritten -= digits_remaining;\n\nSo we actually save up the 4 bytes, not sending them at all until we get\nthem. I wonder if this might be easier to follow if our count was simply\nadvisory. I.e., continue to write data as we get it, but keep a small\nstate machine tracking pktlines (which could even be done in its own\nseparate struct/function pair).\n\nI dunno. It might be about the same level of confusing, but it makes it\neasy to keep the logic separate from rpc_in, and it lets us put all of\nthe policy bits at the end, after we know we've received all of the\ndata (in post_rpc):\n\n if (data->len_buf.len < 4)\n die(\"incomplete packet header\");\n if (data->remaining)\n die(\"incomplete packet\");\n /* imagine we kept the actual pktlen value, instead of just counting\n * down remaining */\n if (data->pktlen)\n die(\"did not end in flush\");\n\nNotably, I'm not sure if your code will complain if the connection dies\nwill reading the 4-byte header (remaining would still be 0). That won't\nleave the caller trying to read the packet (we never sent them the\nheader), but they may still be waiting for a response.\n\n> +\t\t\tif (data->len_buf.len == 4) {\n> +\t\t\t\tdata->remaining = packet_length(data->len_buf.buf);\n> +\t\t\t\tif (data->remaining < 0) {\n> +\t\t\t\t\tdie(_(\"remote-curl: bad line length character: %.4s\"), data->len_buf.buf);\n> +\t\t\t\t} else if (data->remaining <= 1) {\n> +\t\t\t\t\tdata->remaining = 0;\n\nThis treats 0001 delimiters the same as a 0000 flush. Expecting 0 more\nbytes is the right thing, but would we later want to differentiate in\npost_rpc()? I.e., is it ever correct for the server data to end with a\ndelim?\n\n> +\t\t\t\t} else if (data->remaining < 4) {\n> +\t\t\t\t\tdie(_(\"remote-curl: bad line length %d\"), data->remaining);\n\nWe don't use an 0002 or 0003 packet for anything yet, but this would\nneed to be updated if we ever did. I wonder if we should treat them also\nas zero-length packets and quietly pass through, which is likely the\nright thing to do. OTOH, this should complain loudly enough that\nsomebody would presumably notice as soon as they tried to use those\npackets.\n\nRegarding testing, I think the ideal thing would be a proxy layer we\ncould insert that simply eats all data after N bytes. That would be easy\nto do if we could use --upload-pack='git-upload-pack | head -c 500' or\nsomething. But we need it to happen between curl and the server side of\nGit. Possibly we could insert something between apache and\ngit-http-backend (simialr to how we handle broken-smart-http.sh.\n\n-Peff\n"},{"id":"397941","messageId":"20200515214102.GE115445@coredump.intra.peff.net","threadId":"53462","inReplyTo":"20200515210245.GA3278654@generichostname","subject":"Re: [PATCH 6/6] remote-curl: ensure last packet is a flush","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-05-15T21:41:02Z","receivedAt":"2020-05-15T21:41:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, May 15, 2020 at 05:02:45PM -0400, Denton Liu wrote:\n\n> On Wed, May 13, 2020 at 02:04:58PM -0400, Denton Liu wrote:\n> > This is not a complete solution to the problem, however. It is possible\n> > that a flush packet could be sent in the middle of a message and the\n> > connection could die immediately after. Then, remote-curl would not\n> > error out and fetch-pack would still be in the middle of a transaction\n> > and they would enter deadlock. A complete solution would involve\n> > reframing the stateless-connect protocol, possibly by introducing\n> > another control packet (\"0002\"?) as a stateless request separator\n> > packet which is always sent at the end of post_rpc().\n> > \n> > Although this is not a complete solution, it is better than nothing and\n> > it resolves the reported issue for now.\n> \n> I managed to get the implementation of the control packet working. As a\n> result, I will be dropping this patch in the next reroll and replacing\n> it with the more complete solution. For anyone reviewing, feel free to\n> skip this patch.\n\nOK. I'm less concerned about a flush packet in the middle of the\nresponse fooling us into thinking things are done, and more that there\nmay be responses which _don't_ end in a flush. But maybe they all do.\n\nThis (and the previous patch) are definitely adding an extra layer of\nassumptions about what the protocol going over the rpc channel looks\nlike. That seems like it will introduce more fragility.\n\nI do kind of like the idea of a stateless separator packet, if I\nunderstand your meaning correctly. I'll wait to see what the patches\nlook like. :)\n\n-Peff\n"},{"id":"398038","messageId":"20200518090857.GA24557@generichostname","threadId":"53462","inReplyTo":"20200515213844.GD115445@coredump.intra.peff.net","subject":"Re: [PATCH 5/6] remote-curl: error on incomplete packet","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-05-18T09:08:57Z","receivedAt":"2020-05-18T09:09:04Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"Hi Peff,\n\nOn Fri, May 15, 2020 at 05:38:44PM -0400, Jeff King wrote:\n> On Wed, May 13, 2020 at 02:04:57PM -0400, Denton Liu wrote:\n> \n> > Currently, remote-curl acts as a proxy and blindly forwards packets\n> > between an HTTP server and fetch-pack. In the case of a stateless RPC\n> > connection where the connection is terminated with a partially written\n> > packet, remote-curl will blindly send the partially written packet\n> > before waiting on more input from fetch-pack. Meanwhile, fetch-pack will\n> > read the partial packet and continue reading, expecting more input. This\n> > results in a deadlock between the two processes.\n> > \n> > Instead of blindly forwarding packets, inspect each packet to ensure\n> > that it is a full packet, erroring out if a partial packet is sent.\n> \n> Hmm. Right now there's no assumption in rpc_in that we're writing\n> pktlines. Will this always be the case?\n> \n> I think the answer is probably yes. If there's a case where it isn't, it\n> might be something like v0 git-over-http against a server which doesn't\n> have the sideband capability.\n\nAs far as I can tell from skimming the code, v0 uses always pktlines\nalthough I'm far from being an expert on Git's networking stuff. Perhaps\nwe could implement this such that the line-length checking only happens\nfor stateless_connect()?\n"},{"id":"398039","messageId":"20200518091825.GB24557@generichostname","threadId":"53462","inReplyTo":"CAPig+cQyCSMtSrdk54E6MVYL4prhNyWgBKkkBFHXo5hShqQ+=Q@mail.gmail.com","subject":"Re: [PATCH 3/6] transport: combine common cases with a fallthrough","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-05-18T09:18:25Z","receivedAt":"2020-05-18T09:18:31Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"Hi Eric,\n\nOn Wed, May 13, 2020 at 07:14:35PM -0400, Eric Sunshine wrote:\n> > diff --git a/transport.c b/transport.c\n> > @@ -370,15 +370,11 @@ static int fetch_refs_via_pack(struct transport *transport,\n> > switch (data->version) {\n> > - case protocol_v2:\n> > - refs = fetch_pack(&args, data->fd,\n> > - refs_tmp ? refs_tmp : transport->remote_refs,\n> > - to_fetch, nr_heads, &data->shallow,\n> > - &transport->pack_lockfile, data->version);\n> > - break;\n> > - case protocol_v1:\n> > case protocol_v0:\n> > + case protocol_v1:\n> > die_if_server_options(transport);\n> > + /* fallthrough */\n> > + case protocol_v2:\n> > refs = fetch_pack(&args, data->fd,\n> > refs_tmp ? refs_tmp : transport->remote_refs,\n> > to_fetch, nr_heads, &data->shallow,\n> \n> I can't say that I'm a fan of this change. While it may make it clear\n> that the two calls are identical, it makes it more difficult to reason\n> about the distinct v0, v1, and v2 cases.\n\nI would argue that it would make it easier to reason about the distinct\ncases. From the rewritten version, it's more obvious that code used in\nthe v2 case is a subset of the code used in v0 and v1.\n\n> It also makes it more\n> difficult to extend or make changes to one or another case, should\n> that become necessary in the future.\n\nI would say that's the point of this change ;) When I was looking over\nthis code initially, I was scratching my head over the difference\nbetween the two cases because I expected them to be different in the two\ncases. If a change is necessary in the future, then it'll make sense to\nwrite it as two separate calls to fetch_pack() or whatever but until\nthat happens, I think it makes more sense remove the duplication.\n\nActually, thinking about it some more, do you think it would make more\nsense to just pull the fetch_pack() call out of the switch statement\nentirely? We could entirely eliminate the fallthrough if we do this.\n"},{"id":"398079","messageId":"cover.1589816718.git.liu.denton@gmail.com","threadId":"53462","inReplyTo":"cover.1589393036.git.liu.denton@gmail.com","subject":"[PATCH v2 0/7] remote-curl: fix deadlocks when remote server disconnects","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-05-18T15:47:17Z","receivedAt":"2020-05-18T15:47:37Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"The following command hangs forever:\n\n\t$ git -c protocol.version=2 clone https://github.com/git/git.git --shallow-since=20151012\n\tCloning into 'git'...\n\nThis occurs because the --shallow-since arg is incorrect and the server\ndies early. However, remote-curl does not realise that the server\nerrored out and just faithfully forwards the packets to fetch-pack\nbefore waiting on more input from fetch-pack. Meanwhile, fetch-pack\nkeeps reading as it still expects more input. As a result, the processes\ndeadlock. Original analysis by Peff:\nhttps://lore.kernel.org/git/20200328154936.GA1217052@coredump.intra.peff.net/\n\nChanges since v1:\n\n* Remove fallthrough in switch in favour of just extracting the common\n call out of the switch in patch 3\n\n* Add more detail in function comment and use `const char linelen[4]` in\n patch 4\n\n* Implement most of Peff's suggestions[0] in patch 5\n\n* Only operate on stateless_connect() in patch 5\n\n* Add tests in patch 5\n\n* Drop \"remote-curl: ensure last packet is a flush\" in favour of\n \"stateless-connect: send response end packet\"\n\n[0]: https://lore.kernel.org/git/20200515213844.GD115445@coredump.intra.peff.net/\n\nDenton Liu (7):\n remote-curl: fix typo\n remote-curl: remove label indentation\n transport: extract common fetch_pack() call\n pkt-line: extern packet_length()\n remote-curl: error on incomplete packet\n pkt-line: PACKET_READ_RESPONSE_END\n stateless-connect: send response end packet\n\n Documentation/gitremote-helpers.txt | 4 +-\n Documentation/technical/protocol-v2.txt | 2 +\n builtin/fetch-pack.c | 2 +-\n connect.c | 12 +++-\n fetch-pack.c | 12 ++++\n pkt-line.c | 13 +++-\n pkt-line.h | 11 +++\n remote-curl.c | 72 +++++++++++++++++--\n remote.h | 3 +-\n serve.c | 2 +\n t/helper/test-pkt-line.c | 4 ++\n t/lib-httpd.sh | 2 +\n t/lib-httpd/apache.conf | 8 +++\n .../incomplete-body-upload-pack-v2-http.sh | 3 +\n .../incomplete-length-upload-pack-v2-http.sh | 3 +\n t/t5702-protocol-v2.sh | 47 ++++++++++++\n transport.c | 16 ++---\n 17 files changed, 197 insertions(+), 19 deletions(-)\n create mode 100644 t/lib-httpd/incomplete-body-upload-pack-v2-http.sh\n create mode 100644 t/lib-httpd/incomplete-length-upload-pack-v2-http.sh\n\nRange-diff against v1:\n1: b390875f87 = 1: b390875f87 remote-curl: fix typo\n2: a2b28c0b28 = 2: a2b28c0b28 remote-curl: remove label indentation\n3: c89c184100 < -: ---------- transport: combine common cases with a fallthrough\n-: ---------- > 3: 3a42575bd5 transport: extract common fetch_pack() call\n4: 891a39c853 ! 4: c2b9d033bb pkt-line: extern packet_length()\n @@ Commit message\n need to access the length header. In order to simplify this, extern\n packet_length() so that the logic can be reused.\n \n + Change the function parameter from a `const char *` to\n + `const char linelen[4]`. Even though these two types behave identically\n + as function parameters, use the array notation to semantically indicate\n + exactly what this function is expecting as an argument.\n +\n ## pkt-line.c ##\n @@ pkt-line.c: static int get_packet_data(int fd, char **src_buf, size_t *src_size,\n \treturn ret;\n }\n \n -static int packet_length(const char *linelen)\n -+int packet_length(const char *linelen)\n ++int packet_length(const char linelen[4])\n {\n \tint val = hex2chr(linelen);\n \treturn (val < 0) ? val : (val << 8) | hex2chr(linelen + 2);\n @@ pkt-line.h: int write_packetized_from_buf(const char *src_in, size_t len, int fd\n \t\t*buffer, unsigned size, int options);\n \n +/*\n -+ * Reads a packetized line and returns the length header of the packet.\n ++ * Convert a four hex digit packet line length header into its numeric\n ++ * representation. linelen should not be null-terminated.\n ++ *\n ++ * If linelen contains non-hex characters, return -1. Otherwise, return the\n ++ * numeric value of the length header.\n + */\n -+int packet_length(const char *linelen);\n ++int packet_length(const char linelen[4]);\n +\n /*\n * Read a packetized line into a buffer like the 'packet_read()' function but\n5: 3ed7cf87aa < -: ---------- remote-curl: error on incomplete packet\n6: 7a689da2bb < -: ---------- remote-curl: ensure last packet is a flush\n-: ---------- > 5: 52ce5fdffd remote-curl: error on incomplete packet\n-: ---------- > 6: 744b078324 pkt-line: PACKET_READ_RESPONSE_END\n-: ---------- > 7: 4b079bcd83 stateless-connect: send response end packet\n-- \n2.26.2.706.g87896c9627\n\n"},{"id":"398080","messageId":"b390875f87dbf5f3f48909ce40f5a5749b32f5d4.1589816719.git.liu.denton@gmail.com","threadId":"53462","inReplyTo":"cover.1589816718.git.liu.denton@gmail.com","subject":"[PATCH v2 1/7] remote-curl: fix typo","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-05-18T15:47:18Z","receivedAt":"2020-05-18T15:47:38Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"Signed-off-by: Denton Liu \n---\n remote-curl.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/remote-curl.c b/remote-curl.c\nindex 1c9aa3d0ab..6844708f38 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -643,7 +643,7 @@ static size_t rpc_out(void *ptr, size_t eltsize,\n \t\t\treturn 0;\n \t\t}\n \t\t/*\n-\t\t * If avail is non-zerp, the line length for the flush still\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-- \n2.26.2.706.g87896c9627\n\n"},{"id":"398081","messageId":"a2b28c0b28bb972f164ff0a9e48ae6b623a2a125.1589816719.git.liu.denton@gmail.com","threadId":"53462","inReplyTo":"cover.1589816718.git.liu.denton@gmail.com","subject":"[PATCH v2 2/7] remote-curl: remove label indentation","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-05-18T15:47:19Z","receivedAt":"2020-05-18T15:47:40Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"In the codebase, labels are aligned to the leftmost column. Remove the\nspace-indentation from `free_specs:` to conform to this.\n\nSigned-off-by: Denton Liu \n---\n remote-curl.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/remote-curl.c b/remote-curl.c\nindex 6844708f38..da3e07184a 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -1276,7 +1276,7 @@ static void parse_push(struct strbuf *buf)\n \tif (ret)\n \t\texit(128); /* error already reported */\n \n- free_specs:\n+free_specs:\n \targv_array_clear(&specs);\n }\n \n-- \n2.26.2.706.g87896c9627\n\n"},{"id":"398082","messageId":"c2b9d033bb774d3bccb572802c3ae114cad8d830.1589816719.git.liu.denton@gmail.com","threadId":"53462","inReplyTo":"cover.1589816718.git.liu.denton@gmail.com","subject":"[PATCH v2 4/7] pkt-line: extern packet_length()","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-05-18T15:47:21Z","receivedAt":"2020-05-18T15:47:41Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"In a future commit, we will be manually processing packets and we will\nneed to access the length header. In order to simplify this, extern\npacket_length() so that the logic can be reused.\n\nChange the function parameter from a `const char *` to\n`const char linelen[4]`. Even though these two types behave identically\nas function parameters, use the array notation to semantically indicate\nexactly what this function is expecting as an argument.\n\nSigned-off-by: Denton Liu \n---\n pkt-line.c | 2 +-\n pkt-line.h | 9 +++++++++\n 2 files changed, 10 insertions(+), 1 deletion(-)\n\ndiff --git a/pkt-line.c b/pkt-line.c\nindex a0e87b1e81..5c3b4539b5 100644\n--- a/pkt-line.c\n+++ b/pkt-line.c\n@@ -306,7 +306,7 @@ static int get_packet_data(int fd, char **src_buf, size_t *src_size,\n \treturn ret;\n }\n \n-static int packet_length(const char *linelen)\n+int packet_length(const char linelen[4])\n {\n \tint val = hex2chr(linelen);\n \treturn (val < 0) ? val : (val << 8) | hex2chr(linelen + 2);\ndiff --git a/pkt-line.h b/pkt-line.h\nindex fef3a0d792..de81776a7a 100644\n--- a/pkt-line.h\n+++ b/pkt-line.h\n@@ -74,6 +74,15 @@ int write_packetized_from_buf(const char *src_in, size_t len, int fd_out);\n int packet_read(int fd, char **src_buffer, size_t *src_len, char\n \t\t*buffer, unsigned size, int options);\n \n+/*\n+ * Convert a four hex digit packet line length header into its numeric\n+ * representation. linelen should not be null-terminated.\n+ *\n+ * If linelen contains non-hex characters, return -1. Otherwise, return the\n+ * numeric value of the length header.\n+ */\n+int packet_length(const char linelen[4]);\n+\n /*\n * Read a packetized line into a buffer like the 'packet_read()' function but\n * returns an 'enum packet_read_status' which indicates the status of the read.\n-- \n2.26.2.706.g87896c9627\n\n"},{"id":"398083","messageId":"3a42575bd5d124b6b2e536b1511107ebf5ec1091.1589816719.git.liu.denton@gmail.com","threadId":"53462","inReplyTo":"cover.1589816718.git.liu.denton@gmail.com","subject":"[PATCH v2 3/7] transport: extract common fetch_pack() call","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-05-18T15:47:20Z","receivedAt":"2020-05-18T15:47:42Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"In the switch statement, the difference between the `protocol_v2` and\n`protocol_v{1,0}` arms is a preparatory call to die_if_server_options() in\nthe latter. The fetch_pack() call is identical in both arms. However,\nsince this fetch_pack() call has so many parameters, it is not\nimmediately obvious that the call is identical in both cases.\n\nExtract the common fetch_pack() call out of the switch statement so that\ncode duplication is reduced and the logic is more clear for future\nreaders.\n\nSigned-off-by: Denton Liu \n---\n transport.c | 13 +++++--------\n 1 file changed, 5 insertions(+), 8 deletions(-)\n\ndiff --git a/transport.c b/transport.c\nindex 15f5ba4e8f..a6002e502f 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -371,22 +371,19 @@ static int fetch_refs_via_pack(struct transport *transport,\n \n \tswitch (data->version) {\n \tcase protocol_v2:\n-\t\trefs = fetch_pack(&args, data->fd,\n-\t\t\t\t refs_tmp ? refs_tmp : transport->remote_refs,\n-\t\t\t\t to_fetch, nr_heads, &data->shallow,\n-\t\t\t\t &transport->pack_lockfile, data->version);\n+\t\t/* do nothing */\n \t\tbreak;\n \tcase protocol_v1:\n \tcase protocol_v0:\n \t\tdie_if_server_options(transport);\n-\t\trefs = fetch_pack(&args, data->fd,\n-\t\t\t\t refs_tmp ? refs_tmp : transport->remote_refs,\n-\t\t\t\t to_fetch, nr_heads, &data->shallow,\n-\t\t\t\t &transport->pack_lockfile, data->version);\n \t\tbreak;\n \tcase protocol_unknown_version:\n \t\tBUG(\"unknown protocol version\");\n \t}\n+\trefs = fetch_pack(&args, data->fd,\n+\t\t\t refs_tmp ? refs_tmp : transport->remote_refs,\n+\t\t\t to_fetch, nr_heads, &data->shallow,\n+\t\t\t &transport->pack_lockfile, data->version);\n \n \tclose(data->fd[0]);\n \tclose(data->fd[1]);\n-- \n2.26.2.706.g87896c9627\n\n"},{"id":"398084","messageId":"744b0783248efe1b43f80b9be21be6d380a11a08.1589816719.git.liu.denton@gmail.com","threadId":"53462","inReplyTo":"cover.1589816718.git.liu.denton@gmail.com","subject":"[PATCH v2 6/7] pkt-line: PACKET_READ_RESPONSE_END","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-05-18T15:47:23Z","receivedAt":"2020-05-18T15:47:45Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"In a future commit, we will use PACKET_READ_RESPONSE_END to separate\nmessages proxied by remote-curl. To prepare for this, add the\nPACKET_READ_RESPONSE_END enum value.\n\nIn switch statements that need a case added, die() or BUG() when a\nPACKET_READ_RESPONSE_END is unexpected. Otherwise, mirror how\nPACKET_READ_DELIM is implemented (especially in cases where packets are\nbeing forwarded).\n\nSigned-off-by: Denton Liu \n---\n connect.c | 2 ++\n pkt-line.c | 11 +++++++++++\n pkt-line.h | 2 ++\n remote-curl.c | 2 ++\n serve.c | 2 ++\n t/helper/test-pkt-line.c | 4 ++++\n 6 files changed, 23 insertions(+)\n\ndiff --git a/connect.c b/connect.c\nindex 23013c6344..11c6ec70a0 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -127,6 +127,7 @@ enum protocol_version discover_version(struct packet_reader *reader)\n \t\tdie_initial_contact(0);\n \tcase PACKET_READ_FLUSH:\n \tcase PACKET_READ_DELIM:\n+\tcase PACKET_READ_RESPONSE_END:\n \t\tversion = protocol_v0;\n \t\tbreak;\n \tcase PACKET_READ_NORMAL:\n@@ -310,6 +311,7 @@ struct ref **get_remote_heads(struct packet_reader *reader,\n \t\t\tstate = EXPECTING_DONE;\n \t\t\tbreak;\n \t\tcase PACKET_READ_DELIM:\n+\t\tcase PACKET_READ_RESPONSE_END:\n \t\t\tdie(_(\"invalid packet\"));\n \t\t}\n \ndiff --git a/pkt-line.c b/pkt-line.c\nindex 5c3b4539b5..fdd6b90e68 100644\n--- a/pkt-line.c\n+++ b/pkt-line.c\n@@ -99,6 +99,13 @@ void packet_delim(int fd)\n \t\tdie_errno(_(\"unable to write delim packet\"));\n }\n \n+void packet_response_end(int fd)\n+{\n+\tpacket_trace(\"0002\", 4, 1);\n+\tif (write_in_full(fd, \"0002\", 4) < 0)\n+\t\tdie_errno(_(\"unable to write stateless separator packet\"));\n+}\n+\n int packet_flush_gently(int fd)\n {\n \tpacket_trace(\"0000\", 4, 1);\n@@ -337,6 +344,10 @@ enum packet_read_status packet_read_with_status(int fd, char **src_buffer,\n \t\tpacket_trace(\"0001\", 4, 0);\n \t\t*pktlen = 0;\n \t\treturn PACKET_READ_DELIM;\n+\t} else if (len == 2) {\n+\t\tpacket_trace(\"0002\", 4, 0);\n+\t\t*pktlen = 0;\n+\t\treturn PACKET_READ_RESPONSE_END;\n \t} else if (len < 4) {\n \t\tdie(_(\"protocol error: bad line length %d\"), len);\n \t}\ndiff --git a/pkt-line.h b/pkt-line.h\nindex de81776a7a..8156445873 100644\n--- a/pkt-line.h\n+++ b/pkt-line.h\n@@ -22,6 +22,7 @@\n */\n void packet_flush(int fd);\n void packet_delim(int fd);\n+void packet_response_end(int fd);\n void packet_write_fmt(int fd, const char *fmt, ...) __attribute__((format (printf, 2, 3)));\n void packet_buf_flush(struct strbuf *buf);\n void packet_buf_delim(struct strbuf *buf);\n@@ -94,6 +95,7 @@ enum packet_read_status {\n \tPACKET_READ_NORMAL,\n \tPACKET_READ_FLUSH,\n \tPACKET_READ_DELIM,\n+\tPACKET_READ_RESPONSE_END,\n };\n enum packet_read_status packet_read_with_status(int fd, char **src_buffer,\n \t\t\t\t\t\tsize_t *src_len, char *buffer,\ndiff --git a/remote-curl.c b/remote-curl.c\nindex e020140092..d02cb547e9 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -601,6 +601,8 @@ static int rpc_read_from_out(struct rpc_state *rpc, int options,\n \t\tcase PACKET_READ_FLUSH:\n \t\t\tmemcpy(buf - 4, \"0000\", 4);\n \t\t\tbreak;\n+\t\tcase PACKET_READ_RESPONSE_END:\n+\t\t\tdie(_(\"remote server sent stateless separator\"));\n \t\t}\n \t}\n \ndiff --git a/serve.c b/serve.c\nindex 317256c1a4..c046926ba1 100644\n--- a/serve.c\n+++ b/serve.c\n@@ -217,6 +217,8 @@ static int process_request(void)\n \n \t\t\tstate = PROCESS_REQUEST_DONE;\n \t\t\tbreak;\n+\t\tcase PACKET_READ_RESPONSE_END:\n+\t\t\tBUG(\"unexpected stateless separator packet\");\n \t\t}\n \t}\n \ndiff --git a/t/helper/test-pkt-line.c b/t/helper/test-pkt-line.c\nindex 12ca698e17..69152958e5 100644\n--- a/t/helper/test-pkt-line.c\n+++ b/t/helper/test-pkt-line.c\n@@ -46,6 +46,9 @@ static void unpack(void)\n \t\tcase PACKET_READ_DELIM:\n \t\t\tprintf(\"0001\\n\");\n \t\t\tbreak;\n+\t\tcase PACKET_READ_RESPONSE_END:\n+\t\t\tprintf(\"0002\\n\");\n+\t\t\tbreak;\n \t\t}\n \t}\n }\n@@ -75,6 +78,7 @@ static void unpack_sideband(void)\n \t\tcase PACKET_READ_FLUSH:\n \t\t\treturn;\n \t\tcase PACKET_READ_DELIM:\n+\t\tcase PACKET_READ_RESPONSE_END:\n \t\t\tbreak;\n \t\t}\n \t}\n-- \n2.26.2.706.g87896c9627\n\n"},{"id":"398085","messageId":"4b079bcd83ea80b8a0e81b0c1e3d5e083efeb9c6.1589816719.git.liu.denton@gmail.com","threadId":"53462","inReplyTo":"cover.1589816718.git.liu.denton@gmail.com","subject":"[PATCH v2 7/7] stateless-connect: send response end packet","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-05-18T15:47:24Z","receivedAt":"2020-05-18T15:47:46Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"Currently, remote-curl acts as a proxy and blindly forwards packets\nbetween an HTTP server and fetch-pack. In the case of a stateless RPC\nconnection where the connection is terminated before the transaction is\ncomplete, remote-curl will blindly forward the packets before waiting on\nmore input from fetch-pack. Meanwhile, fetch-pack will read the\ntransaction and continue reading, expecting more input to continue the\ntransaction. This results in a deadlock between the two processes.\n\nThis can be seen in the following command which does not terminate:\n\n\t$ git -c protocol.version=2 clone https://github.com/git/git.git --shallow-since=20151012\n\tCloning into 'git'...\n\nwhereas the v1 version does terminate as expected:\n\n\t$ git -c protocol.version=1 clone https://github.com/git/git.git --shallow-since=20151012\n\tCloning into 'git'...\n\tfatal: the remote end hung up unexpectedly\n\nInstead of blindly forwarding packets, make remote-curl insert response\nend and flush packets after proxying the responses from the remote\nserver when using stateless_connect(). On the RPC client side, ensure\nthat each response ends as described.\n\nA separate control packet is chosen because we need to be able to\ndifferentiate between what the remote server sends and remote-curl's\ncontrol packets. By ensuring in the remote-curl code that a server\ncannot send response end packets, we prevent a malicious server from\nbeing able to perform a denial of service attack in which they spoof a\nresponse end packet and cause the described deadlock to happen.\n\nReported-by: Force Charlie \nHelped-by: Jeff King \nSigned-off-by: Denton Liu \n---\n Documentation/gitremote-helpers.txt | 4 +++-\n Documentation/technical/protocol-v2.txt | 2 ++\n builtin/fetch-pack.c | 2 +-\n connect.c | 10 +++++++++-\n fetch-pack.c | 12 ++++++++++++\n remote-curl.c | 7 +++++++\n remote.h | 3 ++-\n t/t5702-protocol-v2.sh | 13 +++++++++++++\n transport.c | 3 ++-\n 9 files changed, 51 insertions(+), 5 deletions(-)\n\ndiff --git a/Documentation/gitremote-helpers.txt b/Documentation/gitremote-helpers.txt\nindex f48a031dc3..84f8e92b23 100644\n--- a/Documentation/gitremote-helpers.txt\n+++ b/Documentation/gitremote-helpers.txt\n@@ -405,7 +405,9 @@ Supported if the helper has the \"connect\" capability.\n \ttrying to fall back). After line feed terminating the positive\n \t(empty) response, the output of the service starts. Messages\n \t(both request and response) must consist of zero or more\n-\tPKT-LINEs, terminating in a flush packet. The client must not\n+\tPKT-LINEs, terminating in a flush packet. Response messages will\n+\thave a response end packet before the flush packet to indicate\n+\tthe end of a response. The client must not\n \texpect the server to store any state in between request-response\n \tpairs. After the connection ends, the remote helper exits.\n +\ndiff --git a/Documentation/technical/protocol-v2.txt b/Documentation/technical/protocol-v2.txt\nindex 7e3766cafb..3996d70891 100644\n--- a/Documentation/technical/protocol-v2.txt\n+++ b/Documentation/technical/protocol-v2.txt\n@@ -33,6 +33,8 @@ In protocol v2 these special packets will have the following semantics:\n \n * '0000' Flush Packet (flush-pkt) - indicates the end of a message\n * '0001' Delimiter Packet (delim-pkt) - separates sections of a message\n+ * '0002' Message Packet (response-end-pkt) - indicates the end of a response\n+ for stateless connections\n \n Initial Client Request\n ----------------------\ndiff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c\nindex 4771100072..94b0c89b82 100644\n--- a/builtin/fetch-pack.c\n+++ b/builtin/fetch-pack.c\n@@ -224,7 +224,7 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)\n \tversion = discover_version(&reader);\n \tswitch (version) {\n \tcase protocol_v2:\n-\t\tget_remote_refs(fd[1], &reader, &ref, 0, NULL, NULL);\n+\t\tget_remote_refs(fd[1], &reader, &ref, 0, NULL, NULL, args.stateless_rpc);\n \t\tbreak;\n \tcase protocol_v1:\n \tcase protocol_v0:\ndiff --git a/connect.c b/connect.c\nindex 11c6ec70a0..12b57f5c0a 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -409,7 +409,8 @@ static int process_ref_v2(const char *line, struct ref ***list)\n struct ref **get_remote_refs(int fd_out, struct packet_reader *reader,\n \t\t\t struct ref **list, int for_push,\n \t\t\t const struct argv_array *ref_prefixes,\n-\t\t\t const struct string_list *server_options)\n+\t\t\t const struct string_list *server_options,\n+\t\t\t int stateless_rpc)\n {\n \tint i;\n \t*list = NULL;\n@@ -446,6 +447,13 @@ struct ref **get_remote_refs(int fd_out, struct packet_reader *reader,\n \tif (reader->status != PACKET_READ_FLUSH)\n \t\tdie(_(\"expected flush after ref listing\"));\n \n+\tif (stateless_rpc) {\n+\t\tif (packet_reader_read(reader) != PACKET_READ_RESPONSE_END)\n+\t\t\tdie(_(\"expected response end packet after ref listing\"));\n+\t\tif (packet_reader_read(reader) != PACKET_READ_FLUSH)\n+\t\t\tdie(_(\"expected flush packet after response end\"));\n+\t}\n+\n \treturn list;\n }\n \ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex f73a2ce6cb..bcbbb7e2fb 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -1468,6 +1468,7 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,\n \tstruct fetch_negotiator negotiator_alloc;\n \tstruct fetch_negotiator *negotiator;\n \tint seen_ack = 0;\n+\tint check_http_delimiter;\n \n \tif (args->no_dependents) {\n \t\tnegotiator = NULL;\n@@ -1486,6 +1487,8 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,\n \t}\n \n \twhile (state != FETCH_DONE) {\n+\t\tcheck_http_delimiter = 0;\n+\n \t\tswitch (state) {\n \t\tcase FETCH_CHECK_LOCAL:\n \t\t\tsort_ref_list(&ref, ref_compare_name);\n@@ -1542,6 +1545,7 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,\n \t\t\t\t/* fallthrough */\n \t\t\tcase NO_COMMON_FOUND:\n \t\t\t\tstate = FETCH_SEND_REQUEST;\n+\t\t\t\tcheck_http_delimiter = 1;\n \t\t\t\tbreak;\n \t\t\t}\n \t\t\tbreak;\n@@ -1562,10 +1566,18 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,\n \t\t\t\tdie(_(\"git fetch-pack: fetch failed.\"));\n \n \t\t\tstate = FETCH_DONE;\n+\t\t\tcheck_http_delimiter = 1;\n \t\t\tbreak;\n \t\tcase FETCH_DONE:\n \t\t\tcontinue;\n \t\t}\n+\n+\t\tif (args->stateless_rpc && check_http_delimiter) {\n+\t\t\tif (packet_reader_read(&reader) != PACKET_READ_RESPONSE_END)\n+\t\t\t\tdie(_(\"git fetch-pack: expected response end packet\"));\n+\t\t\tif (packet_reader_read(&reader) != PACKET_READ_FLUSH)\n+\t\t\t\tdie(_(\"git fetch-pack: expected flush packet\"));\n+\t\t}\n \t}\n \n \tif (negotiator)\ndiff --git a/remote-curl.c b/remote-curl.c\nindex d02cb547e9..8a72b5ee7a 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -703,6 +703,8 @@ static void check_pktline(struct check_pktline_state *state, const char *ptr, si\n \t\t\t\tstate->remaining = packet_length(state->len_buf);\n \t\t\t\tif (state->remaining < 0) {\n \t\t\t\t\tdie(_(\"remote-curl: bad line length character: %.4s\"), state->len_buf);\n+\t\t\t\t} else if (state->remaining == 2) {\n+\t\t\t\t\tdie(_(\"remote-curl: unexpected response end packet\"));\n \t\t\t\t} else if (state->remaining < 4) {\n \t\t\t\t\tstate->remaining = 0;\n \t\t\t\t} else {\n@@ -991,6 +993,11 @@ static int post_rpc(struct rpc_state *rpc, int stateless_connect, int flush_rece\n \tif (rpc_in_data.pktline_state.remaining)\n \t\terr = error(_(\"%d bytes of body are still expected\"), rpc_in_data.pktline_state.remaining);\n \n+\tif (stateless_connect) {\n+\t\tpacket_response_end(rpc->in);\n+\t\tpacket_flush(rpc->in);\n+\t}\n+\n \tcurl_slist_free_all(headers);\n \tfree(gzip_body);\n \treturn err;\ndiff --git a/remote.h b/remote.h\nindex 11d8719b58..5cc26c1b3b 100644\n--- a/remote.h\n+++ b/remote.h\n@@ -179,7 +179,8 @@ struct ref **get_remote_heads(struct packet_reader *reader,\n struct ref **get_remote_refs(int fd_out, struct packet_reader *reader,\n \t\t\t struct ref **list, int for_push,\n \t\t\t const struct argv_array *ref_prefixes,\n-\t\t\t const struct string_list *server_options);\n+\t\t\t const struct string_list *server_options,\n+\t\t\t int stateless_rpc);\n \n int resolve_remote_symref(struct ref *ref, struct ref *list);\n \ndiff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh\nindex 4eb81ba2d4..8da65e60de 100755\n--- a/t/t5702-protocol-v2.sh\n+++ b/t/t5702-protocol-v2.sh\n@@ -620,6 +620,19 @@ test_expect_success 'clone repository with http:// using protocol v2 with incomp\n \ttest_i18ngrep \"bytes of body are still expected\" err\n '\n \n+test_expect_success 'clone with http:// using protocol v2 and invalid parameters' '\n+\ttest_when_finished \"rm -f log\" &&\n+\n+\ttest_must_fail env GIT_TRACE_PACKET=\"$(pwd)/log\" GIT_TRACE_CURL=\"$(pwd)/log\" \\\n+\t\tgit -c protocol.version=2 \\\n+\t\tclone --shallow-since=20151012 \"$HTTPD_URL/smart/http_parent\" http_child_invalid &&\n+\n+\t# Client requested to use protocol v2\n+\tgrep \"Git-Protocol: version=2\" log &&\n+\t# Server responded using protocol v2\n+\tgrep \"git< version 2\" log\n+'\n+\n test_expect_success 'clone big repository with http:// using protocol v2' '\n \ttest_when_finished \"rm -f log\" &&\n \ndiff --git a/transport.c b/transport.c\nindex a6002e502f..182978f4be 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -297,7 +297,8 @@ static struct ref *handshake(struct transport *transport, int for_push,\n \t\tif (must_list_refs)\n \t\t\tget_remote_refs(data->fd[1], &reader, &refs, for_push,\n \t\t\t\t\tref_prefixes,\n-\t\t\t\t\ttransport->server_options);\n+\t\t\t\t\ttransport->server_options,\n+\t\t\t\t\ttransport->stateless_rpc);\n \t\tbreak;\n \tcase protocol_v1:\n \tcase protocol_v0:\n-- \n2.26.2.706.g87896c9627\n\n"},{"id":"398086","messageId":"52ce5fdffd6741eeee8d69b804403383da0d879d.1589816719.git.liu.denton@gmail.com","threadId":"53462","inReplyTo":"cover.1589816718.git.liu.denton@gmail.com","subject":"[PATCH v2 5/7] remote-curl: error on incomplete packet","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-05-18T15:47:22Z","receivedAt":"2020-05-18T15:47:47Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"Currently, remote-curl acts as a proxy and blindly forwards packets\nbetween an HTTP server and fetch-pack. In the case of a stateless RPC\nconnection where the connection is terminated with a partially written\npacket, remote-curl will blindly send the partially written packet\nbefore waiting on more input from fetch-pack. Meanwhile, fetch-pack will\nread the partial packet and continue reading, expecting more input. This\nresults in a deadlock between the two processes.\n\nFor a stateless connection, inspect packets before sending them and\nerror out if a a packet line packet is incomplete.\n\nHelped-by: Jeff King \nSigned-off-by: Denton Liu \n---\n remote-curl.c | 59 ++++++++++++++++++-\n t/lib-httpd.sh | 2 +\n t/lib-httpd/apache.conf | 8 +++\n .../incomplete-body-upload-pack-v2-http.sh | 3 +\n .../incomplete-length-upload-pack-v2-http.sh | 3 +\n t/t5702-protocol-v2.sh | 34 +++++++++++\n 6 files changed, 106 insertions(+), 3 deletions(-)\n create mode 100644 t/lib-httpd/incomplete-body-upload-pack-v2-http.sh\n create mode 100644 t/lib-httpd/incomplete-length-upload-pack-v2-http.sh\n\ndiff --git a/remote-curl.c b/remote-curl.c\nindex da3e07184a..e020140092 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -679,9 +679,53 @@ static curlioerr rpc_ioctl(CURL *handle, int cmd, void *clientp)\n }\n #endif\n \n+struct check_pktline_state {\n+\tchar len_buf[4];\n+\tint len_filled;\n+\tint remaining;\n+};\n+\n+static void check_pktline(struct check_pktline_state *state, const char *ptr, size_t size)\n+{\n+\twhile (size) {\n+\t\tif (!state->remaining) {\n+\t\t\tint digits_remaining = 4 - state->len_filled;\n+\t\t\tif (digits_remaining > size)\n+\t\t\t\tdigits_remaining = size;\n+\t\t\tmemcpy(&state->len_buf[state->len_filled], ptr, digits_remaining);\n+\t\t\tstate->len_filled += digits_remaining;\n+\t\t\tptr += digits_remaining;\n+\t\t\tsize -= digits_remaining;\n+\n+\t\t\tif (state->len_filled == 4) {\n+\t\t\t\tstate->remaining = packet_length(state->len_buf);\n+\t\t\t\tif (state->remaining < 0) {\n+\t\t\t\t\tdie(_(\"remote-curl: bad line length character: %.4s\"), state->len_buf);\n+\t\t\t\t} else if (state->remaining < 4) {\n+\t\t\t\t\tstate->remaining = 0;\n+\t\t\t\t} else {\n+\t\t\t\t\tstate->remaining -= 4;\n+\t\t\t\t}\n+\t\t\t\tstate->len_filled = 0;\n+\t\t\t}\n+\t\t}\n+\n+\t\tif (state->remaining) {\n+\t\t\tint remaining = state->remaining;\n+\t\t\tif (remaining > size)\n+\t\t\t\tremaining = size;\n+\t\t\tptr += remaining;\n+\t\t\tsize -= remaining;\n+\t\t\tstate->remaining -= remaining;\n+\t\t}\n+\t}\n+}\n+\n struct rpc_in_data {\n \tstruct rpc_state *rpc;\n \tstruct active_request_slot *slot;\n+\tint check_pktline;\n+\tstruct check_pktline_state pktline_state;\n };\n \n /*\n@@ -702,6 +746,8 @@ static size_t rpc_in(char *ptr, size_t eltsize,\n \t\treturn size;\n \tif (size)\n \t\tdata->rpc->any_written = 1;\n+\tif (data->check_pktline)\n+\t\tcheck_pktline(&data->pktline_state, ptr, size);\n \twrite_or_die(data->rpc->in, ptr, size);\n \treturn size;\n }\n@@ -778,7 +824,7 @@ static curl_off_t xcurl_off_t(size_t len)\n * If flush_received is true, do not attempt to read any more; just use what's\n * in rpc->buf.\n */\n-static int post_rpc(struct rpc_state *rpc, int flush_received)\n+static int post_rpc(struct rpc_state *rpc, int stateless_connect, int flush_received)\n {\n \tstruct active_request_slot *slot;\n \tstruct curl_slist *headers = http_copy_default_headers();\n@@ -920,6 +966,8 @@ static int post_rpc(struct rpc_state *rpc, int flush_received)\n \tcurl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, rpc_in);\n \trpc_in_data.rpc = rpc;\n \trpc_in_data.slot = slot;\n+\trpc_in_data.check_pktline = stateless_connect;\n+\tmemset(&rpc_in_data.pktline_state, 0, sizeof(rpc_in_data.pktline_state));\n \tcurl_easy_setopt(slot->curl, CURLOPT_FILE, &rpc_in_data);\n \tcurl_easy_setopt(slot->curl, CURLOPT_FAILONERROR, 0);\n \n@@ -936,6 +984,11 @@ static int post_rpc(struct rpc_state *rpc, int flush_received)\n \tif (!rpc->any_written)\n \t\terr = -1;\n \n+\tif (rpc_in_data.pktline_state.len_filled)\n+\t\terr = error(_(\"%d bytes of length header were received\"), rpc_in_data.pktline_state.len_filled);\n+\tif (rpc_in_data.pktline_state.remaining)\n+\t\terr = error(_(\"%d bytes of body are still expected\"), rpc_in_data.pktline_state.remaining);\n+\n \tcurl_slist_free_all(headers);\n \tfree(gzip_body);\n \treturn err;\n@@ -985,7 +1038,7 @@ static int rpc_service(struct rpc_state *rpc, struct discovery *heads,\n \t\t\tbreak;\n \t\trpc->pos = 0;\n \t\trpc->len = n;\n-\t\terr |= post_rpc(rpc, 0);\n+\t\terr |= post_rpc(rpc, 0, 0);\n \t}\n \n \tclose(client.in);\n@@ -1342,7 +1395,7 @@ static int stateless_connect(const char *service_name)\n \t\t\tBUG(\"The entire rpc->buf should be larger than LARGE_PACKET_MAX\");\n \t\tif (status == PACKET_READ_EOF)\n \t\t\tbreak;\n-\t\tif (post_rpc(&rpc, status == PACKET_READ_FLUSH))\n+\t\tif (post_rpc(&rpc, 1, status == PACKET_READ_FLUSH))\n \t\t\t/* We would have an err here */\n \t\t\tbreak;\n \t\t/* Reset the buffer for next request */\ndiff --git a/t/lib-httpd.sh b/t/lib-httpd.sh\nindex 1449ee95e9..d2edfa4c50 100644\n--- a/t/lib-httpd.sh\n+++ b/t/lib-httpd.sh\n@@ -129,6 +129,8 @@ install_script () {\n prepare_httpd() {\n \tmkdir -p \"$HTTPD_DOCUMENT_ROOT_PATH\"\n \tcp \"$TEST_PATH\"/passwd \"$HTTPD_ROOT_PATH\"\n+\tinstall_script incomplete-length-upload-pack-v2-http.sh\n+\tinstall_script incomplete-body-upload-pack-v2-http.sh\n \tinstall_script broken-smart-http.sh\n \tinstall_script error-smart-http.sh\n \tinstall_script error.sh\ndiff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf\nindex 994e5290d6..afa91e38b0 100644\n--- a/t/lib-httpd/apache.conf\n+++ b/t/lib-httpd/apache.conf\n@@ -117,6 +117,8 @@ Alias /auth/dumb/ www/auth/dumb/\n \tSetEnv GIT_EXEC_PATH ${GIT_EXEC_PATH}\n \tSetEnv GIT_HTTP_EXPORT_ALL\n \n+ScriptAlias /smart/incomplete_length/git-upload-pack incomplete-length-upload-pack-v2-http.sh/\n+ScriptAlias /smart/incomplete_body/git-upload-pack incomplete-body-upload-pack-v2-http.sh/\n ScriptAliasMatch /error_git_upload_pack/(.*)/git-upload-pack error.sh/\n ScriptAliasMatch /smart_*[^/]*/(.*) ${GIT_EXEC_PATH}/git-http-backend/$1\n ScriptAlias /broken_smart/ broken-smart-http.sh/\n@@ -126,6 +128,12 @@ ScriptAliasMatch /one_time_perl/(.*) apply-one-time-perl.sh/$1\n \n \tOptions FollowSymlinks\n \n+\n+\tOptions ExecCGI\n+\n+\n+\tOptions ExecCGI\n+\n \n \tOptions ExecCGI\n \ndiff --git a/t/lib-httpd/incomplete-body-upload-pack-v2-http.sh b/t/lib-httpd/incomplete-body-upload-pack-v2-http.sh\nnew file mode 100644\nindex 0000000000..2f5ed9fcf6\n--- /dev/null\n+++ b/t/lib-httpd/incomplete-body-upload-pack-v2-http.sh\n@@ -0,0 +1,3 @@\n+printf \"Content-Type: text/%s\\n\" \"application/x-git-upload-pack-result\"\n+echo\n+printf \"%s%s\\n\" \"0079\" \"45\"\ndiff --git a/t/lib-httpd/incomplete-length-upload-pack-v2-http.sh b/t/lib-httpd/incomplete-length-upload-pack-v2-http.sh\nnew file mode 100644\nindex 0000000000..86c6e648c9\n--- /dev/null\n+++ b/t/lib-httpd/incomplete-length-upload-pack-v2-http.sh\n@@ -0,0 +1,3 @@\n+printf \"Content-Type: text/%s\\n\" \"application/x-git-upload-pack-result\"\n+echo\n+printf \"%s\\n\" \"00\"\ndiff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh\nindex 5039e66dc4..4eb81ba2d4 100755\n--- a/t/t5702-protocol-v2.sh\n+++ b/t/t5702-protocol-v2.sh\n@@ -586,6 +586,40 @@ test_expect_success 'clone with http:// using protocol v2' '\n \t! grep \"Send header: Transfer-Encoding: chunked\" log\n '\n \n+test_expect_success 'clone repository with http:// using protocol v2 with incomplete pktline length' '\n+\ttest_when_finished \"rm -f log\" &&\n+\n+\tgit init \"$HTTPD_DOCUMENT_ROOT_PATH/incomplete_length\" &&\n+\ttest_commit -C \"$HTTPD_DOCUMENT_ROOT_PATH/incomplete_length\" file &&\n+\n+\ttest_must_fail env GIT_TRACE_PACKET=\"$(pwd)/log\" GIT_TRACE_CURL=\"$(pwd)/log\" git -c protocol.version=2 \\\n+\t\tclone \"$HTTPD_URL/smart/incomplete_length\" incomplete_length_child 2>err &&\n+\n+\t# Client requested to use protocol v2\n+\tgrep \"Git-Protocol: version=2\" log &&\n+\t# Server responded using protocol v2\n+\tgrep \"git< version 2\" log &&\n+\t# Client reported appropriate failure\n+\ttest_i18ngrep \"bytes of length header were received\" err\n+'\n+\n+test_expect_success 'clone repository with http:// using protocol v2 with incomplete pktline body' '\n+\ttest_when_finished \"rm -f log\" &&\n+\n+\tgit init \"$HTTPD_DOCUMENT_ROOT_PATH/incomplete_body\" &&\n+\ttest_commit -C \"$HTTPD_DOCUMENT_ROOT_PATH/incomplete_body\" file &&\n+\n+\ttest_must_fail env GIT_TRACE_PACKET=\"$(pwd)/log\" GIT_TRACE_CURL=\"$(pwd)/log\" git -c protocol.version=2 \\\n+\t\tclone \"$HTTPD_URL/smart/incomplete_body\" incomplete_body_child 2>err &&\n+\n+\t# Client requested to use protocol v2\n+\tgrep \"Git-Protocol: version=2\" log &&\n+\t# Server responded using protocol v2\n+\tgrep \"git< version 2\" log &&\n+\t# Client reported appropriate failure\n+\ttest_i18ngrep \"bytes of body are still expected\" err\n+'\n+\n test_expect_success 'clone big repository with http:// using protocol v2' '\n \ttest_when_finished \"rm -f log\" &&\n \n-- \n2.26.2.706.g87896c9627\n\n"},{"id":"398087","messageId":"20200518154937.GB41356@coredump.intra.peff.net","threadId":"53462","inReplyTo":"20200518090857.GA24557@generichostname","subject":"Re: [PATCH 5/6] remote-curl: error on incomplete packet","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-05-18T15:49:37Z","receivedAt":"2020-05-18T15:49:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 18, 2020 at 05:08:57AM -0400, Denton Liu wrote:\n\n> > Hmm. Right now there's no assumption in rpc_in that we're writing\n> > pktlines. Will this always be the case?\n> > \n> > I think the answer is probably yes. If there's a case where it isn't, it\n> > might be something like v0 git-over-http against a server which doesn't\n> > have the sideband capability.\n> \n> As far as I can tell from skimming the code, v0 uses always pktlines\n> although I'm far from being an expert on Git's networking stuff. Perhaps\n> we could implement this such that the line-length checking only happens\n> for stateless_connect()?\n\nYeah, that would certainly limit the possibility of unintended side\neffects (and I don't think there's any benefit to this patch for the\nnon-stateless-connect cases).\n\nWe'd still be locking stateless-connect into always using pktlines, but\nI think that's probably OK in practice. AFAICT it's the case now, and I\ndoubt we'd have any desire to change it in the future (and if we do,\nthis is all local-ish code that we could modify).\n\nOne unfortunate thing about the protocol (but not new to your patch) is\nthat this will be a problem for _any_ remote-helper which claims to\nsupport stateless-connect. So they'd all need to carry similar code to\ndeal with this issue. Right now remote-curl is the only one, but\nprobably git-remote-ext and git-remote-fd could support this, too. Those\nare pretty exotic, though (I don't think anyone has even bothered to\nmake them support v2 yet).\n\n-Peff\n"},{"id":"398090","messageId":"20200518160425.GA42240@coredump.intra.peff.net","threadId":"53462","inReplyTo":"c2b9d033bb774d3bccb572802c3ae114cad8d830.1589816719.git.liu.denton@gmail.com","subject":"Re: [PATCH v2 4/7] pkt-line: extern packet_length()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-05-18T16:04:25Z","receivedAt":"2020-05-18T16:04:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 18, 2020 at 11:47:21AM -0400, Denton Liu wrote:\n\n> In a future commit, we will be manually processing packets and we will\n> need to access the length header. In order to simplify this, extern\n> packet_length() so that the logic can be reused.\n> \n> Change the function parameter from a `const char *` to\n> `const char linelen[4]`. Even though these two types behave identically\n> as function parameters, use the array notation to semantically indicate\n> exactly what this function is expecting as an argument.\n\nOK. I double-checked that we will not run into any problems with passing\npointers or arrays of other sizes (sadly compilers would not tell us if\nwe passed a too-small array, but at least it's documented for humans).\n\n> +/*\n> + * Convert a four hex digit packet line length header into its numeric\n> + * representation. linelen should not be null-terminated.\n\nMinor nit, but it is perfectly fine if there is a NUL. Maybe \"linelen\ndoes not need to be...\"?\n\n-Peff\n"},{"id":"398093","messageId":"20200518162225.GB42240@coredump.intra.peff.net","threadId":"53462","inReplyTo":"52ce5fdffd6741eeee8d69b804403383da0d879d.1589816719.git.liu.denton@gmail.com","subject":"Re: [PATCH v2 5/7] remote-curl: error on incomplete packet","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-05-18T16:22:25Z","receivedAt":"2020-05-18T16:22:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 18, 2020 at 11:47:22AM -0400, Denton Liu wrote:\n\n> +struct check_pktline_state {\n> +\tchar len_buf[4];\n> +\tint len_filled;\n> +\tint remaining;\n> +};\n> +\n> +static void check_pktline(struct check_pktline_state *state, const char *ptr, size_t size)\n\nThanks for converting this. I think having this broken out makes it a\nbit easier to reason about, and it should be much easier to reuse if we\nneed it elsewhere.\n\n> +{\n> +\twhile (size) {\n> +\t\tif (!state->remaining) {\n> +\t\t\tint digits_remaining = 4 - state->len_filled;\n> +\t\t\tif (digits_remaining > size)\n> +\t\t\t\tdigits_remaining = size;\n> +\t\t\tmemcpy(&state->len_buf[state->len_filled], ptr, digits_remaining);\n> +\t\t\tstate->len_filled += digits_remaining;\n> +\t\t\tptr += digits_remaining;\n> +\t\t\tsize -= digits_remaining;\n\nHaving personally written and screwed up this kind of parsing state\nmachine before, I read over this logic quite carefully. ;) I believe\nit's correct.\n\nAnother way would be to loop by single characters:\n\n while (state->len_filled < 4 && size) {\n state->len_buf[state->len_filled++] = *ptr;\n\t ptr++;\n\t size--;\n }\n\nwhich I _think_ is equivalent, and is a bit shorter. I'm OK with either\n(see below, especially).\n\nI'm not sure if it's worth replacing \"4\" with ARRAY_SIZE(state->len_buf).\nI generally try to avoid magic numbers, but it's certainly not like one\ncould change the size of len_buf and this code would still be useful. :)\n\n> +\t\t\tif (state->len_filled == 4) {\n> +\t\t\t\tstate->remaining = packet_length(state->len_buf);\n> +\t\t\t\tif (state->remaining < 0) {\n> +\t\t\t\t\tdie(_(\"remote-curl: bad line length character: %.4s\"), state->len_buf);\n> +\t\t\t\t} else if (state->remaining < 4) {\n> +\t\t\t\t\tstate->remaining = 0;\n> +\t\t\t\t} else {\n> +\t\t\t\t\tstate->remaining -= 4;\n> +\t\t\t\t}\n> +\t\t\t\tstate->len_filled = 0;\n> +\t\t\t}\n> +\t\t}\n\nThis part makes sense. We'll either leave len_filled as 1-3 (incomplete),\nor we'll read a whole packet (for a flush), or we'll be waiting to read\nthe rest of the packet.\n\n> +\t\tif (state->remaining) {\n> +\t\t\tint remaining = state->remaining;\n> +\t\t\tif (remaining > size)\n> +\t\t\t\tremaining = size;\n> +\t\t\tptr += remaining;\n> +\t\t\tsize -= remaining;\n> +\t\t\tstate->remaining -= remaining;\n> +\t\t}\n> +\t}\n> +}\n\nAnd here we most certainly don't want to read character-by-character,\nbecause we're not doing anything with each one, and we expect there to be\na lot more of them. Having the earlier loop match the form of this one is\nperhaps a good reason to leave it alone.\n\n> [...]\n> @@ -936,6 +984,11 @@ static int post_rpc(struct rpc_state *rpc, int flush_received)\n> \tif (!rpc->any_written)\n> \t\terr = -1;\n> \n> +\tif (rpc_in_data.pktline_state.len_filled)\n> +\t\terr = error(_(\"%d bytes of length header were received\"), rpc_in_data.pktline_state.len_filled);\n> +\tif (rpc_in_data.pktline_state.remaining)\n> +\t\terr = error(_(\"%d bytes of body are still expected\"), rpc_in_data.pktline_state.remaining);\n\nAnd here's the payoff for all of the state machine checks. Makes sense.\n\n> @@ -702,6 +746,8 @@ static size_t rpc_in(char *ptr, size_t eltsize,\n> \t\treturn size;\n> \tif (size)\n> \t\tdata->rpc->any_written = 1;\n> +\tif (data->check_pktline)\n> +\t\tcheck_pktline(&data->pktline_state, ptr, size);\n> \twrite_or_die(data->rpc->in, ptr, size);\n> \treturn size;\n> }\n\nAnd this is now conditional. Good...\n\n> @@ -920,6 +966,8 @@ static int post_rpc(struct rpc_state *rpc, int flush_received)\n> \tcurl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, rpc_in);\n> \trpc_in_data.rpc = rpc;\n> \trpc_in_data.slot = slot;\n> +\trpc_in_data.check_pktline = stateless_connect;\n> +\tmemset(&rpc_in_data.pktline_state, 0, sizeof(rpc_in_data.pktline_state));\n\nAnd we enable it only for stateless-connect. Makes perfect sense.\n\n> --- /dev/null\n> +++ b/t/lib-httpd/incomplete-body-upload-pack-v2-http.sh\n> @@ -0,0 +1,3 @@\n> +printf \"Content-Type: text/%s\\n\" \"application/x-git-upload-pack-result\"\n> +echo\n> +printf \"%s%s\\n\" \"0079\" \"45\"\n\nNice. Just having a deterministic half-written packet is way easier to\nreason about than my \"truncating proxy\" suggestion.\n\n> --- /dev/null\n> +++ b/t/lib-httpd/incomplete-length-upload-pack-v2-http.sh\n> @@ -0,0 +1,3 @@\n> +printf \"Content-Type: text/%s\\n\" \"application/x-git-upload-pack-result\"\n> +echo\n> +printf \"%s\\n\" \"00\"\n\nThanks for covering this case, too.\n\nI confirmed (as I'm sure you did) they both cause git-remote-http to hang\nwithout the fix in this patch.\n\n-Peff\n"},{"id":"398094","messageId":"xmqqeerh5hi5.fsf@gitster.c.googlers.com","threadId":"53462","inReplyTo":"20200515214102.GE115445@coredump.intra.peff.net","subject":"Re: [PATCH 6/6] remote-curl: ensure last packet is a flush","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-05-18T16:34:42Z","receivedAt":"2020-05-18T16:34:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Fri, May 15, 2020 at 05:02:45PM -0400, Denton Liu wrote:\n>\n>> On Wed, May 13, 2020 at 02:04:58PM -0400, Denton Liu wrote:\n>> > This is not a complete solution to the problem, however. It is possible\n>> > that a flush packet could be sent in the middle of a message and the\n>> > connection could die immediately after. Then, remote-curl would not\n>> > error out and fetch-pack would still be in the middle of a transaction\n>> > and they would enter deadlock. A complete solution would involve\n>> > reframing the stateless-connect protocol, possibly by introducing\n>> > another control packet (\"0002\"?) as a stateless request separator\n>> > packet which is always sent at the end of post_rpc().\n>> > ...\n> I do kind of like the idea of a stateless separator packet, if I\n> understand your meaning correctly. I'll wait to see what the patches\n> look like. :)\n\nI vaguely recall talking with somebody (perhaps it was Shawn Pearce)\nabout my long-time complaint on the on-the-wire protocol, in that\nthe protocol sometimes uses flush to merely mean \"I've finished\nspeaking one section of my speech\" and sometimes \"I've done talking\nfor now and it's your turn to speak to me\" (the receiving end needs\nto know which one a particular flush means without knowing the\ncontext in which it is issued---which makes it impossible to write a\nprotocol agnostic proxy. \n\nIf the above proposal means that we'll have an explicit way to say\n\"Not just I am done with one section of my speech, but it is your\nturn to speak and I am going to listen\", that would be wonderful.\n\n"},{"id":"398095","messageId":"20200518164308.GC42240@coredump.intra.peff.net","threadId":"53462","inReplyTo":"4b079bcd83ea80b8a0e81b0c1e3d5e083efeb9c6.1589816719.git.liu.denton@gmail.com","subject":"Re: [PATCH v2 7/7] stateless-connect: send response end packet","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-05-18T16:43:08Z","receivedAt":"2020-05-18T16:43:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 18, 2020 at 11:47:24AM -0400, Denton Liu wrote:\n\n> A separate control packet is chosen because we need to be able to\n> differentiate between what the remote server sends and remote-curl's\n> control packets. By ensuring in the remote-curl code that a server\n> cannot send response end packets, we prevent a malicious server from\n> being able to perform a denial of service attack in which they spoof a\n> response end packet and cause the described deadlock to happen.\n\nThanks for thinking about this. I was wondering what mischief a server\nmight be able to cause by sending such a packet.\n\n> @@ -446,6 +447,13 @@ struct ref **get_remote_refs(int fd_out, struct packet_reader *reader,\n> \tif (reader->status != PACKET_READ_FLUSH)\n> \t\tdie(_(\"expected flush after ref listing\"));\n> \n> +\tif (stateless_rpc) {\n> +\t\tif (packet_reader_read(reader) != PACKET_READ_RESPONSE_END)\n> +\t\t\tdie(_(\"expected response end packet after ref listing\"));\n> +\t\tif (packet_reader_read(reader) != PACKET_READ_FLUSH)\n> +\t\t\tdie(_(\"expected flush packet after response end\"));\n> +\t}\n\nHaving two packets here surprised me. We'd have already received the\nactual flush from the server (as you can see in the context above).\nWouldn't a single RESPONSE_END be enough to signal the reader?\n\n> diff --git a/fetch-pack.c b/fetch-pack.c\n> index f73a2ce6cb..bcbbb7e2fb 100644\n> --- a/fetch-pack.c\n> +++ b/fetch-pack.c\n> @@ -1468,6 +1468,7 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,\n> \tstruct fetch_negotiator negotiator_alloc;\n> \tstruct fetch_negotiator *negotiator;\n> \tint seen_ack = 0;\n> +\tint check_http_delimiter;\n\nThis flag was more complicated than I expected, and I'm not sure how we\ncan easily be certain that all necessary paths are covered.\n\nE.g., in FETCH_PROCESS_ACKS, we'll always be reading a response via\nprocess_acks(). Why do COMMON_FOUND and NO_COMMON_FOUND check for\nend-of-response, but READY doesn't? I think the answer is that we'd\ncontinue to read the same response via FETCH_GET_PACK in this instance.\n\nI just wonder if there is a better place to put this logic that would be\nmore certain to catch every place we'd expect to read to the end of a\nresponse. But I suppose not. We could push it down into process_acks(),\nbut it would have the same READY logic that's here. I'll admit part of\nmy complaint is that the existing do_fetch_pack_v2 state-machine switch\nis kind of hard to follow, but that's not your fault.\n\nTwo things that might help:\n\n - put a comment in READY to indicate why we don't need to check\n end-of-response (but do for its sibling cases)\n\n - instead of setting and clearing a variable and then using it to\n check at the end of the loop, call check_http_delimiter(&reader,\n args->stateless_rpc) at the moment we're done with the packet. The\n actual executed code would have the same behavior, I think, but it\n might be easier to understand that we're trying to hit the end of\n each response.\n\nAlso, it probably should be check_stateless_delimiter() or something.\nThere could be other helpers that support stateless-connect besides\nhttp.\n\nSpeaking of which: this is a change to the remote-helper protocol, since\nwe're now expecting stateless-connect helpers to produce these delim\npackets (and failing if they don't). I kind of doubt that anybody but\nremote-curl has implemented v2 stateless-connect, but should we be\nmarking this with an extra capability to be on the safe side?\n\n> +\t\tif (args->stateless_rpc && check_http_delimiter) {\n> +\t\t\tif (packet_reader_read(&reader) != PACKET_READ_RESPONSE_END)\n> +\t\t\t\tdie(_(\"git fetch-pack: expected response end packet\"));\n> +\t\t\tif (packet_reader_read(&reader) != PACKET_READ_FLUSH)\n> +\t\t\t\tdie(_(\"git fetch-pack: expected flush packet\"));\n> +\t\t}\n\nI wondered also if stateless_rpc might catch cases besides\nstateless-connect. But I guess we're in the v2 fetch-pack path, and that\n_only_ understands stateless-connect.\n\n-Peff\n"},{"id":"398097","messageId":"20200518165056.GD42240@coredump.intra.peff.net","threadId":"53462","inReplyTo":"cover.1589816718.git.liu.denton@gmail.com","subject":"Re: [PATCH v2 0/7] remote-curl: fix deadlocks when remote server disconnects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-05-18T16:50:56Z","receivedAt":"2020-05-18T16:50:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 18, 2020 at 11:47:17AM -0400, Denton Liu wrote:\n\n> Changes since v1:\n> \n> * Remove fallthrough in switch in favour of just extracting the common\n> call out of the switch in patch 3\n> \n> * Add more detail in function comment and use `const char linelen[4]` in\n> patch 4\n> \n> * Implement most of Peff's suggestions[0] in patch 5\n> \n> * Only operate on stateless_connect() in patch 5\n> \n> * Add tests in patch 5\n> \n> * Drop \"remote-curl: ensure last packet is a flush\" in favour of\n> \"stateless-connect: send response end packet\"\n\nOverall this looks pretty cleanly done. I left a few minor comments\nthroughout, but the real question is whether we prefer the \"0002\" packet\nin the last one, or if we instead insist that the response end in a\nflush.\n\nAt first glance, the \"0002\" seems like it's more flexible, because we're\nmaking fewer assumptions about what's being transferred over the\nstateless-connect channel. But in reality it still has to be pktlines\n(because we're checking them for incomplete or invalid packets already).\nSo all it really buys us is that the server response doesn't have to end\nwith a flush packet.\n\nSo I dunno. The \"0002\" solution is slightly more flexible, but I'm not\nsure it helps in practice. And it does eat up one of our two remaining\nspecial packet markers.\n\nThere is another solution, which would allow arbitrary data over\nstateless-connect: adding an extra level of pktline framing between the\nhelper and the parent process. But that's rather ugly (inner pktlines\nmay be split across outer ones, so you have to do a bunch of buffer\nreassembly). I think that's actually how v0 http works, IIRC.\nIMHO it probably isn't worth pursuing. That extra layer wrecks the\nelegance to the v2 stateless-connect approach, and we really do expect\nonly pktlines to go over it.\n\nSo I think either of your solutions (enforcing a final flush, or the\n0002 packet) is preferable. I'm on the fence between them.\n\n-Peff\n"},{"id":"398098","messageId":"20200518165105.GA2461535@generichostname","threadId":"53462","inReplyTo":"20200518162225.GB42240@coredump.intra.peff.net","subject":"Re: [PATCH v2 5/7] remote-curl: error on incomplete packet","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-05-18T16:51:05Z","receivedAt":"2020-05-18T16:51:11Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"Hi Peff,\n\nThanks for reading over the patch carefully. It is much appreciated :)\n\nOn Mon, May 18, 2020 at 12:22:25PM -0400, Jeff King wrote:\n> I'm not sure if it's worth replacing \"4\" with ARRAY_SIZE(state->len_buf).\n> I generally try to avoid magic numbers, but it's certainly not like one\n> could change the size of len_buf and this code would still be useful. :)\n\nYeah, I'm a bit sad about all the magic 4's that are strewn in the code\ntoo. I was thinking about consolidating all of the magic 4's into a\nconstant in pkt-line as a preparatory patch but I didn't want to risk\nholding up this series. I think once the dust settles here, I'll go\nahead and do that. For now, though, I think I'll leave it as is.\n"},{"id":"398099","messageId":"20200518165206.GE42240@coredump.intra.peff.net","threadId":"53462","inReplyTo":"xmqqeerh5hi5.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 6/6] remote-curl: ensure last packet is a flush","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-05-18T16:52:06Z","receivedAt":"2020-05-18T16:52:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 18, 2020 at 09:34:42AM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > On Fri, May 15, 2020 at 05:02:45PM -0400, Denton Liu wrote:\n> >\n> >> On Wed, May 13, 2020 at 02:04:58PM -0400, Denton Liu wrote:\n> >> > This is not a complete solution to the problem, however. It is possible\n> >> > that a flush packet could be sent in the middle of a message and the\n> >> > connection could die immediately after. Then, remote-curl would not\n> >> > error out and fetch-pack would still be in the middle of a transaction\n> >> > and they would enter deadlock. A complete solution would involve\n> >> > reframing the stateless-connect protocol, possibly by introducing\n> >> > another control packet (\"0002\"?) as a stateless request separator\n> >> > packet which is always sent at the end of post_rpc().\n> >> > ...\n> > I do kind of like the idea of a stateless separator packet, if I\n> > understand your meaning correctly. I'll wait to see what the patches\n> > look like. :)\n> \n> I vaguely recall talking with somebody (perhaps it was Shawn Pearce)\n> about my long-time complaint on the on-the-wire protocol, in that\n> the protocol sometimes uses flush to merely mean \"I've finished\n> speaking one section of my speech\" and sometimes \"I've done talking\n> for now and it's your turn to speak to me\" (the receiving end needs\n> to know which one a particular flush means without knowing the\n> context in which it is issued---which makes it impossible to write a\n> protocol agnostic proxy. \n> \n> If the above proposal means that we'll have an explicit way to say\n> \"Not just I am done with one section of my speech, but it is your\n> turn to speak and I am going to listen\", that would be wonderful.\n\nI think we already have that now in v2 because of the \"0001\" delim\npacket. All of the flushes are (I think) really \"this is the end of my\nspeech\", and any inner \"my list is ending, but I have more to say\"\ndelimiters are \"0001\".\n\n-Peff\n"},{"id":"398103","messageId":"20200518171249.GA2462058@generichostname","threadId":"53462","inReplyTo":"20200518164308.GC42240@coredump.intra.peff.net","subject":"Re: [PATCH v2 7/7] stateless-connect: send response end packet","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-05-18T17:12:49Z","receivedAt":"2020-05-18T17:12:55Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"Hi Peff,\n\nOn Mon, May 18, 2020 at 12:43:08PM -0400, Jeff King wrote:\n> On Mon, May 18, 2020 at 11:47:24AM -0400, Denton Liu wrote:\n\n[...]\n\n> > @@ -446,6 +447,13 @@ struct ref **get_remote_refs(int fd_out, struct packet_reader *reader,\n> > \tif (reader->status != PACKET_READ_FLUSH)\n> > \t\tdie(_(\"expected flush after ref listing\"));\n> > \n> > +\tif (stateless_rpc) {\n> > +\t\tif (packet_reader_read(reader) != PACKET_READ_RESPONSE_END)\n> > +\t\t\tdie(_(\"expected response end packet after ref listing\"));\n> > +\t\tif (packet_reader_read(reader) != PACKET_READ_FLUSH)\n> > +\t\t\tdie(_(\"expected flush packet after response end\"));\n> > +\t}\n> \n> Having two packets here surprised me. We'd have already received the\n> actual flush from the server (as you can see in the context above).\n> Wouldn't a single RESPONSE_END be enough to signal the reader?\n\nYes, having a single RESPONSE_END is enough. I sent a flush packet\nbecause it seemed appropriate when I was writing the patch to end all\nmessages with a flush but I suppose that's just cargo culting. I'll\nremove it in the next iteration.\n\n> > diff --git a/fetch-pack.c b/fetch-pack.c\n> > index f73a2ce6cb..bcbbb7e2fb 100644\n> > --- a/fetch-pack.c\n> > +++ b/fetch-pack.c\n> > @@ -1468,6 +1468,7 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,\n> > \tstruct fetch_negotiator negotiator_alloc;\n> > \tstruct fetch_negotiator *negotiator;\n> > \tint seen_ack = 0;\n> > +\tint check_http_delimiter;\n> \n> This flag was more complicated than I expected, and I'm not sure how we\n> can easily be certain that all necessary paths are covered.\n\nEssentially, any time we're in a path where state is set to\nFETCH_SEND_REQUEST or FETCH_DONE, we know that we've finished processing\nthe current request and we can check for response end packets. Of course,\nthe one exception to this is the first iteration of the loop when we're\ntransitioning from FETCH_CHECK_LOCAL to FETCH_SEND_REQUEST where we\ncan't check for response end packets since nothing has been requested\nyet.\n\n> E.g., in FETCH_PROCESS_ACKS, we'll always be reading a response via\n> process_acks(). Why do COMMON_FOUND and NO_COMMON_FOUND check for\n> end-of-response, but READY doesn't? I think the answer is that we'd\n> continue to read the same response via FETCH_GET_PACK in this instance.\n> \n> I just wonder if there is a better place to put this logic that would be\n> more certain to catch every place we'd expect to read to the end of a\n> response. But I suppose not. We could push it down into process_acks(),\n> but it would have the same READY logic that's here. I'll admit part of\n> my complaint is that the existing do_fetch_pack_v2 state-machine switch\n> is kind of hard to follow, but that's not your fault.\n\nI debated between the current implementation and doing something like\n\n\tint first_iteration = 1;\n\n\t...\n\n\twhile (state != FETCH_DONE) {\n\t\tswitch (...) {\n\t\t\t...\n\t\t}\n\n\t\tif (args->stateless_rpc && !first_iteration && (state == FETCH_SEND_REQUEST || state == FETCH_DONE)) {\n\t\t\tif (packet_reader_read(&reader) != PACKET_READ_RESPONSE_END)\n\t\t\t\tdie(_(\"git fetch-pack: expected response end packet\"));\n\t\t\tif (packet_reader_read(&reader) != PACKET_READ_FLUSH)\n\t\t\t\tdie(_(\"git fetch-pack: expected flush packet\"));\n\t\t}\n\t\tfirst_iteration = 0;\n\t}\n\nI think that this catches _all_ the cases without fiddling with any of\nthe state machine logic.\n\n[...]\n\n> Also, it probably should be check_stateless_delimiter() or something.\n> There could be other helpers that support stateless-connect besides\n> http.\n\nYeah... I forgot to change the check_http_delimiter name when I was\ndoing cleanup. Whoops.\n\n> Speaking of which: this is a change to the remote-helper protocol, since\n> we're now expecting stateless-connect helpers to produce these delim\n> packets (and failing if they don't). I kind of doubt that anybody but\n> remote-curl has implemented v2 stateless-connect, but should we be\n> marking this with an extra capability to be on the safe side?\n\nI think that we're probably safe from breaking anything external.\nAccording to the gitremote-helpers documentation, \n\n\t'stateless-connect'::\n\t\tExperimental; for internal use only.\n\nso I think that gives us a bit of leeway in terms of the changes we can\nmake to the stateless-connect protocol. They've been warned ;)\n\nThanks,\n\nDenton\n"},{"id":"398110","messageId":"20200518172605.GA52384@coredump.intra.peff.net","threadId":"53462","inReplyTo":"20200518171249.GA2462058@generichostname","subject":"Re: [PATCH v2 7/7] stateless-connect: send response end packet","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-05-18T17:26:05Z","receivedAt":"2020-05-18T17:26:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 18, 2020 at 01:12:49PM -0400, Denton Liu wrote:\n\n> > I just wonder if there is a better place to put this logic that would be\n> > more certain to catch every place we'd expect to read to the end of a\n> > response. But I suppose not. We could push it down into process_acks(),\n> > but it would have the same READY logic that's here. I'll admit part of\n> > my complaint is that the existing do_fetch_pack_v2 state-machine switch\n> > is kind of hard to follow, but that's not your fault.\n> \n> I debated between the current implementation and doing something like\n> \n> \tint first_iteration = 1;\n> \n> \t...\n> \n> \twhile (state != FETCH_DONE) {\n> \t\tswitch (...) {\n> \t\t\t...\n> \t\t}\n> \n> \t\tif (args->stateless_rpc && !first_iteration && (state == FETCH_SEND_REQUEST || state == FETCH_DONE)) {\n> \t\t\tif (packet_reader_read(&reader) != PACKET_READ_RESPONSE_END)\n> \t\t\t\tdie(_(\"git fetch-pack: expected response end packet\"));\n> \t\t\tif (packet_reader_read(&reader) != PACKET_READ_FLUSH)\n> \t\t\t\tdie(_(\"git fetch-pack: expected flush packet\"));\n> \t\t}\n> \t\tfirst_iteration = 0;\n> \t}\n> \n> I think that this catches _all_ the cases without fiddling with any of\n> the state machine logic.\n\nI think you're right that it does, but TBH I find it harder to follow,\nbecause now we're even further away from the actual response reads. What\nI most want to verify is:\n\n - there is no response-reading code path that does not check the\n delimiters\n\n - there is no code path where we check delimiters but did not actually\n read a response\n\nSo to me, putting the check as close to the response-reads as possible\nmakes sense. I think the patch below is equivalent and easier to verify,\nbut I admit it's somewhat subjective:\n\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex 5325649eda..0bf1e5760c 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -1443,6 +1443,17 @@ static void receive_wanted_refs(struct packet_reader *reader,\n \t\tdie(_(\"error processing wanted refs: %d\"), reader->status);\n }\n \n+static void check_stateless_delimiter(struct fetch_pack_args *args,\n+\t\t\t\t struct packet_reader *reader)\n+{\n+\tif (!args->stateless_rpc)\n+\t\treturn; /* not in stateless mode, no delimiter expected */\n+\tif (packet_reader_read(reader) != PACKET_READ_RESPONSE_END)\n+\t\tdie(_(\"git fetch-pack: expected response end packet\"));\n+\tif (packet_reader_read(reader) != PACKET_READ_FLUSH)\n+\t\tdie(_(\"git fetch-pack: expected flush packet\"));\n+}\n+\n enum fetch_state {\n \tFETCH_CHECK_LOCAL = 0,\n \tFETCH_SEND_REQUEST,\n@@ -1469,7 +1480,6 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,\n \tstruct fetch_negotiator negotiator_alloc;\n \tstruct fetch_negotiator *negotiator;\n \tint seen_ack = 0;\n-\tint check_http_delimiter;\n \n \tif (args->no_dependents) {\n \t\tnegotiator = NULL;\n@@ -1488,8 +1498,6 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,\n \t}\n \n \twhile (state != FETCH_DONE) {\n-\t\tcheck_http_delimiter = 0;\n-\n \t\tswitch (state) {\n \t\tcase FETCH_CHECK_LOCAL:\n \t\t\tsort_ref_list(&ref, ref_compare_name);\n@@ -1538,15 +1546,19 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,\n \t\t\t/* Process ACKs/NAKs */\n \t\t\tswitch (process_acks(negotiator, &reader, &common)) {\n \t\t\tcase READY:\n+\t\t\t\t/*\n+\t\t\t\t * Don't check for response delimiter; get_pack() will\n+\t\t\t\t * read the rest of this response.\n+\t\t\t\t */\n \t\t\t\tstate = FETCH_GET_PACK;\n \t\t\t\tbreak;\n \t\t\tcase COMMON_FOUND:\n \t\t\t\tin_vain = 0;\n \t\t\t\tseen_ack = 1;\n \t\t\t\t/* fallthrough */\n \t\t\tcase NO_COMMON_FOUND:\n \t\t\t\tstate = FETCH_SEND_REQUEST;\n-\t\t\t\tcheck_http_delimiter = 1;\n+\t\t\t\tcheck_stateless_delimiter(args, &reader);\n \t\t\t\tbreak;\n \t\t\t}\n \t\t\tbreak;\n@@ -1565,20 +1577,13 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,\n \t\t\tprocess_section_header(&reader, \"packfile\", 0);\n \t\t\tif (get_pack(args, fd, pack_lockfile, sought, nr_sought))\n \t\t\t\tdie(_(\"git fetch-pack: fetch failed.\"));\n+\t\t\tcheck_stateless_delimiter(args, &reader);\n \n \t\t\tstate = FETCH_DONE;\n-\t\t\tcheck_http_delimiter = 1;\n \t\t\tbreak;\n \t\tcase FETCH_DONE:\n \t\t\tcontinue;\n \t\t}\n-\n-\t\tif (args->stateless_rpc && check_http_delimiter) {\n-\t\t\tif (packet_reader_read(&reader) != PACKET_READ_RESPONSE_END)\n-\t\t\t\tdie(_(\"git fetch-pack: expected response end packet\"));\n-\t\t\tif (packet_reader_read(&reader) != PACKET_READ_FLUSH)\n-\t\t\t\tdie(_(\"git fetch-pack: expected flush packet\"));\n-\t\t}\n \t}\n \n \tif (negotiator)\n\n> > Speaking of which: this is a change to the remote-helper protocol, since\n> > we're now expecting stateless-connect helpers to produce these delim\n> > packets (and failing if they don't). I kind of doubt that anybody but\n> > remote-curl has implemented v2 stateless-connect, but should we be\n> > marking this with an extra capability to be on the safe side?\n> \n> I think that we're probably safe from breaking anything external.\n> According to the gitremote-helpers documentation, \n> \n> \t'stateless-connect'::\n> \t\tExperimental; for internal use only.\n> \n> so I think that gives us a bit of leeway in terms of the changes we can\n> make to the stateless-connect protocol. They've been warned ;)\n\nOK, I didn't realize we had explicitly marked it as experimental. I\nagree we're probably fine here, then (now that v2 is starting to settle\na bit more, we might think about lifting that warning, but this patch\nseries shows we're not quite there yet :) ).\n\n-Peff\n"},{"id":"398117","messageId":"20200518173652.GB2462058@generichostname","threadId":"53462","inReplyTo":"20200518165056.GD42240@coredump.intra.peff.net","subject":"Re: [PATCH v2 0/7] remote-curl: fix deadlocks when remote server disconnects","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-05-18T17:36:52Z","receivedAt":"2020-05-18T17:36:58Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"Hi Peff,\n\nOn Mon, May 18, 2020 at 12:50:56PM -0400, Jeff King wrote:\n> On Mon, May 18, 2020 at 11:47:17AM -0400, Denton Liu wrote:\n> \n> > Changes since v1:\n> > \n> > * Remove fallthrough in switch in favour of just extracting the common\n> > call out of the switch in patch 3\n> > \n> > * Add more detail in function comment and use `const char linelen[4]` in\n> > patch 4\n> > \n> > * Implement most of Peff's suggestions[0] in patch 5\n> > \n> > * Only operate on stateless_connect() in patch 5\n> > \n> > * Add tests in patch 5\n> > \n> > * Drop \"remote-curl: ensure last packet is a flush\" in favour of\n> > \"stateless-connect: send response end packet\"\n> \n> Overall this looks pretty cleanly done. I left a few minor comments\n> throughout, but the real question is whether we prefer the \"0002\" packet\n> in the last one, or if we instead insist that the response end in a\n> flush.\n\nThanks for the prompt review!\n\n> At first glance, the \"0002\" seems like it's more flexible, because we're\n> making fewer assumptions about what's being transferred over the\n> stateless-connect channel. But in reality it still has to be pktlines\n> (because we're checking them for incomplete or invalid packets already).\n> So all it really buys us is that the server response doesn't have to end\n> with a flush packet.\n> \n> So I dunno. The \"0002\" solution is slightly more flexible, but I'm not\n> sure it helps in practice. And it does eat up one of our two remaining\n> special packet markers.\n\nYeah, I was worried about consuming a special packet. One alternative\nthat I considered but is kind of gross is sending something like\n\"0028gitremote-helper: response complete\\n\" instead of \"0002\". Then,\ninstead of \"0002\" checks, we can check for that special string instead.\nI don't _think_ that stateless-connect currently allows for completely\narbitrary data but I might be mistaken.\n\n> There is another solution, which would allow arbitrary data over\n> stateless-connect: adding an extra level of pktline framing between the\n> helper and the parent process. But that's rather ugly (inner pktlines\n> may be split across outer ones, so you have to do a bunch of buffer\n> reassembly). I think that's actually how v0 http works, IIRC.\n> IMHO it probably isn't worth pursuing. That extra layer wrecks the\n> elegance to the v2 stateless-connect approach, and we really do expect\n> only pktlines to go over it.\n\nThis was the original approach that I was working on but I found it to\nbe much too invasive for my liking. (Also, I never actually managed to\nget it working ;) ) I think I gave up when I realised I had to insert\nreframing logic into index-pack and unpack-objects.\n\n> So I think either of your solutions (enforcing a final flush, or the\n> 0002 packet) is preferable. I'm on the fence between them.\n\nI'm mostly on the fence too. One advantage of 0002, however, is that a\nmalicious server can't end a request with 0002 as that's explicitly\nprevented. If a malicious server closes a connection after sending a\n0000, I think that they could cause a deadlock to happen if there are\nmultiple flush packets expected in a response. You mentioned in a\nsibling email that this currently doesn't happen wrt stateless-connect\nalthough I'm not sure if in the future, this is something that might\nchange. I dunno.\n\n> -Peff\n"},{"id":"398119","messageId":"CAPig+cQhZyyQ05B3xF1EkK0hpF8bXMbbPEJXGUdTkoZ5JaHO=Q@mail.gmail.com","threadId":"53462","inReplyTo":"20200518160425.GA42240@coredump.intra.peff.net","subject":"Re: [PATCH v2 4/7] pkt-line: extern packet_length()","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-05-18T17:50:52Z","receivedAt":"2020-05-18T18:21:55Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, May 18, 2020 at 12:04 PM Jeff King wrote:\n> On Mon, May 18, 2020 at 11:47:21AM -0400, Denton Liu wrote:\n> > Change the function parameter from a `const char *` to\n> > `const char linelen[4]`. Even though these two types behave identically\n> > as function parameters, use the array notation to semantically indicate\n> > exactly what this function is expecting as an argument.\n>\n> > +/*\n> > + * Convert a four hex digit packet line length header into its numeric\n> > + * representation. linelen should not be null-terminated.\n>\n> Minor nit, but it is perfectly fine if there is a NUL. Maybe \"linelen\n> does not need to be...\"?\n\nI had the exact same reaction when reading \"...should not be\nnull-terminated\", however, I'd prefer to drop mention of\nNUL-termination altogether since talking about it merely confuses the\nissue since it is quite clear both from the declaration (const\nchar[4]) and the documentation \"four hex digits\" that 'linelen' is\nexpected to contain exactly four hex digits and no NUL character(s).\n\nBy the way, I find the argument name \"linelen\" highly confusing; every\ntime I read it, I think it is an integral type, not a string or\ncharacter array. I'd very much prefer to see it renamed to \"s\" (or\nsomething) or dropped altogether:\n\n int packet_length(const char[4]);\n"},{"id":"398120","messageId":"CAPig+cR97uFvxmTBHiV5zi9ZqfZnDKmAi4ZG1rUOngk6QrWEVA@mail.gmail.com","threadId":"53462","inReplyTo":"20200518091825.GB24557@generichostname","subject":"Re: [PATCH 3/6] transport: combine common cases with a fallthrough","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-05-18T17:43:02Z","receivedAt":"2020-05-18T18:28:10Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, May 18, 2020 at 5:18 AM Denton Liu wrote:\n> On Wed, May 13, 2020 at 07:14:35PM -0400, Eric Sunshine wrote:\n> > > + case protocol_v1:\n> > > die_if_server_options(transport);\n> > > + /* fallthrough */\n> > > + case protocol_v2:\n> > > refs = fetch_pack(&args, data->fd,\n> > > refs_tmp ? refs_tmp : transport->remote_refs,\n> > > to_fetch, nr_heads, &data->shallow,\n> >\n> > I can't say that I'm a fan of this change. While it may make it clear\n> > that the two calls are identical, it makes it more difficult to reason\n> > about the distinct v0, v1, and v2 cases.\n>\n> Actually, thinking about it some more, do you think it would make more\n> sense to just pull the fetch_pack() call out of the switch statement\n> entirely? We could entirely eliminate the fallthrough if we do this.\n\nYes, I think that works better and is cleaner and easier to understand\nthan the \"fallthrough\" in v1.\n"},{"id":"398122","messageId":"xmqqtv0d2iov.fsf@gitster.c.googlers.com","threadId":"53462","inReplyTo":"a2b28c0b28bb972f164ff0a9e48ae6b623a2a125.1589816719.git.liu.denton@gmail.com","subject":"Re: [PATCH v2 2/7] remote-curl: remove label indentation","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-05-18T18:37:20Z","receivedAt":"2020-05-18T18:37:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Denton Liu writes:\n\n> In the codebase, labels are aligned to the leftmost column. Remove the\n> space-indentation from `free_specs:` to conform to this.\n\nEven though there are 50+ remaining labels that tells us otherwise,\nwe have 300+ labels that support the position.\n\n;-)\n\n>\n> Signed-off-by: Denton Liu \n> ---\n> remote-curl.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/remote-curl.c b/remote-curl.c\n> index 6844708f38..da3e07184a 100644\n> --- a/remote-curl.c\n> +++ b/remote-curl.c\n> @@ -1276,7 +1276,7 @@ static void parse_push(struct strbuf *buf)\n> \tif (ret)\n> \t\texit(128); /* error already reported */\n> \n> - free_specs:\n> +free_specs:\n> \targv_array_clear(&specs);\n> }\n"},{"id":"398124","messageId":"xmqqpnb12iit.fsf@gitster.c.googlers.com","threadId":"53462","inReplyTo":"3a42575bd5d124b6b2e536b1511107ebf5ec1091.1589816719.git.liu.denton@gmail.com","subject":"Re: [PATCH v2 3/7] transport: extract common fetch_pack() call","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-05-18T18:40:58Z","receivedAt":"2020-05-18T18:41:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Denton Liu writes:\n\n> In the switch statement, the difference between the `protocol_v2` and\n> `protocol_v{1,0}` arms is a preparatory call to die_if_server_options() in\n> the latter. The fetch_pack() call is identical in both arms. However,\n> since this fetch_pack() call has so many parameters, it is not\n> immediately obvious that the call is identical in both cases.\n\nSure.\n\n\tif (data->version < protocol_v2)\n\t\tdie_if_server_options(transport);\n\telse if (data->version == protocol_unknown_version)\n\t\tBUG(\"...\");\n\n\trefs = fetch_pack(...);\n\nmight have been even easier to follow, but that's OK.\n\n> \tswitch (data->version) {\n> \tcase protocol_v2:\n> -\t\trefs = fetch_pack(&args, data->fd,\n> -\t\t\t\t refs_tmp ? refs_tmp : transport->remote_refs,\n> -\t\t\t\t to_fetch, nr_heads, &data->shallow,\n> -\t\t\t\t &transport->pack_lockfile, data->version);\n> +\t\t/* do nothing */\n> \t\tbreak;\n> \tcase protocol_v1:\n> \tcase protocol_v0:\n> \t\tdie_if_server_options(transport);\n> -\t\trefs = fetch_pack(&args, data->fd,\n> -\t\t\t\t refs_tmp ? refs_tmp : transport->remote_refs,\n> -\t\t\t\t to_fetch, nr_heads, &data->shallow,\n> -\t\t\t\t &transport->pack_lockfile, data->version);\n> \t\tbreak;\n> \tcase protocol_unknown_version:\n> \t\tBUG(\"unknown protocol version\");\n> \t}\n> +\trefs = fetch_pack(&args, data->fd,\n> +\t\t\t refs_tmp ? refs_tmp : transport->remote_refs,\n> +\t\t\t to_fetch, nr_heads, &data->shallow,\n> +\t\t\t &transport->pack_lockfile, data->version);\n> \n> \tclose(data->fd[0]);\n> \tclose(data->fd[1]);\n"},{"id":"398125","messageId":"xmqqlflp2id2.fsf@gitster.c.googlers.com","threadId":"53462","inReplyTo":"20200518160425.GA42240@coredump.intra.peff.net","subject":"Re: [PATCH v2 4/7] pkt-line: extern packet_length()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-05-18T18:44:25Z","receivedAt":"2020-05-18T18:44:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> +/*\n>> + * Convert a four hex digit packet line length header into its numeric\n>> + * representation. linelen should not be null-terminated.\n>\n> Minor nit, but it is perfectly fine if there is a NUL. Maybe \"linelen\n> does not need to be...\"?\n\nYup, I was wondering about the same thing. I actually would go\nstronger than \"does not need to be\", as the byte after these four\nwould never be taken as \"the terminator for the linelen bytes\", even\nit happens to be '\\0'.\n\nJust getting rid of the extra sentence would suffice for that, I\nthink. The first sentence makes it clear that it is about\ninterpreting the 4 bytes we are given, and those 4 bytes come from\nthe 'packet line length header' the caller has.\n\n"},{"id":"398135","messageId":"xmqqh7wd2fyk.fsf@gitster.c.googlers.com","threadId":"53462","inReplyTo":"20200518165056.GD42240@coredump.intra.peff.net","subject":"Re: [PATCH v2 0/7] remote-curl: fix deadlocks when remote server disconnects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-05-18T19:36:19Z","receivedAt":"2020-05-18T19:36:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Overall this looks pretty cleanly done. I left a few minor comments\n> throughout, but the real question is whether we prefer the \"0002\" packet\n> in the last one, or if we instead insist that the response end in a\n> flush.\n\nThanks for a review. I was reading the series through and I found\nit a reasonably pleasant read.\n\n> So I think either of your solutions (enforcing a final flush, or the\n> 0002 packet) is preferable. I'm on the fence between them.\n\nI am undecided, too X-<.\n"},{"id":"398140","messageId":"20200518200812.GA63978@coredump.intra.peff.net","threadId":"53462","inReplyTo":"CAPig+cQhZyyQ05B3xF1EkK0hpF8bXMbbPEJXGUdTkoZ5JaHO=Q@mail.gmail.com","subject":"Re: [PATCH v2 4/7] pkt-line: extern packet_length()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-05-18T20:08:12Z","receivedAt":"2020-05-18T20:08:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 18, 2020 at 01:50:52PM -0400, Eric Sunshine wrote:\n\n> On Mon, May 18, 2020 at 12:04 PM Jeff King wrote:\n> > On Mon, May 18, 2020 at 11:47:21AM -0400, Denton Liu wrote:\n> > > Change the function parameter from a `const char *` to\n> > > `const char linelen[4]`. Even though these two types behave identically\n> > > as function parameters, use the array notation to semantically indicate\n> > > exactly what this function is expecting as an argument.\n> >\n> > > +/*\n> > > + * Convert a four hex digit packet line length header into its numeric\n> > > + * representation. linelen should not be null-terminated.\n> >\n> > Minor nit, but it is perfectly fine if there is a NUL. Maybe \"linelen\n> > does not need to be...\"?\n> \n> I had the exact same reaction when reading \"...should not be\n> null-terminated\", however, I'd prefer to drop mention of\n> NUL-termination altogether since talking about it merely confuses the\n> issue since it is quite clear both from the declaration (const\n> char[4]) and the documentation \"four hex digits\" that 'linelen' is\n> expected to contain exactly four hex digits and no NUL character(s).\n\nYeah, I think I have a slight preference for that, too.\n\n> By the way, I find the argument name \"linelen\" highly confusing; every\n> time I read it, I think it is an integral type, not a string or\n> character array. I'd very much prefer to see it renamed to \"s\" (or\n> something) or dropped altogether:\n> \n> int packet_length(const char[4]);\n\nI think giving it _some_ name is useful. Maybe \"const char\nlenbuf_hex[4]\".\n\n-Peff\n"},{"id":"398155","messageId":"20200518205854.GB63978@coredump.intra.peff.net","threadId":"53462","inReplyTo":"20200518173652.GB2462058@generichostname","subject":"Re: [PATCH v2 0/7] remote-curl: fix deadlocks when remote server disconnects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-05-18T20:58:54Z","receivedAt":"2020-05-18T20:58:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 18, 2020 at 01:36:52PM -0400, Denton Liu wrote:\n\n> > So I dunno. The \"0002\" solution is slightly more flexible, but I'm not\n> > sure it helps in practice. And it does eat up one of our two remaining\n> > special packet markers.\n> \n> Yeah, I was worried about consuming a special packet. One alternative\n> that I considered but is kind of gross is sending something like\n> \"0028gitremote-helper: response complete\\n\" instead of \"0002\". Then,\n> instead of \"0002\" checks, we can check for that special string instead.\n> I don't _think_ that stateless-connect currently allows for completely\n> arbitrary data but I might be mistaken.\n\nYeah, I think we should avoid using any packet that could be confused\nwith regular output. A well-functioning server should have sent a flush\nalready, and so the worst case would probably be that we would mistake\ntheir \"0028\" packet for ours if the network happens to cut out right at\nthat moment (and even that is obviously exceedingly unlikely). But it\njust seems easier to reason about if we know we can't get confused.\n\n> This was the original approach that I was working on but I found it to\n> be much too invasive for my liking. (Also, I never actually managed to\n> get it working ;) ) I think I gave up when I realised I had to insert\n> reframing logic into index-pack and unpack-objects.\n\nYuck. :) I think v0 does it by unwrapping the extra layer in fetch-pack,\nbut I'd have to double check. But I'm not going to, because I think we\nboth agree that your other approaches are way nicer.\n\n> > So I think either of your solutions (enforcing a final flush, or the\n> > 0002 packet) is preferable. I'm on the fence between them.\n> \n> I'm mostly on the fence too. One advantage of 0002, however, is that a\n> malicious server can't end a request with 0002 as that's explicitly\n> prevented. If a malicious server closes a connection after sending a\n> 0000, I think that they could cause a deadlock to happen if there are\n> multiple flush packets expected in a response. You mentioned in a\n> sibling email that this currently doesn't happen wrt stateless-connect\n> although I'm not sure if in the future, this is something that might\n> change. I dunno.\n\nYeah, agreed that the efficacy of the \"must end in flush\" strategy\ndepends on there not being internal flushes. At least against a\ndetermined attacker. But it may also be less random than you might\nthink, if the pattern is to send a flush, then do a bunch of work, etc.\nA \"random\" hangup or error might be more likely to happen at the flush\npoints then (from the client's perspective).\n\nSo let's see if we can answer that question with less hand-waving.\n\nThe outer v2 capabilities conversation only writes out the capabilities\nlist, followed by a single flush (the packet_flush() call in\nadvertise_capabilities()). So far so good.\n\nThe only two v2 commands defined in serve.c are \"ls-refs\" and \"fetch\".\n\nFor \"ls-refs\", we end up in ls_refs(). That writes only regular packets\nvia send_ref(), and then concludes with a single flush (we don't even\nsend a delim; the client does that to specify options). Good.\n\nFor \"fetch\", we end up in upload_pack_v2(). We write via\nprocess_haves_and_send_acks(), which will either:\n\n 1. Write nothing and return 1.\n\n 2. Write a delim and return 1.\n\n 3. Write a flush packet and return 0.\n\nIn the final case, we jump to FETCH_DONE. So this really is the final\nthing we say. Good.\n\nIn the first two cases, we jump to FETCH_SEND_PACK. We may write out\nwanted-ref info, followed by a delim. OK. Then shallow info, followed by\na delim. OK. Then the actual packfile via create_pack_file(). There our\nwrite behavior depends on use_sideband. If not set, we just dump data\ndirectly. If set, then we packetize. And it will always be set for v2\n(we do it unconditionally in the beginning of upload_pack_v2()).\n\nSo we only need to care about use_sideband cases. And there we may send\nkeepalives, but those are \"0005\\1\" empty data packets. OK. We may send\ndata via send_client_data(), but those will be real data packets. And\nthen finally we give a single packet_flush(). Good.\n\nSo I think we can say that yes, the protocol as designed will send a\nflush at the end of every response, and will not ever send another\nflush.\n\n_But_ that's if all goes well. If upload-pack sees an error, it may:\n\n - write an ERR packet in the earlier non-sideband steps, and then die\n\n - write a sideband 3 packet in later steps, and then die\n\n - just die() in some cases (e.g., pack-objects failed to start)\n\nObviously these are all an error on the client. And that third one\nespecially is probably a hanging case that we'd like to fix with this\nseries. But in the other two, we'd definitely want to make sure that\nremote-curl doesn't complain too early or too loudly, and that the error\nis shown to the user.\n\nI think if remote-curl is just asking \"did we see a flush packet at the\nend\" then it is likely to complain unnecessarily in that case. And I\ndon't think it should be inspecting packets for ERR or sideband-3. It\ndoesn't know enough about the rest of the conversation to know which is\ncorrect. Though I guess we'd really only have to inspect the final\npacket.\n\nI really wish we had converted all ERRs into packet 0002 as part of the\nv2 conversion. Then somebody reading only at the pktline level could\ntruly understand or proxy a full conversation without understanding\nanything about what's in the pktlines. But it's too late for that now.\n\nSo I think our options are probably:\n\n 1. detect flush packets in remote-curl, and either:\n\n a. don't print an error, just hang up. That prevents a hang in the\n\tcaller and produces no extra message on a real error. It may be\n\tless informative than it could be if the connection hangs up\n\t(though we may print a curl error message, and the caller will\n\tat least say \"the helper hung up\")\n\n b. like (a), but always print an error; this is your original\n\tpatch, but I _suspect_ (but didn't test) that it would produce\n\textra useless messages for errors the server reports\n\n c. between the two: inspect the final packet data for evidence of\n ERR/sideband 3 and suppress any message if found\n\n 2. helper signals end-of-response to caller (then it never produces a\n message itself; only the caller does, and it would abort on an ERR\n packet before then)\n\n a. using a special pktline (your \"0002\" patch)\n\n b. some other out-of-band mechanism (e.g., could be another fd)\n\nI think this is pushing me towards 2a, your \"0002\" patch. It sidesteps\nthe error-message questions entirely (and I think 2b is too convoluted\nto be worth pursuing, especially on Windows where setting up extra pipes\nis tricky). But I'd also be OK with 1a or 1c.\n\n-Peff\n"},{"id":"398156","messageId":"20200518210045.GA67802@coredump.intra.peff.net","threadId":"53462","inReplyTo":"20200518165206.GE42240@coredump.intra.peff.net","subject":"Re: [PATCH 6/6] remote-curl: ensure last packet is a flush","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-05-18T21:00:45Z","receivedAt":"2020-05-18T21:00:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 18, 2020 at 12:52:07PM -0400, Jeff King wrote:\n\n> > I vaguely recall talking with somebody (perhaps it was Shawn Pearce)\n> > about my long-time complaint on the on-the-wire protocol, in that\n> > the protocol sometimes uses flush to merely mean \"I've finished\n> > speaking one section of my speech\" and sometimes \"I've done talking\n> > for now and it's your turn to speak to me\" (the receiving end needs\n> > to know which one a particular flush means without knowing the\n> > context in which it is issued---which makes it impossible to write a\n> > protocol agnostic proxy. \n> > \n> > If the above proposal means that we'll have an explicit way to say\n> > \"Not just I am done with one section of my speech, but it is your\n> > turn to speak and I am going to listen\", that would be wonderful.\n> \n> I think we already have that now in v2 because of the \"0001\" delim\n> packet. All of the flushes are (I think) really \"this is the end of my\n> speech\", and any inner \"my list is ending, but I have more to say\"\n> delimiters are \"0001\".\n\nSadly, this is only _mostly_ true. See my response in:\n\n https://lore.kernel.org/git/20200518205854.GB63978@coredump.intra.peff.net/\n\n-Peff\n"},{"id":"398161","messageId":"xmqqh7wc26v9.fsf@gitster.c.googlers.com","threadId":"53462","inReplyTo":"20200518205854.GB63978@coredump.intra.peff.net","subject":"Re: [PATCH v2 0/7] remote-curl: fix deadlocks when remote server disconnects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-05-18T22:52:42Z","receivedAt":"2020-05-18T22:52:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> So I think our options are probably:\n>\n> 1. detect flush packets in remote-curl, and either:\n>\n> a. don't print an error, just hang up. That prevents a hang in the\n> \tcaller and produces no extra message on a real error. It may be\n> \tless informative than it could be if the connection hangs up\n> \t(though we may print a curl error message, and the caller will\n> \tat least say \"the helper hung up\")\n>\n> b. like (a), but always print an error; this is your original\n> \tpatch, but I _suspect_ (but didn't test) that it would produce\n> \textra useless messages for errors the server reports\n>\n> c. between the two: inspect the final packet data for evidence of\n> ERR/sideband 3 and suppress any message if found\n>\n> 2. helper signals end-of-response to caller (then it never produces a\n> message itself; only the caller does, and it would abort on an ERR\n> packet before then)\n>\n> a. using a special pktline (your \"0002\" patch)\n>\n> b. some other out-of-band mechanism (e.g., could be another fd)\n>\n> I think this is pushing me towards 2a, your \"0002\" patch. It sidesteps\n> the error-message questions entirely (and I think 2b is too convoluted\n> to be worth pursuing, especially on Windows where setting up extra pipes\n> is tricky). But I'd also be OK with 1a or 1c.\n\nThanks for a detailed analysis. I guess we'd take 0002, then?\n"},{"id":"398174","messageId":"20200519023819.GA70483@coredump.intra.peff.net","threadId":"53462","inReplyTo":"xmqqh7wc26v9.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2 0/7] remote-curl: fix deadlocks when remote server disconnects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-05-19T02:38:19Z","receivedAt":"2020-05-19T02:38:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 18, 2020 at 03:52:42PM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > So I think our options are probably:\n> >\n> > 1. detect flush packets in remote-curl, and either:\n> >\n> > a. don't print an error, just hang up. That prevents a hang in the\n> > \tcaller and produces no extra message on a real error. It may be\n> > \tless informative than it could be if the connection hangs up\n> > \t(though we may print a curl error message, and the caller will\n> > \tat least say \"the helper hung up\")\n> >\n> > b. like (a), but always print an error; this is your original\n> > \tpatch, but I _suspect_ (but didn't test) that it would produce\n> > \textra useless messages for errors the server reports\n> >\n> > c. between the two: inspect the final packet data for evidence of\n> > ERR/sideband 3 and suppress any message if found\n> >\n> > 2. helper signals end-of-response to caller (then it never produces a\n> > message itself; only the caller does, and it would abort on an ERR\n> > packet before then)\n> >\n> > a. using a special pktline (your \"0002\" patch)\n> >\n> > b. some other out-of-band mechanism (e.g., could be another fd)\n> >\n> > I think this is pushing me towards 2a, your \"0002\" patch. It sidesteps\n> > the error-message questions entirely (and I think 2b is too convoluted\n> > to be worth pursuing, especially on Windows where setting up extra pipes\n> > is tricky). But I'd also be OK with 1a or 1c.\n> \n> Thanks for a detailed analysis. I guess we'd take 0002, then?\n\nYeah, that's how I lean. I think I did have a few comments on the patch\nthat I'd like Denton to consider, so hopefully we'll see a re-roll.\n\n-Peff\n"},{"id":"398186","messageId":"cover.1589885479.git.liu.denton@gmail.com","threadId":"53462","inReplyTo":"cover.1589816718.git.liu.denton@gmail.com","subject":"[PATCH v3 0/7] remote-curl: fix deadlocks when remote server disconnects","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-05-19T10:53:53Z","receivedAt":"2020-05-19T10:54:12Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"The following command hangs forever:\n\n\t$ git -c protocol.version=2 clone https://github.com/git/git.git --shallow-since=20151012\n\tCloning into 'git'...\n\nThis occurs because the --shallow-since arg is incorrect and the server\ndies early. However, remote-curl does not realise that the server\nerrored out and just faithfully forwards the packets to fetch-pack\nbefore waiting on more input from fetch-pack. Meanwhile, fetch-pack\nkeeps reading as it still expects more input. As a result, the processes\ndeadlock. Original analysis by Peff:\nhttps://lore.kernel.org/git/20200328154936.GA1217052@coredump.intra.peff.net/\n\nChanges since v2:\n\n* Use if-else tower in \"transport: extract common fetch_pack() call\"\n\n* Rename to `lenbuf_hex` and remove null-terminator sentence in\n \"pkt-line: extern packet_length()\"\n\n* Fix \"a a\" typo in \"remote-curl: error on incomplete packet\"\n\n* Don't send flush packet after response end packet\n\n* Move stateless delimiter checks closer to where message processing\n happens in do_fetch_pack_v2()\n\nChanges since v1:\n\n* Remove fallthrough in switch in favour of just extracting the common\n call out of the switch in patch 3\n\n* Add more detail in function comment and use `const char linelen[4]` in\n patch 4\n\n* Implement most of Peff's suggestions[0] in patch 5\n\n* Only operate on stateless_connect() in patch 5\n\n* Add tests in patch 5\n\n* Drop \"remote-curl: ensure last packet is a flush\" in favour of\n \"stateless-connect: send response end packet\"\n\n[0]: https://lore.kernel.org/git/20200515213844.GD115445@coredump.intra.peff.net/\n\nDenton Liu (7):\n remote-curl: fix typo\n remote-curl: remove label indentation\n transport: extract common fetch_pack() call\n pkt-line: extern packet_length()\n remote-curl: error on incomplete packet\n pkt-line: define PACKET_READ_RESPONSE_END\n stateless-connect: send response end packet\n\n Documentation/gitremote-helpers.txt | 4 +-\n Documentation/technical/protocol-v2.txt | 2 +\n builtin/fetch-pack.c | 2 +-\n connect.c | 18 ++++-\n connect.h | 4 ++\n fetch-pack.c | 13 ++++\n pkt-line.c | 17 ++++-\n pkt-line.h | 11 +++\n remote-curl.c | 70 +++++++++++++++++--\n remote.h | 3 +-\n serve.c | 2 +\n t/helper/test-pkt-line.c | 4 ++\n t/lib-httpd.sh | 2 +\n t/lib-httpd/apache.conf | 8 +++\n .../incomplete-body-upload-pack-v2-http.sh | 3 +\n .../incomplete-length-upload-pack-v2-http.sh | 3 +\n t/t5702-protocol-v2.sh | 47 +++++++++++++\n transport.c | 28 +++-----\n 18 files changed, 211 insertions(+), 30 deletions(-)\n create mode 100644 t/lib-httpd/incomplete-body-upload-pack-v2-http.sh\n create mode 100644 t/lib-httpd/incomplete-length-upload-pack-v2-http.sh\n\nRange-diff against v2:\n1: b390875f87 = 1: b390875f87 remote-curl: fix typo\n2: a2b28c0b28 = 2: a2b28c0b28 remote-curl: remove label indentation\n3: 3a42575bd5 < -: ---------- transport: extract common fetch_pack() call\n-: ---------- > 3: c118baa5a2 transport: extract common fetch_pack() call\n4: c2b9d033bb ! 4: 36885943b2 pkt-line: extern packet_length()\n @@ Commit message\n need to access the length header. In order to simplify this, extern\n packet_length() so that the logic can be reused.\n \n - Change the function parameter from a `const char *` to\n - `const char linelen[4]`. Even though these two types behave identically\n - as function parameters, use the array notation to semantically indicate\n - exactly what this function is expecting as an argument.\n + Change the function parameter from `const char *linelen` to\n + `const char lenbuf_hex[4]`. Even though these two types behave\n + identically as function parameters, use the array notation to\n + semantically indicate exactly what this function is expecting as an\n + argument. Also, rename it from linelen to lenbuf_hex as the former\n + sounds like it should be an integral type which is misleading.\n \n ## pkt-line.c ##\n @@ pkt-line.c: static int get_packet_data(int fd, char **src_buf, size_t *src_size,\n @@ pkt-line.c: static int get_packet_data(int fd, char **src_buf, size_t *src_size,\n }\n \n -static int packet_length(const char *linelen)\n -+int packet_length(const char linelen[4])\n ++int packet_length(const char lenbuf_hex[4])\n {\n - \tint val = hex2chr(linelen);\n - \treturn (val < 0) ? val : (val << 8) | hex2chr(linelen + 2);\n +-\tint val = hex2chr(linelen);\n +-\treturn (val < 0) ? val : (val << 8) | hex2chr(linelen + 2);\n ++\tint val = hex2chr(lenbuf_hex);\n ++\treturn (val < 0) ? val : (val << 8) | hex2chr(lenbuf_hex + 2);\n + }\n + \n + enum packet_read_status packet_read_with_status(int fd, char **src_buffer,\n \n ## pkt-line.h ##\n @@ pkt-line.h: int write_packetized_from_buf(const char *src_in, size_t len, int fd_out);\n @@ pkt-line.h: int write_packetized_from_buf(const char *src_in, size_t len, int fd\n \n +/*\n + * Convert a four hex digit packet line length header into its numeric\n -+ * representation. linelen should not be null-terminated.\n ++ * representation.\n + *\n + * If linelen contains non-hex characters, return -1. Otherwise, return the\n + * numeric value of the length header.\n + */\n -+int packet_length(const char linelen[4]);\n ++int packet_length(const char lenbuf_hex[4]);\n +\n /*\n * Read a packetized line into a buffer like the 'packet_read()' function but\n5: 52ce5fdffd ! 5: 91d330620a remote-curl: error on incomplete packet\n @@ Commit message\n results in a deadlock between the two processes.\n \n For a stateless connection, inspect packets before sending them and\n - error out if a a packet line packet is incomplete.\n + error out if a packet line packet is incomplete.\n \n Helped-by: Jeff King \n \n6: 744b078324 ! 6: ff83344e9e pkt-line: PACKET_READ_RESPONSE_END\n @@ Metadata\n Author: Denton Liu \n \n ## Commit message ##\n - pkt-line: PACKET_READ_RESPONSE_END\n + pkt-line: define PACKET_READ_RESPONSE_END\n \n In a future commit, we will use PACKET_READ_RESPONSE_END to separate\n messages proxied by remote-curl. To prepare for this, add the\n7: 4b079bcd83 ! 7: c26e160fbc stateless-connect: send response end packet\n @@ Commit message\n Cloning into 'git'...\n fatal: the remote end hung up unexpectedly\n \n - Instead of blindly forwarding packets, make remote-curl insert response\n - end and flush packets after proxying the responses from the remote\n - server when using stateless_connect(). On the RPC client side, ensure\n - that each response ends as described.\n + Instead of blindly forwarding packets, make remote-curl insert a\n + response end packet after proxying the responses from the remote server\n + when using stateless_connect(). On the RPC client side, ensure that each\n + response ends as described.\n \n A separate control packet is chosen because we need to be able to\n differentiate between what the remote server sends and remote-curl's\n @@ Documentation/gitremote-helpers.txt: Supported if the helper has the \"connect\" c\n \t(both request and response) must consist of zero or more\n -\tPKT-LINEs, terminating in a flush packet. The client must not\n +\tPKT-LINEs, terminating in a flush packet. Response messages will\n -+\thave a response end packet before the flush packet to indicate\n -+\tthe end of a response. The client must not\n ++\tthen have a response end packet after the flush packet to\n ++\tindicate the end of a response. The client must not\n \texpect the server to store any state in between request-response\n \tpairs. After the connection ends, the remote helper exits.\n +\n @@ builtin/fetch-pack.c: int cmd_fetch_pack(int argc, const char **argv, const char\n \n ## connect.c ##\n @@ connect.c: static int process_ref_v2(const char *line, struct ref ***list)\n + \treturn ret;\n + }\n + \n ++void check_stateless_delimiter(int stateless_rpc,\n ++\t\t\t struct packet_reader *reader,\n ++\t\t\t const char *error)\n ++{\n ++\tif (!stateless_rpc)\n ++\t\treturn; /* not in stateless mode, no delimiter expected */\n ++\tif (packet_reader_read(reader) != PACKET_READ_RESPONSE_END)\n ++\t\tdie(\"%s\", error);\n ++}\n ++\n struct ref **get_remote_refs(int fd_out, struct packet_reader *reader,\n \t\t\t struct ref **list, int for_push,\n \t\t\t const struct argv_array *ref_prefixes,\n @@ connect.c: struct ref **get_remote_refs(int fd_out, struct packet_reader *reader\n \tif (reader->status != PACKET_READ_FLUSH)\n \t\tdie(_(\"expected flush after ref listing\"));\n \n -+\tif (stateless_rpc) {\n -+\t\tif (packet_reader_read(reader) != PACKET_READ_RESPONSE_END)\n -+\t\t\tdie(_(\"expected response end packet after ref listing\"));\n -+\t\tif (packet_reader_read(reader) != PACKET_READ_FLUSH)\n -+\t\t\tdie(_(\"expected flush packet after response end\"));\n -+\t}\n ++\tcheck_stateless_delimiter(stateless_rpc, reader,\n ++\t\t\t\t _(\"expected response end packet after ref listing\"));\n +\n \treturn list;\n }\n \n \n - ## fetch-pack.c ##\n -@@ fetch-pack.c: static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,\n - \tstruct fetch_negotiator negotiator_alloc;\n - \tstruct fetch_negotiator *negotiator;\n - \tint seen_ack = 0;\n -+\tint check_http_delimiter;\n + ## connect.h ##\n +@@ connect.h: int server_supports_v2(const char *c, int die_on_error);\n + int server_supports_feature(const char *c, const char *feature,\n + \t\t\t int die_on_error);\n \n - \tif (args->no_dependents) {\n - \t\tnegotiator = NULL;\n -@@ fetch-pack.c: static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,\n - \t}\n - \n - \twhile (state != FETCH_DONE) {\n -+\t\tcheck_http_delimiter = 0;\n ++void check_stateless_delimiter(int stateless_rpc,\n ++\t\t\t struct packet_reader *reader,\n ++\t\t\t const char *error);\n +\n - \t\tswitch (state) {\n - \t\tcase FETCH_CHECK_LOCAL:\n - \t\t\tsort_ref_list(&ref, ref_compare_name);\n + #endif\n +\n + ## fetch-pack.c ##\n +@@ fetch-pack.c: enum fetch_state {\n + \tFETCH_DONE,\n + };\n + \n ++static void do_check_stateless_delimiter(const struct fetch_pack_args *args,\n ++\t\t\t\t\t struct packet_reader *reader)\n ++{\n ++\tcheck_stateless_delimiter(args->stateless_rpc, reader,\n ++\t\t\t\t _(\"git fetch-pack: expected response end packet\"));\n ++}\n ++\n + static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,\n + \t\t\t\t int fd[2],\n + \t\t\t\t const struct ref *orig_ref,\n @@ fetch-pack.c: static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,\n + \t\t\t/* Process ACKs/NAKs */\n + \t\t\tswitch (process_acks(negotiator, &reader, &common)) {\n + \t\t\tcase READY:\n ++\t\t\t\t/*\n ++\t\t\t\t * Don't check for response delimiter; get_pack() will\n ++\t\t\t\t * read the rest of this response.\n ++\t\t\t\t */\n + \t\t\t\tstate = FETCH_GET_PACK;\n + \t\t\t\tbreak;\n + \t\t\tcase COMMON_FOUND:\n +@@ fetch-pack.c: static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,\n + \t\t\t\tseen_ack = 1;\n \t\t\t\t/* fallthrough */\n \t\t\tcase NO_COMMON_FOUND:\n ++\t\t\t\tdo_check_stateless_delimiter(args, &reader);\n \t\t\t\tstate = FETCH_SEND_REQUEST;\n -+\t\t\t\tcheck_http_delimiter = 1;\n \t\t\t\tbreak;\n \t\t\t}\n - \t\t\tbreak;\n @@ fetch-pack.c: static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,\n + \t\t\tprocess_section_header(&reader, \"packfile\", 0);\n + \t\t\tif (get_pack(args, fd, pack_lockfile, sought, nr_sought))\n \t\t\t\tdie(_(\"git fetch-pack: fetch failed.\"));\n ++\t\t\tdo_check_stateless_delimiter(args, &reader);\n \n \t\t\tstate = FETCH_DONE;\n -+\t\t\tcheck_http_delimiter = 1;\n \t\t\tbreak;\n - \t\tcase FETCH_DONE:\n - \t\t\tcontinue;\n - \t\t}\n -+\n -+\t\tif (args->stateless_rpc && check_http_delimiter) {\n -+\t\t\tif (packet_reader_read(&reader) != PACKET_READ_RESPONSE_END)\n -+\t\t\t\tdie(_(\"git fetch-pack: expected response end packet\"));\n -+\t\t\tif (packet_reader_read(&reader) != PACKET_READ_FLUSH)\n -+\t\t\t\tdie(_(\"git fetch-pack: expected flush packet\"));\n -+\t\t}\n - \t}\n - \n - \tif (negotiator)\n \n ## remote-curl.c ##\n @@ remote-curl.c: static void check_pktline(struct check_pktline_state *state, const char *ptr, si\n @@ remote-curl.c: static int post_rpc(struct rpc_state *rpc, int stateless_connect,\n \tif (rpc_in_data.pktline_state.remaining)\n \t\terr = error(_(\"%d bytes of body are still expected\"), rpc_in_data.pktline_state.remaining);\n \n -+\tif (stateless_connect) {\n ++\tif (stateless_connect)\n +\t\tpacket_response_end(rpc->in);\n -+\t\tpacket_flush(rpc->in);\n -+\t}\n +\n \tcurl_slist_free_all(headers);\n \tfree(gzip_body);\n-- \n2.26.2.706.g87896c9627\n\n"},{"id":"398187","messageId":"b390875f87dbf5f3f48909ce40f5a5749b32f5d4.1589885479.git.liu.denton@gmail.com","threadId":"53462","inReplyTo":"cover.1589885479.git.liu.denton@gmail.com","subject":"[PATCH v3 1/7] remote-curl: fix typo","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-05-19T10:53:54Z","receivedAt":"2020-05-19T10:54:16Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"Signed-off-by: Denton Liu \n---\n remote-curl.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/remote-curl.c b/remote-curl.c\nindex 1c9aa3d0ab..6844708f38 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -643,7 +643,7 @@ static size_t rpc_out(void *ptr, size_t eltsize,\n \t\t\treturn 0;\n \t\t}\n \t\t/*\n-\t\t * If avail is non-zerp, the line length for the flush still\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-- \n2.26.2.706.g87896c9627\n\n"},{"id":"398188","messageId":"a2b28c0b28bb972f164ff0a9e48ae6b623a2a125.1589885479.git.liu.denton@gmail.com","threadId":"53462","inReplyTo":"cover.1589885479.git.liu.denton@gmail.com","subject":"[PATCH v3 2/7] remote-curl: remove label indentation","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-05-19T10:53:55Z","receivedAt":"2020-05-19T10:54:16Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"In the codebase, labels are aligned to the leftmost column. Remove the\nspace-indentation from `free_specs:` to conform to this.\n\nSigned-off-by: Denton Liu \n---\n remote-curl.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/remote-curl.c b/remote-curl.c\nindex 6844708f38..da3e07184a 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -1276,7 +1276,7 @@ static void parse_push(struct strbuf *buf)\n \tif (ret)\n \t\texit(128); /* error already reported */\n \n- free_specs:\n+free_specs:\n \targv_array_clear(&specs);\n }\n \n-- \n2.26.2.706.g87896c9627\n\n"},{"id":"398189","messageId":"c118baa5a2e14e16b1f565e485fedcc7806dc505.1589885479.git.liu.denton@gmail.com","threadId":"53462","inReplyTo":"cover.1589885479.git.liu.denton@gmail.com","subject":"[PATCH v3 3/7] transport: extract common fetch_pack() call","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-05-19T10:53:56Z","receivedAt":"2020-05-19T10:54:18Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"In the switch statement, the difference between the `protocol_v2` and\n`protocol_v{1,0}` arms is a preparatory call to die_if_server_options() in\nthe latter. The fetch_pack() call is identical in both arms. However,\nsince this fetch_pack() call has so many parameters, it is not\nimmediately obvious that the call is identical in both cases.\n\nExtract the common fetch_pack() call out of the switch statement so that\ncode duplication is reduced and the logic is more clear for future\nreaders. While we're at it, rewrite the switch statement as an if-else\ntower for increased clarity.\n\nSigned-off-by: Denton Liu \n---\n transport.c | 25 ++++++++-----------------\n 1 file changed, 8 insertions(+), 17 deletions(-)\n\ndiff --git a/transport.c b/transport.c\nindex 15f5ba4e8f..431a93caef 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -369,24 +369,15 @@ static int fetch_refs_via_pack(struct transport *transport,\n \t\trefs_tmp = handshake(transport, 0, NULL, must_list_refs);\n \t}\n \n-\tswitch (data->version) {\n-\tcase protocol_v2:\n-\t\trefs = fetch_pack(&args, data->fd,\n-\t\t\t\t refs_tmp ? refs_tmp : transport->remote_refs,\n-\t\t\t\t to_fetch, nr_heads, &data->shallow,\n-\t\t\t\t &transport->pack_lockfile, data->version);\n-\t\tbreak;\n-\tcase protocol_v1:\n-\tcase protocol_v0:\n-\t\tdie_if_server_options(transport);\n-\t\trefs = fetch_pack(&args, data->fd,\n-\t\t\t\t refs_tmp ? refs_tmp : transport->remote_refs,\n-\t\t\t\t to_fetch, nr_heads, &data->shallow,\n-\t\t\t\t &transport->pack_lockfile, data->version);\n-\t\tbreak;\n-\tcase protocol_unknown_version:\n+\tif (data->version == protocol_unknown_version)\n \t\tBUG(\"unknown protocol version\");\n-\t}\n+\telse if (data->version <= protocol_v1)\n+\t\tdie_if_server_options(transport);\n+\n+\trefs = fetch_pack(&args, data->fd,\n+\t\t\t refs_tmp ? refs_tmp : transport->remote_refs,\n+\t\t\t to_fetch, nr_heads, &data->shallow,\n+\t\t\t &transport->pack_lockfile, data->version);\n \n \tclose(data->fd[0]);\n \tclose(data->fd[1]);\n-- \n2.26.2.706.g87896c9627\n\n"},{"id":"398190","messageId":"36885943b239807eda49e231d8a45e1991f7288e.1589885479.git.liu.denton@gmail.com","threadId":"53462","inReplyTo":"cover.1589885479.git.liu.denton@gmail.com","subject":"[PATCH v3 4/7] pkt-line: extern packet_length()","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-05-19T10:53:57Z","receivedAt":"2020-05-19T10:54:19Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"In a future commit, we will be manually processing packets and we will\nneed to access the length header. In order to simplify this, extern\npacket_length() so that the logic can be reused.\n\nChange the function parameter from `const char *linelen` to\n`const char lenbuf_hex[4]`. Even though these two types behave\nidentically as function parameters, use the array notation to\nsemantically indicate exactly what this function is expecting as an\nargument. Also, rename it from linelen to lenbuf_hex as the former\nsounds like it should be an integral type which is misleading.\n\nSigned-off-by: Denton Liu \n---\n pkt-line.c | 6 +++---\n pkt-line.h | 9 +++++++++\n 2 files changed, 12 insertions(+), 3 deletions(-)\n\ndiff --git a/pkt-line.c b/pkt-line.c\nindex a0e87b1e81..3beab1dc6b 100644\n--- a/pkt-line.c\n+++ b/pkt-line.c\n@@ -306,10 +306,10 @@ static int get_packet_data(int fd, char **src_buf, size_t *src_size,\n \treturn ret;\n }\n \n-static int packet_length(const char *linelen)\n+int packet_length(const char lenbuf_hex[4])\n {\n-\tint val = hex2chr(linelen);\n-\treturn (val < 0) ? val : (val << 8) | hex2chr(linelen + 2);\n+\tint val = hex2chr(lenbuf_hex);\n+\treturn (val < 0) ? val : (val << 8) | hex2chr(lenbuf_hex + 2);\n }\n \n enum packet_read_status packet_read_with_status(int fd, char **src_buffer,\ndiff --git a/pkt-line.h b/pkt-line.h\nindex fef3a0d792..0d92c5e17f 100644\n--- a/pkt-line.h\n+++ b/pkt-line.h\n@@ -74,6 +74,15 @@ int write_packetized_from_buf(const char *src_in, size_t len, int fd_out);\n int packet_read(int fd, char **src_buffer, size_t *src_len, char\n \t\t*buffer, unsigned size, int options);\n \n+/*\n+ * Convert a four hex digit packet line length header into its numeric\n+ * representation.\n+ *\n+ * If linelen contains non-hex characters, return -1. Otherwise, return the\n+ * numeric value of the length header.\n+ */\n+int packet_length(const char lenbuf_hex[4]);\n+\n /*\n * Read a packetized line into a buffer like the 'packet_read()' function but\n * returns an 'enum packet_read_status' which indicates the status of the read.\n-- \n2.26.2.706.g87896c9627\n\n"},{"id":"398191","messageId":"ff83344e9e8e771ee463e4cb056cfc47b4c661b5.1589885479.git.liu.denton@gmail.com","threadId":"53462","inReplyTo":"cover.1589885479.git.liu.denton@gmail.com","subject":"[PATCH v3 6/7] pkt-line: define PACKET_READ_RESPONSE_END","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-05-19T10:53:59Z","receivedAt":"2020-05-19T10:54:21Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"In a future commit, we will use PACKET_READ_RESPONSE_END to separate\nmessages proxied by remote-curl. To prepare for this, add the\nPACKET_READ_RESPONSE_END enum value.\n\nIn switch statements that need a case added, die() or BUG() when a\nPACKET_READ_RESPONSE_END is unexpected. Otherwise, mirror how\nPACKET_READ_DELIM is implemented (especially in cases where packets are\nbeing forwarded).\n\nSigned-off-by: Denton Liu \n---\n connect.c | 2 ++\n pkt-line.c | 11 +++++++++++\n pkt-line.h | 2 ++\n remote-curl.c | 2 ++\n serve.c | 2 ++\n t/helper/test-pkt-line.c | 4 ++++\n 6 files changed, 23 insertions(+)\n\ndiff --git a/connect.c b/connect.c\nindex 23013c6344..11c6ec70a0 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -127,6 +127,7 @@ enum protocol_version discover_version(struct packet_reader *reader)\n \t\tdie_initial_contact(0);\n \tcase PACKET_READ_FLUSH:\n \tcase PACKET_READ_DELIM:\n+\tcase PACKET_READ_RESPONSE_END:\n \t\tversion = protocol_v0;\n \t\tbreak;\n \tcase PACKET_READ_NORMAL:\n@@ -310,6 +311,7 @@ struct ref **get_remote_heads(struct packet_reader *reader,\n \t\t\tstate = EXPECTING_DONE;\n \t\t\tbreak;\n \t\tcase PACKET_READ_DELIM:\n+\t\tcase PACKET_READ_RESPONSE_END:\n \t\t\tdie(_(\"invalid packet\"));\n \t\t}\n \ndiff --git a/pkt-line.c b/pkt-line.c\nindex 3beab1dc6b..8f9bc68ee2 100644\n--- a/pkt-line.c\n+++ b/pkt-line.c\n@@ -99,6 +99,13 @@ void packet_delim(int fd)\n \t\tdie_errno(_(\"unable to write delim packet\"));\n }\n \n+void packet_response_end(int fd)\n+{\n+\tpacket_trace(\"0002\", 4, 1);\n+\tif (write_in_full(fd, \"0002\", 4) < 0)\n+\t\tdie_errno(_(\"unable to write stateless separator packet\"));\n+}\n+\n int packet_flush_gently(int fd)\n {\n \tpacket_trace(\"0000\", 4, 1);\n@@ -337,6 +344,10 @@ enum packet_read_status packet_read_with_status(int fd, char **src_buffer,\n \t\tpacket_trace(\"0001\", 4, 0);\n \t\t*pktlen = 0;\n \t\treturn PACKET_READ_DELIM;\n+\t} else if (len == 2) {\n+\t\tpacket_trace(\"0002\", 4, 0);\n+\t\t*pktlen = 0;\n+\t\treturn PACKET_READ_RESPONSE_END;\n \t} else if (len < 4) {\n \t\tdie(_(\"protocol error: bad line length %d\"), len);\n \t}\ndiff --git a/pkt-line.h b/pkt-line.h\nindex 0d92c5e17f..6cb92d7a5d 100644\n--- a/pkt-line.h\n+++ b/pkt-line.h\n@@ -22,6 +22,7 @@\n */\n void packet_flush(int fd);\n void packet_delim(int fd);\n+void packet_response_end(int fd);\n void packet_write_fmt(int fd, const char *fmt, ...) __attribute__((format (printf, 2, 3)));\n void packet_buf_flush(struct strbuf *buf);\n void packet_buf_delim(struct strbuf *buf);\n@@ -94,6 +95,7 @@ enum packet_read_status {\n \tPACKET_READ_NORMAL,\n \tPACKET_READ_FLUSH,\n \tPACKET_READ_DELIM,\n+\tPACKET_READ_RESPONSE_END,\n };\n enum packet_read_status packet_read_with_status(int fd, char **src_buffer,\n \t\t\t\t\t\tsize_t *src_len, char *buffer,\ndiff --git a/remote-curl.c b/remote-curl.c\nindex e020140092..d02cb547e9 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -601,6 +601,8 @@ static int rpc_read_from_out(struct rpc_state *rpc, int options,\n \t\tcase PACKET_READ_FLUSH:\n \t\t\tmemcpy(buf - 4, \"0000\", 4);\n \t\t\tbreak;\n+\t\tcase PACKET_READ_RESPONSE_END:\n+\t\t\tdie(_(\"remote server sent stateless separator\"));\n \t\t}\n \t}\n \ndiff --git a/serve.c b/serve.c\nindex 317256c1a4..c046926ba1 100644\n--- a/serve.c\n+++ b/serve.c\n@@ -217,6 +217,8 @@ static int process_request(void)\n \n \t\t\tstate = PROCESS_REQUEST_DONE;\n \t\t\tbreak;\n+\t\tcase PACKET_READ_RESPONSE_END:\n+\t\t\tBUG(\"unexpected stateless separator packet\");\n \t\t}\n \t}\n \ndiff --git a/t/helper/test-pkt-line.c b/t/helper/test-pkt-line.c\nindex 12ca698e17..69152958e5 100644\n--- a/t/helper/test-pkt-line.c\n+++ b/t/helper/test-pkt-line.c\n@@ -46,6 +46,9 @@ static void unpack(void)\n \t\tcase PACKET_READ_DELIM:\n \t\t\tprintf(\"0001\\n\");\n \t\t\tbreak;\n+\t\tcase PACKET_READ_RESPONSE_END:\n+\t\t\tprintf(\"0002\\n\");\n+\t\t\tbreak;\n \t\t}\n \t}\n }\n@@ -75,6 +78,7 @@ static void unpack_sideband(void)\n \t\tcase PACKET_READ_FLUSH:\n \t\t\treturn;\n \t\tcase PACKET_READ_DELIM:\n+\t\tcase PACKET_READ_RESPONSE_END:\n \t\t\tbreak;\n \t\t}\n \t}\n-- \n2.26.2.706.g87896c9627\n\n"},{"id":"398192","messageId":"91d330620a18e286ec112747ea2f3a9d39066018.1589885479.git.liu.denton@gmail.com","threadId":"53462","inReplyTo":"cover.1589885479.git.liu.denton@gmail.com","subject":"[PATCH v3 5/7] remote-curl: error on incomplete packet","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-05-19T10:53:58Z","receivedAt":"2020-05-19T10:54:22Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"Currently, remote-curl acts as a proxy and blindly forwards packets\nbetween an HTTP server and fetch-pack. In the case of a stateless RPC\nconnection where the connection is terminated with a partially written\npacket, remote-curl will blindly send the partially written packet\nbefore waiting on more input from fetch-pack. Meanwhile, fetch-pack will\nread the partial packet and continue reading, expecting more input. This\nresults in a deadlock between the two processes.\n\nFor a stateless connection, inspect packets before sending them and\nerror out if a packet line packet is incomplete.\n\nHelped-by: Jeff King \nSigned-off-by: Denton Liu \n---\n remote-curl.c | 59 ++++++++++++++++++-\n t/lib-httpd.sh | 2 +\n t/lib-httpd/apache.conf | 8 +++\n .../incomplete-body-upload-pack-v2-http.sh | 3 +\n .../incomplete-length-upload-pack-v2-http.sh | 3 +\n t/t5702-protocol-v2.sh | 34 +++++++++++\n 6 files changed, 106 insertions(+), 3 deletions(-)\n create mode 100644 t/lib-httpd/incomplete-body-upload-pack-v2-http.sh\n create mode 100644 t/lib-httpd/incomplete-length-upload-pack-v2-http.sh\n\ndiff --git a/remote-curl.c b/remote-curl.c\nindex da3e07184a..e020140092 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -679,9 +679,53 @@ static curlioerr rpc_ioctl(CURL *handle, int cmd, void *clientp)\n }\n #endif\n \n+struct check_pktline_state {\n+\tchar len_buf[4];\n+\tint len_filled;\n+\tint remaining;\n+};\n+\n+static void check_pktline(struct check_pktline_state *state, const char *ptr, size_t size)\n+{\n+\twhile (size) {\n+\t\tif (!state->remaining) {\n+\t\t\tint digits_remaining = 4 - state->len_filled;\n+\t\t\tif (digits_remaining > size)\n+\t\t\t\tdigits_remaining = size;\n+\t\t\tmemcpy(&state->len_buf[state->len_filled], ptr, digits_remaining);\n+\t\t\tstate->len_filled += digits_remaining;\n+\t\t\tptr += digits_remaining;\n+\t\t\tsize -= digits_remaining;\n+\n+\t\t\tif (state->len_filled == 4) {\n+\t\t\t\tstate->remaining = packet_length(state->len_buf);\n+\t\t\t\tif (state->remaining < 0) {\n+\t\t\t\t\tdie(_(\"remote-curl: bad line length character: %.4s\"), state->len_buf);\n+\t\t\t\t} else if (state->remaining < 4) {\n+\t\t\t\t\tstate->remaining = 0;\n+\t\t\t\t} else {\n+\t\t\t\t\tstate->remaining -= 4;\n+\t\t\t\t}\n+\t\t\t\tstate->len_filled = 0;\n+\t\t\t}\n+\t\t}\n+\n+\t\tif (state->remaining) {\n+\t\t\tint remaining = state->remaining;\n+\t\t\tif (remaining > size)\n+\t\t\t\tremaining = size;\n+\t\t\tptr += remaining;\n+\t\t\tsize -= remaining;\n+\t\t\tstate->remaining -= remaining;\n+\t\t}\n+\t}\n+}\n+\n struct rpc_in_data {\n \tstruct rpc_state *rpc;\n \tstruct active_request_slot *slot;\n+\tint check_pktline;\n+\tstruct check_pktline_state pktline_state;\n };\n \n /*\n@@ -702,6 +746,8 @@ static size_t rpc_in(char *ptr, size_t eltsize,\n \t\treturn size;\n \tif (size)\n \t\tdata->rpc->any_written = 1;\n+\tif (data->check_pktline)\n+\t\tcheck_pktline(&data->pktline_state, ptr, size);\n \twrite_or_die(data->rpc->in, ptr, size);\n \treturn size;\n }\n@@ -778,7 +824,7 @@ static curl_off_t xcurl_off_t(size_t len)\n * If flush_received is true, do not attempt to read any more; just use what's\n * in rpc->buf.\n */\n-static int post_rpc(struct rpc_state *rpc, int flush_received)\n+static int post_rpc(struct rpc_state *rpc, int stateless_connect, int flush_received)\n {\n \tstruct active_request_slot *slot;\n \tstruct curl_slist *headers = http_copy_default_headers();\n@@ -920,6 +966,8 @@ static int post_rpc(struct rpc_state *rpc, int flush_received)\n \tcurl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, rpc_in);\n \trpc_in_data.rpc = rpc;\n \trpc_in_data.slot = slot;\n+\trpc_in_data.check_pktline = stateless_connect;\n+\tmemset(&rpc_in_data.pktline_state, 0, sizeof(rpc_in_data.pktline_state));\n \tcurl_easy_setopt(slot->curl, CURLOPT_FILE, &rpc_in_data);\n \tcurl_easy_setopt(slot->curl, CURLOPT_FAILONERROR, 0);\n \n@@ -936,6 +984,11 @@ static int post_rpc(struct rpc_state *rpc, int flush_received)\n \tif (!rpc->any_written)\n \t\terr = -1;\n \n+\tif (rpc_in_data.pktline_state.len_filled)\n+\t\terr = error(_(\"%d bytes of length header were received\"), rpc_in_data.pktline_state.len_filled);\n+\tif (rpc_in_data.pktline_state.remaining)\n+\t\terr = error(_(\"%d bytes of body are still expected\"), rpc_in_data.pktline_state.remaining);\n+\n \tcurl_slist_free_all(headers);\n \tfree(gzip_body);\n \treturn err;\n@@ -985,7 +1038,7 @@ static int rpc_service(struct rpc_state *rpc, struct discovery *heads,\n \t\t\tbreak;\n \t\trpc->pos = 0;\n \t\trpc->len = n;\n-\t\terr |= post_rpc(rpc, 0);\n+\t\terr |= post_rpc(rpc, 0, 0);\n \t}\n \n \tclose(client.in);\n@@ -1342,7 +1395,7 @@ static int stateless_connect(const char *service_name)\n \t\t\tBUG(\"The entire rpc->buf should be larger than LARGE_PACKET_MAX\");\n \t\tif (status == PACKET_READ_EOF)\n \t\t\tbreak;\n-\t\tif (post_rpc(&rpc, status == PACKET_READ_FLUSH))\n+\t\tif (post_rpc(&rpc, 1, status == PACKET_READ_FLUSH))\n \t\t\t/* We would have an err here */\n \t\t\tbreak;\n \t\t/* Reset the buffer for next request */\ndiff --git a/t/lib-httpd.sh b/t/lib-httpd.sh\nindex 1449ee95e9..d2edfa4c50 100644\n--- a/t/lib-httpd.sh\n+++ b/t/lib-httpd.sh\n@@ -129,6 +129,8 @@ install_script () {\n prepare_httpd() {\n \tmkdir -p \"$HTTPD_DOCUMENT_ROOT_PATH\"\n \tcp \"$TEST_PATH\"/passwd \"$HTTPD_ROOT_PATH\"\n+\tinstall_script incomplete-length-upload-pack-v2-http.sh\n+\tinstall_script incomplete-body-upload-pack-v2-http.sh\n \tinstall_script broken-smart-http.sh\n \tinstall_script error-smart-http.sh\n \tinstall_script error.sh\ndiff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf\nindex 994e5290d6..afa91e38b0 100644\n--- a/t/lib-httpd/apache.conf\n+++ b/t/lib-httpd/apache.conf\n@@ -117,6 +117,8 @@ Alias /auth/dumb/ www/auth/dumb/\n \tSetEnv GIT_EXEC_PATH ${GIT_EXEC_PATH}\n \tSetEnv GIT_HTTP_EXPORT_ALL\n \n+ScriptAlias /smart/incomplete_length/git-upload-pack incomplete-length-upload-pack-v2-http.sh/\n+ScriptAlias /smart/incomplete_body/git-upload-pack incomplete-body-upload-pack-v2-http.sh/\n ScriptAliasMatch /error_git_upload_pack/(.*)/git-upload-pack error.sh/\n ScriptAliasMatch /smart_*[^/]*/(.*) ${GIT_EXEC_PATH}/git-http-backend/$1\n ScriptAlias /broken_smart/ broken-smart-http.sh/\n@@ -126,6 +128,12 @@ ScriptAliasMatch /one_time_perl/(.*) apply-one-time-perl.sh/$1\n \n \tOptions FollowSymlinks\n \n+\n+\tOptions ExecCGI\n+\n+\n+\tOptions ExecCGI\n+\n \n \tOptions ExecCGI\n \ndiff --git a/t/lib-httpd/incomplete-body-upload-pack-v2-http.sh b/t/lib-httpd/incomplete-body-upload-pack-v2-http.sh\nnew file mode 100644\nindex 0000000000..2f5ed9fcf6\n--- /dev/null\n+++ b/t/lib-httpd/incomplete-body-upload-pack-v2-http.sh\n@@ -0,0 +1,3 @@\n+printf \"Content-Type: text/%s\\n\" \"application/x-git-upload-pack-result\"\n+echo\n+printf \"%s%s\\n\" \"0079\" \"45\"\ndiff --git a/t/lib-httpd/incomplete-length-upload-pack-v2-http.sh b/t/lib-httpd/incomplete-length-upload-pack-v2-http.sh\nnew file mode 100644\nindex 0000000000..86c6e648c9\n--- /dev/null\n+++ b/t/lib-httpd/incomplete-length-upload-pack-v2-http.sh\n@@ -0,0 +1,3 @@\n+printf \"Content-Type: text/%s\\n\" \"application/x-git-upload-pack-result\"\n+echo\n+printf \"%s\\n\" \"00\"\ndiff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh\nindex 5039e66dc4..4eb81ba2d4 100755\n--- a/t/t5702-protocol-v2.sh\n+++ b/t/t5702-protocol-v2.sh\n@@ -586,6 +586,40 @@ test_expect_success 'clone with http:// using protocol v2' '\n \t! grep \"Send header: Transfer-Encoding: chunked\" log\n '\n \n+test_expect_success 'clone repository with http:// using protocol v2 with incomplete pktline length' '\n+\ttest_when_finished \"rm -f log\" &&\n+\n+\tgit init \"$HTTPD_DOCUMENT_ROOT_PATH/incomplete_length\" &&\n+\ttest_commit -C \"$HTTPD_DOCUMENT_ROOT_PATH/incomplete_length\" file &&\n+\n+\ttest_must_fail env GIT_TRACE_PACKET=\"$(pwd)/log\" GIT_TRACE_CURL=\"$(pwd)/log\" git -c protocol.version=2 \\\n+\t\tclone \"$HTTPD_URL/smart/incomplete_length\" incomplete_length_child 2>err &&\n+\n+\t# Client requested to use protocol v2\n+\tgrep \"Git-Protocol: version=2\" log &&\n+\t# Server responded using protocol v2\n+\tgrep \"git< version 2\" log &&\n+\t# Client reported appropriate failure\n+\ttest_i18ngrep \"bytes of length header were received\" err\n+'\n+\n+test_expect_success 'clone repository with http:// using protocol v2 with incomplete pktline body' '\n+\ttest_when_finished \"rm -f log\" &&\n+\n+\tgit init \"$HTTPD_DOCUMENT_ROOT_PATH/incomplete_body\" &&\n+\ttest_commit -C \"$HTTPD_DOCUMENT_ROOT_PATH/incomplete_body\" file &&\n+\n+\ttest_must_fail env GIT_TRACE_PACKET=\"$(pwd)/log\" GIT_TRACE_CURL=\"$(pwd)/log\" git -c protocol.version=2 \\\n+\t\tclone \"$HTTPD_URL/smart/incomplete_body\" incomplete_body_child 2>err &&\n+\n+\t# Client requested to use protocol v2\n+\tgrep \"Git-Protocol: version=2\" log &&\n+\t# Server responded using protocol v2\n+\tgrep \"git< version 2\" log &&\n+\t# Client reported appropriate failure\n+\ttest_i18ngrep \"bytes of body are still expected\" err\n+'\n+\n test_expect_success 'clone big repository with http:// using protocol v2' '\n \ttest_when_finished \"rm -f log\" &&\n \n-- \n2.26.2.706.g87896c9627\n\n"},{"id":"398193","messageId":"c26e160fbc211f1fbb4e231a1c4a4b84fa347cf6.1589885479.git.liu.denton@gmail.com","threadId":"53462","inReplyTo":"cover.1589885479.git.liu.denton@gmail.com","subject":"[PATCH v3 7/7] stateless-connect: send response end packet","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-05-19T10:54:00Z","receivedAt":"2020-05-19T10:54:23Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"Currently, remote-curl acts as a proxy and blindly forwards packets\nbetween an HTTP server and fetch-pack. In the case of a stateless RPC\nconnection where the connection is terminated before the transaction is\ncomplete, remote-curl will blindly forward the packets before waiting on\nmore input from fetch-pack. Meanwhile, fetch-pack will read the\ntransaction and continue reading, expecting more input to continue the\ntransaction. This results in a deadlock between the two processes.\n\nThis can be seen in the following command which does not terminate:\n\n\t$ git -c protocol.version=2 clone https://github.com/git/git.git --shallow-since=20151012\n\tCloning into 'git'...\n\nwhereas the v1 version does terminate as expected:\n\n\t$ git -c protocol.version=1 clone https://github.com/git/git.git --shallow-since=20151012\n\tCloning into 'git'...\n\tfatal: the remote end hung up unexpectedly\n\nInstead of blindly forwarding packets, make remote-curl insert a\nresponse end packet after proxying the responses from the remote server\nwhen using stateless_connect(). On the RPC client side, ensure that each\nresponse ends as described.\n\nA separate control packet is chosen because we need to be able to\ndifferentiate between what the remote server sends and remote-curl's\ncontrol packets. By ensuring in the remote-curl code that a server\ncannot send response end packets, we prevent a malicious server from\nbeing able to perform a denial of service attack in which they spoof a\nresponse end packet and cause the described deadlock to happen.\n\nReported-by: Force Charlie \nHelped-by: Jeff King \nSigned-off-by: Denton Liu \n---\n Documentation/gitremote-helpers.txt | 4 +++-\n Documentation/technical/protocol-v2.txt | 2 ++\n builtin/fetch-pack.c | 2 +-\n connect.c | 16 +++++++++++++++-\n connect.h | 4 ++++\n fetch-pack.c | 13 +++++++++++++\n remote-curl.c | 5 +++++\n remote.h | 3 ++-\n t/t5702-protocol-v2.sh | 13 +++++++++++++\n transport.c | 3 ++-\n 10 files changed, 60 insertions(+), 5 deletions(-)\n\ndiff --git a/Documentation/gitremote-helpers.txt b/Documentation/gitremote-helpers.txt\nindex f48a031dc3..93baeeb029 100644\n--- a/Documentation/gitremote-helpers.txt\n+++ b/Documentation/gitremote-helpers.txt\n@@ -405,7 +405,9 @@ Supported if the helper has the \"connect\" capability.\n \ttrying to fall back). After line feed terminating the positive\n \t(empty) response, the output of the service starts. Messages\n \t(both request and response) must consist of zero or more\n-\tPKT-LINEs, terminating in a flush packet. The client must not\n+\tPKT-LINEs, terminating in a flush packet. Response messages will\n+\tthen have a response end packet after the flush packet to\n+\tindicate the end of a response. The client must not\n \texpect the server to store any state in between request-response\n \tpairs. After the connection ends, the remote helper exits.\n +\ndiff --git a/Documentation/technical/protocol-v2.txt b/Documentation/technical/protocol-v2.txt\nindex 7e3766cafb..3996d70891 100644\n--- a/Documentation/technical/protocol-v2.txt\n+++ b/Documentation/technical/protocol-v2.txt\n@@ -33,6 +33,8 @@ In protocol v2 these special packets will have the following semantics:\n \n * '0000' Flush Packet (flush-pkt) - indicates the end of a message\n * '0001' Delimiter Packet (delim-pkt) - separates sections of a message\n+ * '0002' Message Packet (response-end-pkt) - indicates the end of a response\n+ for stateless connections\n \n Initial Client Request\n ----------------------\ndiff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c\nindex 4771100072..94b0c89b82 100644\n--- a/builtin/fetch-pack.c\n+++ b/builtin/fetch-pack.c\n@@ -224,7 +224,7 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)\n \tversion = discover_version(&reader);\n \tswitch (version) {\n \tcase protocol_v2:\n-\t\tget_remote_refs(fd[1], &reader, &ref, 0, NULL, NULL);\n+\t\tget_remote_refs(fd[1], &reader, &ref, 0, NULL, NULL, args.stateless_rpc);\n \t\tbreak;\n \tcase protocol_v1:\n \tcase protocol_v0:\ndiff --git a/connect.c b/connect.c\nindex 11c6ec70a0..0df45a1108 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -406,10 +406,21 @@ static int process_ref_v2(const char *line, struct ref ***list)\n \treturn ret;\n }\n \n+void check_stateless_delimiter(int stateless_rpc,\n+\t\t\t struct packet_reader *reader,\n+\t\t\t const char *error)\n+{\n+\tif (!stateless_rpc)\n+\t\treturn; /* not in stateless mode, no delimiter expected */\n+\tif (packet_reader_read(reader) != PACKET_READ_RESPONSE_END)\n+\t\tdie(\"%s\", error);\n+}\n+\n struct ref **get_remote_refs(int fd_out, struct packet_reader *reader,\n \t\t\t struct ref **list, int for_push,\n \t\t\t const struct argv_array *ref_prefixes,\n-\t\t\t const struct string_list *server_options)\n+\t\t\t const struct string_list *server_options,\n+\t\t\t int stateless_rpc)\n {\n \tint i;\n \t*list = NULL;\n@@ -446,6 +457,9 @@ struct ref **get_remote_refs(int fd_out, struct packet_reader *reader,\n \tif (reader->status != PACKET_READ_FLUSH)\n \t\tdie(_(\"expected flush after ref listing\"));\n \n+\tcheck_stateless_delimiter(stateless_rpc, reader,\n+\t\t\t\t _(\"expected response end packet after ref listing\"));\n+\n \treturn list;\n }\n \ndiff --git a/connect.h b/connect.h\nindex 5f2382e018..235bc66254 100644\n--- a/connect.h\n+++ b/connect.h\n@@ -22,4 +22,8 @@ int server_supports_v2(const char *c, int die_on_error);\n int server_supports_feature(const char *c, const char *feature,\n \t\t\t int die_on_error);\n \n+void check_stateless_delimiter(int stateless_rpc,\n+\t\t\t struct packet_reader *reader,\n+\t\t\t const char *error);\n+\n #endif\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex f73a2ce6cb..f096442d4d 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -1450,6 +1450,13 @@ enum fetch_state {\n \tFETCH_DONE,\n };\n \n+static void do_check_stateless_delimiter(const struct fetch_pack_args *args,\n+\t\t\t\t\t struct packet_reader *reader)\n+{\n+\tcheck_stateless_delimiter(args->stateless_rpc, reader,\n+\t\t\t\t _(\"git fetch-pack: expected response end packet\"));\n+}\n+\n static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,\n \t\t\t\t int fd[2],\n \t\t\t\t const struct ref *orig_ref,\n@@ -1534,6 +1541,10 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,\n \t\t\t/* Process ACKs/NAKs */\n \t\t\tswitch (process_acks(negotiator, &reader, &common)) {\n \t\t\tcase READY:\n+\t\t\t\t/*\n+\t\t\t\t * Don't check for response delimiter; get_pack() will\n+\t\t\t\t * read the rest of this response.\n+\t\t\t\t */\n \t\t\t\tstate = FETCH_GET_PACK;\n \t\t\t\tbreak;\n \t\t\tcase COMMON_FOUND:\n@@ -1541,6 +1552,7 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,\n \t\t\t\tseen_ack = 1;\n \t\t\t\t/* fallthrough */\n \t\t\tcase NO_COMMON_FOUND:\n+\t\t\t\tdo_check_stateless_delimiter(args, &reader);\n \t\t\t\tstate = FETCH_SEND_REQUEST;\n \t\t\t\tbreak;\n \t\t\t}\n@@ -1560,6 +1572,7 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,\n \t\t\tprocess_section_header(&reader, \"packfile\", 0);\n \t\t\tif (get_pack(args, fd, pack_lockfile, sought, nr_sought))\n \t\t\t\tdie(_(\"git fetch-pack: fetch failed.\"));\n+\t\t\tdo_check_stateless_delimiter(args, &reader);\n \n \t\t\tstate = FETCH_DONE;\n \t\t\tbreak;\ndiff --git a/remote-curl.c b/remote-curl.c\nindex d02cb547e9..75532a8bae 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -703,6 +703,8 @@ static void check_pktline(struct check_pktline_state *state, const char *ptr, si\n \t\t\t\tstate->remaining = packet_length(state->len_buf);\n \t\t\t\tif (state->remaining < 0) {\n \t\t\t\t\tdie(_(\"remote-curl: bad line length character: %.4s\"), state->len_buf);\n+\t\t\t\t} else if (state->remaining == 2) {\n+\t\t\t\t\tdie(_(\"remote-curl: unexpected response end packet\"));\n \t\t\t\t} else if (state->remaining < 4) {\n \t\t\t\t\tstate->remaining = 0;\n \t\t\t\t} else {\n@@ -991,6 +993,9 @@ static int post_rpc(struct rpc_state *rpc, int stateless_connect, int flush_rece\n \tif (rpc_in_data.pktline_state.remaining)\n \t\terr = error(_(\"%d bytes of body are still expected\"), rpc_in_data.pktline_state.remaining);\n \n+\tif (stateless_connect)\n+\t\tpacket_response_end(rpc->in);\n+\n \tcurl_slist_free_all(headers);\n \tfree(gzip_body);\n \treturn err;\ndiff --git a/remote.h b/remote.h\nindex 11d8719b58..5cc26c1b3b 100644\n--- a/remote.h\n+++ b/remote.h\n@@ -179,7 +179,8 @@ struct ref **get_remote_heads(struct packet_reader *reader,\n struct ref **get_remote_refs(int fd_out, struct packet_reader *reader,\n \t\t\t struct ref **list, int for_push,\n \t\t\t const struct argv_array *ref_prefixes,\n-\t\t\t const struct string_list *server_options);\n+\t\t\t const struct string_list *server_options,\n+\t\t\t int stateless_rpc);\n \n int resolve_remote_symref(struct ref *ref, struct ref *list);\n \ndiff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh\nindex 4eb81ba2d4..8da65e60de 100755\n--- a/t/t5702-protocol-v2.sh\n+++ b/t/t5702-protocol-v2.sh\n@@ -620,6 +620,19 @@ test_expect_success 'clone repository with http:// using protocol v2 with incomp\n \ttest_i18ngrep \"bytes of body are still expected\" err\n '\n \n+test_expect_success 'clone with http:// using protocol v2 and invalid parameters' '\n+\ttest_when_finished \"rm -f log\" &&\n+\n+\ttest_must_fail env GIT_TRACE_PACKET=\"$(pwd)/log\" GIT_TRACE_CURL=\"$(pwd)/log\" \\\n+\t\tgit -c protocol.version=2 \\\n+\t\tclone --shallow-since=20151012 \"$HTTPD_URL/smart/http_parent\" http_child_invalid &&\n+\n+\t# Client requested to use protocol v2\n+\tgrep \"Git-Protocol: version=2\" log &&\n+\t# Server responded using protocol v2\n+\tgrep \"git< version 2\" log\n+'\n+\n test_expect_success 'clone big repository with http:// using protocol v2' '\n \ttest_when_finished \"rm -f log\" &&\n \ndiff --git a/transport.c b/transport.c\nindex 431a93caef..7d50c502ad 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -297,7 +297,8 @@ static struct ref *handshake(struct transport *transport, int for_push,\n \t\tif (must_list_refs)\n \t\t\tget_remote_refs(data->fd[1], &reader, &refs, for_push,\n \t\t\t\t\tref_prefixes,\n-\t\t\t\t\ttransport->server_options);\n+\t\t\t\t\ttransport->server_options,\n+\t\t\t\t\ttransport->stateless_rpc);\n \t\tbreak;\n \tcase protocol_v1:\n \tcase protocol_v0:\n-- \n2.26.2.706.g87896c9627\n\n"},{"id":"398204","messageId":"CAPig+cT1KfkszVW_Wvwz+r7Q0TsP+iTx-Cc3UvasS5ESBGdAAw@mail.gmail.com","threadId":"53462","inReplyTo":"36885943b239807eda49e231d8a45e1991f7288e.1589885479.git.liu.denton@gmail.com","subject":"Re: [PATCH v3 4/7] pkt-line: extern packet_length()","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-05-19T16:23:42Z","receivedAt":"2020-05-19T16:23:56Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, May 19, 2020 at 6:54 AM Denton Liu wrote:\n> [...]\n> Change the function parameter from `const char *linelen` to\n> `const char lenbuf_hex[4]`. Even though these two types behave\n> identically as function parameters, use the array notation to\n> semantically indicate exactly what this function is expecting as an\n> argument. Also, rename it from linelen to lenbuf_hex as the former\n> sounds like it should be an integral type which is misleading.\n>\n> Signed-off-by: Denton Liu \n> ---\n> diff --git a/pkt-line.h b/pkt-line.h\n> @@ -74,6 +74,15 @@ int write_packetized_from_buf(const char *src_in, size_t len, int fd_out);\n> +/*\n> + * Convert a four hex digit packet line length header into its numeric\n> + * representation.\n> + *\n> + * If linelen contains non-hex characters, return -1. Otherwise, return the\n\ns/linelen/lenbuf_hex/\n...or...\ns/lenbuf_hex/input argument/\n\n> + * numeric value of the length header.\n> + */\n> +int packet_length(const char lenbuf_hex[4]);\n"},{"id":"398208","messageId":"20200519184004.GA88536@coredump.intra.peff.net","threadId":"53462","inReplyTo":"cover.1589885479.git.liu.denton@gmail.com","subject":"Re: [PATCH v3 0/7] remote-curl: fix deadlocks when remote server disconnects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-05-19T18:40:04Z","receivedAt":"2020-05-19T18:40:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, May 19, 2020 at 06:53:53AM -0400, Denton Liu wrote:\n\n> Changes since v2:\n> \n> * Use if-else tower in \"transport: extract common fetch_pack() call\"\n> \n> * Rename to `lenbuf_hex` and remove null-terminator sentence in\n> \"pkt-line: extern packet_length()\"\n> \n> * Fix \"a a\" typo in \"remote-curl: error on incomplete packet\"\n> \n> * Don't send flush packet after response end packet\n> \n> * Move stateless delimiter checks closer to where message processing\n> happens in do_fetch_pack_v2()\n\nThis looks good to me, modulo the minor comment fixup from Eric. I did\nhave one final question, though. Our discussion focused a lot on\nchecking the 0002 packets in the success case. But we didn't discuss how\nfetch-pack would deal with an unexpected 0002 packet (i.e., the case\nthat the server response is truncated, but then remote-curl tacks on its\n0002).\n\nIt _seems_ to work, because that's the case your invalid-shallow test is\ncovering. I'm just not sure if it works consistently, or what error we\nmight produce in some cases (e.g., saying \"woah, what's the weird 0002\npacket\" instead of \"the server response ended unexpectedly\" or\nsomething).\n\nI suspect any remaining issues there are cosmetic, and it might be OK to\njust discover them organically through use. But if you happen to have\ndone some poking around there, it would be nice to hear it.\n\nThanks for working on this. When we had the initial discussion, I was\nreally worried the solution was going to be quite nasty, but I think it\nturned out to be reasonably nice.\n\n-Peff\n"},{"id":"398218","messageId":"04fbb741689d5f25b176c8d9709bfc6d19b2dbb5.1589921435.git.liu.denton@gmail.com","threadId":"53462","inReplyTo":"cover.1589885479.git.liu.denton@gmail.com","subject":"[PATCH v3 8/7] fixup! pkt-line: extern packet_length()","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-05-19T20:51:57Z","receivedAt":"2020-05-19T20:52:12Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"Signed-off-by: Denton Liu \n---\n pkt-line.h | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/pkt-line.h b/pkt-line.h\nindex 6cb92d7a5d..5b373fe4cd 100644\n--- a/pkt-line.h\n+++ b/pkt-line.h\n@@ -79,7 +79,7 @@ int packet_read(int fd, char **src_buffer, size_t *src_len, char\n * Convert a four hex digit packet line length header into its numeric\n * representation.\n *\n- * If linelen contains non-hex characters, return -1. Otherwise, return the\n+ * If lenbuf_hex contains non-hex characters, return -1. Otherwise, return the\n * numeric value of the length header.\n */\n int packet_length(const char lenbuf_hex[4]);\n-- \n2.27.0.rc0.114.g141fe7d276\n\n"},{"id":"398219","messageId":"20200519211456.GA5091@generichostname","threadId":"53462","inReplyTo":"20200519184004.GA88536@coredump.intra.peff.net","subject":"Re: [PATCH v3 0/7] remote-curl: fix deadlocks when remote server disconnects","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-05-19T21:14:56Z","receivedAt":"2020-05-19T21:15:02Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"Hi Peff,\n\nOn Tue, May 19, 2020 at 02:40:04PM -0400, Jeff King wrote:\n> It _seems_ to work, because that's the case your invalid-shallow test is\n> covering. I'm just not sure if it works consistently, or what error we\n> might produce in some cases (e.g., saying \"woah, what's the weird 0002\n> packet\" instead of \"the server response ended unexpectedly\" or\n> something).\n> \n> I suspect any remaining issues there are cosmetic, and it might be OK to\n> just discover them organically through use. But if you happen to have\n> done some poking around there, it would be nice to hear it.\n\nFrom what I can tell, every time a packet is read using the reader in\ndo_fetch_pack_v2(), we're careful about checking the packet type so we\nshould be safe there. Also, when piping stuff over to index-pack and\nunpack-objects, it looks like the resulting call to recv_sideband()\ntreats any control packets as flush packets so it should handle the 0002\nfine.\n\nI could have missed checking some spots, though. But as far as I can\ntell, if it can't handle the 0002 properly, it was already buggy to\nbegin with. I agree that we can let any remaining issues be shaken out\nthrough use.\n\n> Thanks for working on this. When we had the initial discussion, I was\n> really worried the solution was going to be quite nasty, but I think it\n> turned out to be reasonably nice.\n\nThanks,\n\nDenton\n\n> -Peff\n"},{"id":"398423","messageId":"4364b38bd027c219d41282bad3f8476daec936f9.1590154401.git.liu.denton@gmail.com","threadId":"53462","inReplyTo":"cover.1589885479.git.liu.denton@gmail.com","subject":"[PATCH v3 9/9] fixup! remote-curl: error on incomplete packet","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-05-22T13:33:47Z","receivedAt":"2020-05-22T13:35:14Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"In the CGI scripts which emulate a connection being prematurely\nterminated, it doesn't make sense for there to be a trailing newline\nafter the simulated connection cut. Remove these newlines.\n\nSigned-off-by: Denton Liu \n---\n t/lib-httpd/incomplete-body-upload-pack-v2-http.sh | 2 +-\n t/lib-httpd/incomplete-length-upload-pack-v2-http.sh | 2 +-\n 2 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/t/lib-httpd/incomplete-body-upload-pack-v2-http.sh b/t/lib-httpd/incomplete-body-upload-pack-v2-http.sh\nindex 2f5ed9fcf6..90e73ef8d5 100644\n--- a/t/lib-httpd/incomplete-body-upload-pack-v2-http.sh\n+++ b/t/lib-httpd/incomplete-body-upload-pack-v2-http.sh\n@@ -1,3 +1,3 @@\n printf \"Content-Type: text/%s\\n\" \"application/x-git-upload-pack-result\"\n echo\n-printf \"%s%s\\n\" \"0079\" \"45\"\n+printf \"%s%s\" \"0079\" \"45\"\ndiff --git a/t/lib-httpd/incomplete-length-upload-pack-v2-http.sh b/t/lib-httpd/incomplete-length-upload-pack-v2-http.sh\nindex 86c6e648c9..dce552e348 100644\n--- a/t/lib-httpd/incomplete-length-upload-pack-v2-http.sh\n+++ b/t/lib-httpd/incomplete-length-upload-pack-v2-http.sh\n@@ -1,3 +1,3 @@\n printf \"Content-Type: text/%s\\n\" \"application/x-git-upload-pack-result\"\n echo\n-printf \"%s\\n\" \"00\"\n+printf \"%s\" \"00\"\n-- \n2.27.0.rc1.69.g3229fa6e15\n\n"},{"id":"398428","messageId":"20200522155410.GA2146293@coredump.intra.peff.net","threadId":"53462","inReplyTo":"4364b38bd027c219d41282bad3f8476daec936f9.1590154401.git.liu.denton@gmail.com","subject":"Re: [PATCH v3 9/9] fixup! remote-curl: error on incomplete packet","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-05-22T15:54:10Z","receivedAt":"2020-05-22T15:54:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, May 22, 2020 at 09:33:47AM -0400, Denton Liu wrote:\n\n> In the CGI scripts which emulate a connection being prematurely\n> terminated, it doesn't make sense for there to be a trailing newline\n> after the simulated connection cut. Remove these newlines.\n\nAh, good catch. I think in the first one it doesn't matter:\n\n> -printf \"%s%s\\n\" \"0079\" \"45\"\n> +printf \"%s%s\" \"0079\" \"45\"\n\nbecause we have a too-short packet, so the fact that it is 3 bytes and\nnot 2 does not change that.\n\nI agree it's clearer without the newline, though. I wonder if:\n\n printf \"0079\" \"fewer than 0x79 bytes\"\n\nwould make it even more self-documenting. :)\n\n> -printf \"%s\\n\" \"00\"\n> +printf \"%s\" \"00\"\n\nThis one is a behavior improvement: we were probably hitting \"oops,\nnewline isn't a valid line-length character\" before, and now we're\nreally hitting the truncated packet.\n\nI don't know if it's worth adding an extra test with a bogus\nline-length. I'm OK with with it either way.\n\n-Peff\n"},{"id":"398430","messageId":"20200522160552.GA677085@generichostname","threadId":"53462","inReplyTo":"20200522155410.GA2146293@coredump.intra.peff.net","subject":"Re: [PATCH v3 9/9] fixup! remote-curl: error on incomplete packet","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-05-22T16:05:52Z","receivedAt":"2020-05-22T16:05:58Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"Hi Peff,\n\nOn Fri, May 22, 2020 at 11:54:10AM -0400, Jeff King wrote:\n> > -printf \"%s\\n\" \"00\"\n> > +printf \"%s\" \"00\"\n> \n> This one is a behavior improvement: we were probably hitting \"oops,\n> newline isn't a valid line-length character\" before, and now we're\n> really hitting the truncated packet.\n\nThe test currently actually greps for the \"incomplete length\" error\nmessage and it passes so the behaviour remains the same. We just got\nlucky that we send \"00\" instead of \"000\" beacuse \"000\\n\" would've\notherwise given us a full length header.\n\n> I don't know if it's worth adding an extra test with a bogus\n> line-length. I'm OK with with it either way.\n\nI think I'll leave this unless anyone really wants this to be tested.\n\nThanks,\n\nDenton\n"},{"id":"398431","messageId":"20200522163123.GA2147475@coredump.intra.peff.net","threadId":"53462","inReplyTo":"20200522160552.GA677085@generichostname","subject":"Re: [PATCH v3 9/9] fixup! remote-curl: error on incomplete packet","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-05-22T16:31:23Z","receivedAt":"2020-05-22T16:31:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, May 22, 2020 at 12:05:52PM -0400, Denton Liu wrote:\n\n> Hi Peff,\n> \n> On Fri, May 22, 2020 at 11:54:10AM -0400, Jeff King wrote:\n> > > -printf \"%s\\n\" \"00\"\n> > > +printf \"%s\" \"00\"\n> > \n> > This one is a behavior improvement: we were probably hitting \"oops,\n> > newline isn't a valid line-length character\" before, and now we're\n> > really hitting the truncated packet.\n> \n> The test currently actually greps for the \"incomplete length\" error\n> message and it passes so the behaviour remains the same. We just got\n> lucky that we send \"00\" instead of \"000\" beacuse \"000\\n\" would've\n> otherwise given us a full length header.\n\nAh, OK. I was surprised it didn't hit your new code in check_pktline()\nthat complains about non-hex chars. But the reason is that we don't\ncheck digit-by-digit as we read them. We wait until we have 4 chars, and\nthen we feed the whole thing to packet_length(). But since we only\nreceive 3, the state machine doesn't move to that step. So that makes\nsense.\n\n> > I don't know if it's worth adding an extra test with a bogus\n> > line-length. I'm OK with with it either way.\n> \n> I think I'll leave this unless anyone really wants this to be tested.\n\nSounds good.\n\n-Peff\n"}]}