upload-pack: handle unexpected delim packets

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.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/ls-refs.c b/ls-refs.c
index 818aef70a09e7cedc2f241f235302e69969b2e3b..50d86866c6eac5957951e5b685d7912fcf12eed7 100644 (file)
--- 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"));
+
        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 (executable)
index 0000000..950cfb2
--- /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 c53249cac19a33351f4f747782b71f877fc0692f..902d0ad5e157fde33ec013476086740946e285df 100644 (file)
--- 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"));
 }
 
 static int process_haves(struct oid_array *haves, struct oid_array *common,