From: Jeff King <peff@peff.net>
To: git@vger.kernel.org
Subject: [PATCH 2/2] upload-pack: handle unexpected delim packets
Date: Fri, 27 Mar 2020 04:03:38 -0400 [thread overview]
Message-ID: <20200327080338.GB605934@coredump.intra.peff.net> (raw)
In-Reply-To: <20200327080210.GA604725@coredump.intra.peff.net>
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>
---
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"));
+
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"));
}
static int process_haves(struct oid_array *haves, struct oid_array *common,
--
2.26.0.581.g322f77c0ee
next prev parent reply other threads:[~2020-03-27 8:03 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-03-27 8:02 [PATCH 0/2] upload-pack: handle unexpected v2 delim packets Jeff King
2020-03-27 8:03 ` [PATCH 1/2] test-lib-functions: make packetize() more efficient Jeff King
2020-03-27 15:16 ` Taylor Blau
2020-03-28 12:25 ` Jeff King
2020-03-27 19:18 ` Junio C Hamano
2020-03-28 11:20 ` Jeff King
2020-03-29 0:11 ` Junio C Hamano
2020-03-29 3:05 ` Junio C Hamano
2020-03-29 14:53 ` Jeff King
2020-03-29 15:44 ` Junio C Hamano
2020-03-29 14:52 ` Jeff King
2020-03-29 15:02 ` [PATCH] test-lib-functions: simplify packetize() stdin code Jeff King
2020-03-29 15:49 ` Junio C Hamano
2020-03-27 8:03 ` Jeff King [this message]
2020-03-27 15:17 ` [PATCH 2/2] upload-pack: handle unexpected delim packets Taylor Blau
2020-03-27 15:18 ` [PATCH 0/2] upload-pack: handle unexpected v2 " Taylor Blau
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=20200327080338.GB605934@coredump.intra.peff.net \
--to=peff@peff.net \
--cc=git@vger.kernel.org \
/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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).