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@xxxxxxxx>
> ---
> 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
