| Message ID | cover.1589885479.git.liu.denton@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | remote-curl: fix deadlocks when remote server disconnects | expand |
On Tue, May 19, 2020 at 06:53:53AM -0400, Denton Liu wrote: > Changes since v2: > > * Use if-else tower in "transport: extract common fetch_pack() call" > > * Rename to `lenbuf_hex` and remove null-terminator sentence in > "pkt-line: extern packet_length()" > > * Fix "a a" typo in "remote-curl: error on incomplete packet" > > * Don't send flush packet after response end packet > > * Move stateless delimiter checks closer to where message processing > happens in do_fetch_pack_v2() This looks good to me, modulo the minor comment fixup from Eric. I did have one final question, though. Our discussion focused a lot on checking the 0002 packets in the success case. But we didn't discuss how fetch-pack would deal with an unexpected 0002 packet (i.e., the case that the server response is truncated, but then remote-curl tacks on its 0002). It _seems_ to work, because that's the case your invalid-shallow test is covering. I'm just not sure if it works consistently, or what error we might produce in some cases (e.g., saying "woah, what's the weird 0002 packet" instead of "the server response ended unexpectedly" or something). I suspect any remaining issues there are cosmetic, and it might be OK to just discover them organically through use. But if you happen to have done some poking around there, it would be nice to hear it. Thanks for working on this. When we had the initial discussion, I was really worried the solution was going to be quite nasty, but I think it turned out to be reasonably nice. -Peff
Hi Peff, On Tue, May 19, 2020 at 02:40:04PM -0400, Jeff King wrote: > It _seems_ to work, because that's the case your invalid-shallow test is > covering. I'm just not sure if it works consistently, or what error we > might produce in some cases (e.g., saying "woah, what's the weird 0002 > packet" instead of "the server response ended unexpectedly" or > something). > > I suspect any remaining issues there are cosmetic, and it might be OK to > just discover them organically through use. But if you happen to have > done some poking around there, it would be nice to hear it. From what I can tell, every time a packet is read using the reader in do_fetch_pack_v2(), we're careful about checking the packet type so we should be safe there. Also, when piping stuff over to index-pack and unpack-objects, it looks like the resulting call to recv_sideband() treats any control packets as flush packets so it should handle the 0002 fine. I could have missed checking some spots, though. But as far as I can tell, if it can't handle the 0002 properly, it was already buggy to begin with. I agree that we can let any remaining issues be shaken out through use. > Thanks for working on this. When we had the initial discussion, I was > really worried the solution was going to be quite nasty, but I think it > turned out to be reasonably nice. Thanks, Denton > -Peff
The following command hangs forever: $ git -c protocol.version=2 clone https://github.com/git/git.git --shallow-since=20151012 Cloning into 'git'... This occurs because the --shallow-since arg is incorrect and the server dies early. However, remote-curl does not realise that the server errored out and just faithfully forwards the packets to fetch-pack before waiting on more input from fetch-pack. Meanwhile, fetch-pack keeps reading as it still expects more input. As a result, the processes deadlock. Original analysis by Peff: https://lore.kernel.org/git/20200328154936.GA1217052@coredump.intra.peff.net/ Changes since v2: * Use if-else tower in "transport: extract common fetch_pack() call" * Rename to `lenbuf_hex` and remove null-terminator sentence in "pkt-line: extern packet_length()" * Fix "a a" typo in "remote-curl: error on incomplete packet" * Don't send flush packet after response end packet * Move stateless delimiter checks closer to where message processing happens in do_fetch_pack_v2() Changes since v1: * Remove fallthrough in switch in favour of just extracting the common call out of the switch in patch 3 * Add more detail in function comment and use `const char linelen[4]` in patch 4 * Implement most of Peff's suggestions[0] in patch 5 * Only operate on stateless_connect() in patch 5 * Add tests in patch 5 * Drop "remote-curl: ensure last packet is a flush" in favour of "stateless-connect: send response end packet" [0]: https://lore.kernel.org/git/20200515213844.GD115445@coredump.intra.peff.net/ Denton Liu (7): remote-curl: fix typo remote-curl: remove label indentation transport: extract common fetch_pack() call pkt-line: extern packet_length() remote-curl: error on incomplete packet pkt-line: define PACKET_READ_RESPONSE_END stateless-connect: send response end packet Documentation/gitremote-helpers.txt | 4 +- Documentation/technical/protocol-v2.txt | 2 + builtin/fetch-pack.c | 2 +- connect.c | 18 ++++- connect.h | 4 ++ fetch-pack.c | 13 ++++ pkt-line.c | 17 ++++- pkt-line.h | 11 +++ remote-curl.c | 70 +++++++++++++++++-- remote.h | 3 +- serve.c | 2 + t/helper/test-pkt-line.c | 4 ++ t/lib-httpd.sh | 2 + t/lib-httpd/apache.conf | 8 +++ .../incomplete-body-upload-pack-v2-http.sh | 3 + .../incomplete-length-upload-pack-v2-http.sh | 3 + t/t5702-protocol-v2.sh | 47 +++++++++++++ transport.c | 28 +++----- 18 files changed, 211 insertions(+), 30 deletions(-) create mode 100644 t/lib-httpd/incomplete-body-upload-pack-v2-http.sh create mode 100644 t/lib-httpd/incomplete-length-upload-pack-v2-http.sh Range-diff against v2: 1: b390875f87 = 1: b390875f87 remote-curl: fix typo 2: a2b28c0b28 = 2: a2b28c0b28 remote-curl: remove label indentation 3: 3a42575bd5 < -: ---------- transport: extract common fetch_pack() call -: ---------- > 3: c118baa5a2 transport: extract common fetch_pack() call 4: c2b9d033bb ! 4: 36885943b2 pkt-line: extern packet_length() @@ Commit message need to access the length header. In order to simplify this, extern packet_length() so that the logic can be reused. - Change the function parameter from a `const char *` to - `const char linelen[4]`. Even though these two types behave identically - as function parameters, use the array notation to semantically indicate - exactly what this function is expecting as an argument. + Change the function parameter from `const char *linelen` to + `const char lenbuf_hex[4]`. Even though these two types behave + identically as function parameters, use the array notation to + semantically indicate exactly what this function is expecting as an + argument. Also, rename it from linelen to lenbuf_hex as the former + sounds like it should be an integral type which is misleading. ## pkt-line.c ## @@ pkt-line.c: static int get_packet_data(int fd, char **src_buf, size_t *src_size, @@ pkt-line.c: static int get_packet_data(int fd, char **src_buf, size_t *src_size, } -static int packet_length(const char *linelen) -+int packet_length(const char linelen[4]) ++int packet_length(const char lenbuf_hex[4]) { - int val = hex2chr(linelen); - return (val < 0) ? val : (val << 8) | hex2chr(linelen + 2); +- int val = hex2chr(linelen); +- return (val < 0) ? val : (val << 8) | hex2chr(linelen + 2); ++ int val = hex2chr(lenbuf_hex); ++ return (val < 0) ? val : (val << 8) | hex2chr(lenbuf_hex + 2); + } + + enum packet_read_status packet_read_with_status(int fd, char **src_buffer, ## pkt-line.h ## @@ pkt-line.h: int write_packetized_from_buf(const char *src_in, size_t len, int fd_out); @@ pkt-line.h: int write_packetized_from_buf(const char *src_in, size_t len, int fd +/* + * Convert a four hex digit packet line length header into its numeric -+ * representation. linelen should not be null-terminated. ++ * representation. + * + * If linelen contains non-hex characters, return -1. Otherwise, return the + * numeric value of the length header. + */ -+int packet_length(const char linelen[4]); ++int packet_length(const char lenbuf_hex[4]); + /* * Read a packetized line into a buffer like the 'packet_read()' function but 5: 52ce5fdffd ! 5: 91d330620a remote-curl: error on incomplete packet @@ Commit message results in a deadlock between the two processes. For a stateless connection, inspect packets before sending them and - error out if a a packet line packet is incomplete. + error out if a packet line packet is incomplete. Helped-by: Jeff King <peff@peff.net> 6: 744b078324 ! 6: ff83344e9e pkt-line: PACKET_READ_RESPONSE_END @@ Metadata Author: Denton Liu <liu.denton@gmail.com> ## Commit message ## - pkt-line: PACKET_READ_RESPONSE_END + pkt-line: define PACKET_READ_RESPONSE_END In a future commit, we will use PACKET_READ_RESPONSE_END to separate messages proxied by remote-curl. To prepare for this, add the 7: 4b079bcd83 ! 7: c26e160fbc stateless-connect: send response end packet @@ Commit message Cloning into 'git'... fatal: the remote end hung up unexpectedly - Instead of blindly forwarding packets, make remote-curl insert response - end and flush packets after proxying the responses from the remote - server when using stateless_connect(). On the RPC client side, ensure - that each response ends as described. + Instead of blindly forwarding packets, make remote-curl insert a + response end packet after proxying the responses from the remote server + when using stateless_connect(). On the RPC client side, ensure that each + response ends as described. A separate control packet is chosen because we need to be able to differentiate between what the remote server sends and remote-curl's @@ Documentation/gitremote-helpers.txt: Supported if the helper has the "connect" c (both request and response) must consist of zero or more - PKT-LINEs, terminating in a flush packet. The client must not + PKT-LINEs, terminating in a flush packet. Response messages will -+ have a response end packet before the flush packet to indicate -+ the end of a response. The client must not ++ then have a response end packet after the flush packet to ++ indicate the end of a response. The client must not expect the server to store any state in between request-response pairs. After the connection ends, the remote helper exits. + @@ builtin/fetch-pack.c: int cmd_fetch_pack(int argc, const char **argv, const char ## connect.c ## @@ connect.c: static int process_ref_v2(const char *line, struct ref ***list) + return ret; + } + ++void check_stateless_delimiter(int stateless_rpc, ++ struct packet_reader *reader, ++ const char *error) ++{ ++ if (!stateless_rpc) ++ return; /* not in stateless mode, no delimiter expected */ ++ if (packet_reader_read(reader) != PACKET_READ_RESPONSE_END) ++ die("%s", error); ++} ++ struct ref **get_remote_refs(int fd_out, struct packet_reader *reader, struct ref **list, int for_push, const struct argv_array *ref_prefixes, @@ connect.c: struct ref **get_remote_refs(int fd_out, struct packet_reader *reader if (reader->status != PACKET_READ_FLUSH) die(_("expected flush after ref listing")); -+ if (stateless_rpc) { -+ if (packet_reader_read(reader) != PACKET_READ_RESPONSE_END) -+ die(_("expected response end packet after ref listing")); -+ if (packet_reader_read(reader) != PACKET_READ_FLUSH) -+ die(_("expected flush packet after response end")); -+ } ++ check_stateless_delimiter(stateless_rpc, reader, ++ _("expected response end packet after ref listing")); + return list; } - ## fetch-pack.c ## -@@ fetch-pack.c: static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args, - struct fetch_negotiator negotiator_alloc; - struct fetch_negotiator *negotiator; - int seen_ack = 0; -+ int check_http_delimiter; + ## connect.h ## +@@ connect.h: int server_supports_v2(const char *c, int die_on_error); + int server_supports_feature(const char *c, const char *feature, + int die_on_error); - if (args->no_dependents) { - negotiator = NULL; -@@ fetch-pack.c: static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args, - } - - while (state != FETCH_DONE) { -+ check_http_delimiter = 0; ++void check_stateless_delimiter(int stateless_rpc, ++ struct packet_reader *reader, ++ const char *error); + - switch (state) { - case FETCH_CHECK_LOCAL: - sort_ref_list(&ref, ref_compare_name); + #endif + + ## fetch-pack.c ## +@@ fetch-pack.c: enum fetch_state { + FETCH_DONE, + }; + ++static void do_check_stateless_delimiter(const struct fetch_pack_args *args, ++ struct packet_reader *reader) ++{ ++ check_stateless_delimiter(args->stateless_rpc, reader, ++ _("git fetch-pack: expected response end packet")); ++} ++ + static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args, + int fd[2], + const struct ref *orig_ref, @@ fetch-pack.c: static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args, + /* Process ACKs/NAKs */ + switch (process_acks(negotiator, &reader, &common)) { + case READY: ++ /* ++ * Don't check for response delimiter; get_pack() will ++ * read the rest of this response. ++ */ + state = FETCH_GET_PACK; + break; + case COMMON_FOUND: +@@ fetch-pack.c: static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args, + seen_ack = 1; /* fallthrough */ case NO_COMMON_FOUND: ++ do_check_stateless_delimiter(args, &reader); state = FETCH_SEND_REQUEST; -+ check_http_delimiter = 1; break; } - break; @@ fetch-pack.c: static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args, + process_section_header(&reader, "packfile", 0); + if (get_pack(args, fd, pack_lockfile, sought, nr_sought)) die(_("git fetch-pack: fetch failed.")); ++ do_check_stateless_delimiter(args, &reader); state = FETCH_DONE; -+ check_http_delimiter = 1; break; - case FETCH_DONE: - continue; - } -+ -+ if (args->stateless_rpc && check_http_delimiter) { -+ if (packet_reader_read(&reader) != PACKET_READ_RESPONSE_END) -+ die(_("git fetch-pack: expected response end packet")); -+ if (packet_reader_read(&reader) != PACKET_READ_FLUSH) -+ die(_("git fetch-pack: expected flush packet")); -+ } - } - - if (negotiator) ## remote-curl.c ## @@ remote-curl.c: static void check_pktline(struct check_pktline_state *state, const char *ptr, si @@ remote-curl.c: static int post_rpc(struct rpc_state *rpc, int stateless_connect, if (rpc_in_data.pktline_state.remaining) err = error(_("%d bytes of body are still expected"), rpc_in_data.pktline_state.remaining); -+ if (stateless_connect) { ++ if (stateless_connect) + packet_response_end(rpc->in); -+ packet_flush(rpc->in); -+ } + curl_slist_free_all(headers); free(gzip_body);