Git Mailing List Archive on lore.kernel.org
 help / color / Atom feed
From: Denton Liu <liu.denton@gmail.com>
To: Git Mailing List <git@vger.kernel.org>
Cc: Jeff King <peff@peff.net>,
	Eric Sunshine <sunshine@sunshineco.com>,
	Junio C Hamano <gitster@pobox.com>
Subject: [PATCH v3 0/7] remote-curl: fix deadlocks when remote server disconnects
Date: Tue, 19 May 2020 06:53:53 -0400
Message-ID: <cover.1589885479.git.liu.denton@gmail.com> (raw)
In-Reply-To: <cover.1589816718.git.liu.denton@gmail.com>

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);
-- 
2.26.2.706.g87896c9627


  parent reply index

Thread overview: 62+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-05-13 18:04 [PATCH 0/6] remote-curl: partial fix for a deadlock with stateless rpc Denton Liu
2020-05-13 18:04 ` [PATCH 1/6] remote-curl: fix typo Denton Liu
2020-05-13 18:04 ` [PATCH 2/6] remote-curl: remove label indentation Denton Liu
2020-05-13 18:04 ` [PATCH 3/6] transport: combine common cases with a fallthrough Denton Liu
2020-05-13 23:14   ` Eric Sunshine
2020-05-18  9:18     ` Denton Liu
2020-05-18 17:43       ` Eric Sunshine
2020-05-13 18:04 ` [PATCH 4/6] pkt-line: extern packet_length() Denton Liu
2020-05-13 23:23   ` Eric Sunshine
2020-05-15 20:56   ` Jeff King
2020-05-15 20:57     ` Jeff King
2020-05-13 18:04 ` [PATCH 5/6] remote-curl: error on incomplete packet Denton Liu
2020-05-15 21:38   ` Jeff King
2020-05-18  9:08     ` Denton Liu
2020-05-18 15:49       ` Jeff King
2020-05-13 18:04 ` [PATCH 6/6] remote-curl: ensure last packet is a flush Denton Liu
2020-05-15 21:02   ` Denton Liu
2020-05-15 21:41     ` Jeff King
2020-05-18 16:34       ` Junio C Hamano
2020-05-18 16:52         ` Jeff King
2020-05-18 21:00           ` Jeff King
2020-05-18 15:47 ` [PATCH v2 0/7] remote-curl: fix deadlocks when remote server disconnects Denton Liu
2020-05-18 15:47   ` [PATCH v2 1/7] remote-curl: fix typo Denton Liu
2020-05-18 15:47   ` [PATCH v2 2/7] remote-curl: remove label indentation Denton Liu
2020-05-18 18:37     ` Junio C Hamano
2020-05-18 15:47   ` [PATCH v2 3/7] transport: extract common fetch_pack() call Denton Liu
2020-05-18 18:40     ` Junio C Hamano
2020-05-18 15:47   ` [PATCH v2 4/7] pkt-line: extern packet_length() Denton Liu
2020-05-18 16:04     ` Jeff King
2020-05-18 17:50       ` Eric Sunshine
2020-05-18 20:08         ` Jeff King
2020-05-18 18:44       ` Junio C Hamano
2020-05-18 15:47   ` [PATCH v2 5/7] remote-curl: error on incomplete packet Denton Liu
2020-05-18 16:22     ` Jeff King
2020-05-18 16:51       ` Denton Liu
2020-05-18 15:47   ` [PATCH v2 6/7] pkt-line: PACKET_READ_RESPONSE_END Denton Liu
2020-05-18 15:47   ` [PATCH v2 7/7] stateless-connect: send response end packet Denton Liu
2020-05-18 16:43     ` Jeff King
2020-05-18 17:12       ` Denton Liu
2020-05-18 17:26         ` Jeff King
2020-05-18 16:50   ` [PATCH v2 0/7] remote-curl: fix deadlocks when remote server disconnects Jeff King
2020-05-18 17:36     ` Denton Liu
2020-05-18 20:58       ` Jeff King
2020-05-18 22:52         ` Junio C Hamano
2020-05-19  2:38           ` Jeff King
2020-05-18 19:36     ` Junio C Hamano
2020-05-19 10:53   ` Denton Liu [this message]
2020-05-19 10:53     ` [PATCH v3 1/7] remote-curl: fix typo Denton Liu
2020-05-19 10:53     ` [PATCH v3 2/7] remote-curl: remove label indentation Denton Liu
2020-05-19 10:53     ` [PATCH v3 3/7] transport: extract common fetch_pack() call Denton Liu
2020-05-19 10:53     ` [PATCH v3 4/7] pkt-line: extern packet_length() Denton Liu
2020-05-19 16:23       ` Eric Sunshine
2020-05-19 10:53     ` [PATCH v3 5/7] remote-curl: error on incomplete packet Denton Liu
2020-05-19 10:53     ` [PATCH v3 6/7] pkt-line: define PACKET_READ_RESPONSE_END Denton Liu
2020-05-19 10:54     ` [PATCH v3 7/7] stateless-connect: send response end packet Denton Liu
2020-05-19 18:40     ` [PATCH v3 0/7] remote-curl: fix deadlocks when remote server disconnects Jeff King
2020-05-19 21:14       ` Denton Liu
2020-05-19 20:51     ` [PATCH v3 8/7] fixup! pkt-line: extern packet_length() Denton Liu
2020-05-22 13:33     ` [PATCH v3 9/9] fixup! remote-curl: error on incomplete packet Denton Liu
2020-05-22 15:54       ` Jeff King
2020-05-22 16:05         ` Denton Liu
2020-05-22 16:31           ` Jeff King

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=cover.1589885479.git.liu.denton@gmail.com \
    --to=liu.denton@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=peff@peff.net \
    --cc=sunshine@sunshineco.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Git Mailing List Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/git/0 git/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 git git/ https://lore.kernel.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.git


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git