{"thread":{"id":"53339","subject":"[PATCH v2] fetch-pack: try harder to read an ERR packet","startedAt":"2020-04-28T07:45:06Z","lastAt":"2020-04-28T21:23:44Z","messageCount":8,"participants":["Christian Couder","Junio C Hamano","Taylor Blau","Jeff King"],"isPatch":true,"patchVersion":2,"patchTotal":null},"messages":[{"id":"396442","messageId":"20200428074442.29830-1-chriscool@tuxfamily.org","threadId":"53339","inReplyTo":null,"subject":"[PATCH v2] fetch-pack: try harder to read an ERR packet","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2020-04-28T07:44:42Z","receivedAt":"2020-04-28T07:45:06Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"From: SZEDER Gábor \n\nWhen the server has hung up after sending an ERR packet to the\nclient, the client might still be writing, for example a \"done\"\nline. Therefore the client might get a write error before reading\nthe ERR packet.\n\nWhen fetching, this could result in the client displaying a\n\"Broken pipe\" error, instead of the more useful error sent by\nthe server in the ERR packet.\n\nInstead of using write_in_full() which immediately returns an\nerror when the server has hung up, let's use a new\nwrite_in_full_read_err() function which will read all the\npackets left before returning an error.\n\nHelped-by: Taylor Blau \nSigned-off-by: SZEDER Gábor \nSigned-off-by: Christian Couder \n---\n cache.h | 10 +++++++++-\n fetch-pack.c | 13 +++++++------\n wrapper.c | 7 +++++--\n 3 files changed, 21 insertions(+), 9 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 0f0485ecfe..efeee637b8 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1808,10 +1808,18 @@ int copy_file_with_time(const char *dst, const char *src, int mode);\n void write_or_die(int fd, const void *buf, size_t count);\n void fsync_or_die(int fd, const char *);\n \n+struct packet_reader;\n+\n ssize_t read_in_full(int fd, void *buf, size_t count);\n-ssize_t write_in_full(int fd, const void *buf, size_t count);\n+ssize_t write_in_full_read_err(int fd, const void *buf, size_t count,\n+\t\t\t struct packet_reader *reader);\n ssize_t pread_in_full(int fd, void *buf, size_t count, off_t offset);\n \n+static inline ssize_t write_in_full(int fd, const void *buf, size_t count)\n+{\n+\treturn write_in_full_read_err(fd, buf, count, NULL);\n+}\n+\n static inline ssize_t write_str_in_full(int fd, const char *str)\n {\n \treturn write_in_full(fd, str, strlen(str));\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex 0b07b3ee73..febdf54bf4 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -185,13 +185,14 @@ static enum ack_type get_ack(struct packet_reader *reader,\n }\n \n static void send_request(struct fetch_pack_args *args,\n-\t\t\t int fd, struct strbuf *buf)\n+\t\t\t int fd, struct strbuf *buf,\n+\t\t\t struct packet_reader *reader)\n {\n \tif (args->stateless_rpc) {\n \t\tsend_sideband(fd, -1, buf->buf, buf->len, LARGE_PACKET_MAX);\n \t\tpacket_flush(fd);\n \t} else {\n-\t\tif (write_in_full(fd, buf->buf, buf->len) < 0)\n+\t\tif (write_in_full_read_err(fd, buf->buf, buf->len, reader) < 0)\n \t\t\tdie_errno(_(\"unable to write to remote\"));\n \t}\n }\n@@ -349,7 +350,7 @@ static int find_common(struct fetch_negotiator *negotiator,\n \t\tconst char *arg;\n \t\tstruct object_id oid;\n \n-\t\tsend_request(args, fd[1], &req_buf);\n+\t\tsend_request(args, fd[1], &req_buf, &reader);\n \t\twhile (packet_reader_read(&reader) == PACKET_READ_NORMAL) {\n \t\t\tif (skip_prefix(reader.line, \"shallow \", &arg)) {\n \t\t\t\tif (get_oid_hex(arg, &oid))\n@@ -372,7 +373,7 @@ static int find_common(struct fetch_negotiator *negotiator,\n \t\t\tdie(_(\"expected shallow/unshallow, got %s\"), reader.line);\n \t\t}\n \t} else if (!args->stateless_rpc)\n-\t\tsend_request(args, fd[1], &req_buf);\n+\t\tsend_request(args, fd[1], &req_buf, &reader);\n \n \tif (!args->stateless_rpc) {\n \t\t/* If we aren't using the stateless-rpc interface\n@@ -395,7 +396,7 @@ static int find_common(struct fetch_negotiator *negotiator,\n \t\t\tint ack;\n \n \t\t\tpacket_buf_flush(&req_buf);\n-\t\t\tsend_request(args, fd[1], &req_buf);\n+\t\t\tsend_request(args, fd[1], &req_buf, &reader);\n \t\t\tstrbuf_setlen(&req_buf, state_len);\n \t\t\tflushes++;\n \t\t\tflush_at = next_flush(args->stateless_rpc, count);\n@@ -470,7 +471,7 @@ static int find_common(struct fetch_negotiator *negotiator,\n \ttrace2_region_leave(\"fetch-pack\", \"negotiation_v0_v1\", the_repository);\n \tif (!got_ready || !no_done) {\n \t\tpacket_buf_write(&req_buf, \"done\\n\");\n-\t\tsend_request(args, fd[1], &req_buf);\n+\t\tsend_request(args, fd[1], &req_buf, &reader);\n \t}\n \tprint_verbose(args, _(\"done\"));\n \tif (retval != 0) {\ndiff --git a/wrapper.c b/wrapper.c\nindex 3a1c0e0526..2da7a15382 100644\n--- a/wrapper.c\n+++ b/wrapper.c\n@@ -3,6 +3,7 @@\n */\n #include \"cache.h\"\n #include \"config.h\"\n+#include \"pkt-line.h\"\n \n static int memory_limit_check(size_t size, int gentle)\n {\n@@ -291,7 +292,8 @@ ssize_t read_in_full(int fd, void *buf, size_t count)\n \treturn total;\n }\n \n-ssize_t write_in_full(int fd, const void *buf, size_t count)\n+ssize_t write_in_full_read_err(int fd, const void *buf, size_t count,\n+\t\t\t struct packet_reader *reader)\n {\n \tconst char *p = buf;\n \tssize_t total = 0;\n@@ -300,7 +302,8 @@ ssize_t write_in_full(int fd, const void *buf, size_t count)\n \t\tssize_t written = xwrite(fd, p, count);\n \t\tif (written < 0)\n \t\t\treturn -1;\n-\t\tif (!written) {\n+\t\tif (!written &&\n+\t\t (!reader || packet_reader_read(reader) == PACKET_READ_EOF)) {\n \t\t\terrno = ENOSPC;\n \t\t\treturn -1;\n \t\t}\n-- \n2.26.2.267.g8b5eb5fb58.dirty\n\n"},{"id":"396492","messageId":"xmqqzhav1kix.fsf@gitster.c.googlers.com","threadId":"53339","inReplyTo":"20200428074442.29830-1-chriscool@tuxfamily.org","subject":"Re: [PATCH v2] fetch-pack: try harder to read an ERR packet","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-04-28T19:24:06Z","receivedAt":"2020-04-28T19:24:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder writes:\n\n> From: SZEDER Gábor \n>\n> When the server has hung up after sending an ERR packet to the\n> client, the client might still be writing, for example a \"done\"\n> line. Therefore the client might get a write error before reading\n> the ERR packet.\n>\n> When fetching, this could result in the client displaying a\n> \"Broken pipe\" error, instead of the more useful error sent by\n> the server in the ERR packet.\n\nHmm, if the connection gets severed just before the ERR packet the\nother side has written, we will see \"Broken pipe\" if we write\n\"done\", and no amount of \"try to read to collect as much what they\nsaid as possible\" would help. If you are lucky and the connection\nis broken after the ERR reaches on this side, such an \"extra effort\"\nmay help, but is it really worth the effort? It is not clear to me\nif the extra complexity, one more API function people need to learn,\nand the need to think which one to use every time they want to say\nwrite_in_full(), are justifiable.\n\nI dunno.\n"},{"id":"396494","messageId":"20200428195957.GA45908@syl.local","threadId":"53339","inReplyTo":"xmqqzhav1kix.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2] fetch-pack: try harder to read an ERR packet","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-04-28T19:59:57Z","receivedAt":"2020-04-28T20:00:05Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Apr 28, 2020 at 12:24:06PM -0700, Junio C Hamano wrote:\n> Christian Couder writes:\n>\n> > From: SZEDER Gábor \n> >\n> > When the server has hung up after sending an ERR packet to the\n> > client, the client might still be writing, for example a \"done\"\n> > line. Therefore the client might get a write error before reading\n> > the ERR packet.\n> >\n> > When fetching, this could result in the client displaying a\n> > \"Broken pipe\" error, instead of the more useful error sent by\n> > the server in the ERR packet.\n>\n> Hmm, if the connection gets severed just before the ERR packet the\n> other side has written, we will see \"Broken pipe\" if we write\n> \"done\", and no amount of \"try to read to collect as much what they\n> said as possible\" would help. If you are lucky and the connection\n> is broken after the ERR reaches on this side, such an \"extra effort\"\n> may help, but is it really worth the effort? It is not clear to me\n> if the extra complexity, one more API function people need to learn,\n> and the need to think which one to use every time they want to say\n> write_in_full(), are justifiable.\n>\n> I dunno.\n\nI think that you're right. The more that I thought about my suggestion,\nthe more dumb I was convinced that it was.\n\nSorry.\n\nThanks,\nTaylor\n"},{"id":"396498","messageId":"20200428204908.GA4000@coredump.intra.peff.net","threadId":"53339","inReplyTo":"xmqqzhav1kix.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2] fetch-pack: try harder to read an ERR packet","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-04-28T20:49:08Z","receivedAt":"2020-04-28T20:49:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Apr 28, 2020 at 12:24:06PM -0700, Junio C Hamano wrote:\n\n> Christian Couder writes:\n> \n> > From: SZEDER Gábor \n> >\n> > When the server has hung up after sending an ERR packet to the\n> > client, the client might still be writing, for example a \"done\"\n> > line. Therefore the client might get a write error before reading\n> > the ERR packet.\n> >\n> > When fetching, this could result in the client displaying a\n> > \"Broken pipe\" error, instead of the more useful error sent by\n> > the server in the ERR packet.\n> \n> Hmm, if the connection gets severed just before the ERR packet the\n> other side has written, we will see \"Broken pipe\" if we write\n> \"done\", and no amount of \"try to read to collect as much what they\n> said as possible\" would help. If you are lucky and the connection\n> is broken after the ERR reaches on this side, such an \"extra effort\"\n> may help, but is it really worth the effort? It is not clear to me\n> if the extra complexity, one more API function people need to learn,\n> and the need to think which one to use every time they want to say\n> write_in_full(), are justifiable.\n\nI think the \"lucky\" case happens pretty routinely. The situation we're\ntrying to catch here is that server does:\n\n packet_write(\"ERR I don't like your request for some reason\");\n die(\"exiting\");\n\nOn the client side, we'll get that final packet and then see that the\nconnection is closed. So if we get EPIPE or similar on write(), that\nmeans we're seeing the closed connection. Which means we'll _always_\nhave gotten the ERR packet (in this situation).\n\nSo the problem it is solving is that there isn't really flow control in\nthe protocol. The server might be aborting and dumping out an error\nresponse while the client is still writing. If the server were to\ncontinue reading the client request before closing the connection, this\nwouldn't happen. And that's what an HTTP server would be doing.\n\nBut I think that's pretty tricky to do in our programs, where the\nprotocol framing isn't so structured, and deciding when the client is\ndone talking often involves parsing their request. E.g., imagine we die\nwith \"not our ref\" due to a \"want\" line. We'd have to return an error up\nthe stack to the code that is reading want/have/done lines, so that it\nknows to keep pumping the socket until it gets to a break point, and\n_then_ die().\n\nI think by contrast, just having the client handle EPIPE more gracefully\nis a simpler fix.\n\n-Peff\n"},{"id":"396499","messageId":"20200428205119.GB4000@coredump.intra.peff.net","threadId":"53339","inReplyTo":"20200428195957.GA45908@syl.local","subject":"Re: [PATCH v2] fetch-pack: try harder to read an ERR packet","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-04-28T20:51:19Z","receivedAt":"2020-04-28T20:51:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Apr 28, 2020 at 01:59:57PM -0600, Taylor Blau wrote:\n\n> > > When the server has hung up after sending an ERR packet to the\n> > > client, the client might still be writing, for example a \"done\"\n> > > line. Therefore the client might get a write error before reading\n> > > the ERR packet.\n> > >\n> > > When fetching, this could result in the client displaying a\n> > > \"Broken pipe\" error, instead of the more useful error sent by\n> > > the server in the ERR packet.\n> >\n> > Hmm, if the connection gets severed just before the ERR packet the\n> > other side has written, we will see \"Broken pipe\" if we write\n> > \"done\", and no amount of \"try to read to collect as much what they\n> > said as possible\" would help. If you are lucky and the connection\n> > is broken after the ERR reaches on this side, such an \"extra effort\"\n> > may help, but is it really worth the effort? It is not clear to me\n> > if the extra complexity, one more API function people need to learn,\n> > and the need to think which one to use every time they want to say\n> > write_in_full(), are justifiable.\n> >\n> > I dunno.\n> \n> I think that you're right. The more that I thought about my suggestion,\n> the more dumb I was convinced that it was.\n\nNow I'm confused about which suggestion. The overall patch's purpose is\nstill good, I think (see my other response). But I agree that having an\nalternate write_in_full() is spreading the hack too far outside of\nfetch-pack (and as I wrote in [1], I really don't understand what it's\ntrying to do).\n\n-Peff\n\n[1] https://lore.kernel.org/git/20200428083336.GA2405176@coredump.intra.peff.net/\n"},{"id":"396502","messageId":"xmqqh7x31fyu.fsf@gitster.c.googlers.com","threadId":"53339","inReplyTo":"20200428204908.GA4000@coredump.intra.peff.net","subject":"Re: [PATCH v2] fetch-pack: try harder to read an ERR packet","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-04-28T21:02:33Z","receivedAt":"2020-04-28T21:02:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> Hmm, if the connection gets severed just before the ERR packet the\n>> other side has written, we will see \"Broken pipe\" if we write\n>> \"done\", and no amount of \"try to read to collect as much what they\n>> said as possible\" would help. If you are lucky and the connection\n>> is broken after the ERR reaches on this side, such an \"extra effort\"\n>> may help, but is it really worth the effort? It is not clear to me\n>> if the extra complexity, one more API function people need to learn,\n>> and the need to think which one to use every time they want to say\n>> write_in_full(), are justifiable.\n>\n> I think the \"lucky\" case happens pretty routinely. The situation we're\n> trying to catch here is that server does:\n>\n> packet_write(\"ERR I don't like your request for some reason\");\n> die(\"exiting\");\n\nOK, if we assume that the communication route is never flaky and\neverything writtten will go through before TCP shutdown that would\nhappen when die() kills the process (or like test environment that\nmost communication goes locally between two processes), sure, it may\nlook common enough to be worth \"fixing\". I simply did not realize\nthat was the shared assumption behind this patch before I went back\nto the original discussion that was about a racy test.\n\nAs long as an extra read on the side that just got a write error\nwon't throw us into a deadlock, I think I am OK, but I am still not\nsure if the code complexity to have two write_in_full() is worth it.\n\nIf the mechanism to do this were limited to the packet IO layer, it\nmay be more palatable, though.\n\n"},{"id":"396510","messageId":"20200428211736.GG4000@coredump.intra.peff.net","threadId":"53339","inReplyTo":"xmqqh7x31fyu.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2] fetch-pack: try harder to read an ERR packet","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-04-28T21:17:36Z","receivedAt":"2020-04-28T21:17:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Apr 28, 2020 at 02:02:33PM -0700, Junio C Hamano wrote:\n\n> > I think the \"lucky\" case happens pretty routinely. The situation we're\n> > trying to catch here is that server does:\n> >\n> > packet_write(\"ERR I don't like your request for some reason\");\n> > die(\"exiting\");\n> \n> OK, if we assume that the communication route is never flaky and\n> everything writtten will go through before TCP shutdown that would\n> happen when die() kills the process (or like test environment that\n> most communication goes locally between two processes), sure, it may\n> look common enough to be worth \"fixing\". I simply did not realize\n> that was the shared assumption behind this patch before I went back\n> to the original discussion that was about a racy test.\n\nCertainly the communication route will _sometimes_ be flaky. And there's\nnothing we can do there except say \"Broken pipe\" or similar, whether the\nother side said ERR or not. So any reads have to be speculative. But\nassuming TCP is functioning, some portion of our ERR packets are simply\ndropped from the incoming TCP buffers. I suspect we don't get more\nreports of this in the wild (and are mostly annoyed by it in racy tests)\nbecause:\n\n - ERR conditions don't happen all that often (though we have been\n adding more recently)\n\n - users may not realize they _could_ have gotten a good message\n (especially since historically many of these conditions did just\n involve the server hanging up)\n\n - I think the race may be easier to win on real networks with latency.\n Locally, if I write \"want bogus\" and then \"done\", the other side may\n process the \"want\" between the two and close the pipe. But if\n there's 50ms between client and server, we've usually managed to\n \"done\" into the systems TCP buffer, if not onto the wire, by the\n time the other side has figured out the error and gotten a FIN\n packet back to us.\n\n> As long as an extra read on the side that just got a write error\n> won't throw us into a deadlock, I think I am OK, but I am still not\n> sure if the code complexity to have two write_in_full() is worth it.\n\nYes, I don't see the point of that.\n\n> If the mechanism to do this were limited to the packet IO layer, it\n> may be more palatable, though.\n\nAgreed. There's not a single place where we write, though, since we\noften form packets in local buffers and then write() them out manually.\nSo it does have to be sprinkled around fetch-pack.c. But certainly the\ndamage can be limited to that client network code.\n\n-Peff\n"},{"id":"396511","messageId":"xmqqzhavz4me.fsf@gitster.c.googlers.com","threadId":"53339","inReplyTo":"20200428211736.GG4000@coredump.intra.peff.net","subject":"Re: [PATCH v2] fetch-pack: try harder to read an ERR packet","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-04-28T21:23:37Z","receivedAt":"2020-04-28T21:23:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> If the mechanism to do this were limited to the packet IO layer, it\n>> may be more palatable, though.\n>\n> Agreed. There's not a single place where we write, though, since we\n> often form packets in local buffers and then write() them out manually.\n> So it does have to be sprinkled around fetch-pack.c. But certainly the\n> damage can be limited to that client network code.\n\nYeah, I do not mind \"sprinkling all over the place in client network\ncode\" at all. The ideal is if we never used write_in_full() and\nalways used this \"write but if we get a write error then check for\nERR packet\" helper consistently (which means we'd convert all the\nwrites done in that layer, and by definition that may have to be\n\"all over the place in the client networking code\").\n\nThanks.\n"}]}