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>,
	Eric Sunshine <sunshine@sunshineco.com>
Subject: Re: [PATCH v2 5/7] remote-curl: error on incomplete packet
Date: Mon, 18 May 2020 12:22:25 -0400
Message-ID: <20200518162225.GB42240@coredump.intra.peff.net> (raw)
In-Reply-To: <52ce5fdffd6741eeee8d69b804403383da0d879d.1589816719.git.liu.denton@gmail.com>

On Mon, May 18, 2020 at 11:47:22AM -0400, Denton Liu wrote:

> +struct check_pktline_state {
> +	char len_buf[4];
> +	int len_filled;
> +	int remaining;
> +};
> +
> +static void check_pktline(struct check_pktline_state *state, const char *ptr, size_t size)

Thanks for converting this. I think having this broken out makes it a
bit easier to reason about, and it should be much easier to reuse if we
need it elsewhere.

> +{
> +	while (size) {
> +		if (!state->remaining) {
> +			int digits_remaining = 4 - state->len_filled;
> +			if (digits_remaining > size)
> +				digits_remaining = size;
> +			memcpy(&state->len_buf[state->len_filled], ptr, digits_remaining);
> +			state->len_filled += digits_remaining;
> +			ptr += digits_remaining;
> +			size -= digits_remaining;

Having personally written and screwed up this kind of parsing state
machine before, I read over this logic quite carefully. ;) I believe
it's correct.

Another way would be to loop by single characters:

  while (state->len_filled < 4 && size) {
          state->len_buf[state->len_filled++] = *ptr;
	  ptr++;
	  size--;
  }

which I _think_ is equivalent, and is a bit shorter. I'm OK with either
(see below, especially).

I'm not sure if it's worth replacing "4" with ARRAY_SIZE(state->len_buf).
I generally try to avoid magic numbers, but it's certainly not like one
could change the size of len_buf and this code would still be useful. :)

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

This part makes sense. We'll either leave len_filled as 1-3 (incomplete),
or we'll read a whole packet (for a flush), or we'll be waiting to read
the rest of the packet.

> +		if (state->remaining) {
> +			int remaining = state->remaining;
> +			if (remaining > size)
> +				remaining = size;
> +			ptr += remaining;
> +			size -= remaining;
> +			state->remaining -= remaining;
> +		}
> +	}
> +}

And here we most certainly don't want to read character-by-character,
because we're not doing anything with each one, and we expect there to be
a lot more of them. Having the earlier loop match the form of this one is
perhaps a good reason to leave it alone.

> [...]
> @@ -936,6 +984,11 @@ static int post_rpc(struct rpc_state *rpc, int flush_received)
>  	if (!rpc->any_written)
>  		err = -1;
>  
> +	if (rpc_in_data.pktline_state.len_filled)
> +		err = error(_("%d bytes of length header were received"), rpc_in_data.pktline_state.len_filled);
> +	if (rpc_in_data.pktline_state.remaining)
> +		err = error(_("%d bytes of body are still expected"), rpc_in_data.pktline_state.remaining);

And here's the payoff for all of the state machine checks. Makes sense.

> @@ -702,6 +746,8 @@ static size_t rpc_in(char *ptr, size_t eltsize,
>  		return size;
>  	if (size)
>  		data->rpc->any_written = 1;
> +	if (data->check_pktline)
> +		check_pktline(&data->pktline_state, ptr, size);
>  	write_or_die(data->rpc->in, ptr, size);
>  	return size;
>  }

And this is now conditional. Good...

> @@ -920,6 +966,8 @@ static int post_rpc(struct rpc_state *rpc, int flush_received)
>  	curl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, rpc_in);
>  	rpc_in_data.rpc = rpc;
>  	rpc_in_data.slot = slot;
> +	rpc_in_data.check_pktline = stateless_connect;
> +	memset(&rpc_in_data.pktline_state, 0, sizeof(rpc_in_data.pktline_state));

And we enable it only for stateless-connect. Makes perfect sense.

> --- /dev/null
> +++ b/t/lib-httpd/incomplete-body-upload-pack-v2-http.sh
> @@ -0,0 +1,3 @@
> +printf "Content-Type: text/%s\n" "application/x-git-upload-pack-result"
> +echo
> +printf "%s%s\n" "0079" "45"

Nice. Just having a deterministic half-written packet is way easier to
reason about than my "truncating proxy" suggestion.

> --- /dev/null
> +++ b/t/lib-httpd/incomplete-length-upload-pack-v2-http.sh
> @@ -0,0 +1,3 @@
> +printf "Content-Type: text/%s\n" "application/x-git-upload-pack-result"
> +echo
> +printf "%s\n" "00"

Thanks for covering this case, too.

I confirmed (as I'm sure you did) they both cause git-remote-http to hang
without the fix in this patch.

-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
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 [this message]
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=20200518162225.GB42240@coredump.intra.peff.net \
    --to=peff@peff.net \
    --cc=git@vger.kernel.org \
    --cc=liu.denton@gmail.com \
    --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