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>
This commit is contained in:
Jeff King 2020-03-27 04:03:38 -04:00 committed by Junio C Hamano
parent 88124ab263
commit 4845b77245
3 changed files with 41 additions and 2 deletions

View File

@ -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);

33
t/t5704-protocol-violations.sh Executable file
View File

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

View File

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