{"thread":{"id":"53108","subject":"[PATCH 0/2] upload-pack: handle unexpected v2 delim packets","startedAt":"2020-03-27T08:02:14Z","lastAt":"2020-03-29T15:49:46Z","messageCount":16,"participants":["Jeff King","Taylor Blau","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"394153","messageId":"20200327080210.GA604725@coredump.intra.peff.net","threadId":"53108","inReplyTo":null,"subject":"[PATCH 0/2] upload-pack: handle unexpected v2 delim packets","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-03-27T08:02:10Z","receivedAt":"2020-03-27T08:02:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We saw an upload-pack segfault in the wild today at GitHub. It's caused\nby a client sending bogus v2 protocol lines (a \"delim\" packet instead of\na \"flush\"). So the client is broken and our only option is to break the\nnetwork connection, but we shouldn't segfault while doing so. :)\n\nI don't think the broken client was Git. It didn't send an \"agent\"\ncapability at all, which makes me suspect it was somebody generating the\nrequest manually (nor was there anything interesting in the transport\nlayer agent; it was just an openssh client).\n\nThe fix was simple enough, and is in the second patch. The first one is\njust a small cleanup / refactor in preparation.\n\n  [1/2]: test-lib-functions: make packetize() more efficient\n  [2/2]: upload-pack: handle unexpected delim packets\n\n ls-refs.c                              |  5 +++-\n t/t5562-http-backend-content-length.sh | 19 +++++++++------\n t/t5704-protocol-violations.sh         | 33 ++++++++++++++++++++++++++\n t/test-lib-functions.sh                | 23 ++++++++++++------\n upload-pack.c                          |  5 +++-\n 5 files changed, 69 insertions(+), 16 deletions(-)\n create mode 100755 t/t5704-protocol-violations.sh\n\n-Peff\n"},{"id":"394154","messageId":"20200327080300.GA605934@coredump.intra.peff.net","threadId":"53108","inReplyTo":"20200327080210.GA604725@coredump.intra.peff.net","subject":"[PATCH 1/2] test-lib-functions: make packetize() more efficient","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-03-27T08:03:00Z","receivedAt":"2020-03-27T08:03:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The packetize() function takes its input on stdin, and requires 4\nseparate sub-processes to format a simple string. We can do much better\nby getting the length via the shell's \"${#packet}\" construct. The one\ncaveat is that the shell can't put a NUL into a variable, so we'll have\nto continue to provide the stdin form for a few calls.\n\nThere are a few other cleanups here in the touched code:\n\n - the stdin form of packetize() had an extra stray \"%s\" when printing\n   the packet\n\n - the converted calls in t5562 can be made simpler by redirecting\n   output as a block, rather than repeated appending\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n t/t5562-http-backend-content-length.sh | 19 ++++++++++++-------\n t/test-lib-functions.sh                | 23 ++++++++++++++++-------\n 2 files changed, 28 insertions(+), 14 deletions(-)\n\ndiff --git a/t/t5562-http-backend-content-length.sh b/t/t5562-http-backend-content-length.sh\nindex 4a110b307e..3f4ac71f83 100755\n--- a/t/t5562-http-backend-content-length.sh\n+++ b/t/t5562-http-backend-content-length.sh\n@@ -53,15 +53,20 @@ test_expect_success 'setup' '\n \ttest_commit c1 &&\n \thash_head=$(git rev-parse HEAD) &&\n \thash_prev=$(git rev-parse HEAD~1) &&\n-\tprintf \"want %s\" \"$hash_head\" | packetize >fetch_body &&\n-\tprintf 0000 >>fetch_body &&\n-\tprintf \"have %s\" \"$hash_prev\" | packetize >>fetch_body &&\n-\tprintf done | packetize >>fetch_body &&\n+\t{\n+\t\tpacketize \"want $hash_head\" &&\n+\t\tprintf 0000 &&\n+\t\tpacketize \"have $hash_prev\" &&\n+\t\tpacketize \"done\"\n+\t} >fetch_body &&\n \ttest_copy_bytes 10 <fetch_body >fetch_body.trunc &&\n \thash_next=$(git commit-tree -p HEAD -m next HEAD^{tree}) &&\n-\tprintf \"%s %s refs/heads/newbranch\\\\0report-status\\\\n\" \"$ZERO_OID\" \"$hash_next\" | packetize >push_body &&\n-\tprintf 0000 >>push_body &&\n-\techo \"$hash_next\" | git pack-objects --stdout >>push_body &&\n+\t{\n+\t\tprintf \"%s %s refs/heads/newbranch\\\\0report-status\\\\n\" \\\n+\t\t\t\"$ZERO_OID\" \"$hash_next\" | packetize &&\n+\t\tprintf 0000 &&\n+\t\techo \"$hash_next\" | git pack-objects --stdout\n+\t} >push_body &&\n \ttest_copy_bytes 10 <push_body >push_body.trunc &&\n \t: >empty_body\n '\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex 352c213d52..216918a58c 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -1362,14 +1362,23 @@ nongit () {\n \t)\n } 7>&2 2>&4\n \n-# convert stdin to pktline representation; note that empty input becomes an\n-# empty packet, not a flush packet (for that you can just print 0000 yourself).\n+# convert function arguments or stdin (if not arguments given) to pktline\n+# representation. If multiple arguments are given, they are separated by\n+# whitespace and put in a single packet. Note that data containing NULs must be\n+# given on stdin, and that empty input becomes an empty packet, not a flush\n+# packet (for that you can just print 0000 yourself).\n packetize() {\n-\tcat >packetize.tmp &&\n-\tlen=$(wc -c <packetize.tmp) &&\n-\tprintf '%04x%s' \"$(($len + 4))\" &&\n-\tcat packetize.tmp &&\n-\trm -f packetize.tmp\n+\tif test $# -gt 0\n+\tthen\n+\t\tpacket=\"$*\"\n+\t\tprintf '%04x%s' \"$((4 + ${#packet}))\" \"$packet\"\n+\telse\n+\t\tcat >packetize.tmp &&\n+\t\tlen=$(wc -c <packetize.tmp) &&\n+\t\tprintf '%04x' \"$(($len + 4))\" &&\n+\t\tcat packetize.tmp &&\n+\t\trm -f packetize.tmp\n+\tfi\n }\n \n # Parse the input as a series of pktlines, writing the result to stdout.\n-- \n2.26.0.581.g322f77c0ee\n\n"},{"id":"394155","messageId":"20200327080338.GB605934@coredump.intra.peff.net","threadId":"53108","inReplyTo":"20200327080210.GA604725@coredump.intra.peff.net","subject":"[PATCH 2/2] upload-pack: handle unexpected delim packets","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-03-27T08:03:38Z","receivedAt":"2020-03-27T08:03:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When processing the arguments list for a v2 ls-refs or fetch command, we\nloop like this:\n\n  while (packet_reader_read(request) != PACKET_READ_FLUSH) {\n          const char *arg = request->line;\n\t  ...handle arg...\n  }\n\nto read and handle packets until we see a flush. The hidden assumption\nhere is that anything except PACKET_READ_FLUSH will give us valid packet\ndata to read. But that's not true; PACKET_READ_DELIM or PACKET_READ_EOF\nwill leave packet->line as NULL, and we'll segfault trying to look at\nit.\n\nInstead, we should follow the more careful model demonstrated on the\nclient side (e.g., in process_capabilities_v2): keep looping as long\nas we get normal packets, and then make sure that we broke out of the\nloop due to a real flush. That fixes the segfault and correctly\ndiagnoses any unexpected input from the client.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n ls-refs.c                      |  5 ++++-\n t/t5704-protocol-violations.sh | 33 +++++++++++++++++++++++++++++++++\n upload-pack.c                  |  5 ++++-\n 3 files changed, 41 insertions(+), 2 deletions(-)\n create mode 100755 t/t5704-protocol-violations.sh\n\ndiff --git a/ls-refs.c b/ls-refs.c\nindex 818aef70a0..50d86866c6 100644\n--- a/ls-refs.c\n+++ b/ls-refs.c\n@@ -93,7 +93,7 @@ int ls_refs(struct repository *r, struct argv_array *keys,\n \n \tgit_config(ls_refs_config, NULL);\n \n-\twhile (packet_reader_read(request) != PACKET_READ_FLUSH) {\n+\twhile (packet_reader_read(request) == PACKET_READ_NORMAL) {\n \t\tconst char *arg = request->line;\n \t\tconst char *out;\n \n@@ -105,6 +105,9 @@ int ls_refs(struct repository *r, struct argv_array *keys,\n \t\t\targv_array_push(&data.prefixes, out);\n \t}\n \n+\tif (request->status != PACKET_READ_FLUSH)\n+\t\tdie(_(\"expected flush after ls-refs arguments\"));\n+\n \thead_ref_namespaced(send_ref, &data);\n \tfor_each_namespaced_ref(send_ref, &data);\n \tpacket_flush(1);\ndiff --git a/t/t5704-protocol-violations.sh b/t/t5704-protocol-violations.sh\nnew file mode 100755\nindex 0000000000..950cfb21fe\n--- /dev/null\n+++ b/t/t5704-protocol-violations.sh\n@@ -0,0 +1,33 @@\n+#!/bin/sh\n+\n+test_description='Test responses to violations of the network protocol. In most\n+of these cases it will generally be acceptable for one side to break off\n+communications if the other side says something unexpected. We are mostly\n+making sure that we do not segfault or otherwise behave badly.'\n+. ./test-lib.sh\n+\n+test_expect_success 'extra delim packet in v2 ls-refs args' '\n+\t{\n+\t\tpacketize command=ls-refs &&\n+\t\tprintf 0001 &&\n+\t\t# protocol expects 0000 flush here\n+\t\tprintf 0001\n+\t} >input &&\n+\ttest_must_fail env GIT_PROTOCOL=version=2 \\\n+\t\tgit upload-pack . <input 2>err &&\n+\ttest_i18ngrep \"expected flush after ls-refs arguments\" err\n+'\n+\n+test_expect_success 'extra delim packet in v2 fetch args' '\n+\t{\n+\t\tpacketize command=fetch &&\n+\t\tprintf 0001 &&\n+\t\t# protocol expects 0000 flush here\n+\t\tprintf 0001\n+\t} >input &&\n+\ttest_must_fail env GIT_PROTOCOL=version=2 \\\n+\t\tgit upload-pack . <input 2>err &&\n+\ttest_i18ngrep \"expected flush after fetch arguments\" err\n+'\n+\n+test_done\ndiff --git a/upload-pack.c b/upload-pack.c\nindex c53249cac1..902d0ad5e1 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -1252,7 +1252,7 @@ static void process_args(struct packet_reader *request,\n \t\t\t struct upload_pack_data *data,\n \t\t\t struct object_array *want_obj)\n {\n-\twhile (packet_reader_read(request) != PACKET_READ_FLUSH) {\n+\twhile (packet_reader_read(request) == PACKET_READ_NORMAL) {\n \t\tconst char *arg = request->line;\n \t\tconst char *p;\n \n@@ -1321,6 +1321,9 @@ static void process_args(struct packet_reader *request,\n \t\t/* ignore unknown lines maybe? */\n \t\tdie(\"unexpected line: '%s'\", arg);\n \t}\n+\n+\tif (request->status != PACKET_READ_FLUSH)\n+\t\tdie(_(\"expected flush after fetch arguments\"));\n }\n \n static int process_haves(struct oid_array *haves, struct oid_array *common,\n-- \n2.26.0.581.g322f77c0ee\n"},{"id":"394172","messageId":"20200327151607.GA30419@syl.local","threadId":"53108","inReplyTo":"20200327080300.GA605934@coredump.intra.peff.net","subject":"Re: [PATCH 1/2] test-lib-functions: make packetize() more efficient","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-03-27T15:16:07Z","receivedAt":"2020-03-27T15:16:12Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, Mar 27, 2020 at 04:03:00AM -0400, Jeff King wrote:\n> The packetize() function takes its input on stdin, and requires 4\n> separate sub-processes to format a simple string. We can do much better\n> by getting the length via the shell's \"${#packet}\" construct. The one\n> caveat is that the shell can't put a NUL into a variable, so we'll have\n> to continue to provide the stdin form for a few calls.\n>\n> There are a few other cleanups here in the touched code:\n>\n>  - the stdin form of packetize() had an extra stray \"%s\" when printing\n>    the packet\n>\n>  - the converted calls in t5562 can be made simpler by redirecting\n>    output as a block, rather than repeated appending\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>  t/t5562-http-backend-content-length.sh | 19 ++++++++++++-------\n>  t/test-lib-functions.sh                | 23 ++++++++++++++++-------\n>  2 files changed, 28 insertions(+), 14 deletions(-)\n>\n> diff --git a/t/t5562-http-backend-content-length.sh b/t/t5562-http-backend-content-length.sh\n> index 4a110b307e..3f4ac71f83 100755\n> --- a/t/t5562-http-backend-content-length.sh\n> +++ b/t/t5562-http-backend-content-length.sh\n> @@ -53,15 +53,20 @@ test_expect_success 'setup' '\n>  \ttest_commit c1 &&\n>  \thash_head=$(git rev-parse HEAD) &&\n>  \thash_prev=$(git rev-parse HEAD~1) &&\n> -\tprintf \"want %s\" \"$hash_head\" | packetize >fetch_body &&\n> -\tprintf 0000 >>fetch_body &&\n> -\tprintf \"have %s\" \"$hash_prev\" | packetize >>fetch_body &&\n> -\tprintf done | packetize >>fetch_body &&\n> +\t{\n> +\t\tpacketize \"want $hash_head\" &&\n> +\t\tprintf 0000 &&\n> +\t\tpacketize \"have $hash_prev\" &&\n> +\t\tpacketize \"done\"\n> +\t} >fetch_body &&\n\nNicely done, this is an obvious improvement in clarity over the\nappending '>>' that was here before. The new version reads mouch more\ncleanly.\n\n>  \ttest_copy_bytes 10 <fetch_body >fetch_body.trunc &&\n>  \thash_next=$(git commit-tree -p HEAD -m next HEAD^{tree}) &&\n> -\tprintf \"%s %s refs/heads/newbranch\\\\0report-status\\\\n\" \"$ZERO_OID\" \"$hash_next\" | packetize >push_body &&\n> -\tprintf 0000 >>push_body &&\n> -\techo \"$hash_next\" | git pack-objects --stdout >>push_body &&\n> +\t{\n> +\t\tprintf \"%s %s refs/heads/newbranch\\\\0report-status\\\\n\" \\\n> +\t\t\t\"$ZERO_OID\" \"$hash_next\" | packetize &&\n> +\t\tprintf 0000 &&\n> +\t\techo \"$hash_next\" | git pack-objects --stdout\n> +\t} >push_body &&\n>  \ttest_copy_bytes 10 <push_body >push_body.trunc &&\n>  \t: >empty_body\n>  '\n> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\n> index 352c213d52..216918a58c 100644\n> --- a/t/test-lib-functions.sh\n> +++ b/t/test-lib-functions.sh\n> @@ -1362,14 +1362,23 @@ nongit () {\n>  \t)\n>  } 7>&2 2>&4\n>\n> -# convert stdin to pktline representation; note that empty input becomes an\n> -# empty packet, not a flush packet (for that you can just print 0000 yourself).\n> +# convert function arguments or stdin (if not arguments given) to pktline\n> +# representation. If multiple arguments are given, they are separated by\n> +# whitespace and put in a single packet. Note that data containing NULs must be\n> +# given on stdin, and that empty input becomes an empty packet, not a flush\n> +# packet (for that you can just print 0000 yourself).\n>  packetize() {\n> -\tcat >packetize.tmp &&\n> -\tlen=$(wc -c <packetize.tmp) &&\n> -\tprintf '%04x%s' \"$(($len + 4))\" &&\n> -\tcat packetize.tmp &&\n> -\trm -f packetize.tmp\n> +\tif test $# -gt 0\n> +\tthen\n> +\t\tpacket=\"$*\"\n\nMentioned off-list in a discussion already, but: though I find this\nbehavior of joining multiple arguments by a whitespace character a\nlittle confusing (i.e., what would callers thing this function does if\nthey hadn't read the documentation?) I think that this is probably the\nmost sane thing that you could do here.\n\nOn the other hand, nowhere in this patch do we currently call packetize\nwith multiple arguments, so perhaps it would be made simpler if we\ninstead wrote \"$1\" anywhere there was \"$packet\".\n\n> +\t\tprintf '%04x%s' \"$((4 + ${#packet}))\" \"$packet\"\n> +\telse\n> +\t\tcat >packetize.tmp &&\n> +\t\tlen=$(wc -c <packetize.tmp) &&\n> +\t\tprintf '%04x' \"$(($len + 4))\" &&\n> +\t\tcat packetize.tmp &&\n> +\t\trm -f packetize.tmp\n> +\tfi\n>  }\n\nRight: if the contents contains an unrepresentable byte, then it has to\nbe passed over stdin. This part is obviously correct.\n\n>  # Parse the input as a series of pktlines, writing the result to stdout.\n> --\n> 2.26.0.581.g322f77c0ee\n\nThanks,\nTaylor\n"},{"id":"394173","messageId":"20200327151739.GB30419@syl.local","threadId":"53108","inReplyTo":"20200327080338.GB605934@coredump.intra.peff.net","subject":"Re: [PATCH 2/2] upload-pack: handle unexpected delim packets","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-03-27T15:17:39Z","receivedAt":"2020-03-27T15:17:46Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, Mar 27, 2020 at 04:03:38AM -0400, Jeff King wrote:\n> When processing the arguments list for a v2 ls-refs or fetch command, we\n> loop like this:\n>\n>   while (packet_reader_read(request) != PACKET_READ_FLUSH) {\n>           const char *arg = request->line;\n> \t  ...handle arg...\n>   }\n>\n> to read and handle packets until we see a flush. The hidden assumption\n> here is that anything except PACKET_READ_FLUSH will give us valid packet\n> data to read. But that's not true; PACKET_READ_DELIM or PACKET_READ_EOF\n> will leave packet->line as NULL, and we'll segfault trying to look at\n> it.\n>\n> Instead, we should follow the more careful model demonstrated on the\n> client side (e.g., in process_capabilities_v2): keep looping as long\n> as we get normal packets, and then make sure that we broke out of the\n> loop due to a real flush. That fixes the segfault and correctly\n> diagnoses any unexpected input from the client.\n\nThis is a very clean fix, thank you.\n\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>  ls-refs.c                      |  5 ++++-\n>  t/t5704-protocol-violations.sh | 33 +++++++++++++++++++++++++++++++++\n>  upload-pack.c                  |  5 ++++-\n>  3 files changed, 41 insertions(+), 2 deletions(-)\n>  create mode 100755 t/t5704-protocol-violations.sh\n>\n> diff --git a/ls-refs.c b/ls-refs.c\n> index 818aef70a0..50d86866c6 100644\n> --- a/ls-refs.c\n> +++ b/ls-refs.c\n> @@ -93,7 +93,7 @@ int ls_refs(struct repository *r, struct argv_array *keys,\n>\n>  \tgit_config(ls_refs_config, NULL);\n>\n> -\twhile (packet_reader_read(request) != PACKET_READ_FLUSH) {\n> +\twhile (packet_reader_read(request) == PACKET_READ_NORMAL) {\n>  \t\tconst char *arg = request->line;\n>  \t\tconst char *out;\n>\n> @@ -105,6 +105,9 @@ int ls_refs(struct repository *r, struct argv_array *keys,\n>  \t\t\targv_array_push(&data.prefixes, out);\n>  \t}\n>\n> +\tif (request->status != PACKET_READ_FLUSH)\n> +\t\tdie(_(\"expected flush after ls-refs arguments\"));\n> +\n\n...and it's implemented faithfully here. Thanks.\n\n>  \thead_ref_namespaced(send_ref, &data);\n>  \tfor_each_namespaced_ref(send_ref, &data);\n>  \tpacket_flush(1);\n> diff --git a/t/t5704-protocol-violations.sh b/t/t5704-protocol-violations.sh\n> new file mode 100755\n> index 0000000000..950cfb21fe\n> --- /dev/null\n> +++ b/t/t5704-protocol-violations.sh\n> @@ -0,0 +1,33 @@\n> +#!/bin/sh\n> +\n> +test_description='Test responses to violations of the network protocol. In most\n> +of these cases it will generally be acceptable for one side to break off\n> +communications if the other side says something unexpected. We are mostly\n> +making sure that we do not segfault or otherwise behave badly.'\n> +. ./test-lib.sh\n> +\n> +test_expect_success 'extra delim packet in v2 ls-refs args' '\n> +\t{\n> +\t\tpacketize command=ls-refs &&\n> +\t\tprintf 0001 &&\n> +\t\t# protocol expects 0000 flush here\n> +\t\tprintf 0001\n> +\t} >input &&\n> +\ttest_must_fail env GIT_PROTOCOL=version=2 \\\n> +\t\tgit upload-pack . <input 2>err &&\n> +\ttest_i18ngrep \"expected flush after ls-refs arguments\" err\n> +'\n> +\n> +test_expect_success 'extra delim packet in v2 fetch args' '\n> +\t{\n> +\t\tpacketize command=fetch &&\n> +\t\tprintf 0001 &&\n> +\t\t# protocol expects 0000 flush here\n> +\t\tprintf 0001\n> +\t} >input &&\n> +\ttest_must_fail env GIT_PROTOCOL=version=2 \\\n> +\t\tgit upload-pack . <input 2>err &&\n> +\ttest_i18ngrep \"expected flush after fetch arguments\" err\n> +'\n>\n> +test_done\n> diff --git a/upload-pack.c b/upload-pack.c\n> index c53249cac1..902d0ad5e1 100644\n> --- a/upload-pack.c\n> +++ b/upload-pack.c\n> @@ -1252,7 +1252,7 @@ static void process_args(struct packet_reader *request,\n>  \t\t\t struct upload_pack_data *data,\n>  \t\t\t struct object_array *want_obj)\n>  {\n> -\twhile (packet_reader_read(request) != PACKET_READ_FLUSH) {\n> +\twhile (packet_reader_read(request) == PACKET_READ_NORMAL) {\n>  \t\tconst char *arg = request->line;\n>  \t\tconst char *p;\n>\n> @@ -1321,6 +1321,9 @@ static void process_args(struct packet_reader *request,\n>  \t\t/* ignore unknown lines maybe? */\n>  \t\tdie(\"unexpected line: '%s'\", arg);\n>  \t}\n> +\n> +\tif (request->status != PACKET_READ_FLUSH)\n> +\t\tdie(_(\"expected flush after fetch arguments\"));\n>  }\n\n...and here, too :).\n\n>  static int process_haves(struct oid_array *haves, struct oid_array *common,\n> --\n> 2.26.0.581.g322f77c0ee\n\nThanks,\nTaylor\n"},{"id":"394174","messageId":"20200327151829.GA30577@syl.local","threadId":"53108","inReplyTo":"20200327080210.GA604725@coredump.intra.peff.net","subject":"Re: [PATCH 0/2] upload-pack: handle unexpected v2 delim packets","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-03-27T15:18:29Z","receivedAt":"2020-03-27T15:18:34Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Hi Peff,\n\nOn Fri, Mar 27, 2020 at 04:02:10AM -0400, Jeff King wrote:\n> We saw an upload-pack segfault in the wild today at GitHub. It's caused\n> by a client sending bogus v2 protocol lines (a \"delim\" packet instead of\n> a \"flush\"). So the client is broken and our only option is to break the\n> network connection, but we shouldn't segfault while doing so. :)\n>\n> I don't think the broken client was Git. It didn't send an \"agent\"\n> capability at all, which makes me suspect it was somebody generating the\n> request manually (nor was there anything interesting in the transport\n> layer agent; it was just an openssh client).\n>\n> The fix was simple enough, and is in the second patch. The first one is\n> just a small cleanup / refactor in preparation.\n>\n>   [1/2]: test-lib-functions: make packetize() more efficient\n>   [2/2]: upload-pack: handle unexpected delim packets\n>\n>  ls-refs.c                              |  5 +++-\n>  t/t5562-http-backend-content-length.sh | 19 +++++++++------\n>  t/t5704-protocol-violations.sh         | 33 ++++++++++++++++++++++++++\n>  t/test-lib-functions.sh                | 23 ++++++++++++------\n>  upload-pack.c                          |  5 +++-\n>  5 files changed, 69 insertions(+), 16 deletions(-)\n>  create mode 100755 t/t5704-protocol-violations.sh\n\nThanks. This series looks good to me, and is certainly improving things.\n\n  Reviewed-by: Taylor Blau <me@ttaylorr.com>\n\n> -Peff\n\nThanks,\nTaylor\n"},{"id":"394191","messageId":"xmqq1rpdhaid.fsf@gitster.c.googlers.com","threadId":"53108","inReplyTo":"20200327080300.GA605934@coredump.intra.peff.net","subject":"Re: [PATCH 1/2] test-lib-functions: make packetize() more efficient","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-03-27T19:18:34Z","receivedAt":"2020-03-27T19:18:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> The packetize() function takes its input on stdin, and requires 4\n> separate sub-processes to format a simple string. We can do much better\n> by getting the length via the shell's \"${#packet}\" construct. The one\n> caveat is that the shell can't put a NUL into a variable, so we'll have\n> to continue to provide the stdin form for a few calls.\n\nYuck.  Binary protocol and shell variables do not mix well.\n\nDocumentation/CodingGuidelines forbids ${#parameter} in the first\nplace and we seem to use it only when we know we are using bash.\n\nPerhaps we should start considering to lift it.  I dunno.\n\n> +# convert function arguments or stdin (if not arguments given) to pktline\n> +# representation. If multiple arguments are given, they are separated by\n> +# whitespace and put in a single packet. Note that data containing NULs must be\n> +# given on stdin, and that empty input becomes an empty packet, not a flush\n> +# packet (for that you can just print 0000 yourself).\n\n>  packetize() {\n> +\tif test $# -gt 0\n> +\tthen\n> +\t\tpacket=\"$*\"\n> +\t\tprintf '%04x%s' \"$((4 + ${#packet}))\" \"$packet\"\n\nThis allows \n\n\t\tpacketize \"want $hash_head\"\n\nto be written like so:\n\n\t\tpacketize want \"$hash_head\"\n\nwhich maybe is a handy thing to do.\n\n> +\telse\n> +\t\tcat >packetize.tmp &&\n> +\t\tlen=$(wc -c <packetize.tmp) &&\n> +\t\tprintf '%04x' \"$(($len + 4))\" &&\n> +\t\tcat packetize.tmp &&\n> +\t\trm -f packetize.tmp\n\n\tperl -e '\n\t\tmy $data = do { local $?; <STDIN> };\n                printf \"%04x%s\", length($data), $data;\n\t'\n\nThat's one process but much heavier than cat/wc/printf/cat, I guess.\n\n> +\tfi\n>  }\n>  \n>  # Parse the input as a series of pktlines, writing the result to stdout.\n"},{"id":"394218","messageId":"20200328112010.GC639140@coredump.intra.peff.net","threadId":"53108","inReplyTo":"xmqq1rpdhaid.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 1/2] test-lib-functions: make packetize() more efficient","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-03-28T11:20:10Z","receivedAt":"2020-03-28T11:20:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 27, 2020 at 12:18:34PM -0700, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > The packetize() function takes its input on stdin, and requires 4\n> > separate sub-processes to format a simple string. We can do much better\n> > by getting the length via the shell's \"${#packet}\" construct. The one\n> > caveat is that the shell can't put a NUL into a variable, so we'll have\n> > to continue to provide the stdin form for a few calls.\n> \n> Yuck.  Binary protocol and shell variables do not mix well.\n> \n> Documentation/CodingGuidelines forbids ${#parameter} in the first\n> place and we seem to use it only when we know we are using bash.\n> \n> Perhaps we should start considering to lift it.  I dunno.\n\nHmph. I had a vague recollection there but checked beforehand:\n\n - we do use it in t9010, which is /bin/sh (and have since 2010). I\n   thought it might not be run on obscure platforms because it's\n   svn-related, but I think it doesn't actually require svn.\n\n - it's in POSIX, at least as far back as 2004 (I couldn't find an easy\n   copy of the 2001 version). That doesn't prove there aren't\n   problematic systems, of course, but it at least passes the bar of\n   \"not even in POSIX\".\n\n - it's not in check-non-portable-shell.pl. :) That doesn't mean\n   CodingGuidelines is wrong, but we should probably reconcile them.\n\nGiven that the first point means we've had a 10-year weather balloon,\nand that the original rule seems to have come from a big list of\nrules[1] rather than a specific known problem shell, I think we should\ndeclare it available.\n\n[1] https://lore.kernel.org/git/7vtznzf5jb.fsf@gitster.siamese.dyndns.org/\n\n> >  packetize() {\n> > +\tif test $# -gt 0\n> > +\tthen\n> > +\t\tpacket=\"$*\"\n> > +\t\tprintf '%04x%s' \"$((4 + ${#packet}))\" \"$packet\"\n> \n> This allows \n> \n> \t\tpacketize \"want $hash_head\"\n> \n> to be written like so:\n> \n> \t\tpacketize want \"$hash_head\"\n> \n> which maybe is a handy thing to do.\n\nYeah. I admit I don't care overly much between that and doing something\nelse sensible with multiple arguments (like putting each one in its own\npacket). I just really didn't want to silently ignore anything after\n\"$1\". This at least behaves like \"echo\".\n\n> > +\telse\n> > +\t\tcat >packetize.tmp &&\n> > +\t\tlen=$(wc -c <packetize.tmp) &&\n> > +\t\tprintf '%04x' \"$(($len + 4))\" &&\n> > +\t\tcat packetize.tmp &&\n> > +\t\trm -f packetize.tmp\n> \n> \tperl -e '\n> \t\tmy $data = do { local $?; <STDIN> };\n>                 printf \"%04x%s\", length($data), $data;\n> \t'\n> \n> That's one process but much heavier than cat/wc/printf/cat, I guess.\n\nYeah, I was tempted to do that, but ${#packet} is even one process\nshorter. :) It might be worth simplifying the stdin path above, but it's\nmuch less important if most of those call-sites go away.\n\n-Peff\n"},{"id":"394222","messageId":"20200328122534.GA642919@coredump.intra.peff.net","threadId":"53108","inReplyTo":"20200327151607.GA30419@syl.local","subject":"Re: [PATCH 1/2] test-lib-functions: make packetize() more efficient","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-03-28T12:25:34Z","receivedAt":"2020-03-28T12:25:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 27, 2020 at 09:16:07AM -0600, Taylor Blau wrote:\n\n> >  packetize() {\n> > -\tcat >packetize.tmp &&\n> > -\tlen=$(wc -c <packetize.tmp) &&\n> > -\tprintf '%04x%s' \"$(($len + 4))\" &&\n> > -\tcat packetize.tmp &&\n> > -\trm -f packetize.tmp\n> > +\tif test $# -gt 0\n> > +\tthen\n> > +\t\tpacket=\"$*\"\n> \n> Mentioned off-list in a discussion already, but: though I find this\n> behavior of joining multiple arguments by a whitespace character a\n> little confusing (i.e., what would callers thing this function does if\n> they hadn't read the documentation?) I think that this is probably the\n> most sane thing that you could do here.\n> \n> On the other hand, nowhere in this patch do we currently call packetize\n> with multiple arguments, so perhaps it would be made simpler if we\n> instead wrote \"$1\" anywhere there was \"$packet\".\n\nOf all the options, I like that the least because somebody doing:\n\n  packetize foo bar\n\nwould have the \"bar\" silently ignored. And it's more lines of code to\ncheck and complain about that than it is to just do something sensible.\n\n-Peff\n"},{"id":"394257","messageId":"xmqqh7y8c94g.fsf@gitster.c.googlers.com","threadId":"53108","inReplyTo":"20200328112010.GC639140@coredump.intra.peff.net","subject":"Re: [PATCH 1/2] test-lib-functions: make packetize() more efficient","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-03-29T00:11:59Z","receivedAt":"2020-03-29T00:12:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n>> Documentation/CodingGuidelines forbids ${#parameter} in the first\n>> place and we seem to use it only when we know we are using bash.\n>> \n>> Perhaps we should start considering to lift it.  I dunno.\n>\n> Hmph. I had a vague recollection there but checked beforehand:\n>\n>  - we do use it in t9010, which is /bin/sh (and have since 2010). I\n>    thought it might not be run on obscure platforms because it's\n>    svn-related, but I think it doesn't actually require svn.\n\nI do not think I ran git-svn stuff myself for the past 10 years,\nthough, after a few projects that matter to me migrated away from\nsvn ;-)\n\n>  - it's in POSIX, at least as far back as 2004 (I couldn't find an easy\n>    copy of the 2001 version). That doesn't prove there aren't\n>    problematic systems, of course, but it at least passes the bar of\n>    \"not even in POSIX\".\n\nYeah, IIRC the list was written in response to a request for _some_\nguidance, so it largely came from in-house rules of my previous\nlife, back when I had to deal with various flavours of UNIXen.\n\n>  - it's not in check-non-portable-shell.pl. :) That doesn't mean\n>    CodingGuidelines is wrong, but we should probably reconcile them.\n\nThat checker came much much later than the guidelines so it is not\nsurprising at all for it to be \"buggy\", in the sense that it does\nnot check everything the guidelines ask.  Yes, we may need bugfixes\nand there may be other bugs, too.\n\n> Yeah, I was tempted to do that, but ${#packet} is even one process\n> shorter. :) It might be worth simplifying the stdin path above, but it's\n> much less important if most of those call-sites go away.\n\nThe largest issue (which is not that large, though) I felt with the\napproach when I saw the patch for the first time was that we need\nthe big warning comment before the helper, i.e.\n\n> +# Note that data containing NULs must be given on stdin,...\n\nBut after thinking about it a bit more, I think it is probably OK.\nI do not think you can assign a string with NUL in it to a variable\nand you can use \"$(command substitution)\" as an argument to the\nhelper to pass such a string, either (not portably anyway).  If such\na string cannot be made into \"$*\", ${#packet} won't have to worry\nabout counting bytes in a string with NUL in it to begin with, so\nthe above note won't even be necessary (it would have to say \"you\ncannot pass data containing NULs as arguments---you are welcome to\ntry, but you won't be able to do so, not portably anyway\"), anyway\n;-).\n"},{"id":"394260","messageId":"xmqq8sjjdfo9.fsf@gitster.c.googlers.com","threadId":"53108","inReplyTo":"xmqqh7y8c94g.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 1/2] test-lib-functions: make packetize() more efficient","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-03-29T03:05:10Z","receivedAt":"2020-03-29T03:05:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\nI guess I didn't give an explicit conclusion in my message.\n\n>>  - it's in POSIX, at least as far back as 2004 (I couldn't find an easy\n>>    copy of the 2001 version). That doesn't prove there aren't\n>>    problematic systems, of course, but it at least passes the bar of\n>>    \"not even in POSIX\".\n>\n> Yeah, IIRC the list was written in response to a request for _some_\n> guidance, so it largely came from in-house rules of my previous\n> life, back when I had to deal with various flavours of UNIXen.\n\nI strongly suspect that most of these historical curiosity systems\ndied out or learned ${#posix}; I wouldn't at all be surprised if\n/bin/sh on Solaris back then was one of the motivating systems that\nled to the forbidding of the use of ${#parameter} in our in-house\nrules, but luckily, we have written it off as unsalvageable in this\nproject ;-)\n\nIt has been in POSIX long enough, and it is useful at times, so\nlet's drop it from the list of guidelines (patch?).\n\n>>  - it's not in check-non-portable-shell.pl. :) That doesn't mean\n>>    CodingGuidelines is wrong, but we should probably reconcile them.\n>\n> That checker came much much later than the guidelines so it is not\n> surprising at all for it to be \"buggy\", in the sense that it does\n> not check everything the guidelines ask.  Yes, we may need bugfixes\n> and there may be other bugs, too.\n\nThis still gives us something to keep an eye on.\n\n-- >8 --\nSubject: CodingGuidelines: allow ${#posix} == strlen($posix)\n\nThe construct has been in POSIX for the past 10+ years, and we have\nused in t9xxx (subversion) series of the tests, so we know it is at\nleast portable across systems that subversion Perl bindings have\nbeen ported to.\n\nLet's loosen the rule; luckily, the check-non-portable-shell script\ndoes not have any rule to find its use, so the only change needed is\na removal of one paragraph from the documentation.\n\nHelped-by: Jeff King <peff@peff.net>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n\n Documentation/CodingGuidelines | 2 --\n 1 file changed, 2 deletions(-)\n\ndiff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines\nindex ed4e443a3c..390ceece52 100644\n--- a/Documentation/CodingGuidelines\n+++ b/Documentation/CodingGuidelines\n@@ -91,8 +91,6 @@ For shell scripts specifically (not exhaustive):\n \n    - No shell arrays.\n \n-   - No strlen ${#parameter}.\n-\n    - No pattern replacement ${parameter/pattern/string}.\n \n  - We use Arithmetic Expansion $(( ... )).\n"},{"id":"394284","messageId":"20200329145224.GA15400@coredump.intra.peff.net","threadId":"53108","inReplyTo":"xmqqh7y8c94g.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 1/2] test-lib-functions: make packetize() more efficient","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-03-29T14:52:24Z","receivedAt":"2020-03-29T14:52:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Mar 28, 2020 at 05:11:59PM -0700, Junio C Hamano wrote:\n\n> >  - we do use it in t9010, which is /bin/sh (and have since 2010). I\n> >    thought it might not be run on obscure platforms because it's\n> >    svn-related, but I think it doesn't actually require svn.\n> \n> I do not think I ran git-svn stuff myself for the past 10 years,\n> though, after a few projects that matter to me migrated away from\n> svn ;-)\n\nMe either. I don't even run the git-svn tests, usually. t9010 is about\nthe svn fast-importer, though. We've all been running its tests for\nyears, even though AFAIK it's not really used for anything.\n\n> The largest issue (which is not that large, though) I felt with the\n> approach when I saw the patch for the first time was that we need\n> the big warning comment before the helper, i.e.\n> \n> > +# Note that data containing NULs must be given on stdin,...\n> \n> But after thinking about it a bit more, I think it is probably OK.\n> I do not think you can assign a string with NUL in it to a variable\n> and you can use \"$(command substitution)\" as an argument to the\n> helper to pass such a string, either (not portably anyway).  If such\n> a string cannot be made into \"$*\", ${#packet} won't have to worry\n> about counting bytes in a string with NUL in it to begin with, so\n> the above note won't even be necessary (it would have to say \"you\n> cannot pass data containing NULs as arguments---you are welcome to\n> try, but you won't be able to do so, not portably anyway\"), anyway\n> ;-).\n\nYes. You could probably screw yourself with:\n\n  packetize \"$(printf \"0020git-upload-pack\\0host=example.com\")\"\n\nbut presumably you'd quickly notice when your test fails (both dash and\nbash just drop the NUL byte entirely, and bash even issues a warning).\n\n-Peff\n"},{"id":"394285","messageId":"20200329145343.GB15400@coredump.intra.peff.net","threadId":"53108","inReplyTo":"xmqq8sjjdfo9.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 1/2] test-lib-functions: make packetize() more efficient","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-03-29T14:53:43Z","receivedAt":"2020-03-29T14:53:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Mar 28, 2020 at 08:05:10PM -0700, Junio C Hamano wrote:\n\n> -- >8 --\n> Subject: CodingGuidelines: allow ${#posix} == strlen($posix)\n\nI'm in favor of this patch, but...\n\n> The construct has been in POSIX for the past 10+ years, and we have\n> used in t9xxx (subversion) series of the tests, so we know it is at\n> least portable across systems that subversion Perl bindings have\n> been ported to.\n\nIt is even stronger than that. t9010 is run for everybody (with the\nexception of the last test, which needs svnadmin).\n\n-Peff\n"},{"id":"394286","messageId":"20200329150226.GA16068@coredump.intra.peff.net","threadId":"53108","inReplyTo":"20200328112010.GC639140@coredump.intra.peff.net","subject":"[PATCH] test-lib-functions: simplify packetize() stdin code","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-03-29T15:02:26Z","receivedAt":"2020-03-29T15:02:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Mar 28, 2020 at 07:20:10AM -0400, Jeff King wrote:\n\n> > \tperl -e '\n> > \t\tmy $data = do { local $?; <STDIN> };\n> >                 printf \"%04x%s\", length($data), $data;\n> > \t'\n> > \n> > That's one process but much heavier than cat/wc/printf/cat, I guess.\n> \n> Yeah, I was tempted to do that, but ${#packet} is even one process\n> shorter. :) It might be worth simplifying the stdin path above, but it's\n> much less important if most of those call-sites go away.\n\nHere it is as a patch on top. I doubt it matters that much (there are\nonly two stdin calls in the whole suite). So I'm not sure if this patch\nshould be \"eh, why not?\" or \"meh, pointless churn\".\n\n-- >8 --\nSubject: [PATCH] test-lib-functions: simplify packetize() stdin code\n\nThe code path in packetize() for reading stdin needs to handle NUL\nbytes, so we can't rely on shell variables. However, the current code\ntakes a whopping 4 processes and uses a temporary file. We can do this\nmuch more simply and efficiently by using a single perl invocation (and\nwe already rely on perl in the matching depacketize() function).\n\nWe'll keep the non-stdin code path as it is, since that uses zero extra\nprocesses.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n t/test-lib-functions.sh | 9 ++++-----\n 1 file changed, 4 insertions(+), 5 deletions(-)\n\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex 216918a58c..88b7dbd69a 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -1373,11 +1373,10 @@ packetize() {\n \t\tpacket=\"$*\"\n \t\tprintf '%04x%s' \"$((4 + ${#packet}))\" \"$packet\"\n \telse\n-\t\tcat >packetize.tmp &&\n-\t\tlen=$(wc -c <packetize.tmp) &&\n-\t\tprintf '%04x' \"$(($len + 4))\" &&\n-\t\tcat packetize.tmp &&\n-\t\trm -f packetize.tmp\n+\t\tperl -e '\n+\t\t\tmy $packet = do { local $/; <STDIN> };\n+\t\t\tprintf \"%04x%s\", 4 + length($packet), $packet;\n+\t\t'\n \tfi\n }\n \n-- \n2.26.0.581.g322f77c0ee\n\n"},{"id":"394287","messageId":"xmqqzhbzb1xt.fsf@gitster.c.googlers.com","threadId":"53108","inReplyTo":"20200329145343.GB15400@coredump.intra.peff.net","subject":"Re: [PATCH 1/2] test-lib-functions: make packetize() more efficient","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-03-29T15:44:46Z","receivedAt":"2020-03-29T15:44:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Sat, Mar 28, 2020 at 08:05:10PM -0700, Junio C Hamano wrote:\n>\n>> -- >8 --\n>> Subject: CodingGuidelines: allow ${#posix} == strlen($posix)\n>\n> I'm in favor of this patch, but...\n>\n>> The construct has been in POSIX for the past 10+ years, and we have\n>> used in t9xxx (subversion) series of the tests, so we know it is at\n>> least portable across systems that subversion Perl bindings have\n>> been ported to.\n>\n> It is even stronger than that. t9010 is run for everybody (with the\n> exception of the last test, which needs svnadmin).\n\nAh, you're right.  Lemme make it stronger.\n\n    The construct has been in POSIX for the past 10+ years, and we have\n    used in t9xxx (subversion) series of the tests, so we know it is at\n    portable across systems that people have run those tests, which is\n    almost everything we'd care about.\n\nThanks.\n"},{"id":"394288","messageId":"xmqqv9mnb1pp.fsf@gitster.c.googlers.com","threadId":"53108","inReplyTo":"20200329150226.GA16068@coredump.intra.peff.net","subject":"Re: [PATCH] test-lib-functions: simplify packetize() stdin code","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-03-29T15:49:38Z","receivedAt":"2020-03-29T15:49:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> Here it is as a patch on top. I doubt it matters that much (there are\n> only two stdin calls in the whole suite). So I'm not sure if this patch\n> should be \"eh, why not?\" or \"meh, pointless churn\".\n\nMe, neither. ;-)\n"}]}