Git Mailing List Archive on lore.kernel.org
 help / color / Atom feed
From: Jeff King <peff@peff.net>
To: Denton Liu <liu.denton@gmail.com>
Cc: Git Mailing List <git@vger.kernel.org>
Subject: Re: [PATCH 5/6] remote-curl: error on incomplete packet
Date: Fri, 15 May 2020 17:38:44 -0400
Message-ID: <20200515213844.GD115445@coredump.intra.peff.net> (raw)
In-Reply-To: <3ed7cf87aaa40ee66b20aa929d89d28fefcec312.1589393036.git.liu.denton@gmail.com>

On Wed, May 13, 2020 at 02:04:57PM -0400, Denton Liu wrote:

> Currently, remote-curl acts as a proxy and blindly forwards packets
> between an HTTP server and fetch-pack. In the case of a stateless RPC
> connection where the connection is terminated with a partially written
> packet, remote-curl will blindly send the partially written packet
> before waiting on more input from fetch-pack. Meanwhile, fetch-pack will
> read the partial packet and continue reading, expecting more input. This
> results in a deadlock between the two processes.
> 
> Instead of blindly forwarding packets, inspect each packet to ensure
> that it is a full packet, erroring out if a partial packet is sent.

Hmm. Right now there's no assumption in rpc_in that we're writing
pktlines. Will this always be the case?

I think the answer is probably yes. If there's a case where it isn't, it
might be something like v0 git-over-http against a server which doesn't
have the sideband capability.

> diff --git a/remote-curl.c b/remote-curl.c
> index da3e07184a..8b740354e5 100644
> --- a/remote-curl.c
> +++ b/remote-curl.c
> @@ -682,6 +682,8 @@ static curlioerr rpc_ioctl(CURL *handle, int cmd, void *clientp)
>  struct rpc_in_data {
>  	struct rpc_state *rpc;
>  	struct active_request_slot *slot;
> +	struct strbuf len_buf;
> +	int remaining;

This remaining is "remaining in the current packet", I assume? An "int"
should be OK for that.

Using a strbuf feels a bit like overkill, because we have to remember to
free it (which I think doesn't actually happen in your patch). Could we
just use a "char len_buf[4]" (you'd need an extra int to count how many
bytes you've received there).

> @@ -702,7 +705,42 @@ static size_t rpc_in(char *ptr, size_t eltsize,
>  		return size;
>  	if (size)
>  		data->rpc->any_written = 1;
> -	write_or_die(data->rpc->in, ptr, size);
> +
> +	while (unwritten) {
> +		if (!data->remaining) {
> +			int digits_remaining = 4 - data->len_buf.len;
> +			if (digits_remaining > unwritten)
> +				digits_remaining = unwritten;
> +			strbuf_add(&data->len_buf, ptr, digits_remaining);
> +			ptr += digits_remaining;
> +			unwritten -= digits_remaining;

So we actually save up the 4 bytes, not sending them at all until we get
them. I wonder if this might be easier to follow if our count was simply
advisory. I.e., continue to write data as we get it, but keep a small
state machine tracking pktlines (which could even be done in its own
separate struct/function pair).

I dunno. It might be about the same level of confusing, but it makes it
easy to keep the logic separate from rpc_in, and it lets us put all of
the policy bits at the end, after we know we've received all of the
data (in post_rpc):

  if (data->len_buf.len < 4)
          die("incomplete packet header");
  if (data->remaining)
          die("incomplete packet");
  /* imagine we kept the actual pktlen value, instead of just counting
   * down remaining */
  if (data->pktlen)
          die("did not end in flush");

Notably, I'm not sure if your code will complain if the connection dies
will reading the 4-byte header (remaining would still be 0). That won't
leave the caller trying to read the packet (we never sent them the
header), but they may still be waiting for a response.

> +			if (data->len_buf.len == 4) {
> +				data->remaining = packet_length(data->len_buf.buf);
> +				if (data->remaining < 0) {
> +					die(_("remote-curl: bad line length character: %.4s"), data->len_buf.buf);
> +				} else if (data->remaining <= 1) {
> +					data->remaining = 0;

This treats 0001 delimiters the same as a 0000 flush. Expecting 0 more
bytes is the right thing, but would we later want to differentiate in
post_rpc()? I.e., is it ever correct for the server data to end with a
delim?

> +				} else if (data->remaining < 4) {
> +					die(_("remote-curl: bad line length %d"), data->remaining);

We don't use an 0002 or 0003 packet for anything yet, but this would
need to be updated if we ever did. I wonder if we should treat them also
as zero-length packets and quietly pass through, which is likely the
right thing to do. OTOH, this should complain loudly enough that
somebody would presumably notice as soon as they tried to use those
packets.

Regarding testing, I think the ideal thing would be a proxy layer we
could insert that simply eats all data after N bytes. That would be easy
to do if we could use --upload-pack='git-upload-pack | head -c 500' or
something. But we need it to happen between curl and the server side of
Git. Possibly we could insert something between apache and
git-http-backend (simialr to how we handle broken-smart-http.sh.

-Peff

  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 [this message]
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   ` [PATCH v3 " Denton Liu
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=20200515213844.GD115445@coredump.intra.peff.net \
    --to=peff@peff.net \
    --cc=git@vger.kernel.org \
    --cc=liu.denton@gmail.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