{"thread":{"id":"56500","subject":"[PATCH 0/9] reducing memory allocations for v2 servers","startedAt":"2021-09-14T15:29:49Z","lastAt":"2021-09-15T18:36:54Z","messageCount":77,"participants":["Jeff King","Taylor Blau","Junio C Hamano","Martin Ågren","Ævar Arnfjörð Bjarmason","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":9},"messages":[{"id":"435869","messageId":"YUC/6n1hhUbMJiLw@coredump.intra.peff.net","threadId":"56500","inReplyTo":null,"subject":"[PATCH 0/9] reducing memory allocations for v2 servers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-14T15:29:46Z","receivedAt":"2021-09-14T15:29:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"While looking at [1], I noticed that v2 servers will read a few bits of\nclient input into strvecs. Even though we expect these to be small-ish,\nthere's nothing preventing a client from sending us a bunch of junk and\nwasting memory.\n\nThis series changes that, putting a cap on how much data we'll receive.\nThe two spots are the \"capabilities\" list we receive (before we even\ndispatch to a particular command like ls-refs or fetch), and the\nref-prefix list we receive for ls-refs.\n\nTo deal with the capabilities issue, we'll just handle each capability\nline as it comes in, rather than storing a list. This requires a bit of\ncleanup, which is why it takes up the first 6 patches. It also needs a\nfew other cleanups from ab/serve-cleanup (and so this series is based on\nthat). The dependencies there are both textual (e.g., using designated\ninitializers in the capabilities table) and functional (dropping the\n\"keys\" parameter from v2 command() functions).\n\nPatch 7 fixes the ls-refs issue by degrading when we see too many\nprefixes (which works because the capability is optional in the first\nplace).\n\nThe final two patches aren't strictly necessary. They're a tightening of\nthe protocol against some bogus input that I noticed while doing the\nearlier cleanups. That bogus input isn't really hurting anything, but I\nthink it makes sense to reject nonsense from the client rather than\nignoring it. The obvious risk is that some client for some reason would\nsend us nonsense, but I don't think Git ever has.\n\nOnce again, these are based on ab/serve-cleanup, which is currently in\nnext.\n\n [1/9]: serve: rename is_command() to parse_command()\n [2/9]: serve: return capability \"value\" from get_capability()\n [3/9]: serve: add \"receive\" method for v2 capabilities table\n [4/9]: serve: provide \"receive\" function for object-format capability\n [5/9]: serve: provide \"receive\" function for session-id capability\n [6/9]: serve: drop \"keys\" strvec\n [7/9]: ls-refs: ignore very long ref-prefix counts\n [8/9]: serve: reject bogus v2 \"command=ls-refs=foo\"\n [9/9]: serve: reject commands used as capabilities\n\n ls-refs.c | 19 ++++++-\n serve.c | 120 +++++++++++++++++++++++--------------------\n t/t5701-git-serve.sh | 60 ++++++++++++++++++++++\n 3 files changed, 142 insertions(+), 57 deletions(-)\n\n-Peff\n\n[1] https://lore.kernel.org/git/YT54CNYgtGcqexwq@coredump.intra.peff.net/\n"},{"id":"435870","messageId":"YUDADFmF0K6xhX6E@coredump.intra.peff.net","threadId":"56500","inReplyTo":"YUC/6n1hhUbMJiLw@coredump.intra.peff.net","subject":"[PATCH 1/9] serve: rename is_command() to parse_command()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-14T15:30:20Z","receivedAt":"2021-09-14T15:30:23Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The is_command() function not only tells us whether the pktline is a\nvalid command string, but it also parses out the command (and complains\nif we see a duplicate). Let's rename it to make those extra functions\na bit more obvious.\n\nSigned-off-by: Jeff King \n---\nObviously not strictly necessary, but the name caused me a fair bit of\nconfusion at first while touching this code.\n\n serve.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/serve.c b/serve.c\nindex 1817edc7f5..fd88b95343 100644\n--- a/serve.c\n+++ b/serve.c\n@@ -163,7 +163,7 @@ static int is_valid_capability(const char *key)\n \treturn c && c->advertise(the_repository, NULL);\n }\n \n-static int is_command(const char *key, struct protocol_capability **command)\n+static int parse_command(const char *key, struct protocol_capability **command)\n {\n \tconst char *out;\n \n@@ -251,7 +251,7 @@ static int process_request(void)\n \t\t\tBUG(\"Should have already died when seeing EOF\");\n \t\tcase PACKET_READ_NORMAL:\n \t\t\t/* collect request; a sequence of keys and values */\n-\t\t\tif (is_command(reader.line, &command) ||\n+\t\t\tif (parse_command(reader.line, &command) ||\n \t\t\t is_valid_capability(reader.line))\n \t\t\t\tstrvec_push(&keys, reader.line);\n \t\t\telse\n-- \n2.33.0.887.g5b1f44e68d\n\n"},{"id":"435871","messageId":"YUDAKg0g9dLf5014@coredump.intra.peff.net","threadId":"56500","inReplyTo":"YUC/6n1hhUbMJiLw@coredump.intra.peff.net","subject":"[PATCH 2/9] serve: return capability \"value\" from get_capability()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-14T15:30:50Z","receivedAt":"2021-09-14T15:30:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When the client sends v2 capabilities, they may be simple, like:\n\n foo\n\nor have a value like:\n\n foo=bar\n\n(all of the current capabilities actually expect a value, but the\nprotocol allows for boolean ones).\n\nWe use get_capability() to make sure the client's pktline matches a\ncapability. In doing so, we parse enough to see the \"=\" and the value\n(if any), but we immediately forget it. Nobody cares for now, because they end\nup parsing the values out later using has_capability(). But in\npreparation for changing that, let's pass back a pointer so the callers\nknow what we found.\n\nNote that unlike has_capability(), we'll return NULL for a \"simple\"\ncapability. Distinguishing these will be useful for some future patches.\n\nSigned-off-by: Jeff King \n---\nWe get rid of has_capability() later, so the inconsistency in return\ntypes will go away.\n\n serve.c | 18 ++++++++++++++----\n 1 file changed, 14 insertions(+), 4 deletions(-)\n\ndiff --git a/serve.c b/serve.c\nindex fd88b95343..78a4e83554 100644\n--- a/serve.c\n+++ b/serve.c\n@@ -139,7 +139,7 @@ void protocol_v2_advertise_capabilities(void)\n \tstrbuf_release(&value);\n }\n \n-static struct protocol_capability *get_capability(const char *key)\n+static struct protocol_capability *get_capability(const char *key, const char **value)\n {\n \tint i;\n \n@@ -149,16 +149,25 @@ static struct protocol_capability *get_capability(const char *key)\n \tfor (i = 0; i < ARRAY_SIZE(capabilities); i++) {\n \t\tstruct protocol_capability *c = &capabilities[i];\n \t\tconst char *out;\n-\t\tif (skip_prefix(key, c->name, &out) && (!*out || *out == '='))\n+\t\tif (!skip_prefix(key, c->name, &out))\n+\t\t\tcontinue;\n+\t\tif (!*out) {\n+\t\t\t*value = NULL;\n \t\t\treturn c;\n+\t\t}\n+\t\tif (*out++ == '=') {\n+\t\t\t*value = out;\n+\t\t\treturn c;\n+\t\t}\n \t}\n \n \treturn NULL;\n }\n \n static int is_valid_capability(const char *key)\n {\n-\tconst struct protocol_capability *c = get_capability(key);\n+\tconst char *value;\n+\tconst struct protocol_capability *c = get_capability(key, &value);\n \n \treturn c && c->advertise(the_repository, NULL);\n }\n@@ -168,7 +177,8 @@ static int parse_command(const char *key, struct protocol_capability **command)\n \tconst char *out;\n \n \tif (skip_prefix(key, \"command=\", &out)) {\n-\t\tstruct protocol_capability *cmd = get_capability(out);\n+\t\tconst char *value;\n+\t\tstruct protocol_capability *cmd = get_capability(out, &value);\n \n \t\tif (*command)\n \t\t\tdie(\"command '%s' requested after already requesting command '%s'\",\n-- \n2.33.0.887.g5b1f44e68d\n\n"},{"id":"435872","messageId":"YUDAOxjz6/XzuOAJ@coredump.intra.peff.net","threadId":"56500","inReplyTo":"YUC/6n1hhUbMJiLw@coredump.intra.peff.net","subject":"[PATCH 3/9] serve: add \"receive\" method for v2 capabilities table","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-14T15:31:07Z","receivedAt":"2021-09-14T15:31:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We have a capabilities table that tells us what we should tell the\nclient we are capable of, and what to do when a client gives us a\nparticular command (e.g., \"command=ls-refs\"). But it doesn't tell us\nwhat to do when the client sends us back a capability (e.g.,\n\"object-format=sha256\"). We just collect them all in a strvec and hope\nsomebody can use them later.\n\nInstead, let's provide a function pointer in the table to act on these.\nThis will eventually help us avoid collecting the strings, which will be\nmore efficient and less prone to mischief.\n\nUsing the new method is optional, which helps in two ways:\n\n - we can move existing capabilities over to this new system gradually\n in individual commits\n\n - some capabilities we don't actually do anything with anyway. For\n example, the client is free to say \"agent=git/1.2.3\" to us, but we\n do not act on the information in any way.\n\nSigned-off-by: Jeff King \n---\n serve.c | 21 ++++++++++++++++++---\n 1 file changed, 18 insertions(+), 3 deletions(-)\n\ndiff --git a/serve.c b/serve.c\nindex 78a4e83554..a161189984 100644\n--- a/serve.c\n+++ b/serve.c\n@@ -70,6 +70,16 @@ struct protocol_capability {\n \t * This field should be NULL for capabilities which are not commands.\n \t */\n \tint (*command)(struct repository *r, struct packet_reader *request);\n+\n+\t/*\n+\t * Function called when a client requests the capability as a\n+\t * non-command. This may be NULL if the capability does nothing.\n+\t *\n+\t * For a capability of the form \"foo=bar\", the value string points to\n+\t * the content after the \"=\" (i.e., \"bar\"). For simple capabilities\n+\t * (just \"foo\"), it is NULL.\n+\t */\n+\tvoid (*receive)(struct repository *r, const char *value);\n };\n \n static struct protocol_capability capabilities[] = {\n@@ -164,12 +174,17 @@ static struct protocol_capability *get_capability(const char *key, const char **\n \treturn NULL;\n }\n \n-static int is_valid_capability(const char *key)\n+static int receive_client_capability(const char *key)\n {\n \tconst char *value;\n \tconst struct protocol_capability *c = get_capability(key, &value);\n \n-\treturn c && c->advertise(the_repository, NULL);\n+\tif (!c || !c->advertise(the_repository, NULL))\n+\t\treturn 0;\n+\n+\tif (c->receive)\n+\t\tc->receive(the_repository, value);\n+\treturn 1;\n }\n \n static int parse_command(const char *key, struct protocol_capability **command)\n@@ -262,7 +277,7 @@ static int process_request(void)\n \t\tcase PACKET_READ_NORMAL:\n \t\t\t/* collect request; a sequence of keys and values */\n \t\t\tif (parse_command(reader.line, &command) ||\n-\t\t\t is_valid_capability(reader.line))\n+\t\t\t receive_client_capability(reader.line))\n \t\t\t\tstrvec_push(&keys, reader.line);\n \t\t\telse\n \t\t\t\tdie(\"unknown capability '%s'\", reader.line);\n-- \n2.33.0.887.g5b1f44e68d\n\n"},{"id":"435873","messageId":"YUDAUi627d6TVmUy@coredump.intra.peff.net","threadId":"56500","inReplyTo":"YUC/6n1hhUbMJiLw@coredump.intra.peff.net","subject":"[PATCH 4/9] serve: provide \"receive\" function for object-format capability","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-14T15:31:30Z","receivedAt":"2021-09-14T15:31:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We get any \"object-format\" specified by the client by searching for it\nin the collected list of capabilities the client sent. We can instead\njust handle it as soon as they send it. This is slightly more efficient,\nand gets us one step closer to dropping that collected list.\n\nNote that we do still have to do our final hash check after receiving\nall capabilities (because they might not have sent an object-format line\nat all, and we still have to check that the default matches our\nrepository algorithm). Since the check_algorithm() function would now be\ndone to a single if() statement, I've just inlined it in its only\ncaller.\n\nThere should be no change of behavior here, except for two\nbroken-protocol cases:\n\n - if the client sends multiple conflicting object-format capabilities\n (which they should not), we'll now choose the last one rather than\n the first. We could also detect and complain about the duplicates\n quite easily now, which we could not before, but I didn't do so\n here.\n\n - if the client sends a bogus \"object-format\" with no equals sign,\n we'll now say so, rather than \"unknown object format: ''\"\n\nSigned-off-by: Jeff King \n---\n serve.c | 34 +++++++++++++++++-----------------\n 1 file changed, 17 insertions(+), 17 deletions(-)\n\ndiff --git a/serve.c b/serve.c\nindex a161189984..f6ea2953eb 100644\n--- a/serve.c\n+++ b/serve.c\n@@ -10,6 +10,7 @@\n #include \"upload-pack.h\"\n \n static int advertise_sid = -1;\n+static int client_hash_algo = GIT_HASH_SHA1;\n \n static int always_advertise(struct repository *r,\n \t\t\t struct strbuf *value)\n@@ -33,6 +34,17 @@ static int object_format_advertise(struct repository *r,\n \treturn 1;\n }\n \n+static void object_format_receive(struct repository *r,\n+\t\t\t\t const char *algo_name)\n+{\n+\tif (!algo_name)\n+\t\tdie(\"object-format capability requires an argument\");\n+\n+\tclient_hash_algo = hash_algo_by_name(algo_name);\n+\tif (client_hash_algo == GIT_HASH_UNKNOWN)\n+\t\tdie(\"unknown object format '%s'\", algo_name);\n+}\n+\n static int session_id_advertise(struct repository *r, struct strbuf *value)\n {\n \tif (advertise_sid == -1 &&\n@@ -104,6 +116,7 @@ static struct protocol_capability capabilities[] = {\n \t{\n \t\t.name = \"object-format\",\n \t\t.advertise = object_format_advertise,\n+\t\t.receive = object_format_receive,\n \t},\n \t{\n \t\t.name = \"session-id\",\n@@ -228,22 +241,6 @@ static int has_capability(const struct strvec *keys, const char *capability,\n \treturn 0;\n }\n \n-static void check_algorithm(struct repository *r, struct strvec *keys)\n-{\n-\tint client = GIT_HASH_SHA1, server = hash_algo_by_ptr(r->hash_algo);\n-\tconst char *algo_name;\n-\n-\tif (has_capability(keys, \"object-format\", &algo_name)) {\n-\t\tclient = hash_algo_by_name(algo_name);\n-\t\tif (client == GIT_HASH_UNKNOWN)\n-\t\t\tdie(\"unknown object format '%s'\", algo_name);\n-\t}\n-\n-\tif (client != server)\n-\t\tdie(\"mismatched object format: server %s; client %s\\n\",\n-\t\t r->hash_algo->name, hash_algos[client].name);\n-}\n-\n enum request_state {\n \tPROCESS_REQUEST_KEYS,\n \tPROCESS_REQUEST_DONE,\n@@ -317,7 +314,10 @@ static int process_request(void)\n \tif (!command)\n \t\tdie(\"no command requested\");\n \n-\tcheck_algorithm(the_repository, &keys);\n+\tif (client_hash_algo != hash_algo_by_ptr(the_repository->hash_algo))\n+\t\tdie(\"mismatched object format: server %s; client %s\\n\",\n+\t\t the_repository->hash_algo->name,\n+\t\t hash_algos[client_hash_algo].name);\n \n \tif (has_capability(&keys, \"session-id\", &client_sid))\n \t\ttrace2_data_string(\"transfer\", NULL, \"client-sid\", client_sid);\n-- \n2.33.0.887.g5b1f44e68d\n\n"},{"id":"435874","messageId":"YUDAtbHcbv6zNFxe@coredump.intra.peff.net","threadId":"56500","inReplyTo":"YUC/6n1hhUbMJiLw@coredump.intra.peff.net","subject":"[PATCH 5/9] serve: provide \"receive\" function for session-id capability","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-14T15:33:09Z","receivedAt":"2021-09-14T15:33:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Rather than pulling the session-id string from the list of collected\ncapabilities, we can handle it as soon as we receive it. This gets us\ncloser to dropping the collected list entirely.\n\nThe behavior should be the same, with one exception. Previously if the\nclient sent us multiple session-id lines, we'd report only the first.\nNow we'll pass each one along to trace2. This shouldn't matter in\npractice, since clients shouldn't do that (and if they do, it's probably\nsensible to log them all).\n\nAs this removes the last caller of the static has_capability(), we can\nremove it, as well (and in fact we must to avoid -Wunused-function\ncomplaining).\n\nSigned-off-by: Jeff King \n---\nI had originally dropped has_capability() in a separate patch, to keep\nthis one more readable. That breaks bisectability, but only with\n-Werror. I'm not sure where we should fall on that spectrum (I generally\nbisect with -Wno-error just because warnings may come and go when\nworking with different compilers than what was normal at the time).\n\nNot that big a deal either way for this patch, but I wonder if people\nhave opinions in general.\n\n serve.c | 33 +++++++++------------------------\n 1 file changed, 9 insertions(+), 24 deletions(-)\n\ndiff --git a/serve.c b/serve.c\nindex f6ea2953eb..6bbf54cbbe 100644\n--- a/serve.c\n+++ b/serve.c\n@@ -57,6 +57,14 @@ static int session_id_advertise(struct repository *r, struct strbuf *value)\n \treturn 1;\n }\n \n+static void session_id_receive(struct repository *r,\n+\t\t\t const char *client_sid)\n+{\n+\tif (!client_sid)\n+\t\tclient_sid = \"\";\n+\ttrace2_data_string(\"transfer\", NULL, \"client-sid\", client_sid);\n+}\n+\n struct protocol_capability {\n \t/*\n \t * The name of the capability. The server uses this name when\n@@ -121,6 +129,7 @@ static struct protocol_capability capabilities[] = {\n \t{\n \t\t.name = \"session-id\",\n \t\t.advertise = session_id_advertise,\n+\t\t.receive = session_id_receive,\n \t},\n \t{\n \t\t.name = \"object-info\",\n@@ -221,26 +230,6 @@ static int parse_command(const char *key, struct protocol_capability **command)\n \treturn 0;\n }\n \n-static int has_capability(const struct strvec *keys, const char *capability,\n-\t\t\t const char **value)\n-{\n-\tint i;\n-\tfor (i = 0; i < keys->nr; i++) {\n-\t\tconst char *out;\n-\t\tif (skip_prefix(keys->v[i], capability, &out) &&\n-\t\t (!*out || *out == '=')) {\n-\t\t\tif (value) {\n-\t\t\t\tif (*out == '=')\n-\t\t\t\t\tout++;\n-\t\t\t\t*value = out;\n-\t\t\t}\n-\t\t\treturn 1;\n-\t\t}\n-\t}\n-\n-\treturn 0;\n-}\n-\n enum request_state {\n \tPROCESS_REQUEST_KEYS,\n \tPROCESS_REQUEST_DONE,\n@@ -252,7 +241,6 @@ static int process_request(void)\n \tstruct packet_reader reader;\n \tstruct strvec keys = STRVEC_INIT;\n \tstruct protocol_capability *command = NULL;\n-\tconst char *client_sid;\n \n \tpacket_reader_init(&reader, 0, NULL, 0,\n \t\t\t PACKET_READ_CHOMP_NEWLINE |\n@@ -319,9 +307,6 @@ static int process_request(void)\n \t\t the_repository->hash_algo->name,\n \t\t hash_algos[client_hash_algo].name);\n \n-\tif (has_capability(&keys, \"session-id\", &client_sid))\n-\t\ttrace2_data_string(\"transfer\", NULL, \"client-sid\", client_sid);\n-\n \tcommand->command(the_repository, &reader);\n \n \tstrvec_clear(&keys);\n-- \n2.33.0.887.g5b1f44e68d\n\n"},{"id":"435875","messageId":"YUDAvjteb/JAtyNz@coredump.intra.peff.net","threadId":"56500","inReplyTo":"YUC/6n1hhUbMJiLw@coredump.intra.peff.net","subject":"[PATCH 6/9] serve: drop \"keys\" strvec","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-14T15:33:18Z","receivedAt":"2021-09-14T15:33:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We collect the set of capabilities the client sends us in a strvec.\nWhile this is usually small, there's no limit to the number of\ncapabilities the client can send us (e.g., they could just send us\n\"agent\" pkt-lines over and over, and we'd keep adding them to the list).\n\nSince all code has been converted away from using this list, let's get\nrid of it. This avoids a potential attack where clients waste our\nmemory.\n\nNote that we do have to replace it with a flag, because some of the\nflush-packet logic checks whether we've seen any valid commands or keys.\n\nSigned-off-by: Jeff King \n---\n serve.c | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/serve.c b/serve.c\nindex 6bbf54cbbe..baa0a17502 100644\n--- a/serve.c\n+++ b/serve.c\n@@ -239,7 +239,7 @@ static int process_request(void)\n {\n \tenum request_state state = PROCESS_REQUEST_KEYS;\n \tstruct packet_reader reader;\n-\tstruct strvec keys = STRVEC_INIT;\n+\tint seen_capability_or_command = 0;\n \tstruct protocol_capability *command = NULL;\n \n \tpacket_reader_init(&reader, 0, NULL, 0,\n@@ -263,10 +263,11 @@ static int process_request(void)\n \t\t\t/* collect request; a sequence of keys and values */\n \t\t\tif (parse_command(reader.line, &command) ||\n \t\t\t receive_client_capability(reader.line))\n-\t\t\t\tstrvec_push(&keys, reader.line);\n+\t\t\t\tseen_capability_or_command = 1;\n \t\t\telse\n \t\t\t\tdie(\"unknown capability '%s'\", reader.line);\n \n+\n \t\t\t/* Consume the peeked line */\n \t\t\tpacket_reader_read(&reader);\n \t\t\tbreak;\n@@ -275,7 +276,7 @@ static int process_request(void)\n \t\t\t * If no command and no keys were given then the client\n \t\t\t * wanted to terminate the connection.\n \t\t\t */\n-\t\t\tif (!keys.nr)\n+\t\t\tif (!seen_capability_or_command)\n \t\t\t\treturn 1;\n \n \t\t\t/*\n@@ -309,7 +310,6 @@ static int process_request(void)\n \n \tcommand->command(the_repository, &reader);\n \n-\tstrvec_clear(&keys);\n \treturn 0;\n }\n \n-- \n2.33.0.887.g5b1f44e68d\n\n"},{"id":"435876","messageId":"YUDBokYvEBnzwsIN@coredump.intra.peff.net","threadId":"56500","inReplyTo":"YUC/6n1hhUbMJiLw@coredump.intra.peff.net","subject":"[PATCH 7/9] ls-refs: ignore very long ref-prefix counts","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-14T15:37:06Z","receivedAt":"2021-09-14T15:37:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Because each \"ref-prefix\" capability from the client comes in its own\npkt-line, there's no limit to the number of them that a misbehaving\nclient may send. We read them all into a strvec, which means the client\ncan waste arbitrary amounts of our memory by just sending us \"ref-prefix\nfoo\" over and over.\n\nOne possible solution is to just drop the connection when the limit is\nreached. If we set it high enough, then only misbehaving or malicious\nclients would hit it. But \"high enough\" is vague, and it's unfriendly if\nwe guess wrong and a legitimate client hits this.\n\nBut we can do better. Since supporting the ref-prefix capability is\noptional anyway, the client has to further cull the response based on\ntheir own patterns. So we can simply ignore the patterns once we cross a\ncertain threshold. Note that we have to ignore _all_ patterns, not just\nthe ones past our limit (since otherwise we'd send too little data).\n\nThe limit here is fairly arbitrary, and probably much higher than anyone\nwould need in practice. It might be worth limiting it further, if only\nbecause we check it linearly (so with \"m\" local refs and \"n\" patterns,\nwe do \"m * n\" string comparisons). But if we care about optimizing this,\nan even better solution may be a more advanced data structure anyway.\n\nI didn't bother making the limit configurable, since it's so high and\nsince Git should behave correctly in either case. It wouldn't be too\nhard to do, but it makes both the code and documentation more complex.\n\nSigned-off-by: Jeff King \n---\nWe're perhaps bending \"optional\" a little here. The client does know if\nwe said \"yes, we support ref-prefix\" and until now, that meant they\ncould trust us to cull. But no version of Git has ever relied on that\n(we tell the transport code \"if you can limit by these prefixes, go for\nit\" but then just post-process the result).\n\nThe other option is that we could just say \"no, you're sending too many\nprefixes\" and hangup. This seemed friendlier to me (though either way, I\nreally find it quite unlikely anybody would legitimately hit this\nlimit).\n\n ls-refs.c | 19 +++++++++++++++++--\n t/t5701-git-serve.sh | 31 +++++++++++++++++++++++++++++++\n 2 files changed, 48 insertions(+), 2 deletions(-)\n\ndiff --git a/ls-refs.c b/ls-refs.c\nindex a1a0250607..839fb0caa9 100644\n--- a/ls-refs.c\n+++ b/ls-refs.c\n@@ -40,6 +40,12 @@ static void ensure_config_read(void)\n \tconfig_read = 1;\n }\n \n+/*\n+ * The maximum number of \"ref-prefix\" lines we'll allow the client to send.\n+ * If they go beyond this, we'll avoid using the prefix feature entirely.\n+ */\n+#define MAX_ALLOWED_PREFIXES 65536\n+\n /*\n * Check if one of the prefixes is a prefix of the ref.\n * If no prefixes were provided, all refs match.\n@@ -141,6 +147,7 @@ static int ls_refs_config(const char *var, const char *value, void *data)\n int ls_refs(struct repository *r, struct packet_reader *request)\n {\n \tstruct ls_refs_data data;\n+\tint too_many_prefixes = 0;\n \n \tmemset(&data, 0, sizeof(data));\n \tstrvec_init(&data.prefixes);\n@@ -156,8 +163,16 @@ int ls_refs(struct repository *r, struct packet_reader *request)\n \t\t\tdata.peel = 1;\n \t\telse if (!strcmp(\"symrefs\", arg))\n \t\t\tdata.symrefs = 1;\n-\t\telse if (skip_prefix(arg, \"ref-prefix \", &out))\n-\t\t\tstrvec_push(&data.prefixes, out);\n+\t\telse if (skip_prefix(arg, \"ref-prefix \", &out)) {\n+\t\t\tif (too_many_prefixes) {\n+\t\t\t\t/* ignore any further ones */\n+\t\t\t} else if (data.prefixes.nr >= MAX_ALLOWED_PREFIXES) {\n+\t\t\t\tstrvec_clear(&data.prefixes);\n+\t\t\t\ttoo_many_prefixes = 1;\n+\t\t\t} else {\n+\t\t\t\tstrvec_push(&data.prefixes, out);\n+\t\t\t}\n+\t\t}\n \t\telse if (!strcmp(\"unborn\", arg))\n \t\t\tdata.unborn = allow_unborn;\n \t}\ndiff --git a/t/t5701-git-serve.sh b/t/t5701-git-serve.sh\nindex 930721f053..b095bfa0ac 100755\n--- a/t/t5701-git-serve.sh\n+++ b/t/t5701-git-serve.sh\n@@ -158,6 +158,37 @@ test_expect_success 'refs/heads prefix' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'ignore very large set of prefixes' '\n+\t# generate a large number of ref-prefixes that we expect\n+\t# to match nothing; the value here exceeds MAX_ALLOWED_PREFIXES\n+\t# from ls-refs.c.\n+\t{\n+\t\techo command=ls-refs &&\n+\t\techo object-format=$(test_oid algo)\n+\t\techo 0001 &&\n+\t\tperl -le \"print \\\"refs/heads/$_\\\" for (1..65536+1)\" &&\n+\t\techo 0000\n+\t} |\n+\ttest-tool pkt-line pack >in &&\n+\n+\t# and then confirm that we see unmatched prefixes anyway (i.e.,\n+\t# that the prefix was not applied).\n+\tcat >expect <<-EOF &&\n+\t$(git rev-parse HEAD) HEAD\n+\t$(git rev-parse refs/heads/dev) refs/heads/dev\n+\t$(git rev-parse refs/heads/main) refs/heads/main\n+\t$(git rev-parse refs/heads/release) refs/heads/release\n+\t$(git rev-parse refs/tags/annotated-tag) refs/tags/annotated-tag\n+\t$(git rev-parse refs/tags/one) refs/tags/one\n+\t$(git rev-parse refs/tags/two) refs/tags/two\n+\t0000\n+\tEOF\n+\n+\ttest-tool serve-v2 --stateless-rpc out &&\n+\ttest-tool pkt-line unpack actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'peel parameter' '\n \ttest-tool pkt-line pack >in <<-EOF &&\n \tcommand=ls-refs\n-- \n2.33.0.887.g5b1f44e68d\n\n"},{"id":"435877","messageId":"YUDBsdTeX8myV1vY@coredump.intra.peff.net","threadId":"56500","inReplyTo":"YUC/6n1hhUbMJiLw@coredump.intra.peff.net","subject":"[PATCH 8/9] serve: reject bogus v2 \"command=ls-refs=foo\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-14T15:37:21Z","receivedAt":"2021-09-14T15:37:23Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we see a line from the client like \"command=ls-refs\", we parse\neverything after the equals sign as a capability, which we check against\nour capabilities table. If we don't recognize the command (e.g.,\n\"command=foo\"), we'll reject it. But we use the same parser that checks\nfor regular capabilities like \"object-format=sha256\". And so we'll\naccept \"ls-refs=foo\", even though everything after the equals is bogus,\nand simply ignored.\n\nThis isn't really hurting anything, but the request does violate the\nspec. Let's tighten it up to prevent any surprising behavior.\n\nSigned-off-by: Jeff King \n---\n serve.c | 2 +-\n t/t5701-git-serve.sh | 10 ++++++++++\n 2 files changed, 11 insertions(+), 1 deletion(-)\n\ndiff --git a/serve.c b/serve.c\nindex baa0a17502..123abbaa83 100644\n--- a/serve.c\n+++ b/serve.c\n@@ -220,7 +220,7 @@ static int parse_command(const char *key, struct protocol_capability **command)\n \t\tif (*command)\n \t\t\tdie(\"command '%s' requested after already requesting command '%s'\",\n \t\t\t out, (*command)->name);\n-\t\tif (!cmd || !cmd->advertise(the_repository, NULL) || !cmd->command)\n+\t\tif (!cmd || !cmd->advertise(the_repository, NULL) || !cmd->command || value)\n \t\t\tdie(\"invalid command '%s'\", out);\n \n \t\t*command = cmd;\ndiff --git a/t/t5701-git-serve.sh b/t/t5701-git-serve.sh\nindex b095bfa0ac..ebb41657ab 100755\n--- a/t/t5701-git-serve.sh\n+++ b/t/t5701-git-serve.sh\n@@ -72,6 +72,16 @@ test_expect_success 'request invalid command' '\n \ttest_i18ngrep \"invalid command\" err\n '\n \n+test_expect_success 'requested command is command=value' '\n+\ttest-tool pkt-line pack >in <<-\\EOF &&\n+\tcommand=ls-refs=whatever\n+\tobject-format=$(test_oid algo)\n+\t0000\n+\tEOF\n+\ttest_must_fail test-tool serve-v2 --stateless-rpc 2>err in <<-EOF &&\n \tcommand=fetch\n-- \n2.33.0.887.g5b1f44e68d\n\n"},{"id":"435878","messageId":"YUDBxrSw4CzZ+wc4@coredump.intra.peff.net","threadId":"56500","inReplyTo":"YUC/6n1hhUbMJiLw@coredump.intra.peff.net","subject":"[PATCH 9/9] serve: reject commands used as capabilities","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-14T15:37:42Z","receivedAt":"2021-09-14T15:37:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Our table of v2 \"capabilities\" contains everything we might tell the\nclient we support. But there are differences in how we expect the client\nto respond. Some of the entries are true capabilities (i.e., we expect\nthe client to say \"yes, I support this\"), and some are ones we expect\nthem to send as commands (with \"command=ls-refs\" or similar).\n\nWhen we receive a capability used as a command, we complain about that.\nBut when we receive a command used as a capability (e.g., just \"ls-refs\"\nin a pkt-line by itself), we silently ignore it.\n\nThis isn't really hurting anything (clients shouldn't send it, and we'll\nignore it), but we can tighten up the protocol to match what we expect\nto happen.\n\nThere are two new tests here. The first one checks a capability used as\na command, which already passes. The second tests a command as a\ncapability, which this patch fixes.\n\nSigned-off-by: Jeff King \n---\n serve.c | 2 +-\n t/t5701-git-serve.sh | 19 +++++++++++++++++++\n 2 files changed, 20 insertions(+), 1 deletion(-)\n\ndiff --git a/serve.c b/serve.c\nindex 123abbaa83..9149fbb2f2 100644\n--- a/serve.c\n+++ b/serve.c\n@@ -201,7 +201,7 @@ static int receive_client_capability(const char *key)\n \tconst char *value;\n \tconst struct protocol_capability *c = get_capability(key, &value);\n \n-\tif (!c || !c->advertise(the_repository, NULL))\n+\tif (!c || c->command || !c->advertise(the_repository, NULL))\n \t\treturn 0;\n \n \tif (c->receive)\ndiff --git a/t/t5701-git-serve.sh b/t/t5701-git-serve.sh\nindex ebb41657ab..209122b747 100755\n--- a/t/t5701-git-serve.sh\n+++ b/t/t5701-git-serve.sh\n@@ -72,6 +72,25 @@ test_expect_success 'request invalid command' '\n \ttest_i18ngrep \"invalid command\" err\n '\n \n+test_expect_success 'request capability as command' '\n+\ttest-tool pkt-line pack >in <<-\\EOF &&\n+\tcommand=agent\n+\t0000\n+\tEOF\n+\ttest_must_fail test-tool serve-v2 --stateless-rpc 2>err in <<-\\EOF &&\n+\tcommand=ls-refs\n+\tfetch\n+\t0000\n+\tEOF\n+\ttest_must_fail test-tool serve-v2 --stateless-rpc 2>err in <<-\\EOF &&\n \tcommand=ls-refs=whatever\n-- \n2.33.0.887.g5b1f44e68d\n"},{"id":"435882","messageId":"YUDT5bnnMNtkc3KK@nand.local","threadId":"56500","inReplyTo":"YUDAtbHcbv6zNFxe@coredump.intra.peff.net","subject":"Re: [PATCH 5/9] serve: provide \"receive\" function for session-id capability","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-09-14T16:55:01Z","receivedAt":"2021-09-14T16:55:05Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Sep 14, 2021 at 11:33:09AM -0400, Jeff King wrote:\n> I had originally dropped has_capability() in a separate patch, to keep\n> this one more readable. That breaks bisectability, but only with\n> -Werror. I'm not sure where we should fall on that spectrum (I generally\n> bisect with -Wno-error just because warnings may come and go when\n> working with different compilers than what was normal at the time).\n\nI tend to fall the same way, especially when bisecting things in ancient\n(to me) versions of Git where my current compiler complains. So I think\nthe approach that you took here is just fine.\n\n(This patch, as with all leading up to it, looks good to me.)\n\nThanks,\nTaylor\n"},{"id":"435883","messageId":"YUDU5gE/AazOus+s@nand.local","threadId":"56500","inReplyTo":"YUDAvjteb/JAtyNz@coredump.intra.peff.net","subject":"Re: [PATCH 6/9] serve: drop \"keys\" strvec","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-09-14T16:59:18Z","receivedAt":"2021-09-14T16:59:24Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Sep 14, 2021 at 11:33:18AM -0400, Jeff King wrote:\n> We collect the set of capabilities the client sends us in a strvec.\n> While this is usually small, there's no limit to the number of\n> capabilities the client can send us (e.g., they could just send us\n> \"agent\" pkt-lines over and over, and we'd keep adding them to the list).\n>\n> Since all code has been converted away from using this list, let's get\n> rid of it. This avoids a potential attack where clients waste our\n> memory.\n\n...because now we only bother to tell capabilities about information the\nclient sent as it happened, instead of accumulating an unbounded set of\nstrings together into a single strvec.\n\n> Note that we do have to replace it with a flag, because some of the\n> flush-packet logic checks whether we've seen any valid commands or keys.\n\nMakes sense.\n\n> Signed-off-by: Jeff King \n> ---\n> serve.c | 8 ++++----\n> 1 file changed, 4 insertions(+), 4 deletions(-)\n>\n> diff --git a/serve.c b/serve.c\n> index 6bbf54cbbe..baa0a17502 100644\n> --- a/serve.c\n> +++ b/serve.c\n> @@ -239,7 +239,7 @@ static int process_request(void)\n> {\n> \tenum request_state state = PROCESS_REQUEST_KEYS;\n> \tstruct packet_reader reader;\n> -\tstruct strvec keys = STRVEC_INIT;\n> +\tint seen_capability_or_command = 0;\n> \tstruct protocol_capability *command = NULL;\n>\n> \tpacket_reader_init(&reader, 0, NULL, 0,\n> @@ -263,10 +263,11 @@ static int process_request(void)\n> \t\t\t/* collect request; a sequence of keys and values */\n> \t\t\tif (parse_command(reader.line, &command) ||\n> \t\t\t receive_client_capability(reader.line))\n> -\t\t\t\tstrvec_push(&keys, reader.line);\n> +\t\t\t\tseen_capability_or_command = 1;\n> \t\t\telse\n> \t\t\t\tdie(\"unknown capability '%s'\", reader.line);\n>\n> +\n\nNit; unnecessary whitespace change (but obviously not worth a re-roll on\nits own).\n\n> \t\t\t/* Consume the peeked line */\n> \t\t\tpacket_reader_read(&reader);\n> \t\t\tbreak;\n> @@ -275,7 +276,7 @@ static int process_request(void)\n> \t\t\t * If no command and no keys were given then the client\n> \t\t\t * wanted to terminate the connection.\n> \t\t\t */\n> -\t\t\tif (!keys.nr)\n> +\t\t\tif (!seen_capability_or_command)\n> \t\t\t\treturn 1;\n>\n> \t\t\t/*\n> @@ -309,7 +310,6 @@ static int process_request(void)\n>\n> \tcommand->command(the_repository, &reader);\n>\n> -\tstrvec_clear(&keys);\n> \treturn 0;\n> }\n>\n> --\n> 2.33.0.887.g5b1f44e68d\n\nThe rest of this change looks obviously good to me.\n\nThanks,\nTaylor\n"},{"id":"435884","messageId":"YUDWoiN7wczfNwU8@coredump.intra.peff.net","threadId":"56500","inReplyTo":"YUDT5bnnMNtkc3KK@nand.local","subject":"Re: [PATCH 5/9] serve: provide \"receive\" function for session-id capability","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-14T17:06:42Z","receivedAt":"2021-09-14T17:06:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 14, 2021 at 12:55:01PM -0400, Taylor Blau wrote:\n\n> On Tue, Sep 14, 2021 at 11:33:09AM -0400, Jeff King wrote:\n> > I had originally dropped has_capability() in a separate patch, to keep\n> > this one more readable. That breaks bisectability, but only with\n> > -Werror. I'm not sure where we should fall on that spectrum (I generally\n> > bisect with -Wno-error just because warnings may come and go when\n> > working with different compilers than what was normal at the time).\n> \n> I tend to fall the same way, especially when bisecting things in ancient\n> (to me) versions of Git where my current compiler complains. So I think\n> the approach that you took here is just fine.\n\nTo be clear, the approach here is conservative: it will bisect even with\n-Werror. I think what you're saying is I _could_ have done the other\napproach, and put the removal into its own commit?\n\n-Peff\n"},{"id":"435885","messageId":"YUDYB0kQQsbKyIH9@nand.local","threadId":"56500","inReplyTo":"YUDWoiN7wczfNwU8@coredump.intra.peff.net","subject":"Re: [PATCH 5/9] serve: provide \"receive\" function for session-id capability","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-09-14T17:12:39Z","receivedAt":"2021-09-14T17:12:45Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Sep 14, 2021 at 01:06:42PM -0400, Jeff King wrote:\n> On Tue, Sep 14, 2021 at 12:55:01PM -0400, Taylor Blau wrote:\n>\n> > On Tue, Sep 14, 2021 at 11:33:09AM -0400, Jeff King wrote:\n> > > I had originally dropped has_capability() in a separate patch, to keep\n> > > this one more readable. That breaks bisectability, but only with\n> > > -Werror. I'm not sure where we should fall on that spectrum (I generally\n> > > bisect with -Wno-error just because warnings may come and go when\n> > > working with different compilers than what was normal at the time).\n> >\n> > I tend to fall the same way, especially when bisecting things in ancient\n> > (to me) versions of Git where my current compiler complains. So I think\n> > the approach that you took here is just fine.\n>\n> To be clear, the approach here is conservative: it will bisect even with\n> -Werror. I think what you're saying is I _could_ have done the other\n> approach, and put the removal into its own commit?\n\nYes. I would have been fine with dropping has_capability() in its own\npatch, since the result would have been more readable (and since I never\nbisect with `-Werror`). But this (the conservative approach of\npersevering bisect-ability even with `-Werror` is fine with me, too).\n\n> -Peff\n\nThanks,\nTaylor\n"},{"id":"435886","messageId":"YUDY1UIFBxOjNA37@coredump.intra.peff.net","threadId":"56500","inReplyTo":"YUDU5gE/AazOus+s@nand.local","subject":"Re: [PATCH 6/9] serve: drop \"keys\" strvec","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-14T17:16:05Z","receivedAt":"2021-09-14T17:16:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 14, 2021 at 12:59:18PM -0400, Taylor Blau wrote:\n\n> On Tue, Sep 14, 2021 at 11:33:18AM -0400, Jeff King wrote:\n> > We collect the set of capabilities the client sends us in a strvec.\n> > While this is usually small, there's no limit to the number of\n> > capabilities the client can send us (e.g., they could just send us\n> > \"agent\" pkt-lines over and over, and we'd keep adding them to the list).\n> >\n> > Since all code has been converted away from using this list, let's get\n> > rid of it. This avoids a potential attack where clients waste our\n> > memory.\n> \n> ...because now we only bother to tell capabilities about information the\n> client sent as it happened, instead of accumulating an unbounded set of\n> strings together into a single strvec.\n\nRight. By the way, one thing I noticed while working on this (that is\nunchanged by my series) is that some capabilities are ignored. That make\nsense for \"agent\", where the primary goal is telling the client about\nour version (and letting them speak their version, which might be\nhelpful for debug logs but doesn't impact the protocol itself). But we\ndo nothing at all with \"server-option\".\n\nThe client wouldn't send one by default, but we do support \"git fetch\n--server-option\". So I assume that it would be a sort of \"I'll tell you\nthis string, and you might feed it to hooks, etc\" function, like\npush-options. But we just throw it away (well, before my series we\nstuffed it into a strvec that nobody looked at).\n\nSo possibly we could stop advertising it, but I wonder if any clients\nwould get unhappy.\n\nI also wonder if jgit uses it. Grepping in jgit.git shows that it is\nrecorded but I don't see anybody doing anything useful with it; but\nmaybe some user of the library does.\n\n> > @@ -263,10 +263,11 @@ static int process_request(void)\n> > \t\t\t/* collect request; a sequence of keys and values */\n> > \t\t\tif (parse_command(reader.line, &command) ||\n> > \t\t\t receive_client_capability(reader.line))\n> > -\t\t\t\tstrvec_push(&keys, reader.line);\n> > +\t\t\t\tseen_capability_or_command = 1;\n> > \t\t\telse\n> > \t\t\t\tdie(\"unknown capability '%s'\", reader.line);\n> >\n> > +\n> \n> Nit; unnecessary whitespace change (but obviously not worth a re-roll on\n> its own).\n\nThanks. While this section ended up quite simple, it got rebased a whole\nlot of times as I figured out all of quirks that led to the final\nprotocol-tightening patches. :)\n\nI don't know how I missed the extra line on my final read-through.\n\n-Peff\n"},{"id":"435887","messageId":"YUDZTvI4hrqRK8Th@nand.local","threadId":"56500","inReplyTo":"YUDBokYvEBnzwsIN@coredump.intra.peff.net","subject":"Re: [PATCH 7/9] ls-refs: ignore very long ref-prefix counts","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-09-14T17:18:06Z","receivedAt":"2021-09-14T17:18:12Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Sep 14, 2021 at 11:37:06AM -0400, Jeff King wrote:\n> Because each \"ref-prefix\" capability from the client comes in its own\n> pkt-line, there's no limit to the number of them that a misbehaving\n> client may send. We read them all into a strvec, which means the client\n> can waste arbitrary amounts of our memory by just sending us \"ref-prefix\n> foo\" over and over.\n>\n> One possible solution is to just drop the connection when the limit is\n> reached. If we set it high enough, then only misbehaving or malicious\n> clients would hit it. But \"high enough\" is vague, and it's unfriendly if\n> we guess wrong and a legitimate client hits this.\n>\n> But we can do better. Since supporting the ref-prefix capability is\n> optional anyway, the client has to further cull the response based on\n> their own patterns. So we can simply ignore the patterns once we cross a\n> certain threshold. Note that we have to ignore _all_ patterns, not just\n> the ones past our limit (since otherwise we'd send too little data).\n\nRight, because each ref-prefix line *adds* references to the advertised\nset instead of culling them out. So as soon as we start ignoring even a\nsingle ref-prefix line, we have to ignore all of them. Makes sense.\n\n> The limit here is fairly arbitrary, and probably much higher than anyone\n> would need in practice. It might be worth limiting it further, if only\n> because we check it linearly (so with \"m\" local refs and \"n\" patterns,\n> we do \"m * n\" string comparisons). But if we care about optimizing this,\n> an even better solution may be a more advanced data structure anyway.\n>\n> I didn't bother making the limit configurable, since it's so high and\n> since Git should behave correctly in either case. It wouldn't be too\n> hard to do, but it makes both the code and documentation more complex.\n\nAgreed. I don't think it's worth making it configurable because the\nlimit is so absurdly high that probably nobody will ever want to tweak\nit.\n\n> Signed-off-by: Jeff King \n> ---\n> We're perhaps bending \"optional\" a little here. The client does know if\n> we said \"yes, we support ref-prefix\" and until now, that meant they\n> could trust us to cull. But no version of Git has ever relied on that\n> (we tell the transport code \"if you can limit by these prefixes, go for\n> it\" but then just post-process the result).\n>\n> The other option is that we could just say \"no, you're sending too many\n> prefixes\" and hangup. This seemed friendlier to me (though either way, I\n> really find it quite unlikely anybody would legitimately hit this\n> limit).\n\nFWIW, either (dropping the connection or the approach you took here)\nwould have been fine with me, but I find it unlikely that any real users\nwill notice ;).\n\n> ls-refs.c | 19 +++++++++++++++++--\n> t/t5701-git-serve.sh | 31 +++++++++++++++++++++++++++++++\n> 2 files changed, 48 insertions(+), 2 deletions(-)\n>\n> diff --git a/ls-refs.c b/ls-refs.c\n> index a1a0250607..839fb0caa9 100644\n> --- a/ls-refs.c\n> +++ b/ls-refs.c\n> @@ -40,6 +40,12 @@ static void ensure_config_read(void)\n> \tconfig_read = 1;\n> }\n>\n> +/*\n> + * The maximum number of \"ref-prefix\" lines we'll allow the client to send.\n> + * If they go beyond this, we'll avoid using the prefix feature entirely.\n> + */\n> +#define MAX_ALLOWED_PREFIXES 65536\n> +\n> /*\n> * Check if one of the prefixes is a prefix of the ref.\n> * If no prefixes were provided, all refs match.\n> @@ -141,6 +147,7 @@ static int ls_refs_config(const char *var, const char *value, void *data)\n> int ls_refs(struct repository *r, struct packet_reader *request)\n> {\n> \tstruct ls_refs_data data;\n> +\tint too_many_prefixes = 0;\n>\n> \tmemset(&data, 0, sizeof(data));\n> \tstrvec_init(&data.prefixes);\n> @@ -156,8 +163,16 @@ int ls_refs(struct repository *r, struct packet_reader *request)\n> \t\t\tdata.peel = 1;\n> \t\telse if (!strcmp(\"symrefs\", arg))\n> \t\t\tdata.symrefs = 1;\n> -\t\telse if (skip_prefix(arg, \"ref-prefix \", &out))\n> -\t\t\tstrvec_push(&data.prefixes, out);\n> +\t\telse if (skip_prefix(arg, \"ref-prefix \", &out)) {\n> +\t\t\tif (too_many_prefixes) {\n> +\t\t\t\t/* ignore any further ones */\n> +\t\t\t} else if (data.prefixes.nr >= MAX_ALLOWED_PREFIXES) {\n> +\t\t\t\tstrvec_clear(&data.prefixes);\n> +\t\t\t\ttoo_many_prefixes = 1;\n> +\t\t\t} else {\n> +\t\t\t\tstrvec_push(&data.prefixes, out);\n> +\t\t\t}\n> +\t\t}\n\nThe order of this if-statement is a little odd to me, but obviously\ncorrect. I might have wrote:\n\n if (too_many_prefixes)\n continue;\n\n if (data.prefixes.nr < MAX_ALLOWED_PREFIXES) {\n strvec_push(&data.prefixes, out);\n } else {\n too_many_prefixes = 1;\n strvec_clear(&data.prefixes);\n }\n\nBut certainly what you wrote here works just fine (so this is a cosmetic\ncomment, and not a functional one).\n\n> \t\telse if (!strcmp(\"unborn\", arg))\n> \t\t\tdata.unborn = allow_unborn;\n> \t}\n> diff --git a/t/t5701-git-serve.sh b/t/t5701-git-serve.sh\n> index 930721f053..b095bfa0ac 100755\n> --- a/t/t5701-git-serve.sh\n> +++ b/t/t5701-git-serve.sh\n> @@ -158,6 +158,37 @@ test_expect_success 'refs/heads prefix' '\n> \ttest_cmp expect actual\n> '\n>\n> +test_expect_success 'ignore very large set of prefixes' '\n> +\t# generate a large number of ref-prefixes that we expect\n> +\t# to match nothing; the value here exceeds MAX_ALLOWED_PREFIXES\n> +\t# from ls-refs.c.\n> +\t{\n> +\t\techo command=ls-refs &&\n> +\t\techo object-format=$(test_oid algo)\n> +\t\techo 0001 &&\n> +\t\tperl -le \"print \\\"refs/heads/$_\\\" for (1..65536+1)\" &&\n> +\t\techo 0000\n> +\t} |\n> +\ttest-tool pkt-line pack >in &&\n> +\n> +\t# and then confirm that we see unmatched prefixes anyway (i.e.,\n> +\t# that the prefix was not applied).\n> +\tcat >expect <<-EOF &&\n> +\t$(git rev-parse HEAD) HEAD\n> +\t$(git rev-parse refs/heads/dev) refs/heads/dev\n> +\t$(git rev-parse refs/heads/main) refs/heads/main\n> +\t$(git rev-parse refs/heads/release) refs/heads/release\n> +\t$(git rev-parse refs/tags/annotated-tag) refs/tags/annotated-tag\n> +\t$(git rev-parse refs/tags/one) refs/tags/one\n> +\t$(git rev-parse refs/tags/two) refs/tags/two\n\nYou could have written this as a loop over the unmatched prefixes, but I\nvastly prefer the result you came up with, which is much more explicit\nand doesn't require readers to parse out what the loop does.\n\nSo this part looks very good to me.\n\nThanks,\nTaylor\n"},{"id":"435888","messageId":"YUDZ/HZAti2OVVX6@nand.local","threadId":"56500","inReplyTo":"YUDBsdTeX8myV1vY@coredump.intra.peff.net","subject":"Re: [PATCH 8/9] serve: reject bogus v2 \"command=ls-refs=foo\"","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-09-14T17:21:00Z","receivedAt":"2021-09-14T17:21:03Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Sep 14, 2021 at 11:37:21AM -0400, Jeff King wrote:\n> This isn't really hurting anything, but the request does violate the\n> spec. Let's tighten it up to prevent any surprising behavior.\n\nYeah. I seriously doubt that anybody is relying on this obviously broken\nbehavior, and it's definitely a spec violation. So I have no trouble\nrejecting 'command=ls-refs=foo' over the wire.\n\nThanks,\nTaylor\n"},{"id":"435890","messageId":"YUDakl7dcJpfve0T@coredump.intra.peff.net","threadId":"56500","inReplyTo":"YUDBokYvEBnzwsIN@coredump.intra.peff.net","subject":"Re: [PATCH 7/9] ls-refs: ignore very long ref-prefix counts","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-14T17:23:30Z","receivedAt":"2021-09-14T17:23:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 14, 2021 at 11:37:06AM -0400, Jeff King wrote:\n\n> The limit here is fairly arbitrary, and probably much higher than anyone\n> would need in practice. It might be worth limiting it further, if only\n> because we check it linearly (so with \"m\" local refs and \"n\" patterns,\n> we do \"m * n\" string comparisons). But if we care about optimizing this,\n> an even better solution may be a more advanced data structure anyway.\n\nThe limit I picked is 65536, because it seemed round and high. But note\nthat somebody can put up to almost-64k in a single ref-prefix line,\nwhich means ultimately you can allocate 4GB. I do wonder if dropping\nthis to something like 1024 might be reasonable.\n\nIn practice I'd expect it to be a handful in most cases (refs/heads/*,\nrefs/tags/*, HEAD). But if you do something like:\n\n git fetch $remote 1 2 3 4 5 6 7 ...\n\nthen we'll prefix-expand those names with the usual lookup rules into\nrefs/1, refs/heads/1, refs/2, refs/heads/2, and so on.\n\nAt some point it becomes silly and works counter to the purpose of the\noptimization (you send more prefix constraints than the actual ref\nadvertisement, not to mention that client bandwidth may not be\nsymmetric). I'm not sure what we want to declare as a reasonable limit.\n\nAnd this is just about protecting the server; probably it makes sense\nfor the client to realize it's going to send a ridiculous number of\nprefixes and just skip the feature entirely (since that's what actually\nsaves the bandwidth).\n\n-Peff\n"},{"id":"435891","messageId":"YUDbgBfvFU+33Fuz@coredump.intra.peff.net","threadId":"56500","inReplyTo":"YUDZTvI4hrqRK8Th@nand.local","subject":"Re: [PATCH 7/9] ls-refs: ignore very long ref-prefix counts","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-14T17:27:28Z","receivedAt":"2021-09-14T17:27:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 14, 2021 at 01:18:06PM -0400, Taylor Blau wrote:\n\n> > @@ -156,8 +163,16 @@ int ls_refs(struct repository *r, struct packet_reader *request)\n> > \t\t\tdata.peel = 1;\n> > \t\telse if (!strcmp(\"symrefs\", arg))\n> > \t\t\tdata.symrefs = 1;\n> > -\t\telse if (skip_prefix(arg, \"ref-prefix \", &out))\n> > -\t\t\tstrvec_push(&data.prefixes, out);\n> > +\t\telse if (skip_prefix(arg, \"ref-prefix \", &out)) {\n> > +\t\t\tif (too_many_prefixes) {\n> > +\t\t\t\t/* ignore any further ones */\n> > +\t\t\t} else if (data.prefixes.nr >= MAX_ALLOWED_PREFIXES) {\n> > +\t\t\t\tstrvec_clear(&data.prefixes);\n> > +\t\t\t\ttoo_many_prefixes = 1;\n> > +\t\t\t} else {\n> > +\t\t\t\tstrvec_push(&data.prefixes, out);\n> > +\t\t\t}\n> > +\t\t}\n> \n> The order of this if-statement is a little odd to me, but obviously\n> correct. I might have wrote:\n> \n> if (too_many_prefixes)\n> continue;\n> \n> if (data.prefixes.nr < MAX_ALLOWED_PREFIXES) {\n> strvec_push(&data.prefixes, out);\n> } else {\n> too_many_prefixes = 1;\n> strvec_clear(&data.prefixes);\n> }\n> \n> But certainly what you wrote here works just fine (so this is a cosmetic\n> comment, and not a functional one).\n\nMy view of it was: check every case that may avoid us pushing a prefix,\nand then finally push one. But that may have been related to my goal in\nwriting the patch. :)\n\n> > +test_expect_success 'ignore very large set of prefixes' '\n> > +\t# generate a large number of ref-prefixes that we expect\n> > +\t# to match nothing; the value here exceeds MAX_ALLOWED_PREFIXES\n> > +\t# from ls-refs.c.\n> > +\t{\n> > +\t\techo command=ls-refs &&\n> > +\t\techo object-format=$(test_oid algo)\n> > +\t\techo 0001 &&\n> > +\t\tperl -le \"print \\\"refs/heads/$_\\\" for (1..65536+1)\" &&\n> > +\t\techo 0000\n> > +\t} |\n> > +\ttest-tool pkt-line pack >in &&\n> > +\n> > +\t# and then confirm that we see unmatched prefixes anyway (i.e.,\n> > +\t# that the prefix was not applied).\n> > +\tcat >expect <<-EOF &&\n> > +\t$(git rev-parse HEAD) HEAD\n> > +\t$(git rev-parse refs/heads/dev) refs/heads/dev\n> > +\t$(git rev-parse refs/heads/main) refs/heads/main\n> > +\t$(git rev-parse refs/heads/release) refs/heads/release\n> > +\t$(git rev-parse refs/tags/annotated-tag) refs/tags/annotated-tag\n> > +\t$(git rev-parse refs/tags/one) refs/tags/one\n> > +\t$(git rev-parse refs/tags/two) refs/tags/two\n> \n> You could have written this as a loop over the unmatched prefixes, but I\n> vastly prefer the result you came up with, which is much more explicit\n> and doesn't require readers to parse out what the loop does.\n\nI actually think that:\n\n git for-each-ref --format='%(objectname) %(refname)' >expect\n\nis pretty readable (and is much more efficient, and nicely avoids the\nmaster/main brittle-ness, which I ran into while backporting this). But:\n\n - this matches what the rest of the script does\n\n - for-each-ref doesn't report on HEAD, so we have to add that in\n separately\n\n - the \"pkt-line unpack\" will include the flush packet, so we'd have to\n add that in, too.\n\n-Peff\n"},{"id":"435893","messageId":"YUDcK3/rCCybKk1w@nand.local","threadId":"56500","inReplyTo":"YUC/6n1hhUbMJiLw@coredump.intra.peff.net","subject":"Re: [PATCH 0/9] reducing memory allocations for v2 servers","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-09-14T17:30:19Z","receivedAt":"2021-09-14T17:30:23Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Sep 14, 2021 at 11:29:46AM -0400, Jeff King wrote:\n> While looking at [1], I noticed that v2 servers will read a few bits of\n> client input into strvecs. Even though we expect these to be small-ish,\n> there's nothing preventing a client from sending us a bunch of junk and\n> wasting memory.\n>\n> This series changes that, putting a cap on how much data we'll receive.\n> The two spots are the \"capabilities\" list we receive (before we even\n> dispatch to a particular command like ls-refs or fetch), and the\n> ref-prefix list we receive for ls-refs.\n>\n> [...]\n\nThanks. I reviewed this series carefully and all of my comments were\neither of the form \"you could have written it this way, but I'm equally\nhappy with what you wrote here\" or \"this behavior change won't affect\nreal users and so I'm OK with it\".\n\n Reviewed-by: Taylor Blau \n\nThanks,\nTaylor\n"},{"id":"435899","messageId":"xmqqy27z11gn.fsf@gitster.g","threadId":"56500","inReplyTo":"YUC/6n1hhUbMJiLw@coredump.intra.peff.net","subject":"Re: [PATCH 0/9] reducing memory allocations for v2 servers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-09-14T18:00:56Z","receivedAt":"2021-09-14T18:00:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> While looking at [1], I noticed that v2 servers will read a few bits of\n> client input into strvecs. Even though we expect these to be small-ish,\n> there's nothing preventing a client from sending us a bunch of junk and\n> wasting memory.\n\nThe title of the topic says \"reducing\", but it smells more to me\nlike this is about \"limiting\" allocations to protect ourselves. Am\nI reading the series correctly?\n\n> This series changes that, putting a cap on how much data we'll receive.\n> The two spots are the \"capabilities\" list we receive (before we even\n> dispatch to a particular command like ls-refs or fetch), and the\n> ref-prefix list we receive for ls-refs.\n>\n> To deal with the capabilities issue, we'll just handle each capability\n> line as it comes in, rather than storing a list. This requires a bit of\n> cleanup, which is why it takes up the first 6 patches. It also needs a\n> few other cleanups from ab/serve-cleanup (and so this series is based on\n> that). The dependencies there are both textual (e.g., using designated\n> initializers in the capabilities table) and functional (dropping the\n> \"keys\" parameter from v2 command() functions).\n>\n> Patch 7 fixes the ls-refs issue by degrading when we see too many\n> prefixes (which works because the capability is optional in the first\n> place).\n>\n> The final two patches aren't strictly necessary. They're a tightening of\n> the protocol against some bogus input that I noticed while doing the\n> earlier cleanups. That bogus input isn't really hurting anything, but I\n> think it makes sense to reject nonsense from the client rather than\n> ignoring it. The obvious risk is that some client for some reason would\n> send us nonsense, but I don't think Git ever has.\n\nYour cover letters (as proposed log messages for individual commits)\nare always to the point and very pleasant to read. Very much\nappreciated.\n\nThanks.\n"},{"id":"435902","messageId":"YUDsJNV2mvKl5g29@coredump.intra.peff.net","threadId":"56500","inReplyTo":"xmqqy27z11gn.fsf@gitster.g","subject":"Re: [PATCH 0/9] reducing memory allocations for v2 servers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-14T18:38:28Z","receivedAt":"2021-09-14T18:38:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 14, 2021 at 11:00:56AM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > While looking at [1], I noticed that v2 servers will read a few bits of\n> > client input into strvecs. Even though we expect these to be small-ish,\n> > there's nothing preventing a client from sending us a bunch of junk and\n> > wasting memory.\n> \n> The title of the topic says \"reducing\", but it smells more to me\n> like this is about \"limiting\" allocations to protect ourselves. Am\n> I reading the series correctly?\n\nYeah, I think \"limiting\" is probably a better word. We will still\nallocate for ref-prefix, of course, but we'll limit the number of\nallocations. We did \"reduce\" the number of spots that allocate, since\ncapabilities no longer do so. :)\n\nBut I think your understanding of the series is correct.\n\n-Peff\n"},{"id":"435904","messageId":"CAN0heSoRLHzuv0S26YLnKWpiisshD_fiRmfL5ZMYxAkht-uXWg@mail.gmail.com","threadId":"56500","inReplyTo":"YUDAUi627d6TVmUy@coredump.intra.peff.net","subject":"Re: [PATCH 4/9] serve: provide \"receive\" function for object-format capability","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2021-09-14T18:59:09Z","receivedAt":"2021-09-14T18:59:23Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On Tue, 14 Sept 2021 at 17:33, Jeff King wrote:\n> repository algorithm). Since the check_algorithm() function would now be\n> done to a single if() statement, I've just inlined it in its only\n> caller.\n\ns/done/down/, I believe.\n\n> There should be no change of behavior here, except for two\n> broken-protocol cases:\n\nFWIW, I agree with this.\n\n\nMartin\n"},{"id":"435905","messageId":"CAN0heSrCibpcU0NXfZfJvb7puRVLPRz2qEbko6+SEh_BcKtgvA@mail.gmail.com","threadId":"56500","inReplyTo":"YUDAtbHcbv6zNFxe@coredump.intra.peff.net","subject":"Re: [PATCH 5/9] serve: provide \"receive\" function for session-id capability","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2021-09-14T19:02:10Z","receivedAt":"2021-09-14T19:02:24Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On Tue, 14 Sept 2021 at 17:34, Jeff King wrote:\n>\n> Rather than pulling the session-id string from the list of collected\n> capabilities, we can handle it as soon as we receive it. This gets us\n> closer to dropping the collected list entirely.\n\nLooking good.\n\n> As this removes the last caller of the static has_capability(), we can\n> remove it, as well (and in fact we must to avoid -Wunused-function\n> complaining).\n\n> I had originally dropped has_capability() in a separate patch, to keep\n> this one more readable. That breaks bisectability, but only with\n> -Werror. I'm not sure where we should fall on that spectrum (I generally\n> bisect with -Wno-error just because warnings may come and go when\n> working with different compilers than what was normal at the time).\n>\n> Not that big a deal either way for this patch, but I wonder if people\n> have opinions in general.\n\nFirst of all, agreed about the \"not that big a deal\" part. Just a random\nthought: You could do the opposite of what Elijah sometimes does by\nfirst adding a \"MAYBE_UNUSED\" function, then actually using it. You'd\nadd \"MAYBE_UNUSED\" here, then the next commit would drop the whole\nthing. It could be worth it if you're removing many many lines so that\nthe \"actual\" change gets lost in the noise. But this patch isn't near\nany such threshold, IMHO (if there even is such a \"threshold\").\n\n> +static void session_id_receive(struct repository *r,\n> + const char *client_sid)\n> +{\n> + if (!client_sid)\n> + client_sid = \"\";\n> + trace2_data_string(\"transfer\", NULL, \"client-sid\", client_sid);\n> +}\n\nHandling NULL. Nice. :)\n\n\nMartin\n"},{"id":"435908","messageId":"CAN0heSp9RdFwSo+x5buHSCAOa0Kay7Wcs1tNKTDXWmkCM8Latw@mail.gmail.com","threadId":"56500","inReplyTo":"YUDBokYvEBnzwsIN@coredump.intra.peff.net","subject":"Re: [PATCH 7/9] ls-refs: ignore very long ref-prefix counts","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2021-09-14T19:06:55Z","receivedAt":"2021-09-14T19:07:09Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On Tue, 14 Sept 2021 at 17:38, Jeff King wrote:\n>\n> One possible solution is to just drop the connection when the limit is\n> reached. If we set it high enough, then only misbehaving or malicious\n> clients would hit it. But \"high enough\" is vague, and it's unfriendly if\n> we guess wrong and a legitimate client hits this.\n>\n> But we can do better. Since supporting the ref-prefix capability is\n> optional anyway, the client has to further cull the response based on\n> their own patterns. So we can simply ignore the patterns once we cross a\n> certain threshold. Note that we have to ignore _all_ patterns, not just\n> the ones past our limit (since otherwise we'd send too little data).\n\nThis all makes sense to me. At some point, we should be able to go \"I\ndon't know what you're trying to do, but let me just ignore all this\ncraziness and instead try to give you a useful result sooner rather than\nlater\".\n\nI do wonder if we should document that the client can't trust us to\nactually do all this culling. In general, I find that it's a matter of\nhygiene for the client to do its own checks, but with this change they\nactually *need* to do them. (Unless they know our limit and that they're\non the right side of it, but that kind of magic is even less hygienic.)\n\n> + else if (skip_prefix(arg, \"ref-prefix \", &out)) {\n> + if (too_many_prefixes) {\n> + /* ignore any further ones */\n> + } else if (data.prefixes.nr >= MAX_ALLOWED_PREFIXES) {\n> + strvec_clear(&data.prefixes);\n> + too_many_prefixes = 1;\n> + } else {\n> + strvec_push(&data.prefixes, out);\n> + }\n> + }\n\nIs it easier to reason about with something like this\n(whitespace-damaged) on top?\n\ndiff --git a/ls-refs.c b/ls-refs.c\nindex 839fb0caa9..b3101ff361 100644\n--- a/ls-refs.c\n+++ b/ls-refs.c\n@@ -147,7 +147,6 @@ static int ls_refs_config(const char *var, const\nchar *value, void *data)\n int ls_refs(struct repository *r, struct packet_reader *request)\n {\n struct ls_refs_data data;\n- int too_many_prefixes = 0;\n\n memset(&data, 0, sizeof(data));\n strvec_init(&data.prefixes);\n@@ -164,14 +163,8 @@ int ls_refs(struct repository *r, struct\npacket_reader *request)\n else if (!strcmp(\"symrefs\", arg))\n data.symrefs = 1;\n else if (skip_prefix(arg, \"ref-prefix \", &out)) {\n- if (too_many_prefixes) {\n- /* ignore any further ones */\n- } else if (data.prefixes.nr >= MAX_ALLOWED_PREFIXES) {\n- strvec_clear(&data.prefixes);\n- too_many_prefixes = 1;\n- } else {\n+ if (data.prefixes.nr <= MAX_ALLOWED_PREFIXES)\n strvec_push(&data.prefixes, out);\n- }\n }\n else if (!strcmp(\"unborn\", arg))\n data.unborn = allow_unborn;\n@@ -180,6 +173,9 @@ int ls_refs(struct repository *r, struct\npacket_reader *request)\n if (request->status != PACKET_READ_FLUSH)\n die(_(\"expected flush after ls-refs arguments\"));\n\n+ if (data.prefixes.nr > MAX_ALLOWED_PREFIXES)\n+ strvec_clear(&data.prefixes);\n+\n send_possibly_unborn_head(&data);\n if (!data.prefixes.nr)\n strvec_push(&data.prefixes, \"\");\n\nMaybe even name the macro TOO_MANY_PREFIXES (and bump it by one)\nto make the logic instead be\n\n if (data.prefixes.nr < TOO_MANY_PREFIXES)\n strvec_push(&data.prefixes, out);\n ...\n if (data.prefixes.nr >= TOO_MANY_PREFIXES)\n strvec_clear(&data.prefixes);\n\nJust a thought. I'm reaching to try to find a way to improve this\nseries. ;-) It was a nice read.\n\n\nMartin\n"},{"id":"435911","messageId":"YUD0hXddD1+BF82a@coredump.intra.peff.net","threadId":"56500","inReplyTo":"CAN0heSrCibpcU0NXfZfJvb7puRVLPRz2qEbko6+SEh_BcKtgvA@mail.gmail.com","subject":"Re: [PATCH 5/9] serve: provide \"receive\" function for session-id capability","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-14T19:14:13Z","receivedAt":"2021-09-14T19:14:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 14, 2021 at 09:02:10PM +0200, Martin Ågren wrote:\n\n> > I had originally dropped has_capability() in a separate patch, to keep\n> > this one more readable. That breaks bisectability, but only with\n> > -Werror. I'm not sure where we should fall on that spectrum (I generally\n> > bisect with -Wno-error just because warnings may come and go when\n> > working with different compilers than what was normal at the time).\n> >\n> > Not that big a deal either way for this patch, but I wonder if people\n> > have opinions in general.\n> \n> First of all, agreed about the \"not that big a deal\" part. Just a random\n> thought: You could do the opposite of what Elijah sometimes does by\n> first adding a \"MAYBE_UNUSED\" function, then actually using it. You'd\n> add \"MAYBE_UNUSED\" here, then the next commit would drop the whole\n> thing. It could be worth it if you're removing many many lines so that\n> the \"actual\" change gets lost in the noise. But this patch isn't near\n> any such threshold, IMHO (if there even is such a \"threshold\").\n\nYeah, I considered that (because I had seen Elijah do it; I didn't think\nof it myself). I don't love it, if only because now the extra\nMAYBE_UNUSED is a head-scratcher for somebody reading the patch. I think\nit makes sense if the code will exist in that maybe-unused state for a\nwhile, but here it's just going away immediately anyway. I dunno.\n\n> > +static void session_id_receive(struct repository *r,\n> > + const char *client_sid)\n> > +{\n> > + if (!client_sid)\n> > + client_sid = \"\";\n> > + trace2_data_string(\"transfer\", NULL, \"client-sid\", client_sid);\n> > +}\n> \n> Handling NULL. Nice. :)\n\nOtherwise segfault if the client just says \"session-id\". :)\n\nTo be clear, the old code behaved the same way. It's just that\nhas_capability() returned the empty string for this case instead of\nNULL. I changed get_capability() to distinguish the two so that the\nlater fixes for \"command=ls-refs=whatever\" could treat them differently.\n\nI didn't add tests for this case (nor for \"object-format\" without a\nvalue), but we could do that if anybody cares.\n\n-Peff\n"},{"id":"435912","messageId":"YUD2ZEqKSU/pMzws@coredump.intra.peff.net","threadId":"56500","inReplyTo":"CAN0heSp9RdFwSo+x5buHSCAOa0Kay7Wcs1tNKTDXWmkCM8Latw@mail.gmail.com","subject":"Re: [PATCH 7/9] ls-refs: ignore very long ref-prefix counts","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-14T19:22:12Z","receivedAt":"2021-09-14T19:22:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 14, 2021 at 09:06:55PM +0200, Martin Ågren wrote:\n\n> > But we can do better. Since supporting the ref-prefix capability is\n> > optional anyway, the client has to further cull the response based on\n> > their own patterns. So we can simply ignore the patterns once we cross a\n> > certain threshold. Note that we have to ignore _all_ patterns, not just\n> > the ones past our limit (since otherwise we'd send too little data).\n> \n> This all makes sense to me. At some point, we should be able to go \"I\n> don't know what you're trying to do, but let me just ignore all this\n> craziness and instead try to give you a useful result sooner rather than\n> later\".\n> \n> I do wonder if we should document that the client can't trust us to\n> actually do all this culling. In general, I find that it's a matter of\n> hygiene for the client to do its own checks, but with this change they\n> actually *need* to do them. (Unless they know our limit and that they're\n> on the right side of it, but that kind of magic is even less hygienic.)\n\nPerhaps we could say so more explicitly in the v2 protocol spec. I'll\ntake a look.\n\n> > + else if (skip_prefix(arg, \"ref-prefix \", &out)) {\n> > + if (too_many_prefixes) {\n> > + /* ignore any further ones */\n> > + } else if (data.prefixes.nr >= MAX_ALLOWED_PREFIXES) {\n> > + strvec_clear(&data.prefixes);\n> > + too_many_prefixes = 1;\n> > + } else {\n> > + strvec_push(&data.prefixes, out);\n> > + }\n> > + }\n> \n> Is it easier to reason about with something like this\n> (whitespace-damaged) on top?\n\nYou're the second person to complain about this if-else chain. I'll take\nthe hint. ;)\n\n> diff --git a/ls-refs.c b/ls-refs.c\n> index 839fb0caa9..b3101ff361 100644\n> --- a/ls-refs.c\n> +++ b/ls-refs.c\n> @@ -147,7 +147,6 @@ static int ls_refs_config(const char *var, const\n> char *value, void *data)\n> int ls_refs(struct repository *r, struct packet_reader *request)\n> {\n> struct ls_refs_data data;\n> - int too_many_prefixes = 0;\n> \n> memset(&data, 0, sizeof(data));\n> strvec_init(&data.prefixes);\n> @@ -164,14 +163,8 @@ int ls_refs(struct repository *r, struct\n> packet_reader *request)\n> else if (!strcmp(\"symrefs\", arg))\n> data.symrefs = 1;\n> else if (skip_prefix(arg, \"ref-prefix \", &out)) {\n> - if (too_many_prefixes) {\n> - /* ignore any further ones */\n> - } else if (data.prefixes.nr >= MAX_ALLOWED_PREFIXES) {\n> - strvec_clear(&data.prefixes);\n> - too_many_prefixes = 1;\n> - } else {\n> + if (data.prefixes.nr <= MAX_ALLOWED_PREFIXES)\n> strvec_push(&data.prefixes, out);\n> - }\n> }\n\nHmm. At first I liked this, because it reduces the number of cases (and\nvariables!). But there's something really subtle going on here. I\nthought at first it should be \"<\", but you are intentionally\nover-allocating by one entry to indicate the overflow. I.e., you've\nessentially stuffed the too_many_prefixes boolean into the count.\n\n> @@ -180,6 +173,9 @@ int ls_refs(struct repository *r, struct\n> packet_reader *request)\n> if (request->status != PACKET_READ_FLUSH)\n> die(_(\"expected flush after ls-refs arguments\"));\n> \n> + if (data.prefixes.nr > MAX_ALLOWED_PREFIXES)\n> + strvec_clear(&data.prefixes);\n> +\n\nThis is far from the parser, but I think that's OK. I'd probably couple\nit with a comment explaining why we need to clear rather than using what\nwe got.\n\n> Maybe even name the macro TOO_MANY_PREFIXES (and bump it by one)\n> to make the logic instead be\n> \n> if (data.prefixes.nr < TOO_MANY_PREFIXES)\n> strvec_push(&data.prefixes, out);\n> ...\n> if (data.prefixes.nr >= TOO_MANY_PREFIXES)\n> strvec_clear(&data.prefixes);\n\nAt first I thought this was just being cute, but it's an attempt to\ncompensate for the off-by-one subtlety in the early check. I'll give it\nsome thought. I kind of like it, but the fact that it took me a minute\nor three to be sure the code is correct makes me worried it's being too\nclever.\n\n-Peff\n"},{"id":"435933","messageId":"YUEdqRQ4qQdHuSV/@coredump.intra.peff.net","threadId":"56500","inReplyTo":"YUDBokYvEBnzwsIN@coredump.intra.peff.net","subject":"Re: [PATCH 7/9] ls-refs: ignore very long ref-prefix counts","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-14T22:09:45Z","receivedAt":"2021-09-14T22:09:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 14, 2021 at 11:37:06AM -0400, Jeff King wrote:\n\n> +test_expect_success 'ignore very large set of prefixes' '\n> +\t# generate a large number of ref-prefixes that we expect\n> +\t# to match nothing; the value here exceeds MAX_ALLOWED_PREFIXES\n> +\t# from ls-refs.c.\n> +\t{\n> +\t\techo command=ls-refs &&\n> +\t\techo object-format=$(test_oid algo)\n> +\t\techo 0001 &&\n> +\t\tperl -le \"print \\\"refs/heads/$_\\\" for (1..65536+1)\" &&\n> +\t\techo 0000\n> +\t} |\n> +\ttest-tool pkt-line pack >in &&\n\nYuck. While double-checking some refactoring, I realized this test does\nnot actually generate the correct input!\n\nIt omits the \"ref-prefix\" header. _And_ it accidentally expands $_ in\nthe shell rather than in perl.\n\nThe test does work once corrected. And in fact I had originally written\nit correctly as a \"test_seq | sed\" pipeline, but generating 64k+ lines\nin the shell seemed a bit much (we do avoid sub-processes, but the \"set\n-x\" output is unwieldy).\n\nI'll fix it in a re-roll.\n\n-Peff\n"},{"id":"435934","messageId":"YUEeFENv04Ht5zXd@nand.local","threadId":"56500","inReplyTo":"YUEdqRQ4qQdHuSV/@coredump.intra.peff.net","subject":"Re: [PATCH 7/9] ls-refs: ignore very long ref-prefix counts","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-09-14T22:11:32Z","receivedAt":"2021-09-14T22:11:39Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Sep 14, 2021 at 06:09:45PM -0400, Jeff King wrote:\n> On Tue, Sep 14, 2021 at 11:37:06AM -0400, Jeff King wrote:\n>\n> > +test_expect_success 'ignore very large set of prefixes' '\n> > +\t# generate a large number of ref-prefixes that we expect\n> > +\t# to match nothing; the value here exceeds MAX_ALLOWED_PREFIXES\n> > +\t# from ls-refs.c.\n> > +\t{\n> > +\t\techo command=ls-refs &&\n> > +\t\techo object-format=$(test_oid algo)\n> > +\t\techo 0001 &&\n> > +\t\tperl -le \"print \\\"refs/heads/$_\\\" for (1..65536+1)\" &&\n> > +\t\techo 0000\n> > +\t} |\n> > +\ttest-tool pkt-line pack >in &&\n>\n> Yuck. While double-checking some refactoring, I realized this test does\n> not actually generate the correct input!\n>\n> It omits the \"ref-prefix\" header. _And_ it accidentally expands $_ in\n> the shell rather than in perl.\n\nHah, nice find. You'd think that one of us would have caught it earlier\ngiven that we both discussed it.\n\nThanks,\nTaylor\n"},{"id":"435935","messageId":"YUEfDBjAAsH9SLeY@coredump.intra.peff.net","threadId":"56500","inReplyTo":"YUEeFENv04Ht5zXd@nand.local","subject":"Re: [PATCH 7/9] ls-refs: ignore very long ref-prefix counts","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-14T22:15:40Z","receivedAt":"2021-09-14T22:15:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 14, 2021 at 06:11:32PM -0400, Taylor Blau wrote:\n\n> On Tue, Sep 14, 2021 at 06:09:45PM -0400, Jeff King wrote:\n> > On Tue, Sep 14, 2021 at 11:37:06AM -0400, Jeff King wrote:\n> >\n> > > +test_expect_success 'ignore very large set of prefixes' '\n> > > +\t# generate a large number of ref-prefixes that we expect\n> > > +\t# to match nothing; the value here exceeds MAX_ALLOWED_PREFIXES\n> > > +\t# from ls-refs.c.\n> > > +\t{\n> > > +\t\techo command=ls-refs &&\n> > > +\t\techo object-format=$(test_oid algo)\n> > > +\t\techo 0001 &&\n> > > +\t\tperl -le \"print \\\"refs/heads/$_\\\" for (1..65536+1)\" &&\n> > > +\t\techo 0000\n> > > +\t} |\n> > > +\ttest-tool pkt-line pack >in &&\n> >\n> > Yuck. While double-checking some refactoring, I realized this test does\n> > not actually generate the correct input!\n> >\n> > It omits the \"ref-prefix\" header. _And_ it accidentally expands $_ in\n> > the shell rather than in perl.\n> \n> Hah, nice find. You'd think that one of us would have caught it earlier\n> given that we both discussed it.\n\nReally, I'd have thought that ls-refs would complain about a totally\nbogus capability. I'll see if I can fix that, as well.\n\n-Peff\n"},{"id":"435940","messageId":"YUE1alo58cGyTw6/@coredump.intra.peff.net","threadId":"56500","inReplyTo":"YUC/6n1hhUbMJiLw@coredump.intra.peff.net","subject":"[PATCH v2 0/11] limit memory allocations for v2 servers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-14T23:51:06Z","receivedAt":"2021-09-14T23:51:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Here's a re-roll of my series to limit the memory a v2 server is willing\nto allocate on behalf of a client. See v1:\n\n https://lore.kernel.org/git/YUC%2F6n1hhUbMJiLw@coredump.intra.peff.net/\n\nfor an overview. The existing patches are mostly small fixups pointed\nout by reviewers (thanks!), but I did take Martin's TOO_MANY_PREFIXES\nsuggestion in patch 7 (without the change to the name of the constant it\nseemed too clever to me, but with it it seems just the right amount of\nclever).\n\nThere are two new patches here:\n\n - patch 8 explicitly documents the v2 ref-prefix \"best effort\" behavior\n\n - patch 11 teaches ls-refs to reject bogus arguments, which violates\n the spec (the current behavior caused a broken test to go unnoticed)\n\nFull range-diff is below the diffstat.\n\n [01/11]: serve: rename is_command() to parse_command()\n [02/11]: serve: return capability \"value\" from get_capability()\n [03/11]: serve: add \"receive\" method for v2 capabilities table\n [04/11]: serve: provide \"receive\" function for object-format capability\n [05/11]: serve: provide \"receive\" function for session-id capability\n [06/11]: serve: drop \"keys\" strvec\n [07/11]: ls-refs: ignore very long ref-prefix counts\n [08/11]: docs/protocol-v2: clarify some ls-refs ref-prefix details\n [09/11]: serve: reject bogus v2 \"command=ls-refs=foo\"\n [10/11]: serve: reject commands used as capabilities\n [11/11]: ls-refs: reject unknown arguments\n\n Documentation/technical/protocol-v2.txt | 6 +-\n ls-refs.c | 22 ++++-\n serve.c | 119 +++++++++++++-----------\n t/t5701-git-serve.sh | 73 +++++++++++++++\n 4 files changed, 162 insertions(+), 58 deletions(-)\n\n 1: eb8e7b21a1 = 1: eb8e7b21a1 serve: rename is_command() to parse_command()\n 2: 8cc66cae41 = 2: 8cc66cae41 serve: return capability \"value\" from get_capability()\n 3: 3343f9bb0a = 3: 3343f9bb0a serve: add \"receive\" method for v2 capabilities table\n 4: c4cc80fe7a ! 4: 0319b69881 serve: provide \"receive\" function for object-format capability\n @@ Commit message\n all capabilities (because they might not have sent an object-format line\n at all, and we still have to check that the default matches our\n repository algorithm). Since the check_algorithm() function would now be\n - done to a single if() statement, I've just inlined it in its only\n + down to a single if() statement, I've just inlined it in its only\n caller.\n \n There should be no change of behavior here, except for two\n 5: c8527ca5a7 = 5: efe207c35c serve: provide \"receive\" function for session-id capability\n 6: 250e4723ba ! 6: 463aa7faa3 serve: drop \"keys\" strvec\n @@ serve.c: static int process_request(void)\n \t\t\telse\n \t\t\t\tdie(\"unknown capability '%s'\", reader.line);\n \n -+\n - \t\t\t/* Consume the peeked line */\n - \t\t\tpacket_reader_read(&reader);\n - \t\t\tbreak;\n @@ serve.c: static int process_request(void)\n \t\t\t * If no command and no keys were given then the client\n \t\t\t * wanted to terminate the connection.\n 7: 1218d62247 ! 7: da2043f42f ls-refs: ignore very long ref-prefix counts\n @@ ls-refs.c: static void ensure_config_read(void)\n }\n \n +/*\n -+ * The maximum number of \"ref-prefix\" lines we'll allow the client to send.\n -+ * If they go beyond this, we'll avoid using the prefix feature entirely.\n ++ * If we see this many or more \"ref-prefix\" lines from the client, we consider\n ++ * it \"too many\" and will avoid using the prefix feature entirely.\n + */\n -+#define MAX_ALLOWED_PREFIXES 65536\n ++#define TOO_MANY_PREFIXES 65536\n +\n /*\n * Check if one of the prefixes is a prefix of the ref.\n * If no prefixes were provided, all refs match.\n -@@ ls-refs.c: static int ls_refs_config(const char *var, const char *value, void *data)\n - int ls_refs(struct repository *r, struct packet_reader *request)\n - {\n - \tstruct ls_refs_data data;\n -+\tint too_many_prefixes = 0;\n - \n - \tmemset(&data, 0, sizeof(data));\n - \tstrvec_init(&data.prefixes);\n @@ ls-refs.c: int ls_refs(struct repository *r, struct packet_reader *request)\n \t\t\tdata.peel = 1;\n \t\telse if (!strcmp(\"symrefs\", arg))\n \t\t\tdata.symrefs = 1;\n -\t\telse if (skip_prefix(arg, \"ref-prefix \", &out))\n -\t\t\tstrvec_push(&data.prefixes, out);\n +\t\telse if (skip_prefix(arg, \"ref-prefix \", &out)) {\n -+\t\t\tif (too_many_prefixes) {\n -+\t\t\t\t/* ignore any further ones */\n -+\t\t\t} else if (data.prefixes.nr >= MAX_ALLOWED_PREFIXES) {\n -+\t\t\t\tstrvec_clear(&data.prefixes);\n -+\t\t\t\ttoo_many_prefixes = 1;\n -+\t\t\t} else {\n ++\t\t\tif (data.prefixes.nr < TOO_MANY_PREFIXES)\n +\t\t\t\tstrvec_push(&data.prefixes, out);\n -+\t\t\t}\n +\t\t}\n \t\telse if (!strcmp(\"unborn\", arg))\n \t\t\tdata.unborn = allow_unborn;\n \t}\n + \n + \tif (request->status != PACKET_READ_FLUSH)\n + \t\tdie(_(\"expected flush after ls-refs arguments\"));\n + \n ++\t/*\n ++\t * If we saw too many prefixes, we must avoid using them at all; as\n ++\t * soon as we have any prefix, they are meant to form a comprehensive\n ++\t * list.\n ++\t */\n ++\tif (data.prefixes.nr >= TOO_MANY_PREFIXES)\n ++\t\tstrvec_clear(&data.prefixes);\n ++\n + \tsend_possibly_unborn_head(&data);\n + \tif (!data.prefixes.nr)\n + \t\tstrvec_push(&data.prefixes, \"\");\n \n ## t/t5701-git-serve.sh ##\n @@ t/t5701-git-serve.sh: test_expect_success 'refs/heads prefix' '\n @@ t/t5701-git-serve.sh: test_expect_success 'refs/heads prefix' '\n \n +test_expect_success 'ignore very large set of prefixes' '\n +\t# generate a large number of ref-prefixes that we expect\n -+\t# to match nothing; the value here exceeds MAX_ALLOWED_PREFIXES\n ++\t# to match nothing; the value here exceeds TOO_MANY_PREFIXES\n +\t# from ls-refs.c.\n +\t{\n +\t\techo command=ls-refs &&\n +\t\techo object-format=$(test_oid algo)\n +\t\techo 0001 &&\n -+\t\tperl -le \"print \\\"refs/heads/$_\\\" for (1..65536+1)\" &&\n ++\t\tperl -le \"print \\\"ref-prefix refs/heads/\\$_\\\" for (1..65536)\" &&\n +\t\techo 0000\n +\t} |\n +\ttest-tool pkt-line pack >in &&\n -: ---------- > 8: ee540a4ef7 docs/protocol-v2: clarify some ls-refs ref-prefix details\n 8: b1567fdc82 = 9: 481c07cfac serve: reject bogus v2 \"command=ls-refs=foo\"\n 9: 9786b9a11f = 10: dff965c1d2 serve: reject commands used as capabilities\n -: ---------- > 11: f7339f924b ls-refs: reject unknown arguments\n"},{"id":"435941","messageId":"YUE1dXos1UP9kO+h@coredump.intra.peff.net","threadId":"56500","inReplyTo":"YUE1alo58cGyTw6/@coredump.intra.peff.net","subject":"[PATCH v2 01/11] serve: rename is_command() to parse_command()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-14T23:51:17Z","receivedAt":"2021-09-14T23:51:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The is_command() function not only tells us whether the pktline is a\nvalid command string, but it also parses out the command (and complains\nif we see a duplicate). Let's rename it to make those extra functions\na bit more obvious.\n\nSigned-off-by: Jeff King \n---\n serve.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/serve.c b/serve.c\nindex 1817edc7f5..fd88b95343 100644\n--- a/serve.c\n+++ b/serve.c\n@@ -163,7 +163,7 @@ static int is_valid_capability(const char *key)\n \treturn c && c->advertise(the_repository, NULL);\n }\n \n-static int is_command(const char *key, struct protocol_capability **command)\n+static int parse_command(const char *key, struct protocol_capability **command)\n {\n \tconst char *out;\n \n@@ -251,7 +251,7 @@ static int process_request(void)\n \t\t\tBUG(\"Should have already died when seeing EOF\");\n \t\tcase PACKET_READ_NORMAL:\n \t\t\t/* collect request; a sequence of keys and values */\n-\t\t\tif (is_command(reader.line, &command) ||\n+\t\t\tif (parse_command(reader.line, &command) ||\n \t\t\t is_valid_capability(reader.line))\n \t\t\t\tstrvec_push(&keys, reader.line);\n \t\t\telse\n-- \n2.33.0.917.gae6ecbedc7\n\n"},{"id":"435942","messageId":"YUE1eSumQN2EXxiX@coredump.intra.peff.net","threadId":"56500","inReplyTo":"YUE1alo58cGyTw6/@coredump.intra.peff.net","subject":"[PATCH v2 02/11] serve: return capability \"value\" from get_capability()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-14T23:51:21Z","receivedAt":"2021-09-14T23:51:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When the client sends v2 capabilities, they may be simple, like:\n\n foo\n\nor have a value like:\n\n foo=bar\n\n(all of the current capabilities actually expect a value, but the\nprotocol allows for boolean ones).\n\nWe use get_capability() to make sure the client's pktline matches a\ncapability. In doing so, we parse enough to see the \"=\" and the value\n(if any), but we immediately forget it. Nobody cares for now, because they end\nup parsing the values out later using has_capability(). But in\npreparation for changing that, let's pass back a pointer so the callers\nknow what we found.\n\nNote that unlike has_capability(), we'll return NULL for a \"simple\"\ncapability. Distinguishing these will be useful for some future patches.\n\nSigned-off-by: Jeff King \n---\n serve.c | 18 ++++++++++++++----\n 1 file changed, 14 insertions(+), 4 deletions(-)\n\ndiff --git a/serve.c b/serve.c\nindex fd88b95343..78a4e83554 100644\n--- a/serve.c\n+++ b/serve.c\n@@ -139,7 +139,7 @@ void protocol_v2_advertise_capabilities(void)\n \tstrbuf_release(&value);\n }\n \n-static struct protocol_capability *get_capability(const char *key)\n+static struct protocol_capability *get_capability(const char *key, const char **value)\n {\n \tint i;\n \n@@ -149,16 +149,25 @@ static struct protocol_capability *get_capability(const char *key)\n \tfor (i = 0; i < ARRAY_SIZE(capabilities); i++) {\n \t\tstruct protocol_capability *c = &capabilities[i];\n \t\tconst char *out;\n-\t\tif (skip_prefix(key, c->name, &out) && (!*out || *out == '='))\n+\t\tif (!skip_prefix(key, c->name, &out))\n+\t\t\tcontinue;\n+\t\tif (!*out) {\n+\t\t\t*value = NULL;\n \t\t\treturn c;\n+\t\t}\n+\t\tif (*out++ == '=') {\n+\t\t\t*value = out;\n+\t\t\treturn c;\n+\t\t}\n \t}\n \n \treturn NULL;\n }\n \n static int is_valid_capability(const char *key)\n {\n-\tconst struct protocol_capability *c = get_capability(key);\n+\tconst char *value;\n+\tconst struct protocol_capability *c = get_capability(key, &value);\n \n \treturn c && c->advertise(the_repository, NULL);\n }\n@@ -168,7 +177,8 @@ static int parse_command(const char *key, struct protocol_capability **command)\n \tconst char *out;\n \n \tif (skip_prefix(key, \"command=\", &out)) {\n-\t\tstruct protocol_capability *cmd = get_capability(out);\n+\t\tconst char *value;\n+\t\tstruct protocol_capability *cmd = get_capability(out, &value);\n \n \t\tif (*command)\n \t\t\tdie(\"command '%s' requested after already requesting command '%s'\",\n-- \n2.33.0.917.gae6ecbedc7\n\n"},{"id":"435943","messageId":"YUE1fGZc1FuuyUNH@coredump.intra.peff.net","threadId":"56500","inReplyTo":"YUE1alo58cGyTw6/@coredump.intra.peff.net","subject":"[PATCH v2 03/11] serve: add \"receive\" method for v2 capabilities table","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-14T23:51:24Z","receivedAt":"2021-09-14T23:51:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We have a capabilities table that tells us what we should tell the\nclient we are capable of, and what to do when a client gives us a\nparticular command (e.g., \"command=ls-refs\"). But it doesn't tell us\nwhat to do when the client sends us back a capability (e.g.,\n\"object-format=sha256\"). We just collect them all in a strvec and hope\nsomebody can use them later.\n\nInstead, let's provide a function pointer in the table to act on these.\nThis will eventually help us avoid collecting the strings, which will be\nmore efficient and less prone to mischief.\n\nUsing the new method is optional, which helps in two ways:\n\n - we can move existing capabilities over to this new system gradually\n in individual commits\n\n - some capabilities we don't actually do anything with anyway. For\n example, the client is free to say \"agent=git/1.2.3\" to us, but we\n do not act on the information in any way.\n\nSigned-off-by: Jeff King \n---\n serve.c | 21 ++++++++++++++++++---\n 1 file changed, 18 insertions(+), 3 deletions(-)\n\ndiff --git a/serve.c b/serve.c\nindex 78a4e83554..a161189984 100644\n--- a/serve.c\n+++ b/serve.c\n@@ -70,6 +70,16 @@ struct protocol_capability {\n \t * This field should be NULL for capabilities which are not commands.\n \t */\n \tint (*command)(struct repository *r, struct packet_reader *request);\n+\n+\t/*\n+\t * Function called when a client requests the capability as a\n+\t * non-command. This may be NULL if the capability does nothing.\n+\t *\n+\t * For a capability of the form \"foo=bar\", the value string points to\n+\t * the content after the \"=\" (i.e., \"bar\"). For simple capabilities\n+\t * (just \"foo\"), it is NULL.\n+\t */\n+\tvoid (*receive)(struct repository *r, const char *value);\n };\n \n static struct protocol_capability capabilities[] = {\n@@ -164,12 +174,17 @@ static struct protocol_capability *get_capability(const char *key, const char **\n \treturn NULL;\n }\n \n-static int is_valid_capability(const char *key)\n+static int receive_client_capability(const char *key)\n {\n \tconst char *value;\n \tconst struct protocol_capability *c = get_capability(key, &value);\n \n-\treturn c && c->advertise(the_repository, NULL);\n+\tif (!c || !c->advertise(the_repository, NULL))\n+\t\treturn 0;\n+\n+\tif (c->receive)\n+\t\tc->receive(the_repository, value);\n+\treturn 1;\n }\n \n static int parse_command(const char *key, struct protocol_capability **command)\n@@ -262,7 +277,7 @@ static int process_request(void)\n \t\tcase PACKET_READ_NORMAL:\n \t\t\t/* collect request; a sequence of keys and values */\n \t\t\tif (parse_command(reader.line, &command) ||\n-\t\t\t is_valid_capability(reader.line))\n+\t\t\t receive_client_capability(reader.line))\n \t\t\t\tstrvec_push(&keys, reader.line);\n \t\t\telse\n \t\t\t\tdie(\"unknown capability '%s'\", reader.line);\n-- \n2.33.0.917.gae6ecbedc7\n\n"},{"id":"435944","messageId":"YUE1fy0eA0wQnn3f@coredump.intra.peff.net","threadId":"56500","inReplyTo":"YUE1alo58cGyTw6/@coredump.intra.peff.net","subject":"[PATCH v2 04/11] serve: provide \"receive\" function for object-format capability","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-14T23:51:27Z","receivedAt":"2021-09-14T23:51:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We get any \"object-format\" specified by the client by searching for it\nin the collected list of capabilities the client sent. We can instead\njust handle it as soon as they send it. This is slightly more efficient,\nand gets us one step closer to dropping that collected list.\n\nNote that we do still have to do our final hash check after receiving\nall capabilities (because they might not have sent an object-format line\nat all, and we still have to check that the default matches our\nrepository algorithm). Since the check_algorithm() function would now be\ndown to a single if() statement, I've just inlined it in its only\ncaller.\n\nThere should be no change of behavior here, except for two\nbroken-protocol cases:\n\n - if the client sends multiple conflicting object-format capabilities\n (which they should not), we'll now choose the last one rather than\n the first. We could also detect and complain about the duplicates\n quite easily now, which we could not before, but I didn't do so\n here.\n\n - if the client sends a bogus \"object-format\" with no equals sign,\n we'll now say so, rather than \"unknown object format: ''\"\n\nSigned-off-by: Jeff King \n---\n serve.c | 34 +++++++++++++++++-----------------\n 1 file changed, 17 insertions(+), 17 deletions(-)\n\ndiff --git a/serve.c b/serve.c\nindex a161189984..f6ea2953eb 100644\n--- a/serve.c\n+++ b/serve.c\n@@ -10,6 +10,7 @@\n #include \"upload-pack.h\"\n \n static int advertise_sid = -1;\n+static int client_hash_algo = GIT_HASH_SHA1;\n \n static int always_advertise(struct repository *r,\n \t\t\t struct strbuf *value)\n@@ -33,6 +34,17 @@ static int object_format_advertise(struct repository *r,\n \treturn 1;\n }\n \n+static void object_format_receive(struct repository *r,\n+\t\t\t\t const char *algo_name)\n+{\n+\tif (!algo_name)\n+\t\tdie(\"object-format capability requires an argument\");\n+\n+\tclient_hash_algo = hash_algo_by_name(algo_name);\n+\tif (client_hash_algo == GIT_HASH_UNKNOWN)\n+\t\tdie(\"unknown object format '%s'\", algo_name);\n+}\n+\n static int session_id_advertise(struct repository *r, struct strbuf *value)\n {\n \tif (advertise_sid == -1 &&\n@@ -104,6 +116,7 @@ static struct protocol_capability capabilities[] = {\n \t{\n \t\t.name = \"object-format\",\n \t\t.advertise = object_format_advertise,\n+\t\t.receive = object_format_receive,\n \t},\n \t{\n \t\t.name = \"session-id\",\n@@ -228,22 +241,6 @@ static int has_capability(const struct strvec *keys, const char *capability,\n \treturn 0;\n }\n \n-static void check_algorithm(struct repository *r, struct strvec *keys)\n-{\n-\tint client = GIT_HASH_SHA1, server = hash_algo_by_ptr(r->hash_algo);\n-\tconst char *algo_name;\n-\n-\tif (has_capability(keys, \"object-format\", &algo_name)) {\n-\t\tclient = hash_algo_by_name(algo_name);\n-\t\tif (client == GIT_HASH_UNKNOWN)\n-\t\t\tdie(\"unknown object format '%s'\", algo_name);\n-\t}\n-\n-\tif (client != server)\n-\t\tdie(\"mismatched object format: server %s; client %s\\n\",\n-\t\t r->hash_algo->name, hash_algos[client].name);\n-}\n-\n enum request_state {\n \tPROCESS_REQUEST_KEYS,\n \tPROCESS_REQUEST_DONE,\n@@ -317,7 +314,10 @@ static int process_request(void)\n \tif (!command)\n \t\tdie(\"no command requested\");\n \n-\tcheck_algorithm(the_repository, &keys);\n+\tif (client_hash_algo != hash_algo_by_ptr(the_repository->hash_algo))\n+\t\tdie(\"mismatched object format: server %s; client %s\\n\",\n+\t\t the_repository->hash_algo->name,\n+\t\t hash_algos[client_hash_algo].name);\n \n \tif (has_capability(&keys, \"session-id\", &client_sid))\n \t\ttrace2_data_string(\"transfer\", NULL, \"client-sid\", client_sid);\n-- \n2.33.0.917.gae6ecbedc7\n\n"},{"id":"435945","messageId":"YUE1gvovwj/X1wWV@coredump.intra.peff.net","threadId":"56500","inReplyTo":"YUE1alo58cGyTw6/@coredump.intra.peff.net","subject":"[PATCH v2 05/11] serve: provide \"receive\" function for session-id capability","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-14T23:51:30Z","receivedAt":"2021-09-14T23:51:33Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Rather than pulling the session-id string from the list of collected\ncapabilities, we can handle it as soon as we receive it. This gets us\ncloser to dropping the collected list entirely.\n\nThe behavior should be the same, with one exception. Previously if the\nclient sent us multiple session-id lines, we'd report only the first.\nNow we'll pass each one along to trace2. This shouldn't matter in\npractice, since clients shouldn't do that (and if they do, it's probably\nsensible to log them all).\n\nAs this removes the last caller of the static has_capability(), we can\nremove it, as well (and in fact we must to avoid -Wunused-function\ncomplaining).\n\nSigned-off-by: Jeff King \n---\n serve.c | 33 +++++++++------------------------\n 1 file changed, 9 insertions(+), 24 deletions(-)\n\ndiff --git a/serve.c b/serve.c\nindex f6ea2953eb..6bbf54cbbe 100644\n--- a/serve.c\n+++ b/serve.c\n@@ -57,6 +57,14 @@ static int session_id_advertise(struct repository *r, struct strbuf *value)\n \treturn 1;\n }\n \n+static void session_id_receive(struct repository *r,\n+\t\t\t const char *client_sid)\n+{\n+\tif (!client_sid)\n+\t\tclient_sid = \"\";\n+\ttrace2_data_string(\"transfer\", NULL, \"client-sid\", client_sid);\n+}\n+\n struct protocol_capability {\n \t/*\n \t * The name of the capability. The server uses this name when\n@@ -121,6 +129,7 @@ static struct protocol_capability capabilities[] = {\n \t{\n \t\t.name = \"session-id\",\n \t\t.advertise = session_id_advertise,\n+\t\t.receive = session_id_receive,\n \t},\n \t{\n \t\t.name = \"object-info\",\n@@ -221,26 +230,6 @@ static int parse_command(const char *key, struct protocol_capability **command)\n \treturn 0;\n }\n \n-static int has_capability(const struct strvec *keys, const char *capability,\n-\t\t\t const char **value)\n-{\n-\tint i;\n-\tfor (i = 0; i < keys->nr; i++) {\n-\t\tconst char *out;\n-\t\tif (skip_prefix(keys->v[i], capability, &out) &&\n-\t\t (!*out || *out == '=')) {\n-\t\t\tif (value) {\n-\t\t\t\tif (*out == '=')\n-\t\t\t\t\tout++;\n-\t\t\t\t*value = out;\n-\t\t\t}\n-\t\t\treturn 1;\n-\t\t}\n-\t}\n-\n-\treturn 0;\n-}\n-\n enum request_state {\n \tPROCESS_REQUEST_KEYS,\n \tPROCESS_REQUEST_DONE,\n@@ -252,7 +241,6 @@ static int process_request(void)\n \tstruct packet_reader reader;\n \tstruct strvec keys = STRVEC_INIT;\n \tstruct protocol_capability *command = NULL;\n-\tconst char *client_sid;\n \n \tpacket_reader_init(&reader, 0, NULL, 0,\n \t\t\t PACKET_READ_CHOMP_NEWLINE |\n@@ -319,9 +307,6 @@ static int process_request(void)\n \t\t the_repository->hash_algo->name,\n \t\t hash_algos[client_hash_algo].name);\n \n-\tif (has_capability(&keys, \"session-id\", &client_sid))\n-\t\ttrace2_data_string(\"transfer\", NULL, \"client-sid\", client_sid);\n-\n \tcommand->command(the_repository, &reader);\n \n \tstrvec_clear(&keys);\n-- \n2.33.0.917.gae6ecbedc7\n\n"},{"id":"435946","messageId":"YUE1hz/KKM0XebCP@coredump.intra.peff.net","threadId":"56500","inReplyTo":"YUE1alo58cGyTw6/@coredump.intra.peff.net","subject":"[PATCH v2 07/11] ls-refs: ignore very long ref-prefix counts","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-14T23:51:35Z","receivedAt":"2021-09-14T23:51:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Because each \"ref-prefix\" capability from the client comes in its own\npkt-line, there's no limit to the number of them that a misbehaving\nclient may send. We read them all into a strvec, which means the client\ncan waste arbitrary amounts of our memory by just sending us \"ref-prefix\nfoo\" over and over.\n\nOne possible solution is to just drop the connection when the limit is\nreached. If we set it high enough, then only misbehaving or malicious\nclients would hit it. But \"high enough\" is vague, and it's unfriendly if\nwe guess wrong and a legitimate client hits this.\n\nBut we can do better. Since supporting the ref-prefix capability is\noptional anyway, the client has to further cull the response based on\ntheir own patterns. So we can simply ignore the patterns once we cross a\ncertain threshold. Note that we have to ignore _all_ patterns, not just\nthe ones past our limit (since otherwise we'd send too little data).\n\nThe limit here is fairly arbitrary, and probably much higher than anyone\nwould need in practice. It might be worth limiting it further, if only\nbecause we check it linearly (so with \"m\" local refs and \"n\" patterns,\nwe do \"m * n\" string comparisons). But if we care about optimizing this,\nan even better solution may be a more advanced data structure anyway.\n\nI didn't bother making the limit configurable, since it's so high and\nsince Git should behave correctly in either case. It wouldn't be too\nhard to do, but it makes both the code and documentation more complex.\n\nSigned-off-by: Jeff King \n---\n ls-refs.c | 20 ++++++++++++++++++--\n t/t5701-git-serve.sh | 31 +++++++++++++++++++++++++++++++\n 2 files changed, 49 insertions(+), 2 deletions(-)\n\ndiff --git a/ls-refs.c b/ls-refs.c\nindex a1a0250607..18c4f41e87 100644\n--- a/ls-refs.c\n+++ b/ls-refs.c\n@@ -40,6 +40,12 @@ static void ensure_config_read(void)\n \tconfig_read = 1;\n }\n \n+/*\n+ * If we see this many or more \"ref-prefix\" lines from the client, we consider\n+ * it \"too many\" and will avoid using the prefix feature entirely.\n+ */\n+#define TOO_MANY_PREFIXES 65536\n+\n /*\n * Check if one of the prefixes is a prefix of the ref.\n * If no prefixes were provided, all refs match.\n@@ -156,15 +162,25 @@ int ls_refs(struct repository *r, struct packet_reader *request)\n \t\t\tdata.peel = 1;\n \t\telse if (!strcmp(\"symrefs\", arg))\n \t\t\tdata.symrefs = 1;\n-\t\telse if (skip_prefix(arg, \"ref-prefix \", &out))\n-\t\t\tstrvec_push(&data.prefixes, out);\n+\t\telse if (skip_prefix(arg, \"ref-prefix \", &out)) {\n+\t\t\tif (data.prefixes.nr < TOO_MANY_PREFIXES)\n+\t\t\t\tstrvec_push(&data.prefixes, out);\n+\t\t}\n \t\telse if (!strcmp(\"unborn\", arg))\n \t\t\tdata.unborn = allow_unborn;\n \t}\n \n \tif (request->status != PACKET_READ_FLUSH)\n \t\tdie(_(\"expected flush after ls-refs arguments\"));\n \n+\t/*\n+\t * If we saw too many prefixes, we must avoid using them at all; as\n+\t * soon as we have any prefix, they are meant to form a comprehensive\n+\t * list.\n+\t */\n+\tif (data.prefixes.nr >= TOO_MANY_PREFIXES)\n+\t\tstrvec_clear(&data.prefixes);\n+\n \tsend_possibly_unborn_head(&data);\n \tif (!data.prefixes.nr)\n \t\tstrvec_push(&data.prefixes, \"\");\ndiff --git a/t/t5701-git-serve.sh b/t/t5701-git-serve.sh\nindex 930721f053..3bc96ebcde 100755\n--- a/t/t5701-git-serve.sh\n+++ b/t/t5701-git-serve.sh\n@@ -158,6 +158,37 @@ test_expect_success 'refs/heads prefix' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'ignore very large set of prefixes' '\n+\t# generate a large number of ref-prefixes that we expect\n+\t# to match nothing; the value here exceeds TOO_MANY_PREFIXES\n+\t# from ls-refs.c.\n+\t{\n+\t\techo command=ls-refs &&\n+\t\techo object-format=$(test_oid algo)\n+\t\techo 0001 &&\n+\t\tperl -le \"print \\\"ref-prefix refs/heads/\\$_\\\" for (1..65536)\" &&\n+\t\techo 0000\n+\t} |\n+\ttest-tool pkt-line pack >in &&\n+\n+\t# and then confirm that we see unmatched prefixes anyway (i.e.,\n+\t# that the prefix was not applied).\n+\tcat >expect <<-EOF &&\n+\t$(git rev-parse HEAD) HEAD\n+\t$(git rev-parse refs/heads/dev) refs/heads/dev\n+\t$(git rev-parse refs/heads/main) refs/heads/main\n+\t$(git rev-parse refs/heads/release) refs/heads/release\n+\t$(git rev-parse refs/tags/annotated-tag) refs/tags/annotated-tag\n+\t$(git rev-parse refs/tags/one) refs/tags/one\n+\t$(git rev-parse refs/tags/two) refs/tags/two\n+\t0000\n+\tEOF\n+\n+\ttest-tool serve-v2 --stateless-rpc out &&\n+\ttest-tool pkt-line unpack actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'peel parameter' '\n \ttest-tool pkt-line pack >in <<-EOF &&\n \tcommand=ls-refs\n-- \n2.33.0.917.gae6ecbedc7\n\n"},{"id":"435947","messageId":"YUE1hExkU9V12iZv@coredump.intra.peff.net","threadId":"56500","inReplyTo":"YUE1alo58cGyTw6/@coredump.intra.peff.net","subject":"[PATCH v2 06/11] serve: drop \"keys\" strvec","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-14T23:51:32Z","receivedAt":"2021-09-14T23:51:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We collect the set of capabilities the client sends us in a strvec.\nWhile this is usually small, there's no limit to the number of\ncapabilities the client can send us (e.g., they could just send us\n\"agent\" pkt-lines over and over, and we'd keep adding them to the list).\n\nSince all code has been converted away from using this list, let's get\nrid of it. This avoids a potential attack where clients waste our\nmemory.\n\nNote that we do have to replace it with a flag, because some of the\nflush-packet logic checks whether we've seen any valid commands or keys.\n\nSigned-off-by: Jeff King \n---\n serve.c | 7 +++----\n 1 file changed, 3 insertions(+), 4 deletions(-)\n\ndiff --git a/serve.c b/serve.c\nindex 6bbf54cbbe..5ea6c915cb 100644\n--- a/serve.c\n+++ b/serve.c\n@@ -239,7 +239,7 @@ static int process_request(void)\n {\n \tenum request_state state = PROCESS_REQUEST_KEYS;\n \tstruct packet_reader reader;\n-\tstruct strvec keys = STRVEC_INIT;\n+\tint seen_capability_or_command = 0;\n \tstruct protocol_capability *command = NULL;\n \n \tpacket_reader_init(&reader, 0, NULL, 0,\n@@ -263,7 +263,7 @@ static int process_request(void)\n \t\t\t/* collect request; a sequence of keys and values */\n \t\t\tif (parse_command(reader.line, &command) ||\n \t\t\t receive_client_capability(reader.line))\n-\t\t\t\tstrvec_push(&keys, reader.line);\n+\t\t\t\tseen_capability_or_command = 1;\n \t\t\telse\n \t\t\t\tdie(\"unknown capability '%s'\", reader.line);\n \n@@ -275,7 +275,7 @@ static int process_request(void)\n \t\t\t * If no command and no keys were given then the client\n \t\t\t * wanted to terminate the connection.\n \t\t\t */\n-\t\t\tif (!keys.nr)\n+\t\t\tif (!seen_capability_or_command)\n \t\t\t\treturn 1;\n \n \t\t\t/*\n@@ -309,7 +309,6 @@ static int process_request(void)\n \n \tcommand->command(the_repository, &reader);\n \n-\tstrvec_clear(&keys);\n \treturn 0;\n }\n \n-- \n2.33.0.917.gae6ecbedc7\n\n"},{"id":"435948","messageId":"YUE1wjqaFlRkCQcH@coredump.intra.peff.net","threadId":"56500","inReplyTo":"YUE1alo58cGyTw6/@coredump.intra.peff.net","subject":"[PATCH v2 08/11] docs/protocol-v2: clarify some ls-refs ref-prefix details","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-14T23:52:34Z","receivedAt":"2021-09-14T23:52:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We've never documented the fact that a client can provide multiple\nref-prefix capabilities. Let's describe the behavior.\n\nWe also never discussed the \"best effort\" nature of the prefixes. The\nclient side of git.git has always treated them this way, filtering the\nresult with local patterns. And indeed any client must do this, because\nthe prefix patterns are not sufficient to express the usual refspecs\n(and so for \"foo\" we ask for \"refs/heads/foo\", \"refs/tags/foo\", and so\non).\n\nSo this may be considered a change in the spec with respect to client\nexpectations / requirements, but it's mostly codifying existing\nbehavior.\n\nSigned-off-by: Jeff King \n---\n Documentation/technical/protocol-v2.txt | 6 +++++-\n 1 file changed, 5 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/technical/protocol-v2.txt b/Documentation/technical/protocol-v2.txt\nindex 213538f1d0..9347b2ad13 100644\n--- a/Documentation/technical/protocol-v2.txt\n+++ b/Documentation/technical/protocol-v2.txt\n@@ -193,7 +193,11 @@ ls-refs takes in the following arguments:\n \tShow peeled tags.\n ref-prefix \n \tWhen specified, only references having a prefix matching one of\n-\tthe provided prefixes are displayed.\n+\tthe provided prefixes are displayed. Multiple instances may be\n+\tgiven, in which case references matching any prefix will be\n+\tshown. Note that this is purely for optimization; a server MAY\n+\tshow refs not matching the prefix if it chooses, and clients\n+\tshould filter the result themselves.\n \n If the 'unborn' feature is advertised the following argument can be\n included in the client's request.\n-- \n2.33.0.917.gae6ecbedc7\n\n"},{"id":"435949","messageId":"YUE1ym1dALRQLztq@coredump.intra.peff.net","threadId":"56500","inReplyTo":"YUE1alo58cGyTw6/@coredump.intra.peff.net","subject":"[PATCH v2 09/11] serve: reject bogus v2 \"command=ls-refs=foo\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-14T23:52:42Z","receivedAt":"2021-09-14T23:52:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we see a line from the client like \"command=ls-refs\", we parse\neverything after the equals sign as a capability, which we check against\nour capabilities table. If we don't recognize the command (e.g.,\n\"command=foo\"), we'll reject it. But we use the same parser that checks\nfor regular capabilities like \"object-format=sha256\". And so we'll\naccept \"ls-refs=foo\", even though everything after the equals is bogus,\nand simply ignored.\n\nThis isn't really hurting anything, but the request does violate the\nspec. Let's tighten it up to prevent any surprising behavior.\n\nSigned-off-by: Jeff King \n---\n serve.c | 2 +-\n t/t5701-git-serve.sh | 10 ++++++++++\n 2 files changed, 11 insertions(+), 1 deletion(-)\n\ndiff --git a/serve.c b/serve.c\nindex 5ea6c915cb..63ee1be7ff 100644\n--- a/serve.c\n+++ b/serve.c\n@@ -220,7 +220,7 @@ static int parse_command(const char *key, struct protocol_capability **command)\n \t\tif (*command)\n \t\t\tdie(\"command '%s' requested after already requesting command '%s'\",\n \t\t\t out, (*command)->name);\n-\t\tif (!cmd || !cmd->advertise(the_repository, NULL) || !cmd->command)\n+\t\tif (!cmd || !cmd->advertise(the_repository, NULL) || !cmd->command || value)\n \t\t\tdie(\"invalid command '%s'\", out);\n \n \t\t*command = cmd;\ndiff --git a/t/t5701-git-serve.sh b/t/t5701-git-serve.sh\nindex 3bc96ebcde..ab15078bc0 100755\n--- a/t/t5701-git-serve.sh\n+++ b/t/t5701-git-serve.sh\n@@ -72,6 +72,16 @@ test_expect_success 'request invalid command' '\n \ttest_i18ngrep \"invalid command\" err\n '\n \n+test_expect_success 'requested command is command=value' '\n+\ttest-tool pkt-line pack >in <<-\\EOF &&\n+\tcommand=ls-refs=whatever\n+\tobject-format=$(test_oid algo)\n+\t0000\n+\tEOF\n+\ttest_must_fail test-tool serve-v2 --stateless-rpc 2>err in <<-EOF &&\n \tcommand=fetch\n-- \n2.33.0.917.gae6ecbedc7\n\n"},{"id":"435950","messageId":"YUE1zZ+DXBET15VS@coredump.intra.peff.net","threadId":"56500","inReplyTo":"YUE1alo58cGyTw6/@coredump.intra.peff.net","subject":"[PATCH v2 10/11] serve: reject commands used as capabilities","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-14T23:52:45Z","receivedAt":"2021-09-14T23:52:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Our table of v2 \"capabilities\" contains everything we might tell the\nclient we support. But there are differences in how we expect the client\nto respond. Some of the entries are true capabilities (i.e., we expect\nthe client to say \"yes, I support this\"), and some are ones we expect\nthem to send as commands (with \"command=ls-refs\" or similar).\n\nWhen we receive a capability used as a command, we complain about that.\nBut when we receive a command used as a capability (e.g., just \"ls-refs\"\nin a pkt-line by itself), we silently ignore it.\n\nThis isn't really hurting anything (clients shouldn't send it, and we'll\nignore it), but we can tighten up the protocol to match what we expect\nto happen.\n\nThere are two new tests here. The first one checks a capability used as\na command, which already passes. The second tests a command as a\ncapability, which this patch fixes.\n\nSigned-off-by: Jeff King \n---\n serve.c | 2 +-\n t/t5701-git-serve.sh | 19 +++++++++++++++++++\n 2 files changed, 20 insertions(+), 1 deletion(-)\n\ndiff --git a/serve.c b/serve.c\nindex 63ee1be7ff..0636b79f92 100644\n--- a/serve.c\n+++ b/serve.c\n@@ -201,7 +201,7 @@ static int receive_client_capability(const char *key)\n \tconst char *value;\n \tconst struct protocol_capability *c = get_capability(key, &value);\n \n-\tif (!c || !c->advertise(the_repository, NULL))\n+\tif (!c || c->command || !c->advertise(the_repository, NULL))\n \t\treturn 0;\n \n \tif (c->receive)\ndiff --git a/t/t5701-git-serve.sh b/t/t5701-git-serve.sh\nindex ab15078bc0..b027ba9b06 100755\n--- a/t/t5701-git-serve.sh\n+++ b/t/t5701-git-serve.sh\n@@ -72,6 +72,25 @@ test_expect_success 'request invalid command' '\n \ttest_i18ngrep \"invalid command\" err\n '\n \n+test_expect_success 'request capability as command' '\n+\ttest-tool pkt-line pack >in <<-\\EOF &&\n+\tcommand=agent\n+\t0000\n+\tEOF\n+\ttest_must_fail test-tool serve-v2 --stateless-rpc 2>err in <<-\\EOF &&\n+\tcommand=ls-refs\n+\tfetch\n+\t0000\n+\tEOF\n+\ttest_must_fail test-tool serve-v2 --stateless-rpc 2>err in <<-\\EOF &&\n \tcommand=ls-refs=whatever\n-- \n2.33.0.917.gae6ecbedc7\n\n"},{"id":"435951","messageId":"YUE2J74PP8TGthOZ@coredump.intra.peff.net","threadId":"56500","inReplyTo":"YUE1alo58cGyTw6/@coredump.intra.peff.net","subject":"[PATCH v2 11/11] ls-refs: reject unknown arguments","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-14T23:54:15Z","receivedAt":"2021-09-14T23:54:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The v2 ls-refs command may receive extra arguments from the client, one\nper pkt-line. The spec is pretty clear that the arguments must come from\na specified set, but we silently ignore any unknown entries. For a\nwell-behaved client this doesn't matter, but it makes testing and\ndebugging more confusing. Let's tighten this up to match the spec.\n\nIn theory this liberal behavior _could_ be useful for extending the\nprotocol. But:\n\n - every other part of the protocol requires that the server first\n indicate that it supports the argument; this includes the fetch and\n object-info commands, plus the \"unborn\" capability added to ls-refs\n itself\n\n - it's not a very good extension mechanism anyway; without the server\n advertising support, clients would have no idea if the argument was\n silently ignored, or accepted and simply had no effect\n\nSo we're not really losing anything by tightening this.\n\nSigned-off-by: Jeff King \n---\n ls-refs.c | 2 ++\n t/t5701-git-serve.sh | 13 +++++++++++++\n 2 files changed, 15 insertions(+)\n\ndiff --git a/ls-refs.c b/ls-refs.c\nindex 18c4f41e87..460ac9b229 100644\n--- a/ls-refs.c\n+++ b/ls-refs.c\n@@ -168,6 +168,8 @@ int ls_refs(struct repository *r, struct packet_reader *request)\n \t\t}\n \t\telse if (!strcmp(\"unborn\", arg))\n \t\t\tdata.unborn = allow_unborn;\n+\t\telse\n+\t\t\tdie(_(\"unexpected line: '%s'\"), arg);\n \t}\n \n \tif (request->status != PACKET_READ_FLUSH)\ndiff --git a/t/t5701-git-serve.sh b/t/t5701-git-serve.sh\nindex b027ba9b06..e4d60bc605 100755\n--- a/t/t5701-git-serve.sh\n+++ b/t/t5701-git-serve.sh\n@@ -145,6 +145,19 @@ test_expect_success 'basics of ls-refs' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'ls-refs complains about unknown options' '\n+\ttest-tool pkt-line pack >in <<-EOF &&\n+\tcommand=ls-refs\n+\tobject-format=$(test_oid algo)\n+\t0001\n+\tno-such-arg\n+\t0000\n+\tEOF\n+\n+\ttest_must_fail test-tool serve-v2 --stateless-rpc 2>err in <<-EOF &&\n \tcommand=ls-refs\n-- \n2.33.0.917.gae6ecbedc7\n"},{"id":"435953","messageId":"875yv2ofkq.fsf@evledraar.gmail.com","threadId":"56500","inReplyTo":"YUE2J74PP8TGthOZ@coredump.intra.peff.net","subject":"Re: [PATCH v2 11/11] ls-refs: reject unknown arguments","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-15T00:09:16Z","receivedAt":"2021-09-15T00:19:54Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Sep 14 2021, Jeff King wrote:\n\n> The v2 ls-refs command may receive extra arguments from the client, one\n> per pkt-line. The spec is pretty clear that the arguments must come from\n> a specified set, but we silently ignore any unknown entries. For a\n> well-behaved client this doesn't matter, but it makes testing and\n> debugging more confusing. Let's tighten this up to match the spec.\n>\n> In theory this liberal behavior _could_ be useful for extending the\n> protocol. But:\n>\n> - every other part of the protocol requires that the server first\n> indicate that it supports the argument; this includes the fetch and\n> object-info commands, plus the \"unborn\" capability added to ls-refs\n> itself\n>\n> - it's not a very good extension mechanism anyway; without the server\n> advertising support, clients would have no idea if the argument was\n> silently ignored, or accepted and simply had no effect\n>\n> So we're not really losing anything by tightening this.\n>\n> Signed-off-by: Jeff King \n> ---\n> ls-refs.c | 2 ++\n> t/t5701-git-serve.sh | 13 +++++++++++++\n> 2 files changed, 15 insertions(+)\n>\n> diff --git a/ls-refs.c b/ls-refs.c\n> index 18c4f41e87..460ac9b229 100644\n> --- a/ls-refs.c\n> +++ b/ls-refs.c\n> @@ -168,6 +168,8 @@ int ls_refs(struct repository *r, struct packet_reader *request)\n> \t\t}\n> \t\telse if (!strcmp(\"unborn\", arg))\n> \t\t\tdata.unborn = allow_unborn;\n> +\t\telse\n> +\t\t\tdie(_(\"unexpected line: '%s'\"), arg);\n> \t}\n> \n> \tif (request->status != PACKET_READ_FLUSH)\n> diff --git a/t/t5701-git-serve.sh b/t/t5701-git-serve.sh\n> index b027ba9b06..e4d60bc605 100755\n> --- a/t/t5701-git-serve.sh\n> +++ b/t/t5701-git-serve.sh\n> @@ -145,6 +145,19 @@ test_expect_success 'basics of ls-refs' '\n> \ttest_cmp expect actual\n> '\n> \n> +test_expect_success 'ls-refs complains about unknown options' '\n> +\ttest-tool pkt-line pack >in <<-EOF &&\n> +\tcommand=ls-refs\n> +\tobject-format=$(test_oid algo)\n> +\t0001\n> +\tno-such-arg\n> +\t0000\n> +\tEOF\n> +\n> +\ttest_must_fail test-tool serve-v2 --stateless-rpc 2>err +\tgrep unexpected.line.*no-such-arg err\n> +'\n> +\n> test_expect_success 'basic ref-prefixes' '\n> \ttest-tool pkt-line pack >in <<-EOF &&\n> \tcommand=ls-refs\n\nLooks good. For what it's worth some of this series is stuff I've been\nmeaning to submit after my current in-flight series, so I guess we'll be\nplaying some rebase ping-pong in this area.\n\nIn my version of this I'd led with a change to do changes like these for\nall the protocol verbs:\n\t\n\tdiff --git a/connect.c b/connect.c\n\tindex 70b13389ba5..5e991a92279 100644\n\t--- a/connect.c\n\t+++ b/connect.c\n\t@@ -514,6 +514,8 @@ struct ref **get_remote_refs(int fd_out, struct packet_reader *reader,\n\t packet_write_fmt(fd_out, \"symrefs\\n\");\n\t if (server_supports_feature(\"ls-refs\", \"unborn\", 0))\n\t packet_write_fmt(fd_out, \"unborn\\n\");\n\t+ if (git_env_bool(\"GIT_TEST_PROTOCOL_BAD_LS_REFS\", 0))\n\t+ packet_write_fmt(fd_out, \"test-bad-client\\n\");\n\t for (i = 0; ref_prefixes && i < ref_prefixes->nr; i++) {\n\t packet_write_fmt(fd_out, \"ref-prefix %s\\n\",\n\t ref_prefixes->v[i]);\n\nSo the below patch isn't anywhere near applying, but you can see the\ntest coverage (those tests are new) & what changes if we instrument a\nclient to actually send this bad data.\n\nThat packet_client_error() function is new, part of what I'm doing there\nis converting all of this error emitting from die() to properly sending\nERR packets.\n\nI think a bug in your versio is that you're using _() here, if your\nserver program happens to be started in Chinese you probably still want\nto emit errors to clients in LANG=C.\n\nThat's why I'm using N_() in my version, it knows to emit the die()\nlocalized, but the ERR packet in LANG=C, no you'll get the i18n message\nin the server terminal, but the client gets LANG=C.\n\nOf course that actually working is subject to various races that I may\nor may not be able to untangle...\n\n\n-- >8 --\nprotocol v2: correct & adjust \"ls-refs\" ERR behavior\n\nChange the protocol v2 \"ls-refs\" to error out on unknown arguments,\nbefore this we'd silently ignore unknown arguments.\n\nThis brings us in line with the behavior we've had in\n\"fetch\" (i.e. upload-pack.c) since 3145ea957d2 (upload-pack: introduce\nfetch server command, 2018-03-15) (see the \"/* ignore unknown lines\nmaybe? */\" comment in \"process_args()\".\n\nThe looser behavior in \"ls-refs\" seems to have been unintentionally\nadded in 72d0ea0056b (ls-refs: introduce ls-refs server command,\n2018-03-15).\n\nI've also changed the wording of the \"expected flush\" message in\n\"ls-refs\" to be consistent with \"fetch\" and \"object-info\".\n\nIn the changes leading up to this we had to grep out the racy \"fatal:\nthe remote end hung up unexpectedly\" messages. Now we need to guard\nthe full test_cmp with a test_expect_failure. I could also write a\nlooser test, but this makes subsequent changes smaller, and is more\naccurate.\n\nMost of the time the test_expect_failure() succeeds, but in some cases\nthe two messages will be out-of-order, or e.g. only the STDERR one\nwill make it, and not the ERR message, or the other way\naround. I.e. now that we emit both an ERR line and die() on the\nremote, we might see ERR before die(), or die() before ERR.\n\nThis race does not happen in the t5701-git-serve.sh and other\n\"test-tool pkt-line\" tests, which only use one process.\n\nIdeally we should only emit ERR and not also something on STDERR when\nwe die() on the server. Subsequent commits will address that, but for\nnow let's keep that behavior. I.e. we'll now emit two mostly duplicate\nerrors in some cases from the client, or one where we emitted none (or\none that made no sense) before.\n\nThe current client implementation doesn't expect the server to be this\noverly helpful in giving us errors both via pkt-line and STDERR, so we\nusually get two lines, and in some cases just one (but never\nzero).\n\nSubsequent commits will address this. I.e. we can assume that the\nclient can format the ERR lines, and that we can just exit(128) or\ndisconnect on the server, not die().\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n ls-refs.c | 6 +++++-\n t/t5703-protocol-v2-file.sh | 24 +++++++++++++++++-------\n t/t5704-protocol-v2-git.sh | 20 +++++++++++++-------\n t/t5705-protocol-v2-http.sh | 25 +++++++++++++++++++------\n t/t5710-protocol-violations.sh | 2 +-\n 5 files changed, 55 insertions(+), 22 deletions(-)\n\ndiff --git a/ls-refs.c b/ls-refs.c\nindex 1e50e785665..f9bd577dcd2 100644\n--- a/ls-refs.c\n+++ b/ls-refs.c\n@@ -150,11 +150,15 @@ int ls_refs(struct repository *r, struct packet_reader *request)\n \t\t\tstrvec_push(&data.prefixes, out);\n \t\telse if (!strcmp(\"unborn\", arg))\n \t\t\tdata.unborn = allow_unborn;\n+\t\telse\n+\t\t\tpacket_client_error(&data.writer,\n+\t\t\t\t\t N_(\"ls-refs: unexpected argument: '%s'\"),\n+\t\t\t\t\t request->line);\n \t}\n \n \tif (request->status != PACKET_READ_FLUSH)\n \t\tpacket_client_error(&data.writer,\n-\t\t\t\t N_(\"expected flush after ls-refs arguments\"));\n+\t\t\t\t N_(\"ls-refs: expected flush after arguments\"));\n \n \tsend_possibly_unborn_head(&data);\n \tif (!data.prefixes.nr)\ndiff --git a/t/t5703-protocol-v2-file.sh b/t/t5703-protocol-v2-file.sh\nindex e572767ee00..79457eaf101 100755\n--- a/t/t5703-protocol-v2-file.sh\n+++ b/t/t5703-protocol-v2-file.sh\n@@ -32,18 +32,28 @@ test_expect_success 'list refs with file:// using protocol v2' '\n test_expect_success 'ls-remote handling a bad client using file:// protocol v2' '\n \ttest_when_finished \"rm -f log\" &&\n \n-\tcat >expect <<-EOF &&\n-\t$(git -C file_parent rev-parse refs/heads/main)$(printf \"\\t\")refs/heads/main\n+\tcat >log.expect <<-\\EOF &&\n+\tpacket: upload-pack> ERR ls-refs: unexpected argument: '\"'\"'test-bad-client'\"'\"'\n+\tpacket: git< ERR ls-refs: unexpected argument: '\"'\"'test-bad-client'\"'\"'\n \tEOF\n-\tenv \\\n+\tcat >err.expect <<-\\EOF &&\n+\tfatal: ls-refs: unexpected argument: '\"'\"'test-bad-client'\"'\"'\n+\tfatal: remote error: ls-refs: unexpected argument: '\"'\"'test-bad-client'\"'\"'\n+\tEOF\n+\ttest_must_fail env \\\n \t\tGIT_TRACE_PACKET=\"$(pwd)/log\" \\\n \t\tGIT_TEST_PROTOCOL_BAD_LS_REFS=true \\\n \t\tgit -c protocol.version=2 \\\n-\t\tls-remote \"file://$(pwd)/file_parent\" main >actual 2>err &&\n+\t\tls-remote \"file://$(pwd)/file_parent\" main >out 2>err.actual &&\n \n-\ttest_must_be_empty err &&\n-\ttest_cmp expect actual &&\n-\tgrep \"git> test-bad-client$\" log\n+\tgrep \"unexpected argument.*test-bad-client\" err.actual &&\n+\ttest_must_be_empty out &&\n+\tgrep ERR log >log.actual &&\n+\ttest_cmp log.expect log.actual\n+'\n+\n+test_expect_failure 'ls-remote ERR and die() is racy under file:// protocol v2' '\n+\ttest_cmp err.expect err.actual\n '\n \n test_expect_success 'ref advertisement is filtered with ls-remote using protocol v2' '\ndiff --git a/t/t5704-protocol-v2-git.sh b/t/t5704-protocol-v2-git.sh\nindex c9293bbaad2..123c3f56ef8 100755\n--- a/t/t5704-protocol-v2-git.sh\n+++ b/t/t5704-protocol-v2-git.sh\n@@ -48,19 +48,25 @@ test_expect_success 'ref advertisement is filtered with ls-remote using protocol\n \ttest_cmp expect actual\n '\n \n-test_expect_success 'ls-remote handling a bad client using git:// protocol v2' '\n+test_expect_success 'ls-remote handling a bad client using protocol v2' '\n \ttest_when_finished \"rm -f log\" &&\n \n-\tgit ls-remote \"$daemon_parent\" >expect &&\n-\tenv \\\n+\tcat >err.expect <<-EOF &&\n+\tfatal: remote error: ls-refs: unexpected argument: '\"'\"'test-bad-client'\"'\"'\n+\tEOF\n+\ttest_must_fail env \\\n \t\tGIT_TRACE_PACKET=\"$(pwd)/log\" \\\n \t\tGIT_TEST_PROTOCOL_BAD_LS_REFS=true \\\n \t\tgit -c protocol.version=2 \\\n-\t\tls-remote \"$GIT_DAEMON_URL/parent\" >actual 2>err &&\n+\t\tls-remote \"$GIT_DAEMON_URL/parent\" main >out.actual 2>err.actual &&\n \n-\ttest_must_be_empty err &&\n-\ttest_cmp expect actual &&\n-\tgrep \"git> test-bad-client$\" log\n+\ttest_must_be_empty out.actual &&\n+\tgrep \"unexpected argument.*test-bad-client\" err.actual &&\n+\tgrep \"ERR ls-refs: unexpected argument.*test-bad-client\" log\n+'\n+\n+test_expect_failure 'ls-remote ERR and die() is racy under file:// protocol v2' '\n+\ttest_cmp err.expect err.actual\n '\n \n test_expect_success 'clone with git:// using protocol v2' '\ndiff --git a/t/t5705-protocol-v2-http.sh b/t/t5705-protocol-v2-http.sh\nindex 5982c38743e..6f526e0829f 100755\n--- a/t/t5705-protocol-v2-http.sh\n+++ b/t/t5705-protocol-v2-http.sh\n@@ -192,16 +192,29 @@ test_expect_success 'fetch from namespaced repo respects namespaces' '\n test_expect_success 'ls-remote handling a bad client using http:// protocol v2' '\n \ttest_when_finished \"rm -f log\" &&\n \n-\tgit ls-remote \"$HTTPD_DOCUMENT_ROOT_PATH/http_parent\" >expect &&\n-\tenv \\\n+\tcat >log.expect <<-\\EOF &&\n+\tpacket: upload-pack> ERR ls-refs: unexpected argument: '\"'\"'test-bad-client'\"'\"'\n+\tpacket: git< ERR ls-refs: unexpected argument: '\"'\"'test-bad-client'\"'\"'\n+\tEOF\n+\n+\tcat >err.expect <<-\\EOF &&\n+\tfatal: ls-refs: unexpected argument: '\"'\"'test-bad-client'\"'\"'\n+\tfatal: remote error: ls-refs: unexpected argument: '\"'\"'test-bad-client'\"'\"'\n+\tEOF\n+\ttest_must_fail env \\\n \t\tGIT_TRACE_PACKET=\"$(pwd)/log\" \\\n \t\tGIT_TEST_PROTOCOL_BAD_LS_REFS=true \\\n \t\tgit -c protocol.version=2 \\\n-\t\tls-remote \"$HTTPD_DOCUMENT_ROOT_PATH/http_parent\" >actual 2>err &&\n+\t\tls-remote \"$HTTPD_DOCUMENT_ROOT_PATH/http_parent\" >out 2>err.actual &&\n \n-\ttest_must_be_empty err &&\n-\ttest_cmp expect actual &&\n-\tgrep \"git> test-bad-client$\" log\n+\tgrep \"unexpected argument.*test-bad-client\" err.actual &&\n+\ttest_must_be_empty out &&\n+\tgrep ERR log >log.actual &&\n+\ttest_cmp log.expect log.actual\n+'\n+\n+test_expect_failure 'ls-remote ERR and die() is racy under http:// protocol v2' '\n+\ttest_cmp err.expect err.actual\n '\n \n test_expect_success 'ls-remote with v2 http sends only one POST' '\ndiff --git a/t/t5710-protocol-violations.sh b/t/t5710-protocol-violations.sh\nindex 44e2c0d3ded..75a457d39ca 100755\n--- a/t/t5710-protocol-violations.sh\n+++ b/t/t5710-protocol-violations.sh\n@@ -16,7 +16,7 @@ test_expect_success 'extra delim packet in v2 ls-refs args' '\n \tEOF\n \n \tcat >err.expect <<-\\EOF &&\n-\tfatal: expected flush after ls-refs arguments\n+\tfatal: ls-refs: expected flush after arguments\n \tEOF\n \ttest_must_fail env GIT_PROTOCOL=version=2 \\\n \t\tgit upload-pack . err.actual &&\n-- \n2.33.0.1013.ge8323766266\n\n"},{"id":"435954","messageId":"871r5qof8k.fsf@evledraar.gmail.com","threadId":"56500","inReplyTo":"YUC/6n1hhUbMJiLw@coredump.intra.peff.net","subject":"Re: [PATCH 0/9] reducing memory allocations for v2 servers","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-15T00:25:49Z","receivedAt":"2021-09-15T00:27:13Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Sep 14 2021, Jeff King wrote:\n\n> While looking at [1], I noticed that v2 servers will read a few bits of\n> client input into strvecs. Even though we expect these to be small-ish,\n> there's nothing preventing a client from sending us a bunch of junk and\n> wasting memory.\n\n[...]\n\n>\n> [1] https://lore.kernel.org/git/YT54CNYgtGcqexwq@coredump.intra.peff.net/\n\nWhen I first read this I expected it to be a link to\nhttps://lore.kernel.org/git/YPCsDLoiiAG%2FC%2Fft@coredump.intra.peff.net/;\ni.e. that the object-info leak discussion back in July had encouraged\nyou to work on this ... :)\n"},{"id":"435955","messageId":"87wnnin0ix.fsf@evledraar.gmail.com","threadId":"56500","inReplyTo":"YUE1ym1dALRQLztq@coredump.intra.peff.net","subject":"Re: [PATCH v2 09/11] serve: reject bogus v2 \"command=ls-refs=foo\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-15T00:27:35Z","receivedAt":"2021-09-15T00:30:21Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Sep 14 2021, Jeff King wrote:\n\n> When we see a line from the client like \"command=ls-refs\", we parse\n> everything after the equals sign as a capability, which we check against\n> our capabilities table. If we don't recognize the command (e.g.,\n> \"command=foo\"), we'll reject it. But we use the same parser that checks\n> for regular capabilities like \"object-format=sha256\". And so we'll\n> accept \"ls-refs=foo\", even though everything after the equals is bogus,\n> and simply ignored.\n>\n> This isn't really hurting anything, but the request does violate the\n> spec. Let's tighten it up to prevent any surprising behavior.\n\nDoesn't need a re-roll, but just for my own sanity:\n\nBy violating the spec you mean it doesn't coform to the \"key\" in the\n\"Capability Advertisement\" section of protocol-v2.txt, one might skim\nthis and think values with \"=\" in them are OK, but a command=WHATEVER\nhas a WHATEVER as a \"key\", not a \"value\", that's for other parts of the\ndialog.\n\nBut you could also have meant that it violates the spec because there's\nno such command as \"ls-refs=whatever\", just like there isn't \"foobar\",\nbut I don't think that's what you mean...\n\n> Signed-off-by: Jeff King \n> ---\n> serve.c | 2 +-\n> t/t5701-git-serve.sh | 10 ++++++++++\n> 2 files changed, 11 insertions(+), 1 deletion(-)\n>\n> diff --git a/serve.c b/serve.c\n> index 5ea6c915cb..63ee1be7ff 100644\n> --- a/serve.c\n> +++ b/serve.c\n> @@ -220,7 +220,7 @@ static int parse_command(const char *key, struct protocol_capability **command)\n> \t\tif (*command)\n> \t\t\tdie(\"command '%s' requested after already requesting command '%s'\",\n> \t\t\t out, (*command)->name);\n> -\t\tif (!cmd || !cmd->advertise(the_repository, NULL) || !cmd->command)\n> +\t\tif (!cmd || !cmd->advertise(the_repository, NULL) || !cmd->command || value)\n> \t\t\tdie(\"invalid command '%s'\", out);\n> \n> \t\t*command = cmd;\n> diff --git a/t/t5701-git-serve.sh b/t/t5701-git-serve.sh\n> index 3bc96ebcde..ab15078bc0 100755\n> --- a/t/t5701-git-serve.sh\n> +++ b/t/t5701-git-serve.sh\n> @@ -72,6 +72,16 @@ test_expect_success 'request invalid command' '\n> \ttest_i18ngrep \"invalid command\" err\n> '\n> \n> +test_expect_success 'requested command is command=value' '\n> +\ttest-tool pkt-line pack >in <<-\\EOF &&\n> +\tcommand=ls-refs=whatever\n> +\tobject-format=$(test_oid algo)\n> +\t0000\n> +\tEOF\n> +\ttest_must_fail test-tool serve-v2 --stateless-rpc 2>err +\tgrep invalid.command.*ls-refs=whatever err\n> +'\n> +\n> test_expect_success 'wrong object-format' '\n> \ttest-tool pkt-line pack >in <<-EOF &&\n> \tcommand=fetch\n\n"},{"id":"435956","messageId":"87sfy6mzzg.fsf@evledraar.gmail.com","threadId":"56500","inReplyTo":"YUE1fGZc1FuuyUNH@coredump.intra.peff.net","subject":"Re: [PATCH v2 03/11] serve: add \"receive\" method for v2 capabilities table","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-15T00:31:28Z","receivedAt":"2021-09-15T00:42:00Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Sep 14 2021, Jeff King wrote:\n\n> -static int is_valid_capability(const char *key)\n> +static int receive_client_capability(const char *key)\n> {\n> \tconst char *value;\n> \tconst struct protocol_capability *c = get_capability(key, &value);\n> \n> -\treturn c && c->advertise(the_repository, NULL);\n> +\tif (!c || !c->advertise(the_repository, NULL))\n> +\t\treturn 0;\n> +\n> +\tif (c->receive)\n> +\t\tc->receive(the_repository, value);\n> +\treturn 1;\n> }\n> \n\nI haven't actually run this yet (and need to zZzZ soon), but AFAICT at\nthe end of this series you leave the existing advertise semantics of\nadvertise() be (which is fine). I have this unsubmitted patch locally\nwhich you may or may not want to work into this.\n\nI considered splitting up the advertise() method as well, i.e. we could\nhave a new \"is_advertised\" boolean callback, and then a\n\"capability_string\" or whatever. \"server-option\" and \"object-info\" never\nadd anything, so they could leave that as NULL.\n\nBut it's probably not worth it, just food for thought...\n\n-- >8 -- serve: document that \"advertise\" is called in two modes\n\nIf we're being called with a non-NULL argument we're sending out the\nadvertisement line, but if it's NULL we're actually in the middle of a\nrequest.\n\nSo you can use the check for NULL to emit your own \"die\" on \"return\n0\", like \"wtf, I said I don't support command xyz, why are you calling\nit?\", as opposed to the default \"invalid command '%s'\".\n\nMaybe nobody cares, and we can't assume that we're going from an\nadvertisement to a command for the same request anyway (can\nwe?). I.e. are we serving multiple clients?\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n serve.c | 6 ++++++\n 1 file changed, 6 insertions(+)\n\ndiff --git a/serve.c b/serve.c\nindex aa8209f147e..b187ce26911 100644\n--- a/serve.c\n+++ b/serve.c\n@@ -55,6 +55,12 @@ struct protocol_capability {\n \t * Optionally a value can be specified by adding it to 'value'.\n \t * If a value is added to 'value', the server will advertise this\n \t * capability as \"=\" instead of \"\".\n+\t *\n+\t * When called with a NULL value we're past the advertisement\n+\t * itself, and are checking during a request whether we\n+\t * support this command. This can be checked and used used to\n+\t * e.g. emit a die(\"we support this, but don't have it\n+\t * enabled!\") error.\n \t */\n \tint (*advertise)(struct repository *r, struct strbuf *value);\n \n-- \n2.33.0.1013.ge8323766266\n\n"},{"id":"435958","messageId":"YUFzhC5Eox8IND4O@nand.local","threadId":"56500","inReplyTo":"YUE1hz/KKM0XebCP@coredump.intra.peff.net","subject":"Re: [PATCH v2 07/11] ls-refs: ignore very long ref-prefix counts","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-09-15T04:16:04Z","receivedAt":"2021-09-15T04:16:08Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Sep 14, 2021 at 07:51:35PM -0400, Jeff King wrote:\n> @@ -156,15 +162,25 @@ int ls_refs(struct repository *r, struct packet_reader *request)\n> \t\t\tdata.peel = 1;\n> \t\telse if (!strcmp(\"symrefs\", arg))\n> \t\t\tdata.symrefs = 1;\n> -\t\telse if (skip_prefix(arg, \"ref-prefix \", &out))\n> -\t\t\tstrvec_push(&data.prefixes, out);\n> +\t\telse if (skip_prefix(arg, \"ref-prefix \", &out)) {\n> +\t\t\tif (data.prefixes.nr < TOO_MANY_PREFIXES)\n> +\t\t\t\tstrvec_push(&data.prefixes, out);\n> +\t\t}\n> \t\telse if (!strcmp(\"unborn\", arg))\n> \t\t\tdata.unborn = allow_unborn;\n> \t}\n>\n> \tif (request->status != PACKET_READ_FLUSH)\n> \t\tdie(_(\"expected flush after ls-refs arguments\"));\n>\n> +\t/*\n> +\t * If we saw too many prefixes, we must avoid using them at all; as\n> +\t * soon as we have any prefix, they are meant to form a comprehensive\n> +\t * list.\n> +\t */\n> +\tif (data.prefixes.nr >= TOO_MANY_PREFIXES)\n> +\t\tstrvec_clear(&data.prefixes);\n> +\n\nGreat, I find this version even better than what I suggested where the\ncase where the list already has too many prefixes `continue`s through\nthe loop before calling strvec_add().\n\nJust noting aloud, the `>` part of this conditional will never happen\n(since data.prefixes.nr will be at most equal to TOO_MANY_PREFIXES, but\nnever larger than it).\n\nObviously not wrong, but I figured it'd be worth mentioning in case it\ncaught the attention of other reviewers.\n\nThanks,\nTaylor\n"},{"id":"435959","messageId":"YUFz6RqVaBunxJ7y@nand.local","threadId":"56500","inReplyTo":"YUE1alo58cGyTw6/@coredump.intra.peff.net","subject":"Re: [PATCH v2 0/11] limit memory allocations for v2 servers","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-09-15T04:17:45Z","receivedAt":"2021-09-15T04:17:48Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Sep 14, 2021 at 07:51:06PM -0400, Jeff King wrote:\n> Here's a re-roll of my series to limit the memory a v2 server is willing\n> to allocate on behalf of a client. See v1:\n>\n> https://lore.kernel.org/git/YUC%2F6n1hhUbMJiLw@coredump.intra.peff.net/\n>\n> for an overview. The existing patches are mostly small fixups pointed\n> out by reviewers (thanks!), but I did take Martin's TOO_MANY_PREFIXES\n> suggestion in patch 7 (without the change to the name of the constant it\n> seemed too clever to me, but with it it seems just the right amount of\n> clever).\n\nThanks for this. The two new patches look good to me, and I took a light\nskim over the ones which were modified since v1. Everything I saw seemed\nvery reasonable to me.\n\n Reviewed-by: Taylor Blau \n\nThanks,\nTaylor\n"},{"id":"435962","messageId":"CAPig+cQxSTzo0tCUEb3uMhyRLS0hyuOM5ksZyH+h2StFupQ9Yw@mail.gmail.com","threadId":"56500","inReplyTo":"YUE1hz/KKM0XebCP@coredump.intra.peff.net","subject":"Re: [PATCH v2 07/11] ls-refs: ignore very long ref-prefix counts","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2021-09-15T05:00:01Z","receivedAt":"2021-09-15T05:00:15Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Sep 14, 2021 at 7:51 PM Jeff King wrote:\n> diff --git a/t/t5701-git-serve.sh b/t/t5701-git-serve.sh\n> @@ -158,6 +158,37 @@ test_expect_success 'refs/heads prefix' '\n> +test_expect_success 'ignore very large set of prefixes' '\n> + # generate a large number of ref-prefixes that we expect\n> + # to match nothing; the value here exceeds TOO_MANY_PREFIXES\n> + # from ls-refs.c.\n> + {\n> + echo command=ls-refs &&\n> + echo object-format=$(test_oid algo)\n> + echo 0001 &&\n> + perl -le \"print \\\"ref-prefix refs/heads/\\$_\\\" for (1..65536)\" &&\n> + echo 0000\n> + } |\n> + test-tool pkt-line pack >in &&\n\nBroken &&-chain in {...}.\n\n(Granted, it doesn't matter in this case since the exit code gets lost\ndown the pipe, but better to be consistent about it.)\n"},{"id":"435963","messageId":"CAPig+cQNipgVwYZw3r2nQMZ_AOOsFEp1=MgEdmZcdAPnFE7gZA@mail.gmail.com","threadId":"56500","inReplyTo":"YUE1ym1dALRQLztq@coredump.intra.peff.net","subject":"Re: [PATCH v2 09/11] serve: reject bogus v2 \"command=ls-refs=foo\"","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2021-09-15T05:09:19Z","receivedAt":"2021-09-15T05:09:33Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Sep 14, 2021 at 7:52 PM Jeff King wrote:\n> When we see a line from the client like \"command=ls-refs\", we parse\n> everything after the equals sign as a capability, which we check against\n> our capabilities table. If we don't recognize the command (e.g.,\n> \"command=foo\"), we'll reject it. But we use the same parser that checks\n> for regular capabilities like \"object-format=sha256\". And so we'll\n> accept \"ls-refs=foo\", even though everything after the equals is bogus,\n> and simply ignored.\n>\n> This isn't really hurting anything, but the request does violate the\n> spec. Let's tighten it up to prevent any surprising behavior.\n>\n> Signed-off-by: Jeff King \n> ---\n> diff --git a/t/t5701-git-serve.sh b/t/t5701-git-serve.sh\n> @@ -72,6 +72,16 @@ test_expect_success 'request invalid command' '\n> +test_expect_success 'requested command is command=value' '\n> + test-tool pkt-line pack >in <<-\\EOF &&\n> + command=ls-refs=whatever\n> + object-format=$(test_oid algo)\n> + 0000\n> + EOF\n\nThis here-doc uses <<-\\EOF yet (presumably) expects $(test_oid algo)\nto be expanded. I'm guessing that you meant <<-EOF but never\nnoticed...\n\n> + test_must_fail test-tool serve-v2 --stateless-rpc 2>err + grep invalid.command.*ls-refs=whatever err\n> +'\n"},{"id":"435986","messageId":"YUIedCI/KR3zrTA8@coredump.intra.peff.net","threadId":"56500","inReplyTo":"875yv2ofkq.fsf@evledraar.gmail.com","subject":"Re: [PATCH v2 11/11] ls-refs: reject unknown arguments","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-15T16:25:24Z","receivedAt":"2021-09-15T16:25:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 15, 2021 at 02:09:16AM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> So the below patch isn't anywhere near applying, but you can see the\n> test coverage (those tests are new) & what changes if we instrument a\n> client to actually send this bad data.\n> \n> That packet_client_error() function is new, part of what I'm doing there\n> is converting all of this error emitting from die() to properly sending\n> ERR packets.\n\nRight, I noticed that none of this is likely to make it to a client\n(unless they are using file:// or an ssh channel which passes back\nstderr).\n\nI agree that we should probably be passing these back via ERR packets,\nbut note that the client is racy here. If the server reports an error\nand dies while the client is still writing, they'll see SIGPIPE and exit\nwithout showing the user the ERR packet. The solution may be something\nalong these lines:\n\n https://public-inbox.org/git/20200422163357.27056-1-chriscool@tuxfamily.org/\n\nAlternatively, the server can pump the client data to /dev/null until we\nhit a flush, and _then_ write the ERR. But that doesn't work for some\nprotocol-level errors (like \"oops, I'm having trouble reading your\npkt-lines).\n\n> I think a bug in your versio is that you're using _() here, if your\n> server program happens to be started in Chinese you probably still want\n> to emit errors to clients in LANG=C.\n\nI'm not sure it's a bug. If you set LANG=zh on your server, that might\nbe harmful (if you serve a multi-lingual international audience), or it\nmight be helpful (if it's a company server where everybody speaks the\nlanguage). Likewise, for a file:// or ssh:// operation, your local LANG\nwould kick in.\n\nI don't really have a strong opinion either way on whether it's helpful\nor hindering overall. But I did follow the convention of nearby code in\nthis series, so I think we don't need to worry about it now (it also\nseemed inconsistent to me; serve.c does not mark for translation, but\nls-refs.c does).\n\n> Of course that actually working is subject to various races that I may\n> or may not be able to untangle...\n\nOh good, so you know about them. :)\n\n-Peff\n"},{"id":"435987","messageId":"YUIfR5yLiXeprhvL@coredump.intra.peff.net","threadId":"56500","inReplyTo":"87wnnin0ix.fsf@evledraar.gmail.com","subject":"Re: [PATCH v2 09/11] serve: reject bogus v2 \"command=ls-refs=foo\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-15T16:28:55Z","receivedAt":"2021-09-15T16:28:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 15, 2021 at 02:27:35AM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> On Tue, Sep 14 2021, Jeff King wrote:\n> \n> > When we see a line from the client like \"command=ls-refs\", we parse\n> > everything after the equals sign as a capability, which we check against\n> > our capabilities table. If we don't recognize the command (e.g.,\n> > \"command=foo\"), we'll reject it. But we use the same parser that checks\n> > for regular capabilities like \"object-format=sha256\". And so we'll\n> > accept \"ls-refs=foo\", even though everything after the equals is bogus,\n> > and simply ignored.\n> >\n> > This isn't really hurting anything, but the request does violate the\n> > spec. Let's tighten it up to prevent any surprising behavior.\n> \n> Doesn't need a re-roll, but just for my own sanity:\n> \n> By violating the spec you mean it doesn't coform to the \"key\" in the\n> \"Capability Advertisement\" section of protocol-v2.txt, one might skim\n> this and think values with \"=\" in them are OK, but a command=WHATEVER\n> has a WHATEVER as a \"key\", not a \"value\", that's for other parts of the\n> dialog.\n> \n> But you could also have meant that it violates the spec because there's\n> no such command as \"ls-refs=whatever\", just like there isn't \"foobar\",\n> but I don't think that's what you mean...\n\nBoth, I think. :) The line \"command=foo=bar\" is obvious nonsense by the\nspec, no matter which way you interpret it:\n\n - \"foo\" is a command name, but adding a value does not make sense when\n it is used in this context, so it's invalid\n\n - \"foo=bar\" is not a command name\n\nOur parser does pick out \"foo\", so in terms of the code it is more like\nthe former. But since it is invalid either way, I'm not sure the\ndistinction matters.\n\n-Peff\n"},{"id":"435988","messageId":"YUIgLWbvfVOy1njt@coredump.intra.peff.net","threadId":"56500","inReplyTo":"CAPig+cQNipgVwYZw3r2nQMZ_AOOsFEp1=MgEdmZcdAPnFE7gZA@mail.gmail.com","subject":"Re: [PATCH v2 09/11] serve: reject bogus v2 \"command=ls-refs=foo\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-15T16:32:45Z","receivedAt":"2021-09-15T16:32:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 15, 2021 at 01:09:19AM -0400, Eric Sunshine wrote:\n\n> > diff --git a/t/t5701-git-serve.sh b/t/t5701-git-serve.sh\n> > @@ -72,6 +72,16 @@ test_expect_success 'request invalid command' '\n> > +test_expect_success 'requested command is command=value' '\n> > + test-tool pkt-line pack >in <<-\\EOF &&\n> > + command=ls-refs=whatever\n> > + object-format=$(test_oid algo)\n> > + 0000\n> > + EOF\n> \n> This here-doc uses <<-\\EOF yet (presumably) expects $(test_oid algo)\n> to be expanded. I'm guessing that you meant <<-EOF but never\n> noticed...\n\nAh, thanks. It's muscle memory to inhibit expansion in my here-docs. :)\n\n> > + test_must_fail test-tool serve-v2 --stateless-rpc 2>err \n> ... because of this test_must_fail().\n> \n> > + grep invalid.command.*ls-refs=whatever err\n> > +'\n\nActually, it's a little more complicated. The \"grep\" here would notice\nif we failed for the wrong reason. But we never actually look at the\nobject-format line at all, because we barf as soon as we see the bogus\ncommand line.\n\nSo the object-format line is superfluous in this test. I added it mostly\nfor consistency with the other tests (including the existing\nunknown-command test, which is in the same boat). But it also makes the\ntest slightly more robust, in that the command is more likely to succeed\n(and thus fail the test) if the code accidentally did not notice the\nwrong command line.\n\nBut obviously it's worth fixing. Thanks for noticing.\n\n-Peff\n"},{"id":"435990","messageId":"YUIg1vCvmwk3J13P@coredump.intra.peff.net","threadId":"56500","inReplyTo":"87sfy6mzzg.fsf@evledraar.gmail.com","subject":"Re: [PATCH v2 03/11] serve: add \"receive\" method for v2 capabilities table","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-15T16:35:34Z","receivedAt":"2021-09-15T16:35:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 15, 2021 at 02:31:28AM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> I haven't actually run this yet (and need to zZzZ soon), but AFAICT at\n> the end of this series you leave the existing advertise semantics of\n> advertise() be (which is fine). I have this unsubmitted patch locally\n> which you may or may not want to work into this.\n>\n> I considered splitting up the advertise() method as well, i.e. we could\n> have a new \"is_advertised\" boolean callback, and then a\n> \"capability_string\" or whatever. \"server-option\" and \"object-info\" never\n> add anything, so they could leave that as NULL.\n> \n> But it's probably not worth it, just food for thought...\n\nYes, I noticed the advertise() function is really doing double-duty\nhere. Nothing changes with respect to it in my series, but I agree it's\na bit confusing.\n\nThe functions are simple enough that splitting up the two functions of\nadvertise() might make sense. Likewise, your documentation seems\nreasonable. I'd prefer to punt on it for now, though, as my series isn't\notherwise touching this function.\n\nI'm sure there are textual conflicts with what I'm doing here, since\nit's nearby, but I'd prefer to just build more cleanups on top later.\n\n-Peff\n"},{"id":"435991","messageId":"YUIhtvBWvzTnF2Xw@coredump.intra.peff.net","threadId":"56500","inReplyTo":"YUFzhC5Eox8IND4O@nand.local","subject":"Re: [PATCH v2 07/11] ls-refs: ignore very long ref-prefix counts","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-15T16:39:18Z","receivedAt":"2021-09-15T16:39:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 15, 2021 at 12:16:04AM -0400, Taylor Blau wrote:\n\n> > +\t/*\n> > +\t * If we saw too many prefixes, we must avoid using them at all; as\n> > +\t * soon as we have any prefix, they are meant to form a comprehensive\n> > +\t * list.\n> > +\t */\n> > +\tif (data.prefixes.nr >= TOO_MANY_PREFIXES)\n> > +\t\tstrvec_clear(&data.prefixes);\n> > +\n> \n> Great, I find this version even better than what I suggested where the\n> case where the list already has too many prefixes `continue`s through\n> the loop before calling strvec_add().\n> \n> Just noting aloud, the `>` part of this conditional will never happen\n> (since data.prefixes.nr will be at most equal to TOO_MANY_PREFIXES, but\n> never larger than it).\n> \n> Obviously not wrong, but I figured it'd be worth mentioning in case it\n> caught the attention of other reviewers.\n\nYeah, your analysis is right. Long ago I read some advice (I think from\nK&R, either the C book or a related article) that suggested always using\nbounding checks like this rather than equality, because they do the\nright thing even if the variable ends up with a surprising value.\n\nI can certainly see an argument against it (like that if you're so\nworried about it, maybe you ought to detect it and figure out who is\nsetting the variable to be unexpectedly high). But it seems like a\nreasonable defensive measure to me (against an off-by-one mis-analysis,\nor against the earlier code changing in the future).\n\n-Peff\n"},{"id":"435992","messageId":"YUIiAYCAj+YC6w41@coredump.intra.peff.net","threadId":"56500","inReplyTo":"CAPig+cQxSTzo0tCUEb3uMhyRLS0hyuOM5ksZyH+h2StFupQ9Yw@mail.gmail.com","subject":"Re: [PATCH v2 07/11] ls-refs: ignore very long ref-prefix counts","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-15T16:40:33Z","receivedAt":"2021-09-15T16:40:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 15, 2021 at 01:00:01AM -0400, Eric Sunshine wrote:\n\n> On Tue, Sep 14, 2021 at 7:51 PM Jeff King wrote:\n> > diff --git a/t/t5701-git-serve.sh b/t/t5701-git-serve.sh\n> > @@ -158,6 +158,37 @@ test_expect_success 'refs/heads prefix' '\n> > +test_expect_success 'ignore very large set of prefixes' '\n> > + # generate a large number of ref-prefixes that we expect\n> > + # to match nothing; the value here exceeds TOO_MANY_PREFIXES\n> > + # from ls-refs.c.\n> > + {\n> > + echo command=ls-refs &&\n> > + echo object-format=$(test_oid algo)\n> > + echo 0001 &&\n> > + perl -le \"print \\\"ref-prefix refs/heads/\\$_\\\" for (1..65536)\" &&\n> > + echo 0000\n> > + } |\n> > + test-tool pkt-line pack >in &&\n> \n> Broken &&-chain in {...}.\n\nThanks, will fix.\n\n> (Granted, it doesn't matter in this case since the exit code gets lost\n> down the pipe, but better to be consistent about it.)\n\nYep. I think what happened is that I started to convert this to a\nhere-doc to match the nearby tests, but then realized that expanding the\nmulti-line bit was weird (even more so when it was a shell loop with a\npipe, before I switched it to perl).\n\n-Peff\n"},{"id":"435993","messageId":"xmqqpmt9yeo7.fsf@gitster.g","threadId":"56500","inReplyTo":"YUE1fGZc1FuuyUNH@coredump.intra.peff.net","subject":"Re: [PATCH v2 03/11] serve: add \"receive\" method for v2 capabilities table","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-09-15T16:41:28Z","receivedAt":"2021-09-15T16:41:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> +\t/*\n> +\t * Function called when a client requests the capability as a\n> +\t * non-command. This may be NULL if the capability does nothing.\n> +\t *\n> +\t * For a capability of the form \"foo=bar\", the value string points to\n> +\t * the content after the \"=\" (i.e., \"bar\"). For simple capabilities\n> +\t * (just \"foo\"), it is NULL.\n> +\t */\n> +\tvoid (*receive)(struct repository *r, const char *value);\n\nWhat does \"as a non-command\" mean? To put it another way, when a\nclient requests the capability as a command, what does the receive\nmethod do differently?\n\n> @@ -164,12 +174,17 @@ static struct protocol_capability *get_capability(const char *key, const char **\n> \treturn NULL;\n> }\n> \n> -static int is_valid_capability(const char *key)\n> +static int receive_client_capability(const char *key)\n> {\n> \tconst char *value;\n> \tconst struct protocol_capability *c = get_capability(key, &value);\n> \n> -\treturn c && c->advertise(the_repository, NULL);\n> +\tif (!c || !c->advertise(the_repository, NULL))\n> +\t\treturn 0;\n> +\n> +\tif (c->receive)\n> +\t\tc->receive(the_repository, value);\n> +\treturn 1;\n> }\n> \n> static int parse_command(const char *key, struct protocol_capability **command)\n> @@ -262,7 +277,7 @@ static int process_request(void)\n> \t\tcase PACKET_READ_NORMAL:\n> \t\t\t/* collect request; a sequence of keys and values */\n\nThe comment tentatively gets slightly stale here, but that will be\ncorrected at the end, so it would be fine ;-)\n\n> \t\t\tif (parse_command(reader.line, &command) ||\n> -\t\t\t is_valid_capability(reader.line))\n> +\t\t\t receive_client_capability(reader.line))\n> \t\t\t\tstrvec_push(&keys, reader.line);\n> \t\t\telse\n> \t\t\t\tdie(\"unknown capability '%s'\", reader.line);\n"},{"id":"435994","messageId":"YUIiUp0CiqAbMvPw@coredump.intra.peff.net","threadId":"56500","inReplyTo":"871r5qof8k.fsf@evledraar.gmail.com","subject":"Re: [PATCH 0/9] reducing memory allocations for v2 servers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-15T16:41:54Z","receivedAt":"2021-09-15T16:42:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 15, 2021 at 02:25:49AM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> On Tue, Sep 14 2021, Jeff King wrote:\n> \n> > While looking at [1], I noticed that v2 servers will read a few bits of\n> > client input into strvecs. Even though we expect these to be small-ish,\n> > there's nothing preventing a client from sending us a bunch of junk and\n> > wasting memory.\n> \n> [...]\n> \n> >\n> > [1] https://lore.kernel.org/git/YT54CNYgtGcqexwq@coredump.intra.peff.net/\n> \n> When I first read this I expected it to be a link to\n> https://lore.kernel.org/git/YPCsDLoiiAG%2FC%2Fft@coredump.intra.peff.net/;\n> i.e. that the object-info leak discussion back in July had encouraged\n> you to work on this ... :)\n\nNope, I got terrified by the \"yes | upload-pack\" example I showed\nearlier. :)\n\nI was really worried it could turn into a heap overflow, but it turns\nout that it cannot. But I still think tightening things up (and avoiding\nany memory-consumption attacks) is worth doing.\n\n-Peff\n"},{"id":"435998","messageId":"xmqqlf3xye1r.fsf@gitster.g","threadId":"56500","inReplyTo":"YUE1fy0eA0wQnn3f@coredump.intra.peff.net","subject":"Re: [PATCH v2 04/11] serve: provide \"receive\" function for object-format capability","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-09-15T16:54:56Z","receivedAt":"2021-09-15T16:55:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> There should be no change of behavior here, except for two\n> broken-protocol cases:\n>\n> - if the client sends multiple conflicting object-format capabilities\n> (which they should not), we'll now choose the last one rather than\n> the first. We could also detect and complain about the duplicates\n> quite easily now, which we could not before, but I didn't do so\n> here.\n\nI'd imagine that it would have been easy to retain the \"use the\nfirst one\" behaviour if we wanted to; I do not think this behaviour\nchange really matters in practice, so it's OK.\n\n> - if the client sends a bogus \"object-format\" with no equals sign,\n> we'll now say so, rather than \"unknown object format: ''\"\n\nOK.\n\n> @@ -228,22 +241,6 @@ static int has_capability(const struct strvec *keys, const char *capability,\n> \treturn 0;\n> }\n> \n> -static void check_algorithm(struct repository *r, struct strvec *keys)\n> -{\n> -\tint client = GIT_HASH_SHA1, server = hash_algo_by_ptr(r->hash_algo);\n> -\tconst char *algo_name;\n> -\n> -\tif (has_capability(keys, \"object-format\", &algo_name)) {\n> -\t\tclient = hash_algo_by_name(algo_name);\n> -\t\tif (client == GIT_HASH_UNKNOWN)\n> -\t\t\tdie(\"unknown object format '%s'\", algo_name);\n> -\t}\n> -\n> -\tif (client != server)\n> -\t\tdie(\"mismatched object format: server %s; client %s\\n\",\n> -\t\t r->hash_algo->name, hash_algos[client].name);\n> -}\n> -\n> enum request_state {\n> \tPROCESS_REQUEST_KEYS,\n> \tPROCESS_REQUEST_DONE,\n> @@ -317,7 +314,10 @@ static int process_request(void)\n> \tif (!command)\n> \t\tdie(\"no command requested\");\n> \n> -\tcheck_algorithm(the_repository, &keys);\n> +\tif (client_hash_algo != hash_algo_by_ptr(the_repository->hash_algo))\n> +\t\tdie(\"mismatched object format: server %s; client %s\\n\",\n> +\t\t the_repository->hash_algo->name,\n> +\t\t hash_algos[client_hash_algo].name);\n\nOK.\n"},{"id":"435999","messageId":"xmqqh7elydyr.fsf@gitster.g","threadId":"56500","inReplyTo":"YUE1gvovwj/X1wWV@coredump.intra.peff.net","subject":"Re: [PATCH v2 05/11] serve: provide \"receive\" function for session-id capability","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-09-15T16:56:44Z","receivedAt":"2021-09-15T16:56:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Rather than pulling the session-id string from the list of collected\n> capabilities, we can handle it as soon as we receive it. This gets us\n> closer to dropping the collected list entirely.\n>\n> The behavior should be the same, with one exception. Previously if the\n> client sent us multiple session-id lines, we'd report only the first.\n> Now we'll pass each one along to trace2. This shouldn't matter in\n> practice, since clients shouldn't do that (and if they do, it's probably\n> sensible to log them all).\n>\n> As this removes the last caller of the static has_capability(), we can\n> remove it, as well (and in fact we must to avoid -Wunused-function\n> complaining).\n\nNice.\n\n>\n> Signed-off-by: Jeff King \n> ---\n> serve.c | 33 +++++++++------------------------\n> 1 file changed, 9 insertions(+), 24 deletions(-)\n>\n> diff --git a/serve.c b/serve.c\n> index f6ea2953eb..6bbf54cbbe 100644\n> --- a/serve.c\n> +++ b/serve.c\n> @@ -57,6 +57,14 @@ static int session_id_advertise(struct repository *r, struct strbuf *value)\n> \treturn 1;\n> }\n> \n> +static void session_id_receive(struct repository *r,\n> +\t\t\t const char *client_sid)\n> +{\n> +\tif (!client_sid)\n> +\t\tclient_sid = \"\";\n> +\ttrace2_data_string(\"transfer\", NULL, \"client-sid\", client_sid);\n> +}\n> +\n> struct protocol_capability {\n> \t/*\n> \t * The name of the capability. The server uses this name when\n> @@ -121,6 +129,7 @@ static struct protocol_capability capabilities[] = {\n> \t{\n> \t\t.name = \"session-id\",\n> \t\t.advertise = session_id_advertise,\n> +\t\t.receive = session_id_receive,\n> \t},\n> \t{\n> \t\t.name = \"object-info\",\n> @@ -221,26 +230,6 @@ static int parse_command(const char *key, struct protocol_capability **command)\n> \treturn 0;\n> }\n> \n> -static int has_capability(const struct strvec *keys, const char *capability,\n> -\t\t\t const char **value)\n> -{\n> -\tint i;\n> -\tfor (i = 0; i < keys->nr; i++) {\n> -\t\tconst char *out;\n> -\t\tif (skip_prefix(keys->v[i], capability, &out) &&\n> -\t\t (!*out || *out == '=')) {\n> -\t\t\tif (value) {\n> -\t\t\t\tif (*out == '=')\n> -\t\t\t\t\tout++;\n> -\t\t\t\t*value = out;\n> -\t\t\t}\n> -\t\t\treturn 1;\n> -\t\t}\n> -\t}\n> -\n> -\treturn 0;\n> -}\n> -\n> enum request_state {\n> \tPROCESS_REQUEST_KEYS,\n> \tPROCESS_REQUEST_DONE,\n> @@ -252,7 +241,6 @@ static int process_request(void)\n> \tstruct packet_reader reader;\n> \tstruct strvec keys = STRVEC_INIT;\n> \tstruct protocol_capability *command = NULL;\n> -\tconst char *client_sid;\n> \n> \tpacket_reader_init(&reader, 0, NULL, 0,\n> \t\t\t PACKET_READ_CHOMP_NEWLINE |\n> @@ -319,9 +307,6 @@ static int process_request(void)\n> \t\t the_repository->hash_algo->name,\n> \t\t hash_algos[client_hash_algo].name);\n> \n> -\tif (has_capability(&keys, \"session-id\", &client_sid))\n> -\t\ttrace2_data_string(\"transfer\", NULL, \"client-sid\", client_sid);\n> -\n> \tcommand->command(the_repository, &reader);\n> \n> \tstrvec_clear(&keys);\n"},{"id":"436000","messageId":"YUIl8hhzi3aE1FA/@coredump.intra.peff.net","threadId":"56500","inReplyTo":"xmqqpmt9yeo7.fsf@gitster.g","subject":"Re: [PATCH v2 03/11] serve: add \"receive\" method for v2 capabilities table","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-15T16:57:22Z","receivedAt":"2021-09-15T16:57:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 15, 2021 at 09:41:28AM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > +\t/*\n> > +\t * Function called when a client requests the capability as a\n> > +\t * non-command. This may be NULL if the capability does nothing.\n> > +\t *\n> > +\t * For a capability of the form \"foo=bar\", the value string points to\n> > +\t * the content after the \"=\" (i.e., \"bar\"). For simple capabilities\n> > +\t * (just \"foo\"), it is NULL.\n> > +\t */\n> > +\tvoid (*receive)(struct repository *r, const char *value);\n> \n> What does \"as a non-command\" mean? To put it another way, when a\n> client requests the capability as a command, what does the receive\n> method do differently?\n\nFor each entry in this capability table, clients can say:\n\n command=foo\n\nor just:\n\n foo\n\nThe latter is a non-command. The \"receive\" function is not called at all\nif it is a \"command\". I think this is a bit more clear when read\ntogether with the existing function just above (which you can't see in\nthe diff context):\n\n /*\n * Function called when a client requests the capability as a command.\n * Will be provided a struct packet_reader 'request' which it should\n * use to read the command specific part of the request. Every command\n * MUST read until a flush packet is seen before sending a response.\n *\n * This field should be NULL for capabilities which are not commands.\n */\n int (*command)(struct repository *r, struct packet_reader *request);\n\nI guess these could define \"as a command\", but I think it's pretty clear\nin the context.\n\n> > static int parse_command(const char *key, struct protocol_capability **command)\n> > @@ -262,7 +277,7 @@ static int process_request(void)\n> > \t\tcase PACKET_READ_NORMAL:\n> > \t\t\t/* collect request; a sequence of keys and values */\n> \n> The comment tentatively gets slightly stale here, but that will be\n> corrected at the end, so it would be fine ;-)\n\nHmm. I think it is not stale here, as we are still collecting the \"keys\"\nstrvec. But it _does_ get stale by the end, when we stop doing so.\n\nI'm preparing a re-roll for the test issues Eric noted, so I'll drop\nthat comment in the appropriate patch.\n\n-Peff\n"},{"id":"436003","messageId":"xmqqczp9ydqn.fsf@gitster.g","threadId":"56500","inReplyTo":"YUE1hExkU9V12iZv@coredump.intra.peff.net","subject":"Re: [PATCH v2 06/11] serve: drop \"keys\" strvec","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-09-15T17:01:36Z","receivedAt":"2021-09-15T17:01:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> We collect the set of capabilities the client sends us in a strvec.\n> While this is usually small, there's no limit to the number of\n> capabilities the client can send us (e.g., they could just send us\n> \"agent\" pkt-lines over and over, and we'd keep adding them to the list).\n>\n> Since all code has been converted away from using this list, let's get\n> rid of it. This avoids a potential attack where clients waste our\n> memory.\n>\n> Note that we do have to replace it with a flag, because some of the\n> flush-packet logic checks whether we've seen any valid commands or keys.\n>\n> Signed-off-by: Jeff King \n> ---\n> serve.c | 7 +++----\n> 1 file changed, 3 insertions(+), 4 deletions(-)\n>\n> diff --git a/serve.c b/serve.c\n> index 6bbf54cbbe..5ea6c915cb 100644\n> --- a/serve.c\n> +++ b/serve.c\n> @@ -239,7 +239,7 @@ static int process_request(void)\n> {\n> \tenum request_state state = PROCESS_REQUEST_KEYS;\n> \tstruct packet_reader reader;\n> -\tstruct strvec keys = STRVEC_INIT;\n> +\tint seen_capability_or_command = 0;\n> \tstruct protocol_capability *command = NULL;\n> \n> \tpacket_reader_init(&reader, 0, NULL, 0,\n> @@ -263,7 +263,7 @@ static int process_request(void)\n> \t\t\t/* collect request; a sequence of keys and values */\n> \t\t\tif (parse_command(reader.line, &command) ||\n> \t\t\t receive_client_capability(reader.line))\n> -\t\t\t\tstrvec_push(&keys, reader.line);\n> +\t\t\t\tseen_capability_or_command = 1;\n\nOK, we no longer \"collect\" request in the keys strvec, but I guess\nwhat receive_client_capability() does still counts as \"collecting\",\nso the \"tentatively stale\" comment is not wrong after all at the end\n(we have tentatively been collecting in two different places and one\nof them is dropped here).\n\n> @@ -275,7 +275,7 @@ static int process_request(void)\n> \t\t\t * If no command and no keys were given then the client\n> \t\t\t * wanted to terminate the connection.\n> \t\t\t */\n> -\t\t\tif (!keys.nr)\n> +\t\t\tif (!seen_capability_or_command)\n> \t\t\t\treturn 1;\n> \n> \t\t\t/*\n> @@ -309,7 +309,6 @@ static int process_request(void)\n> \n> \tcommand->command(the_repository, &reader);\n> \n> -\tstrvec_clear(&keys);\n> \treturn 0;\n> }\n\nNice.\n"},{"id":"436007","messageId":"xmqq8rzxyc9f.fsf@gitster.g","threadId":"56500","inReplyTo":"YUE1ym1dALRQLztq@coredump.intra.peff.net","subject":"Re: [PATCH v2 09/11] serve: reject bogus v2 \"command=ls-refs=foo\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-09-15T17:33:32Z","receivedAt":"2021-09-15T17:33:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> When we see a line from the client like \"command=ls-refs\", we parse\n> everything after the equals sign as a capability, which we check against\n> our capabilities table. If we don't recognize the command (e.g.,\n> \"command=foo\"), we'll reject it. But we use the same parser that checks\n> for regular capabilities like \"object-format=sha256\". And so we'll\n> accept \"ls-refs=foo\", even though everything after the equals is bogus,\n> and simply ignored.\n\nMaybe I am slow but I had to read the above a few times and finally\nlook at the implementation of parse_command() to realize that what\nthe last sentence describes is:\n\n When parse_command() is fed \"command=ls-refs=foo\", it strips\n \"command=\", feeds \"ls-refs=foo\" to get_capability(), and because\n we do not ensure value is NULL, we silently ignore \"=foo\" that\n is bogus.\n\nAnd it makes sense. It would probably have helped if I peeked the\nupdated test ;-)\n\n> This isn't really hurting anything, but the request does violate the\n> spec. Let's tighten it up to prevent any surprising behavior.\n\nGood.\n"},{"id":"436008","messageId":"YUIvtEYybqnvpSQ/@coredump.intra.peff.net","threadId":"56500","inReplyTo":"xmqq8rzxyc9f.fsf@gitster.g","subject":"Re: [PATCH v2 09/11] serve: reject bogus v2 \"command=ls-refs=foo\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-15T17:39:00Z","receivedAt":"2021-09-15T17:39:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 15, 2021 at 10:33:32AM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > When we see a line from the client like \"command=ls-refs\", we parse\n> > everything after the equals sign as a capability, which we check against\n> > our capabilities table. If we don't recognize the command (e.g.,\n> > \"command=foo\"), we'll reject it. But we use the same parser that checks\n> > for regular capabilities like \"object-format=sha256\". And so we'll\n> > accept \"ls-refs=foo\", even though everything after the equals is bogus,\n> > and simply ignored.\n> \n> Maybe I am slow but I had to read the above a few times and finally\n> look at the implementation of parse_command() to realize that what\n> the last sentence describes is:\n> \n> When parse_command() is fed \"command=ls-refs=foo\", it strips\n> \"command=\", feeds \"ls-refs=foo\" to get_capability(), and because\n> we do not ensure value is NULL, we silently ignore \"=foo\" that\n> is bogus.\n> \n> And it makes sense. It would probably have helped if I peeked the\n> updated test ;-)\n\nSince I'm re-rolling anyway, I'll expand it a bit (and also cover Ævar's\n\"what exactly does violate mean here\" question).\n\n-Peff\n"},{"id":"436022","messageId":"YUI8kBkK6SJYJcDO@coredump.intra.peff.net","threadId":"56500","inReplyTo":"YUE1alo58cGyTw6/@coredump.intra.peff.net","subject":"Re: [PATCH v2 0/11] limit memory allocations for v2 servers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-15T18:33:52Z","receivedAt":"2021-09-15T18:33:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"OK, hopefully third time's the charm. The changes from v2 are pretty\nminor:\n\n - test typos noticed by Eric (none affected the outcome of the tests)\n\n - I added \"object-format=$(test_oid algo)\" to the input of a few of the\n new tests even where it doesn't change the outcome, for consistency\n with the existing tests\n\n - dropped out-dated \"collect\" comment noticed by Junio\n\n - explained the command=ls-refs=foo case a little further, including\n specific references in how it violates the spec\n\nRange-diff is below.\n\n [01/11]: serve: rename is_command() to parse_command()\n [02/11]: serve: return capability \"value\" from get_capability()\n [03/11]: serve: add \"receive\" method for v2 capabilities table\n [04/11]: serve: provide \"receive\" function for object-format capability\n [05/11]: serve: provide \"receive\" function for session-id capability\n [06/11]: serve: drop \"keys\" strvec\n [07/11]: ls-refs: ignore very long ref-prefix counts\n [08/11]: docs/protocol-v2: clarify some ls-refs ref-prefix details\n [09/11]: serve: reject bogus v2 \"command=ls-refs=foo\"\n [10/11]: serve: reject commands used as capabilities\n [11/11]: ls-refs: reject unknown arguments\n\n Documentation/technical/protocol-v2.txt | 6 +-\n ls-refs.c | 22 ++++-\n serve.c | 120 +++++++++++++-----------\n t/t5701-git-serve.sh | 75 +++++++++++++++\n 4 files changed, 164 insertions(+), 59 deletions(-)\n\n 1: e642ced1e8 = 1: 71003eb21a serve: rename is_command() to parse_command()\n 2: 75d119ae49 = 2: 1e86c31477 serve: return capability \"value\" from get_capability()\n 3: 9fb21cab1d = 3: 12a159d5c8 serve: add \"receive\" method for v2 capabilities table\n 4: 5b661c9ed5 = 4: f5c29b5cdf serve: provide \"receive\" function for object-format capability\n 5: 34e0ce5c12 = 5: 063cb60d1e serve: provide \"receive\" function for session-id capability\n 6: 8e42fa9aec ! 6: 0ed0b946ea serve: drop \"keys\" strvec\n @@ serve.c: static int process_request(void)\n \n \tpacket_reader_init(&reader, 0, NULL, 0,\n @@ serve.c: static int process_request(void)\n - \t\t\t/* collect request; a sequence of keys and values */\n + \t\tcase PACKET_READ_EOF:\n + \t\t\tBUG(\"Should have already died when seeing EOF\");\n + \t\tcase PACKET_READ_NORMAL:\n +-\t\t\t/* collect request; a sequence of keys and values */\n \t\t\tif (parse_command(reader.line, &command) ||\n \t\t\t receive_client_capability(reader.line))\n -\t\t\t\tstrvec_push(&keys, reader.line);\n 7: de7ac11ad3 ! 7: a9392d0e68 ls-refs: ignore very long ref-prefix counts\n @@ t/t5701-git-serve.sh: test_expect_success 'refs/heads prefix' '\n +\t# from ls-refs.c.\n +\t{\n +\t\techo command=ls-refs &&\n -+\t\techo object-format=$(test_oid algo)\n ++\t\techo object-format=$(test_oid algo) &&\n +\t\techo 0001 &&\n +\t\tperl -le \"print \\\"ref-prefix refs/heads/\\$_\\\" for (1..65536)\" &&\n +\t\techo 0000\n 8: 3f78422fd3 = 8: 0a982f676a docs/protocol-v2: clarify some ls-refs ref-prefix details\n 9: 6c55a7412d ! 9: 553db6f83e serve: reject bogus v2 \"command=ls-refs=foo\"\n @@ Commit message\n When we see a line from the client like \"command=ls-refs\", we parse\n everything after the equals sign as a capability, which we check against\n our capabilities table. If we don't recognize the command (e.g.,\n - \"command=foo\"), we'll reject it. But we use the same parser that checks\n - for regular capabilities like \"object-format=sha256\". And so we'll\n - accept \"ls-refs=foo\", even though everything after the equals is bogus,\n - and simply ignored.\n + \"command=foo\"), we'll reject it.\n \n - This isn't really hurting anything, but the request does violate the\n - spec. Let's tighten it up to prevent any surprising behavior.\n + But in parse_command(), we use the same get_capability() parser for\n + parsing non-command lines. So if we see \"command=ls-refs=foo\", we will\n + feed \"ls-refs=foo\" to get_capability(), which will say \"OK, that's\n + ls-refs, with value 'foo'\". But then we simply ignore the value\n + entirely.\n +\n + The client is violating the spec here, which says:\n +\n + command = PKT-LINE(\"command=\" key LF)\n + key = 1*(ALPHA | DIGIT | \"-_\")\n +\n + I.e., the key is not even allowed to have an equals sign in it. Whereas\n + a real non-command capability does allow a value:\n +\n + capability = PKT-LINE(key[=value] LF)\n +\n + So by reusing the same get_capability() parser, we are mixing up the\n + \"key\" and \"capability\" tokens. However, since that parser tells us\n + whether it saw an \"=\", we can still use it; we just reject any input\n + that produces a non-NULL value field.\n +\n + The current behavior isn't really hurting anything (the client should\n + never send such a request, and if it does, we just ignore the \"value\"\n + part). But since it does violate the spec, let's tighten it up to\n + prevent any surprising behavior.\n \n Signed-off-by: Jeff King \n \n @@ t/t5701-git-serve.sh: test_expect_success 'request invalid command' '\n '\n \n +test_expect_success 'requested command is command=value' '\n -+\ttest-tool pkt-line pack >in <<-\\EOF &&\n ++\ttest-tool pkt-line pack >in <<-EOF &&\n +\tcommand=ls-refs=whatever\n +\tobject-format=$(test_oid algo)\n +\t0000\n10: bbb12669b9 ! 10: 93216b9eaa serve: reject commands used as capabilities\n @@ t/t5701-git-serve.sh: test_expect_success 'request invalid command' '\n '\n \n +test_expect_success 'request capability as command' '\n -+\ttest-tool pkt-line pack >in <<-\\EOF &&\n ++\ttest-tool pkt-line pack >in <<-EOF &&\n +\tcommand=agent\n ++\tobject-format=$(test_oid algo)\n +\t0000\n +\tEOF\n +\ttest_must_fail test-tool serve-v2 --stateless-rpc 2>err in <<-\\EOF &&\n ++\ttest-tool pkt-line pack >in <<-EOF &&\n +\tcommand=ls-refs\n ++\tobject-format=$(test_oid algo)\n +\tfetch\n +\t0000\n +\tEOF\n @@ t/t5701-git-serve.sh: test_expect_success 'request invalid command' '\n +'\n +\n test_expect_success 'requested command is command=value' '\n - \ttest-tool pkt-line pack >in <<-\\EOF &&\n + \ttest-tool pkt-line pack >in <<-EOF &&\n \tcommand=ls-refs=whatever\n11: 375a85b9a6 = 11: 30802eeb54 ls-refs: reject unknown arguments\n"},{"id":"436023","messageId":"YUI8z5SiyvgrDBas@coredump.intra.peff.net","threadId":"56500","inReplyTo":"YUI8kBkK6SJYJcDO@coredump.intra.peff.net","subject":"[PATCH v3 0/11] limit memory allocations for v2 servers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-15T18:34:55Z","receivedAt":"2021-09-15T18:34:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 15, 2021 at 02:33:53PM -0400, Jeff King wrote:\n\n> OK, hopefully third time's the charm. The changes from v2 are pretty\n> minor:\n> \n> - test typos noticed by Eric (none affected the outcome of the tests)\n> \n> - I added \"object-format=$(test_oid algo)\" to the input of a few of the\n> new tests even where it doesn't change the outcome, for consistency\n> with the existing tests\n> \n> - dropped out-dated \"collect\" comment noticed by Junio\n> \n> - explained the command=ls-refs=foo case a little further, including\n> specific references in how it violates the spec\n> \n> Range-diff is below.\n\nEr, that should have been \"v3\" in the subject, as this one now is.\n\n-Peff\n"},{"id":"436024","messageId":"YUI82g8+b4VbVvOH@coredump.intra.peff.net","threadId":"56500","inReplyTo":"YUI8z5SiyvgrDBas@coredump.intra.peff.net","subject":"[PATCH v3 01/11] serve: rename is_command() to parse_command()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-15T18:35:06Z","receivedAt":"2021-09-15T18:35:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The is_command() function not only tells us whether the pktline is a\nvalid command string, but it also parses out the command (and complains\nif we see a duplicate). Let's rename it to make those extra functions\na bit more obvious.\n\nSigned-off-by: Jeff King \n---\n serve.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/serve.c b/serve.c\nindex 1817edc7f5..fd88b95343 100644\n--- a/serve.c\n+++ b/serve.c\n@@ -163,7 +163,7 @@ static int is_valid_capability(const char *key)\n \treturn c && c->advertise(the_repository, NULL);\n }\n \n-static int is_command(const char *key, struct protocol_capability **command)\n+static int parse_command(const char *key, struct protocol_capability **command)\n {\n \tconst char *out;\n \n@@ -251,7 +251,7 @@ static int process_request(void)\n \t\t\tBUG(\"Should have already died when seeing EOF\");\n \t\tcase PACKET_READ_NORMAL:\n \t\t\t/* collect request; a sequence of keys and values */\n-\t\t\tif (is_command(reader.line, &command) ||\n+\t\t\tif (parse_command(reader.line, &command) ||\n \t\t\t is_valid_capability(reader.line))\n \t\t\t\tstrvec_push(&keys, reader.line);\n \t\t\telse\n-- \n2.33.0.917.g33ebf6a5f6\n\n"},{"id":"436025","messageId":"YUI84S26UeLRUjVg@coredump.intra.peff.net","threadId":"56500","inReplyTo":"YUI8z5SiyvgrDBas@coredump.intra.peff.net","subject":"[PATCH v3 02/11] serve: return capability \"value\" from get_capability()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-15T18:35:13Z","receivedAt":"2021-09-15T18:35:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When the client sends v2 capabilities, they may be simple, like:\n\n foo\n\nor have a value like:\n\n foo=bar\n\n(all of the current capabilities actually expect a value, but the\nprotocol allows for boolean ones).\n\nWe use get_capability() to make sure the client's pktline matches a\ncapability. In doing so, we parse enough to see the \"=\" and the value\n(if any), but we immediately forget it. Nobody cares for now, because they end\nup parsing the values out later using has_capability(). But in\npreparation for changing that, let's pass back a pointer so the callers\nknow what we found.\n\nNote that unlike has_capability(), we'll return NULL for a \"simple\"\ncapability. Distinguishing these will be useful for some future patches.\n\nSigned-off-by: Jeff King \n---\n serve.c | 18 ++++++++++++++----\n 1 file changed, 14 insertions(+), 4 deletions(-)\n\ndiff --git a/serve.c b/serve.c\nindex fd88b95343..78a4e83554 100644\n--- a/serve.c\n+++ b/serve.c\n@@ -139,7 +139,7 @@ void protocol_v2_advertise_capabilities(void)\n \tstrbuf_release(&value);\n }\n \n-static struct protocol_capability *get_capability(const char *key)\n+static struct protocol_capability *get_capability(const char *key, const char **value)\n {\n \tint i;\n \n@@ -149,16 +149,25 @@ static struct protocol_capability *get_capability(const char *key)\n \tfor (i = 0; i < ARRAY_SIZE(capabilities); i++) {\n \t\tstruct protocol_capability *c = &capabilities[i];\n \t\tconst char *out;\n-\t\tif (skip_prefix(key, c->name, &out) && (!*out || *out == '='))\n+\t\tif (!skip_prefix(key, c->name, &out))\n+\t\t\tcontinue;\n+\t\tif (!*out) {\n+\t\t\t*value = NULL;\n \t\t\treturn c;\n+\t\t}\n+\t\tif (*out++ == '=') {\n+\t\t\t*value = out;\n+\t\t\treturn c;\n+\t\t}\n \t}\n \n \treturn NULL;\n }\n \n static int is_valid_capability(const char *key)\n {\n-\tconst struct protocol_capability *c = get_capability(key);\n+\tconst char *value;\n+\tconst struct protocol_capability *c = get_capability(key, &value);\n \n \treturn c && c->advertise(the_repository, NULL);\n }\n@@ -168,7 +177,8 @@ static int parse_command(const char *key, struct protocol_capability **command)\n \tconst char *out;\n \n \tif (skip_prefix(key, \"command=\", &out)) {\n-\t\tstruct protocol_capability *cmd = get_capability(out);\n+\t\tconst char *value;\n+\t\tstruct protocol_capability *cmd = get_capability(out, &value);\n \n \t\tif (*command)\n \t\t\tdie(\"command '%s' requested after already requesting command '%s'\",\n-- \n2.33.0.917.g33ebf6a5f6\n\n"},{"id":"436026","messageId":"YUI85uvjn+7LB5gN@coredump.intra.peff.net","threadId":"56500","inReplyTo":"YUI8z5SiyvgrDBas@coredump.intra.peff.net","subject":"[PATCH v3 03/11] serve: add \"receive\" method for v2 capabilities table","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-15T18:35:18Z","receivedAt":"2021-09-15T18:35:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We have a capabilities table that tells us what we should tell the\nclient we are capable of, and what to do when a client gives us a\nparticular command (e.g., \"command=ls-refs\"). But it doesn't tell us\nwhat to do when the client sends us back a capability (e.g.,\n\"object-format=sha256\"). We just collect them all in a strvec and hope\nsomebody can use them later.\n\nInstead, let's provide a function pointer in the table to act on these.\nThis will eventually help us avoid collecting the strings, which will be\nmore efficient and less prone to mischief.\n\nUsing the new method is optional, which helps in two ways:\n\n - we can move existing capabilities over to this new system gradually\n in individual commits\n\n - some capabilities we don't actually do anything with anyway. For\n example, the client is free to say \"agent=git/1.2.3\" to us, but we\n do not act on the information in any way.\n\nSigned-off-by: Jeff King \n---\n serve.c | 21 ++++++++++++++++++---\n 1 file changed, 18 insertions(+), 3 deletions(-)\n\ndiff --git a/serve.c b/serve.c\nindex 78a4e83554..a161189984 100644\n--- a/serve.c\n+++ b/serve.c\n@@ -70,6 +70,16 @@ struct protocol_capability {\n \t * This field should be NULL for capabilities which are not commands.\n \t */\n \tint (*command)(struct repository *r, struct packet_reader *request);\n+\n+\t/*\n+\t * Function called when a client requests the capability as a\n+\t * non-command. This may be NULL if the capability does nothing.\n+\t *\n+\t * For a capability of the form \"foo=bar\", the value string points to\n+\t * the content after the \"=\" (i.e., \"bar\"). For simple capabilities\n+\t * (just \"foo\"), it is NULL.\n+\t */\n+\tvoid (*receive)(struct repository *r, const char *value);\n };\n \n static struct protocol_capability capabilities[] = {\n@@ -164,12 +174,17 @@ static struct protocol_capability *get_capability(const char *key, const char **\n \treturn NULL;\n }\n \n-static int is_valid_capability(const char *key)\n+static int receive_client_capability(const char *key)\n {\n \tconst char *value;\n \tconst struct protocol_capability *c = get_capability(key, &value);\n \n-\treturn c && c->advertise(the_repository, NULL);\n+\tif (!c || !c->advertise(the_repository, NULL))\n+\t\treturn 0;\n+\n+\tif (c->receive)\n+\t\tc->receive(the_repository, value);\n+\treturn 1;\n }\n \n static int parse_command(const char *key, struct protocol_capability **command)\n@@ -262,7 +277,7 @@ static int process_request(void)\n \t\tcase PACKET_READ_NORMAL:\n \t\t\t/* collect request; a sequence of keys and values */\n \t\t\tif (parse_command(reader.line, &command) ||\n-\t\t\t is_valid_capability(reader.line))\n+\t\t\t receive_client_capability(reader.line))\n \t\t\t\tstrvec_push(&keys, reader.line);\n \t\t\telse\n \t\t\t\tdie(\"unknown capability '%s'\", reader.line);\n-- \n2.33.0.917.g33ebf6a5f6\n\n"},{"id":"436027","messageId":"YUI864c4ubcMQyOj@coredump.intra.peff.net","threadId":"56500","inReplyTo":"YUI8z5SiyvgrDBas@coredump.intra.peff.net","subject":"[PATCH v3 04/11] serve: provide \"receive\" function for object-format capability","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-15T18:35:23Z","receivedAt":"2021-09-15T18:35:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We get any \"object-format\" specified by the client by searching for it\nin the collected list of capabilities the client sent. We can instead\njust handle it as soon as they send it. This is slightly more efficient,\nand gets us one step closer to dropping that collected list.\n\nNote that we do still have to do our final hash check after receiving\nall capabilities (because they might not have sent an object-format line\nat all, and we still have to check that the default matches our\nrepository algorithm). Since the check_algorithm() function would now be\ndown to a single if() statement, I've just inlined it in its only\ncaller.\n\nThere should be no change of behavior here, except for two\nbroken-protocol cases:\n\n - if the client sends multiple conflicting object-format capabilities\n (which they should not), we'll now choose the last one rather than\n the first. We could also detect and complain about the duplicates\n quite easily now, which we could not before, but I didn't do so\n here.\n\n - if the client sends a bogus \"object-format\" with no equals sign,\n we'll now say so, rather than \"unknown object format: ''\"\n\nSigned-off-by: Jeff King \n---\n serve.c | 34 +++++++++++++++++-----------------\n 1 file changed, 17 insertions(+), 17 deletions(-)\n\ndiff --git a/serve.c b/serve.c\nindex a161189984..f6ea2953eb 100644\n--- a/serve.c\n+++ b/serve.c\n@@ -10,6 +10,7 @@\n #include \"upload-pack.h\"\n \n static int advertise_sid = -1;\n+static int client_hash_algo = GIT_HASH_SHA1;\n \n static int always_advertise(struct repository *r,\n \t\t\t struct strbuf *value)\n@@ -33,6 +34,17 @@ static int object_format_advertise(struct repository *r,\n \treturn 1;\n }\n \n+static void object_format_receive(struct repository *r,\n+\t\t\t\t const char *algo_name)\n+{\n+\tif (!algo_name)\n+\t\tdie(\"object-format capability requires an argument\");\n+\n+\tclient_hash_algo = hash_algo_by_name(algo_name);\n+\tif (client_hash_algo == GIT_HASH_UNKNOWN)\n+\t\tdie(\"unknown object format '%s'\", algo_name);\n+}\n+\n static int session_id_advertise(struct repository *r, struct strbuf *value)\n {\n \tif (advertise_sid == -1 &&\n@@ -104,6 +116,7 @@ static struct protocol_capability capabilities[] = {\n \t{\n \t\t.name = \"object-format\",\n \t\t.advertise = object_format_advertise,\n+\t\t.receive = object_format_receive,\n \t},\n \t{\n \t\t.name = \"session-id\",\n@@ -228,22 +241,6 @@ static int has_capability(const struct strvec *keys, const char *capability,\n \treturn 0;\n }\n \n-static void check_algorithm(struct repository *r, struct strvec *keys)\n-{\n-\tint client = GIT_HASH_SHA1, server = hash_algo_by_ptr(r->hash_algo);\n-\tconst char *algo_name;\n-\n-\tif (has_capability(keys, \"object-format\", &algo_name)) {\n-\t\tclient = hash_algo_by_name(algo_name);\n-\t\tif (client == GIT_HASH_UNKNOWN)\n-\t\t\tdie(\"unknown object format '%s'\", algo_name);\n-\t}\n-\n-\tif (client != server)\n-\t\tdie(\"mismatched object format: server %s; client %s\\n\",\n-\t\t r->hash_algo->name, hash_algos[client].name);\n-}\n-\n enum request_state {\n \tPROCESS_REQUEST_KEYS,\n \tPROCESS_REQUEST_DONE,\n@@ -317,7 +314,10 @@ static int process_request(void)\n \tif (!command)\n \t\tdie(\"no command requested\");\n \n-\tcheck_algorithm(the_repository, &keys);\n+\tif (client_hash_algo != hash_algo_by_ptr(the_repository->hash_algo))\n+\t\tdie(\"mismatched object format: server %s; client %s\\n\",\n+\t\t the_repository->hash_algo->name,\n+\t\t hash_algos[client_hash_algo].name);\n \n \tif (has_capability(&keys, \"session-id\", &client_sid))\n \t\ttrace2_data_string(\"transfer\", NULL, \"client-sid\", client_sid);\n-- \n2.33.0.917.g33ebf6a5f6\n\n"},{"id":"436028","messageId":"YUI87iQT/h7zfVGU@coredump.intra.peff.net","threadId":"56500","inReplyTo":"YUI8z5SiyvgrDBas@coredump.intra.peff.net","subject":"[PATCH v3 05/11] serve: provide \"receive\" function for session-id capability","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-15T18:35:26Z","receivedAt":"2021-09-15T18:35:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Rather than pulling the session-id string from the list of collected\ncapabilities, we can handle it as soon as we receive it. This gets us\ncloser to dropping the collected list entirely.\n\nThe behavior should be the same, with one exception. Previously if the\nclient sent us multiple session-id lines, we'd report only the first.\nNow we'll pass each one along to trace2. This shouldn't matter in\npractice, since clients shouldn't do that (and if they do, it's probably\nsensible to log them all).\n\nAs this removes the last caller of the static has_capability(), we can\nremove it, as well (and in fact we must to avoid -Wunused-function\ncomplaining).\n\nSigned-off-by: Jeff King \n---\n serve.c | 33 +++++++++------------------------\n 1 file changed, 9 insertions(+), 24 deletions(-)\n\ndiff --git a/serve.c b/serve.c\nindex f6ea2953eb..6bbf54cbbe 100644\n--- a/serve.c\n+++ b/serve.c\n@@ -57,6 +57,14 @@ static int session_id_advertise(struct repository *r, struct strbuf *value)\n \treturn 1;\n }\n \n+static void session_id_receive(struct repository *r,\n+\t\t\t const char *client_sid)\n+{\n+\tif (!client_sid)\n+\t\tclient_sid = \"\";\n+\ttrace2_data_string(\"transfer\", NULL, \"client-sid\", client_sid);\n+}\n+\n struct protocol_capability {\n \t/*\n \t * The name of the capability. The server uses this name when\n@@ -121,6 +129,7 @@ static struct protocol_capability capabilities[] = {\n \t{\n \t\t.name = \"session-id\",\n \t\t.advertise = session_id_advertise,\n+\t\t.receive = session_id_receive,\n \t},\n \t{\n \t\t.name = \"object-info\",\n@@ -221,26 +230,6 @@ static int parse_command(const char *key, struct protocol_capability **command)\n \treturn 0;\n }\n \n-static int has_capability(const struct strvec *keys, const char *capability,\n-\t\t\t const char **value)\n-{\n-\tint i;\n-\tfor (i = 0; i < keys->nr; i++) {\n-\t\tconst char *out;\n-\t\tif (skip_prefix(keys->v[i], capability, &out) &&\n-\t\t (!*out || *out == '=')) {\n-\t\t\tif (value) {\n-\t\t\t\tif (*out == '=')\n-\t\t\t\t\tout++;\n-\t\t\t\t*value = out;\n-\t\t\t}\n-\t\t\treturn 1;\n-\t\t}\n-\t}\n-\n-\treturn 0;\n-}\n-\n enum request_state {\n \tPROCESS_REQUEST_KEYS,\n \tPROCESS_REQUEST_DONE,\n@@ -252,7 +241,6 @@ static int process_request(void)\n \tstruct packet_reader reader;\n \tstruct strvec keys = STRVEC_INIT;\n \tstruct protocol_capability *command = NULL;\n-\tconst char *client_sid;\n \n \tpacket_reader_init(&reader, 0, NULL, 0,\n \t\t\t PACKET_READ_CHOMP_NEWLINE |\n@@ -319,9 +307,6 @@ static int process_request(void)\n \t\t the_repository->hash_algo->name,\n \t\t hash_algos[client_hash_algo].name);\n \n-\tif (has_capability(&keys, \"session-id\", &client_sid))\n-\t\ttrace2_data_string(\"transfer\", NULL, \"client-sid\", client_sid);\n-\n \tcommand->command(the_repository, &reader);\n \n \tstrvec_clear(&keys);\n-- \n2.33.0.917.g33ebf6a5f6\n\n"},{"id":"436029","messageId":"YUI88Zu5UydDxLre@coredump.intra.peff.net","threadId":"56500","inReplyTo":"YUI8z5SiyvgrDBas@coredump.intra.peff.net","subject":"[PATCH v3 06/11] serve: drop \"keys\" strvec","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-15T18:35:29Z","receivedAt":"2021-09-15T18:35:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We collect the set of capabilities the client sends us in a strvec.\nWhile this is usually small, there's no limit to the number of\ncapabilities the client can send us (e.g., they could just send us\n\"agent\" pkt-lines over and over, and we'd keep adding them to the list).\n\nSince all code has been converted away from using this list, let's get\nrid of it. This avoids a potential attack where clients waste our\nmemory.\n\nNote that we do have to replace it with a flag, because some of the\nflush-packet logic checks whether we've seen any valid commands or keys.\n\nSigned-off-by: Jeff King \n---\n serve.c | 8 +++-----\n 1 file changed, 3 insertions(+), 5 deletions(-)\n\ndiff --git a/serve.c b/serve.c\nindex 6bbf54cbbe..1a7c8a118f 100644\n--- a/serve.c\n+++ b/serve.c\n@@ -239,7 +239,7 @@ static int process_request(void)\n {\n \tenum request_state state = PROCESS_REQUEST_KEYS;\n \tstruct packet_reader reader;\n-\tstruct strvec keys = STRVEC_INIT;\n+\tint seen_capability_or_command = 0;\n \tstruct protocol_capability *command = NULL;\n \n \tpacket_reader_init(&reader, 0, NULL, 0,\n@@ -260,10 +260,9 @@ static int process_request(void)\n \t\tcase PACKET_READ_EOF:\n \t\t\tBUG(\"Should have already died when seeing EOF\");\n \t\tcase PACKET_READ_NORMAL:\n-\t\t\t/* collect request; a sequence of keys and values */\n \t\t\tif (parse_command(reader.line, &command) ||\n \t\t\t receive_client_capability(reader.line))\n-\t\t\t\tstrvec_push(&keys, reader.line);\n+\t\t\t\tseen_capability_or_command = 1;\n \t\t\telse\n \t\t\t\tdie(\"unknown capability '%s'\", reader.line);\n \n@@ -275,7 +274,7 @@ static int process_request(void)\n \t\t\t * If no command and no keys were given then the client\n \t\t\t * wanted to terminate the connection.\n \t\t\t */\n-\t\t\tif (!keys.nr)\n+\t\t\tif (!seen_capability_or_command)\n \t\t\t\treturn 1;\n \n \t\t\t/*\n@@ -309,7 +308,6 @@ static int process_request(void)\n \n \tcommand->command(the_repository, &reader);\n \n-\tstrvec_clear(&keys);\n \treturn 0;\n }\n \n-- \n2.33.0.917.g33ebf6a5f6\n\n"},{"id":"436030","messageId":"YUI88/ok92tZ+JNo@coredump.intra.peff.net","threadId":"56500","inReplyTo":"YUI8z5SiyvgrDBas@coredump.intra.peff.net","subject":"[PATCH v3 07/11] ls-refs: ignore very long ref-prefix counts","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-15T18:35:31Z","receivedAt":"2021-09-15T18:35:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Because each \"ref-prefix\" capability from the client comes in its own\npkt-line, there's no limit to the number of them that a misbehaving\nclient may send. We read them all into a strvec, which means the client\ncan waste arbitrary amounts of our memory by just sending us \"ref-prefix\nfoo\" over and over.\n\nOne possible solution is to just drop the connection when the limit is\nreached. If we set it high enough, then only misbehaving or malicious\nclients would hit it. But \"high enough\" is vague, and it's unfriendly if\nwe guess wrong and a legitimate client hits this.\n\nBut we can do better. Since supporting the ref-prefix capability is\noptional anyway, the client has to further cull the response based on\ntheir own patterns. So we can simply ignore the patterns once we cross a\ncertain threshold. Note that we have to ignore _all_ patterns, not just\nthe ones past our limit (since otherwise we'd send too little data).\n\nThe limit here is fairly arbitrary, and probably much higher than anyone\nwould need in practice. It might be worth limiting it further, if only\nbecause we check it linearly (so with \"m\" local refs and \"n\" patterns,\nwe do \"m * n\" string comparisons). But if we care about optimizing this,\nan even better solution may be a more advanced data structure anyway.\n\nI didn't bother making the limit configurable, since it's so high and\nsince Git should behave correctly in either case. It wouldn't be too\nhard to do, but it makes both the code and documentation more complex.\n\nSigned-off-by: Jeff King \n---\n ls-refs.c | 20 ++++++++++++++++++--\n t/t5701-git-serve.sh | 31 +++++++++++++++++++++++++++++++\n 2 files changed, 49 insertions(+), 2 deletions(-)\n\ndiff --git a/ls-refs.c b/ls-refs.c\nindex a1a0250607..18c4f41e87 100644\n--- a/ls-refs.c\n+++ b/ls-refs.c\n@@ -40,6 +40,12 @@ static void ensure_config_read(void)\n \tconfig_read = 1;\n }\n \n+/*\n+ * If we see this many or more \"ref-prefix\" lines from the client, we consider\n+ * it \"too many\" and will avoid using the prefix feature entirely.\n+ */\n+#define TOO_MANY_PREFIXES 65536\n+\n /*\n * Check if one of the prefixes is a prefix of the ref.\n * If no prefixes were provided, all refs match.\n@@ -156,15 +162,25 @@ int ls_refs(struct repository *r, struct packet_reader *request)\n \t\t\tdata.peel = 1;\n \t\telse if (!strcmp(\"symrefs\", arg))\n \t\t\tdata.symrefs = 1;\n-\t\telse if (skip_prefix(arg, \"ref-prefix \", &out))\n-\t\t\tstrvec_push(&data.prefixes, out);\n+\t\telse if (skip_prefix(arg, \"ref-prefix \", &out)) {\n+\t\t\tif (data.prefixes.nr < TOO_MANY_PREFIXES)\n+\t\t\t\tstrvec_push(&data.prefixes, out);\n+\t\t}\n \t\telse if (!strcmp(\"unborn\", arg))\n \t\t\tdata.unborn = allow_unborn;\n \t}\n \n \tif (request->status != PACKET_READ_FLUSH)\n \t\tdie(_(\"expected flush after ls-refs arguments\"));\n \n+\t/*\n+\t * If we saw too many prefixes, we must avoid using them at all; as\n+\t * soon as we have any prefix, they are meant to form a comprehensive\n+\t * list.\n+\t */\n+\tif (data.prefixes.nr >= TOO_MANY_PREFIXES)\n+\t\tstrvec_clear(&data.prefixes);\n+\n \tsend_possibly_unborn_head(&data);\n \tif (!data.prefixes.nr)\n \t\tstrvec_push(&data.prefixes, \"\");\ndiff --git a/t/t5701-git-serve.sh b/t/t5701-git-serve.sh\nindex 930721f053..520672f842 100755\n--- a/t/t5701-git-serve.sh\n+++ b/t/t5701-git-serve.sh\n@@ -158,6 +158,37 @@ test_expect_success 'refs/heads prefix' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'ignore very large set of prefixes' '\n+\t# generate a large number of ref-prefixes that we expect\n+\t# to match nothing; the value here exceeds TOO_MANY_PREFIXES\n+\t# from ls-refs.c.\n+\t{\n+\t\techo command=ls-refs &&\n+\t\techo object-format=$(test_oid algo) &&\n+\t\techo 0001 &&\n+\t\tperl -le \"print \\\"ref-prefix refs/heads/\\$_\\\" for (1..65536)\" &&\n+\t\techo 0000\n+\t} |\n+\ttest-tool pkt-line pack >in &&\n+\n+\t# and then confirm that we see unmatched prefixes anyway (i.e.,\n+\t# that the prefix was not applied).\n+\tcat >expect <<-EOF &&\n+\t$(git rev-parse HEAD) HEAD\n+\t$(git rev-parse refs/heads/dev) refs/heads/dev\n+\t$(git rev-parse refs/heads/main) refs/heads/main\n+\t$(git rev-parse refs/heads/release) refs/heads/release\n+\t$(git rev-parse refs/tags/annotated-tag) refs/tags/annotated-tag\n+\t$(git rev-parse refs/tags/one) refs/tags/one\n+\t$(git rev-parse refs/tags/two) refs/tags/two\n+\t0000\n+\tEOF\n+\n+\ttest-tool serve-v2 --stateless-rpc out &&\n+\ttest-tool pkt-line unpack actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'peel parameter' '\n \ttest-tool pkt-line pack >in <<-EOF &&\n \tcommand=ls-refs\n-- \n2.33.0.917.g33ebf6a5f6\n\n"},{"id":"436031","messageId":"YUI89iI+KRVK+YAz@coredump.intra.peff.net","threadId":"56500","inReplyTo":"YUI8z5SiyvgrDBas@coredump.intra.peff.net","subject":"[PATCH v3 08/11] docs/protocol-v2: clarify some ls-refs ref-prefix details","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-15T18:35:34Z","receivedAt":"2021-09-15T18:35:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We've never documented the fact that a client can provide multiple\nref-prefix capabilities. Let's describe the behavior.\n\nWe also never discussed the \"best effort\" nature of the prefixes. The\nclient side of git.git has always treated them this way, filtering the\nresult with local patterns. And indeed any client must do this, because\nthe prefix patterns are not sufficient to express the usual refspecs\n(and so for \"foo\" we ask for \"refs/heads/foo\", \"refs/tags/foo\", and so\non).\n\nSo this may be considered a change in the spec with respect to client\nexpectations / requirements, but it's mostly codifying existing\nbehavior.\n\nSigned-off-by: Jeff King \n---\n Documentation/technical/protocol-v2.txt | 6 +++++-\n 1 file changed, 5 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/technical/protocol-v2.txt b/Documentation/technical/protocol-v2.txt\nindex 213538f1d0..9347b2ad13 100644\n--- a/Documentation/technical/protocol-v2.txt\n+++ b/Documentation/technical/protocol-v2.txt\n@@ -193,7 +193,11 @@ ls-refs takes in the following arguments:\n \tShow peeled tags.\n ref-prefix \n \tWhen specified, only references having a prefix matching one of\n-\tthe provided prefixes are displayed.\n+\tthe provided prefixes are displayed. Multiple instances may be\n+\tgiven, in which case references matching any prefix will be\n+\tshown. Note that this is purely for optimization; a server MAY\n+\tshow refs not matching the prefix if it chooses, and clients\n+\tshould filter the result themselves.\n \n If the 'unborn' feature is advertised the following argument can be\n included in the client's request.\n-- \n2.33.0.917.g33ebf6a5f6\n\n"},{"id":"436032","messageId":"YUI9MRXNAFxzVoA3@coredump.intra.peff.net","threadId":"56500","inReplyTo":"YUI8z5SiyvgrDBas@coredump.intra.peff.net","subject":"[PATCH v3 09/11] serve: reject bogus v2 \"command=ls-refs=foo\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-15T18:36:33Z","receivedAt":"2021-09-15T18:36:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we see a line from the client like \"command=ls-refs\", we parse\neverything after the equals sign as a capability, which we check against\nour capabilities table. If we don't recognize the command (e.g.,\n\"command=foo\"), we'll reject it.\n\nBut in parse_command(), we use the same get_capability() parser for\nparsing non-command lines. So if we see \"command=ls-refs=foo\", we will\nfeed \"ls-refs=foo\" to get_capability(), which will say \"OK, that's\nls-refs, with value 'foo'\". But then we simply ignore the value\nentirely.\n\nThe client is violating the spec here, which says:\n\n command = PKT-LINE(\"command=\" key LF)\n key = 1*(ALPHA | DIGIT | \"-_\")\n\nI.e., the key is not even allowed to have an equals sign in it. Whereas\na real non-command capability does allow a value:\n\n capability = PKT-LINE(key[=value] LF)\n\nSo by reusing the same get_capability() parser, we are mixing up the\n\"key\" and \"capability\" tokens. However, since that parser tells us\nwhether it saw an \"=\", we can still use it; we just need to reject any\ninput that produces a non-NULL value field.\n\nThe current behavior isn't really hurting anything (the client should\nnever send such a request, and if it does, we just ignore the \"value\"\npart). But since it does violate the spec, let's tighten it up to\nprevent any surprising behavior.\n\nSigned-off-by: Jeff King \n---\n serve.c | 2 +-\n t/t5701-git-serve.sh | 10 ++++++++++\n 2 files changed, 11 insertions(+), 1 deletion(-)\n\ndiff --git a/serve.c b/serve.c\nindex 1a7c8a118f..db5ecfed2d 100644\n--- a/serve.c\n+++ b/serve.c\n@@ -220,7 +220,7 @@ static int parse_command(const char *key, struct protocol_capability **command)\n \t\tif (*command)\n \t\t\tdie(\"command '%s' requested after already requesting command '%s'\",\n \t\t\t out, (*command)->name);\n-\t\tif (!cmd || !cmd->advertise(the_repository, NULL) || !cmd->command)\n+\t\tif (!cmd || !cmd->advertise(the_repository, NULL) || !cmd->command || value)\n \t\t\tdie(\"invalid command '%s'\", out);\n \n \t\t*command = cmd;\ndiff --git a/t/t5701-git-serve.sh b/t/t5701-git-serve.sh\nindex 520672f842..2e51886def 100755\n--- a/t/t5701-git-serve.sh\n+++ b/t/t5701-git-serve.sh\n@@ -72,6 +72,16 @@ test_expect_success 'request invalid command' '\n \ttest_i18ngrep \"invalid command\" err\n '\n \n+test_expect_success 'requested command is command=value' '\n+\ttest-tool pkt-line pack >in <<-EOF &&\n+\tcommand=ls-refs=whatever\n+\tobject-format=$(test_oid algo)\n+\t0000\n+\tEOF\n+\ttest_must_fail test-tool serve-v2 --stateless-rpc 2>err in <<-EOF &&\n \tcommand=fetch\n-- \n2.33.0.917.g33ebf6a5f6\n\n"},{"id":"436033","messageId":"YUI9NKMvGn/tsjW1@coredump.intra.peff.net","threadId":"56500","inReplyTo":"YUI8z5SiyvgrDBas@coredump.intra.peff.net","subject":"[PATCH v3 10/11] serve: reject commands used as capabilities","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-15T18:36:36Z","receivedAt":"2021-09-15T18:36:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Our table of v2 \"capabilities\" contains everything we might tell the\nclient we support. But there are differences in how we expect the client\nto respond. Some of the entries are true capabilities (i.e., we expect\nthe client to say \"yes, I support this\"), and some are ones we expect\nthem to send as commands (with \"command=ls-refs\" or similar).\n\nWhen we receive a capability used as a command, we complain about that.\nBut when we receive a command used as a capability (e.g., just \"ls-refs\"\nin a pkt-line by itself), we silently ignore it.\n\nThis isn't really hurting anything (clients shouldn't send it, and we'll\nignore it), but we can tighten up the protocol to match what we expect\nto happen.\n\nThere are two new tests here. The first one checks a capability used as\na command, which already passes. The second tests a command as a\ncapability, which this patch fixes.\n\nSigned-off-by: Jeff King \n---\n serve.c | 2 +-\n t/t5701-git-serve.sh | 21 +++++++++++++++++++++\n 2 files changed, 22 insertions(+), 1 deletion(-)\n\ndiff --git a/serve.c b/serve.c\nindex db5ecfed2d..b3fe9b5126 100644\n--- a/serve.c\n+++ b/serve.c\n@@ -201,7 +201,7 @@ static int receive_client_capability(const char *key)\n \tconst char *value;\n \tconst struct protocol_capability *c = get_capability(key, &value);\n \n-\tif (!c || !c->advertise(the_repository, NULL))\n+\tif (!c || c->command || !c->advertise(the_repository, NULL))\n \t\treturn 0;\n \n \tif (c->receive)\ndiff --git a/t/t5701-git-serve.sh b/t/t5701-git-serve.sh\nindex 2e51886def..3928424e1b 100755\n--- a/t/t5701-git-serve.sh\n+++ b/t/t5701-git-serve.sh\n@@ -72,6 +72,27 @@ test_expect_success 'request invalid command' '\n \ttest_i18ngrep \"invalid command\" err\n '\n \n+test_expect_success 'request capability as command' '\n+\ttest-tool pkt-line pack >in <<-EOF &&\n+\tcommand=agent\n+\tobject-format=$(test_oid algo)\n+\t0000\n+\tEOF\n+\ttest_must_fail test-tool serve-v2 --stateless-rpc 2>err in <<-EOF &&\n+\tcommand=ls-refs\n+\tobject-format=$(test_oid algo)\n+\tfetch\n+\t0000\n+\tEOF\n+\ttest_must_fail test-tool serve-v2 --stateless-rpc 2>err in <<-EOF &&\n \tcommand=ls-refs=whatever\n-- \n2.33.0.917.g33ebf6a5f6\n\n"},{"id":"436034","messageId":"YUI9NmyHeCNw0+zz@coredump.intra.peff.net","threadId":"56500","inReplyTo":"YUI8z5SiyvgrDBas@coredump.intra.peff.net","subject":"[PATCH v3 11/11] ls-refs: reject unknown arguments","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-15T18:36:38Z","receivedAt":"2021-09-15T18:36:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The v2 ls-refs command may receive extra arguments from the client, one\nper pkt-line. The spec is pretty clear that the arguments must come from\na specified set, but we silently ignore any unknown entries. For a\nwell-behaved client this doesn't matter, but it makes testing and\ndebugging more confusing. Let's tighten this up to match the spec.\n\nIn theory this liberal behavior _could_ be useful for extending the\nprotocol. But:\n\n - every other part of the protocol requires that the server first\n indicate that it supports the argument; this includes the fetch and\n object-info commands, plus the \"unborn\" capability added to ls-refs\n itself\n\n - it's not a very good extension mechanism anyway; without the server\n advertising support, clients would have no idea if the argument was\n silently ignored, or accepted and simply had no effect\n\nSo we're not really losing anything by tightening this.\n\nSigned-off-by: Jeff King \n---\n ls-refs.c | 2 ++\n t/t5701-git-serve.sh | 13 +++++++++++++\n 2 files changed, 15 insertions(+)\n\ndiff --git a/ls-refs.c b/ls-refs.c\nindex 18c4f41e87..460ac9b229 100644\n--- a/ls-refs.c\n+++ b/ls-refs.c\n@@ -168,6 +168,8 @@ int ls_refs(struct repository *r, struct packet_reader *request)\n \t\t}\n \t\telse if (!strcmp(\"unborn\", arg))\n \t\t\tdata.unborn = allow_unborn;\n+\t\telse\n+\t\t\tdie(_(\"unexpected line: '%s'\"), arg);\n \t}\n \n \tif (request->status != PACKET_READ_FLUSH)\ndiff --git a/t/t5701-git-serve.sh b/t/t5701-git-serve.sh\nindex 3928424e1b..aa1827d841 100755\n--- a/t/t5701-git-serve.sh\n+++ b/t/t5701-git-serve.sh\n@@ -147,6 +147,19 @@ test_expect_success 'basics of ls-refs' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'ls-refs complains about unknown options' '\n+\ttest-tool pkt-line pack >in <<-EOF &&\n+\tcommand=ls-refs\n+\tobject-format=$(test_oid algo)\n+\t0001\n+\tno-such-arg\n+\t0000\n+\tEOF\n+\n+\ttest_must_fail test-tool serve-v2 --stateless-rpc 2>err in <<-EOF &&\n \tcommand=ls-refs\n-- \n2.33.0.917.g33ebf6a5f6\n"}]}