Re: [PATCH 2/2] upload-pack: handle unexpected delim packets

[Taylor Blau <me@ttaylorr.com>]

On Fri, Mar 27, 2020 at 04:03:38AM -0400, Jeff King wrote:
> When processing the arguments list for a v2 ls-refs or fetch command, we
> loop like this:
>
>   while (packet_reader_read(request) != PACKET_READ_FLUSH) {
>           const char *arg = request->line;
> 	  ...handle arg...
>   }
>
> to read and handle packets until we see a flush. The hidden assumption
> here is that anything except PACKET_READ_FLUSH will give us valid packet
> data to read. But that's not true; PACKET_READ_DELIM or PACKET_READ_EOF
> will leave packet->line as NULL, and we'll segfault trying to look at
> it.
>
> Instead, we should follow the more careful model demonstrated on the
> client side (e.g., in process_capabilities_v2): keep looping as long
> as we get normal packets, and then make sure that we broke out of the
> loop due to a real flush. That fixes the segfault and correctly
> diagnoses any unexpected input from the client.

This is a very clean fix, thank you.

> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  ls-refs.c                      |  5 ++++-
>  t/t5704-protocol-violations.sh | 33 +++++++++++++++++++++++++++++++++
>  upload-pack.c                  |  5 ++++-
>  3 files changed, 41 insertions(+), 2 deletions(-)
>  create mode 100755 t/t5704-protocol-violations.sh
>
> diff --git a/ls-refs.c b/ls-refs.c
> index 818aef70a0..50d86866c6 100644
> --- a/ls-refs.c
> +++ b/ls-refs.c
> @@ -93,7 +93,7 @@ int ls_refs(struct repository *r, struct argv_array *keys,
>
>  	git_config(ls_refs_config, NULL);
>
> -	while (packet_reader_read(request) != PACKET_READ_FLUSH) {
> +	while (packet_reader_read(request) == PACKET_READ_NORMAL) {
>  		const char *arg = request->line;
>  		const char *out;
>
> @@ -105,6 +105,9 @@ int ls_refs(struct repository *r, struct argv_array *keys,
>  			argv_array_push(&data.prefixes, out);
>  	}
>
> +	if (request->status != PACKET_READ_FLUSH)
> +		die(_("expected flush after ls-refs arguments"));
> +

...and it's implemented faithfully here. Thanks.

>  	head_ref_namespaced(send_ref, &data);
>  	for_each_namespaced_ref(send_ref, &data);
>  	packet_flush(1);
> diff --git a/t/t5704-protocol-violations.sh b/t/t5704-protocol-violations.sh
> new file mode 100755
> index 0000000000..950cfb21fe
> --- /dev/null
> +++ b/t/t5704-protocol-violations.sh
> @@ -0,0 +1,33 @@
> +#!/bin/sh
> +
> +test_description='Test responses to violations of the network protocol. In most
> +of these cases it will generally be acceptable for one side to break off
> +communications if the other side says something unexpected. We are mostly
> +making sure that we do not segfault or otherwise behave badly.'
> +. ./test-lib.sh
> +
> +test_expect_success 'extra delim packet in v2 ls-refs args' '
> +	{
> +		packetize command=ls-refs &&
> +		printf 0001 &&
> +		# protocol expects 0000 flush here
> +		printf 0001
> +	} >input &&
> +	test_must_fail env GIT_PROTOCOL=version=2 \
> +		git upload-pack . <input 2>err &&
> +	test_i18ngrep "expected flush after ls-refs arguments" err
> +'
> +
> +test_expect_success 'extra delim packet in v2 fetch args' '
> +	{
> +		packetize command=fetch &&
> +		printf 0001 &&
> +		# protocol expects 0000 flush here
> +		printf 0001
> +	} >input &&
> +	test_must_fail env GIT_PROTOCOL=version=2 \
> +		git upload-pack . <input 2>err &&
> +	test_i18ngrep "expected flush after fetch arguments" err
> +'
>
> +test_done
> diff --git a/upload-pack.c b/upload-pack.c
> index c53249cac1..902d0ad5e1 100644
> --- a/upload-pack.c
> +++ b/upload-pack.c
> @@ -1252,7 +1252,7 @@ static void process_args(struct packet_reader *request,
>  			 struct upload_pack_data *data,
>  			 struct object_array *want_obj)
>  {
> -	while (packet_reader_read(request) != PACKET_READ_FLUSH) {
> +	while (packet_reader_read(request) == PACKET_READ_NORMAL) {
>  		const char *arg = request->line;
>  		const char *p;
>
> @@ -1321,6 +1321,9 @@ static void process_args(struct packet_reader *request,
>  		/* ignore unknown lines maybe? */
>  		die("unexpected line: '%s'", arg);
>  	}
> +
> +	if (request->status != PACKET_READ_FLUSH)
> +		die(_("expected flush after fetch arguments"));
>  }

...and here, too :).

>  static int process_haves(struct oid_array *haves, struct oid_array *common,
> --
> 2.26.0.581.g322f77c0ee

Thanks,
Taylor