{"thread":{"id":"61025","subject":"[PATCH 0/9] bound upload-pack memory allocations","startedAt":"2024-02-28T22:37:09Z","lastAt":"2024-04-29T20:49:57Z","messageCount":17,"participants":["Jeff King","Junio C Hamano","Patrick Steinhardt","Taylor Blau"],"isPatch":true,"patchVersion":1,"patchTotal":9},"messages":[{"id":"489626","messageId":"20240228223700.GA1157826@coredump.intra.peff.net","threadId":"61025","inReplyTo":null,"subject":"[PATCH 0/9] bound upload-pack memory allocations","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-02-28T22:37:00Z","receivedAt":"2024-02-28T22:37:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Benjamin Flesch reported to the security list that a client can convince\nupload-pack to allocate arbitrary amounts of memory by sending a few\nrepeated nonsense directives. Patches 1-6 here eliminate those spots.\nwhich roughly bounds the amount of memory that upload-pack will allocate\nto scale with the number of refs and objects in the repository. I also\nfound a few spots where upload-pack is a little too eager to hold object\ncontents in memory (so still bounded, but something like \"all the trees\"\ncan get pretty big). That's patches 7-9.\n\nWe're not doing a coordinated disclosure or special release for this.\nEven after these patches, it's possible to get upload-pack to allocate\nquite a bit of memory, especially for a large repository. Not to mention\nthat pack-objects may also allocate quite a bit to serve the pack\nitself. So while this is low-hanging fruit, a public-facing Git site\nprobably still needs to have some kind of external tooling to kill\nhungry processes (even if it's just OOM-killing them so they don't hurt\nother clients).\n\nThere are a few unbounded parts of receive-pack (e.g., you can send an\ninfinite number of refs to push). That may be something we want to put\nsome configurable boundaries on, but I punted on it for this series.\nIMHO it is a lot less interesting since you'd usually have to\nauthenticate to invoke receive-pack in the first place.\n\n [1/9]: upload-pack: drop separate v2 \"haves\" array\n [2/9]: upload-pack: switch deepen-not list to an oid_array\n [3/9]: upload-pack: use oidset for deepen_not list\n [4/9]: upload-pack: use a strmap for want-ref lines\n [5/9]: upload-pack: accept only a single packfile-uri line\n [6/9]: upload-pack: disallow object-info capability by default\n [7/9]: upload-pack: always turn off save_commit_buffer\n [8/9]: upload-pack: use PARSE_OBJECT_SKIP_HASH_CHECK in more places\n [9/9]: upload-pack: free tree buffers after parsing\n\n Documentation/config/transfer.txt | 4 +\n Documentation/gitprotocol-v2.txt | 6 +-\n builtin/upload-pack.c | 2 +\n object.c | 14 ++++\n object.h | 1 +\n revision.c | 3 +-\n serve.c | 14 +++-\n t/t5555-http-smart-common.sh | 1 -\n t/t5701-git-serve.sh | 24 +++++-\n upload-pack.c | 117 +++++++++++++-----------------\n 10 files changed, 113 insertions(+), 73 deletions(-)\n\n-Peff\n"},{"id":"489627","messageId":"20240228223713.GA1158131@coredump.intra.peff.net","threadId":"61025","inReplyTo":"20240228223700.GA1157826@coredump.intra.peff.net","subject":"[PATCH 1/9] upload-pack: drop separate v2 \"haves\" array","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-02-28T22:37:13Z","receivedAt":"2024-02-28T22:37:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When upload-pack sees a \"have\" line in the v0 protocol, it immediately\ncalls got_oid() with its argument and potentially produces an ACK\nresponse. In the v2 protocol, we simply record the argument in an\noid_array, and only later process all of the \"have\" objects by calling\nthe equivalent of got_oid() on the contents of the array.\n\nThis makes some sense, as v2 is a pure request/response protocol, as\nopposed to v0's asynchronous negotiation phase. But there's a downside:\na client can send us an infinite number of garbage \"have\" lines, which\nwe'll happily slurp into the array, consuming memory. Whereas in v0,\nthey are limited by the number of objects in the repository (because\ngot_oid() only records objects we have ourselves, and we avoid\nduplicates by setting a flag on the object struct).\n\nWe can make v2 behave more like v0 by also calling got_oid() directly\nwhen v2 parses a \"have\" line. Calling it early like this is OK because\ngot_oid() itself does not interact with the client; it only confirms\nthat we have the object and sets a few flags. Note that unlike v0, v2\ndoes not ever (before or after this patch) check the return code of\ngot_oid(), which lets the caller know whether we have the object. But\nagain, that makes sense; v0 is using it to asynchronously tell the\nclient to stop sending. In v2's synchronous protocol, we just discard\nthose entries (and decide how to ACK at the end of each round).\n\nThere is one slight tweak we need, though. In v2's state machine, we\nreach the SEND_ACKS state if the other side sent us any \"have\" lines,\nwhether they were useful or not. Right now we do that by checking\nwhether the \"have\" array had any entries, but if we record only the\nuseful ones, that doesn't work. Instead, we can add a simple boolean\nthat tells us whether we saw any have line (even if it was useless).\n\nThis lets us drop the \"haves\" array entirely, as we're now placing\nobjects directly into the \"have_obj\" object array (which is where\ngot_oid() put them in the long run anyway). And as a bonus, we can drop\nthe secondary \"common\" array used in process_haves_and_send_acks(). It\nwas essentially a copy of \"haves\" minus the objects we do not have. But\nnow that we are using \"have_obj\" directly, we know everything in it is\nuseful. So in addition to protecting ourselves against malicious input,\nwe should slightly lower our memory usage for normal inputs.\n\nNote that there is one user-visible effect. The trace2 output records\nthe number of \"haves\". Previously this was the total number of \"have\"\nlines we saw, but now is the number of useful ones. We could retain the\noriginal meaning by keeping a separate counter, but it doesn't seem\nworth the effort; this trace info is for debugging and metrics, and\narguably the count of common oids is at least as useful as the total\ncount.\n\nReported-by: Benjamin Flesch \nSigned-off-by: Jeff King \n---\n upload-pack.c | 48 ++++++++++--------------------------------------\n 1 file changed, 10 insertions(+), 38 deletions(-)\n\ndiff --git a/upload-pack.c b/upload-pack.c\nindex 2537affa90..7a83127121 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -61,7 +61,6 @@ struct upload_pack_data {\n \tstruct string_list symref;\t\t\t\t/* v0 only */\n \tstruct object_array want_obj;\n \tstruct object_array have_obj;\n-\tstruct oid_array haves;\t\t\t\t\t/* v2 only */\n \tstruct string_list wanted_refs;\t\t\t\t/* v2 only */\n \tstruct strvec hidden_refs;\n \n@@ -113,6 +112,7 @@ struct upload_pack_data {\n \tunsigned done : 1;\t\t\t\t\t/* v2 only */\n \tunsigned allow_ref_in_want : 1;\t\t\t\t/* v2 only */\n \tunsigned allow_sideband_all : 1;\t\t\t/* v2 only */\n+\tunsigned seen_haves : 1;\t\t\t\t/* v2 only */\n \tunsigned advertise_sid : 1;\n \tunsigned sent_capabilities : 1;\n };\n@@ -124,7 +124,6 @@ static void upload_pack_data_init(struct upload_pack_data *data)\n \tstruct strvec hidden_refs = STRVEC_INIT;\n \tstruct object_array want_obj = OBJECT_ARRAY_INIT;\n \tstruct object_array have_obj = OBJECT_ARRAY_INIT;\n-\tstruct oid_array haves = OID_ARRAY_INIT;\n \tstruct object_array shallows = OBJECT_ARRAY_INIT;\n \tstruct string_list deepen_not = STRING_LIST_INIT_DUP;\n \tstruct string_list uri_protocols = STRING_LIST_INIT_DUP;\n@@ -137,7 +136,6 @@ static void upload_pack_data_init(struct upload_pack_data *data)\n \tdata->hidden_refs = hidden_refs;\n \tdata->want_obj = want_obj;\n \tdata->have_obj = have_obj;\n-\tdata->haves = haves;\n \tdata->shallows = shallows;\n \tdata->deepen_not = deepen_not;\n \tdata->uri_protocols = uri_protocols;\n@@ -159,7 +157,6 @@ static void upload_pack_data_clear(struct upload_pack_data *data)\n \tstrvec_clear(&data->hidden_refs);\n \tobject_array_clear(&data->want_obj);\n \tobject_array_clear(&data->have_obj);\n-\toid_array_clear(&data->haves);\n \tobject_array_clear(&data->shallows);\n \tstring_list_clear(&data->deepen_not, 0);\n \tobject_array_clear(&data->extra_edge_obj);\n@@ -1532,15 +1529,14 @@ static int parse_want_ref(struct packet_writer *writer, const char *line,\n \treturn 0;\n }\n \n-static int parse_have(const char *line, struct oid_array *haves)\n+static int parse_have(const char *line, struct upload_pack_data *data)\n {\n \tconst char *arg;\n \tif (skip_prefix(line, \"have \", &arg)) {\n \t\tstruct object_id oid;\n \n-\t\tif (get_oid_hex(arg, &oid))\n-\t\t\tdie(\"git upload-pack: expected SHA1 object, got '%s'\", arg);\n-\t\toid_array_append(haves, &oid);\n+\t\tgot_oid(data, arg, &oid);\n+\t\tdata->seen_haves = 1;\n \t\treturn 1;\n \t}\n \n@@ -1552,7 +1548,7 @@ static void trace2_fetch_info(struct upload_pack_data *data)\n \tstruct json_writer jw = JSON_WRITER_INIT;\n \n \tjw_object_begin(&jw, 0);\n-\tjw_object_intmax(&jw, \"haves\", data->haves.nr);\n+\tjw_object_intmax(&jw, \"haves\", data->have_obj.nr);\n \tjw_object_intmax(&jw, \"wants\", data->want_obj.nr);\n \tjw_object_intmax(&jw, \"want-refs\", data->wanted_refs.nr);\n \tjw_object_intmax(&jw, \"depth\", data->depth);\n@@ -1586,7 +1582,7 @@ static void process_args(struct packet_reader *request,\n \t\t\t\t &data->hidden_refs, &data->want_obj))\n \t\t\tcontinue;\n \t\t/* process have line */\n-\t\tif (parse_have(arg, &data->haves))\n+\t\tif (parse_have(arg, data))\n \t\t\tcontinue;\n \n \t\t/* process args like thin-pack */\n@@ -1664,27 +1660,7 @@ static void process_args(struct packet_reader *request,\n \t\ttrace2_fetch_info(data);\n }\n \n-static int process_haves(struct upload_pack_data *data, struct oid_array *common)\n-{\n-\tint i;\n-\n-\t/* Process haves */\n-\tfor (i = 0; i < data->haves.nr; i++) {\n-\t\tconst struct object_id *oid = &data->haves.oid[i];\n-\n-\t\tif (!repo_has_object_file_with_flags(the_repository, oid,\n-\t\t\t\t\t\t OBJECT_INFO_QUICK | OBJECT_INFO_SKIP_FETCH_OBJECT))\n-\t\t\tcontinue;\n-\n-\t\toid_array_append(common, oid);\n-\n-\t\tdo_got_oid(data, oid);\n-\t}\n-\n-\treturn 0;\n-}\n-\n-static int send_acks(struct upload_pack_data *data, struct oid_array *acks)\n+static int send_acks(struct upload_pack_data *data, struct object_array *acks)\n {\n \tint i;\n \n@@ -1696,7 +1672,7 @@ static int send_acks(struct upload_pack_data *data, struct oid_array *acks)\n \n \tfor (i = 0; i < acks->nr; i++) {\n \t\tpacket_writer_write(&data->writer, \"ACK %s\\n\",\n-\t\t\t\t oid_to_hex(&acks->oid[i]));\n+\t\t\t\t oid_to_hex(&acks->objects[i].item->oid));\n \t}\n \n \tif (!data->wait_for_done && ok_to_give_up(data)) {\n@@ -1710,13 +1686,11 @@ static int send_acks(struct upload_pack_data *data, struct oid_array *acks)\n \n static int process_haves_and_send_acks(struct upload_pack_data *data)\n {\n-\tstruct oid_array common = OID_ARRAY_INIT;\n \tint ret = 0;\n \n-\tprocess_haves(data, &common);\n \tif (data->done) {\n \t\tret = 1;\n-\t} else if (send_acks(data, &common)) {\n+\t} else if (send_acks(data, &data->have_obj)) {\n \t\tpacket_writer_delim(&data->writer);\n \t\tret = 1;\n \t} else {\n@@ -1725,8 +1699,6 @@ static int process_haves_and_send_acks(struct upload_pack_data *data)\n \t\tret = 0;\n \t}\n \n-\toid_array_clear(&data->haves);\n-\toid_array_clear(&common);\n \treturn ret;\n }\n \n@@ -1796,7 +1768,7 @@ int upload_pack_v2(struct repository *r UNUSED, struct packet_reader *request)\n \t\t\t\t * they didn't want anything.\n \t\t\t\t */\n \t\t\t\tstate = FETCH_DONE;\n-\t\t\t} else if (data.haves.nr) {\n+\t\t\t} else if (data.seen_haves) {\n \t\t\t\t/*\n \t\t\t\t * Request had 'have' lines, so lets ACK them.\n \t\t\t\t */\n-- \n2.44.0.rc2.424.gbdbf4d014b\n\n"},{"id":"489628","messageId":"20240228223720.GB1158131@coredump.intra.peff.net","threadId":"61025","inReplyTo":"20240228223700.GA1157826@coredump.intra.peff.net","subject":"[PATCH 2/9] upload-pack: switch deepen-not list to an oid_array","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-02-28T22:37:20Z","receivedAt":"2024-02-28T22:37:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we see a \"deepen-not\" line from the client, we verify that the\ngiven name can be resolved as a ref, and then add it to a string list to\nbe passed later to an internal \"rev-list --not\" traversal. We record the\nactual refname in the string list (so the traversal resolves it again\nlater), but we'd be better off recording the resolved oid:\n\n 1. There's a tiny bit of wasted work in resolving it twice.\n\n 2. There's a small race condition with simultaneous updates; the later\n traversal may resolve to a different value (or not at all). This\n shouldn't cause any bad behavior (we do not care about the value\n in this first resolution, so whatever value rev-list gets is OK)\n but it could mean a confusing error message (if upload-pack fails\n to resolve the ref it produces a useful message, but a failing\n traversal later results in just \"revision walk setup failed\").\n\n 3. It makes it simpler to de-duplicate the results. We don't de-dup at\n all right now, but we will in the next patch.\n\nFrom the client's perspective the behavior should be the same.\n\nSigned-off-by: Jeff King \n---\n upload-pack.c | 14 +++++++-------\n 1 file changed, 7 insertions(+), 7 deletions(-)\n\ndiff --git a/upload-pack.c b/upload-pack.c\nindex 7a83127121..f6395b210e 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -65,7 +65,7 @@ struct upload_pack_data {\n \tstruct strvec hidden_refs;\n \n \tstruct object_array shallows;\n-\tstruct string_list deepen_not;\n+\tstruct oid_array deepen_not;\n \tstruct object_array extra_edge_obj;\n \tint depth;\n \ttimestamp_t deepen_since;\n@@ -125,7 +125,7 @@ static void upload_pack_data_init(struct upload_pack_data *data)\n \tstruct object_array want_obj = OBJECT_ARRAY_INIT;\n \tstruct object_array have_obj = OBJECT_ARRAY_INIT;\n \tstruct object_array shallows = OBJECT_ARRAY_INIT;\n-\tstruct string_list deepen_not = STRING_LIST_INIT_DUP;\n+\tstruct oid_array deepen_not = OID_ARRAY_INIT;\n \tstruct string_list uri_protocols = STRING_LIST_INIT_DUP;\n \tstruct object_array extra_edge_obj = OBJECT_ARRAY_INIT;\n \tstruct string_list allowed_filters = STRING_LIST_INIT_DUP;\n@@ -158,7 +158,7 @@ static void upload_pack_data_clear(struct upload_pack_data *data)\n \tobject_array_clear(&data->want_obj);\n \tobject_array_clear(&data->have_obj);\n \tobject_array_clear(&data->shallows);\n-\tstring_list_clear(&data->deepen_not, 0);\n+\toid_array_clear(&data->deepen_not);\n \tobject_array_clear(&data->extra_edge_obj);\n \tlist_objects_filter_release(&data->filter_options);\n \tstring_list_clear(&data->allowed_filters, 0);\n@@ -926,8 +926,8 @@ static int send_shallow_list(struct upload_pack_data *data)\n \t\tif (data->deepen_not.nr) {\n \t\t\tstrvec_push(&av, \"--not\");\n \t\t\tfor (i = 0; i < data->deepen_not.nr; i++) {\n-\t\t\t\tstruct string_list_item *s = data->deepen_not.items + i;\n-\t\t\t\tstrvec_push(&av, s->string);\n+\t\t\t\tstruct object_id *oid = data->deepen_not.oid + i;\n+\t\t\t\tstrvec_push(&av, oid_to_hex(oid));\n \t\t\t}\n \t\t\tstrvec_push(&av, \"--not\");\n \t\t}\n@@ -1004,15 +1004,15 @@ static int process_deepen_since(const char *line, timestamp_t *deepen_since, int\n \treturn 0;\n }\n \n-static int process_deepen_not(const char *line, struct string_list *deepen_not, int *deepen_rev_list)\n+static int process_deepen_not(const char *line, struct oid_array *deepen_not, int *deepen_rev_list)\n {\n \tconst char *arg;\n \tif (skip_prefix(line, \"deepen-not \", &arg)) {\n \t\tchar *ref = NULL;\n \t\tstruct object_id oid;\n \t\tif (expand_ref(the_repository, arg, strlen(arg), &oid, &ref) != 1)\n \t\t\tdie(\"git upload-pack: ambiguous deepen-not: %s\", line);\n-\t\tstring_list_append(deepen_not, ref);\n+\t\toid_array_append(deepen_not, &oid);\n \t\tfree(ref);\n \t\t*deepen_rev_list = 1;\n \t\treturn 1;\n-- \n2.44.0.rc2.424.gbdbf4d014b\n\n"},{"id":"489629","messageId":"20240228223744.GC1158131@coredump.intra.peff.net","threadId":"61025","inReplyTo":"20240228223700.GA1157826@coredump.intra.peff.net","subject":"[PATCH 3/9] upload-pack: use oidset for deepen_not list","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-02-28T22:37:44Z","receivedAt":"2024-02-28T22:37:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We record the oid of every deepen-not line the client sends to us. For a\nwell-behaved client, the resulting array should be bounded by the number\nof unique refs we have. But because there's no de-duplication, a\nmalicious client can cause the array to grow unbounded by just sending\nthe same \"refs/heads/foo\" over and over (assuming such a ref exists).\n\nSince the deepen-not list is just being fed to a \"rev-list --not\"\ntraversal, the order of items doesn't matter. So we can replace the\noid_array with an oidset which notices and skips duplicates.\n\nThat bounds the memory in malicious cases to be linear in the number of\nunique refs. And even in non-malicious cases, there may be a slight\nimprovement in memory usage if multiple refs point to the same oid\n(though in practice this list is probably pretty tiny anyway, as it\ncomes from the user specifying \"--shallow-exclude\" on the client fetch).\n\nNote that in the trace2 output we'll now output the number of\nde-duplicated objects, rather than the total number of \"deepen-not\"\nlines we received. This is arguably a more useful value for tracing /\ndebugging anyway.\n\nReported-by: Benjamin Flesch \nSigned-off-by: Jeff King \n---\n upload-pack.c | 21 +++++++++++----------\n 1 file changed, 11 insertions(+), 10 deletions(-)\n\ndiff --git a/upload-pack.c b/upload-pack.c\nindex f6395b210e..ebad9026d7 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -65,7 +65,7 @@ struct upload_pack_data {\n \tstruct strvec hidden_refs;\n \n \tstruct object_array shallows;\n-\tstruct oid_array deepen_not;\n+\tstruct oidset deepen_not;\n \tstruct object_array extra_edge_obj;\n \tint depth;\n \ttimestamp_t deepen_since;\n@@ -125,7 +125,7 @@ static void upload_pack_data_init(struct upload_pack_data *data)\n \tstruct object_array want_obj = OBJECT_ARRAY_INIT;\n \tstruct object_array have_obj = OBJECT_ARRAY_INIT;\n \tstruct object_array shallows = OBJECT_ARRAY_INIT;\n-\tstruct oid_array deepen_not = OID_ARRAY_INIT;\n+\tstruct oidset deepen_not = OID_ARRAY_INIT;\n \tstruct string_list uri_protocols = STRING_LIST_INIT_DUP;\n \tstruct object_array extra_edge_obj = OBJECT_ARRAY_INIT;\n \tstruct string_list allowed_filters = STRING_LIST_INIT_DUP;\n@@ -158,7 +158,7 @@ static void upload_pack_data_clear(struct upload_pack_data *data)\n \tobject_array_clear(&data->want_obj);\n \tobject_array_clear(&data->have_obj);\n \tobject_array_clear(&data->shallows);\n-\toid_array_clear(&data->deepen_not);\n+\toidset_clear(&data->deepen_not);\n \tobject_array_clear(&data->extra_edge_obj);\n \tlist_objects_filter_release(&data->filter_options);\n \tstring_list_clear(&data->allowed_filters, 0);\n@@ -923,12 +923,13 @@ static int send_shallow_list(struct upload_pack_data *data)\n \t\tstrvec_push(&av, \"rev-list\");\n \t\tif (data->deepen_since)\n \t\t\tstrvec_pushf(&av, \"--max-age=%\"PRItime, data->deepen_since);\n-\t\tif (data->deepen_not.nr) {\n+\t\tif (oidset_size(&data->deepen_not)) {\n+\t\t\tconst struct object_id *oid;\n+\t\t\tstruct oidset_iter iter;\n \t\t\tstrvec_push(&av, \"--not\");\n-\t\t\tfor (i = 0; i < data->deepen_not.nr; i++) {\n-\t\t\t\tstruct object_id *oid = data->deepen_not.oid + i;\n+\t\t\toidset_iter_init(&data->deepen_not, &iter);\n+\t\t\twhile ((oid = oidset_iter_next(&iter)))\n \t\t\t\tstrvec_push(&av, oid_to_hex(oid));\n-\t\t\t}\n \t\t\tstrvec_push(&av, \"--not\");\n \t\t}\n \t\tfor (i = 0; i < data->want_obj.nr; i++) {\n@@ -1004,15 +1005,15 @@ static int process_deepen_since(const char *line, timestamp_t *deepen_since, int\n \treturn 0;\n }\n \n-static int process_deepen_not(const char *line, struct oid_array *deepen_not, int *deepen_rev_list)\n+static int process_deepen_not(const char *line, struct oidset *deepen_not, int *deepen_rev_list)\n {\n \tconst char *arg;\n \tif (skip_prefix(line, \"deepen-not \", &arg)) {\n \t\tchar *ref = NULL;\n \t\tstruct object_id oid;\n \t\tif (expand_ref(the_repository, arg, strlen(arg), &oid, &ref) != 1)\n \t\t\tdie(\"git upload-pack: ambiguous deepen-not: %s\", line);\n-\t\toid_array_append(deepen_not, &oid);\n+\t\toidset_insert(deepen_not, &oid);\n \t\tfree(ref);\n \t\t*deepen_rev_list = 1;\n \t\treturn 1;\n@@ -1554,7 +1555,7 @@ static void trace2_fetch_info(struct upload_pack_data *data)\n \tjw_object_intmax(&jw, \"depth\", data->depth);\n \tjw_object_intmax(&jw, \"shallows\", data->shallows.nr);\n \tjw_object_bool(&jw, \"deepen-since\", data->deepen_since);\n-\tjw_object_intmax(&jw, \"deepen-not\", data->deepen_not.nr);\n+\tjw_object_intmax(&jw, \"deepen-not\", oidset_size(&data->deepen_not));\n \tjw_object_bool(&jw, \"deepen-relative\", data->deepen_relative);\n \tif (data->filter_options.choice)\n \t\tjw_object_string(&jw, \"filter\", list_object_filter_config_name(data->filter_options.choice));\n-- \n2.44.0.rc2.424.gbdbf4d014b\n\n"},{"id":"489630","messageId":"20240228223840.GD1158131@coredump.intra.peff.net","threadId":"61025","inReplyTo":"20240228223700.GA1157826@coredump.intra.peff.net","subject":"[PATCH 4/9] upload-pack: use a strmap for want-ref lines","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-02-28T22:38:40Z","receivedAt":"2024-02-28T22:38:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When the \"ref-in-want\" capability is advertised (which it is not by\ndefault), then upload-pack processes a \"want-ref\" line from the client\nby checking that the name is a valid ref and recording it in a\nstring-list.\n\nIn theory this list should grow no larger than the number of refs in the\nserver-side repository. But since we don't do any de-duplication, a\nclient which sends \"want-ref refs/heads/foo\" over and over will cause\nthe array to grow without bound.\n\nWe can fix this by switching to strmap, which efficiently detects\nduplicates. There are two client-visible changes here:\n\n 1. The \"wanted-refs\" response will now be in an apparently-random\n order (based on iterating the hashmap) rather than the order given\n by the client. The protocol documentation is quiet on ordering\n here. The current fetch-pack implementation is happy with any\n order, as it looks up each returned ref using a binary search in\n its local sorted list. JGit seems to implement want-ref on the\n server side, but has no client-side support. libgit2 doesn't\n support either side.\n\n It would obviously be possible to record the original order or to\n use the strmap as an auxiliary data structure. But if the client\n doesn't care, we may as well do the simplest thing.\n\n 2. We'll now reject duplicates explicitly as a protocol error. The\n client should never send them (and our current implementation, even\n when asked to \"git fetch master:one master:two\" will de-dup on the\n client side).\n\n If we wanted to be more forgiving, we could perhaps just throw away\n the duplicates. But then our \"wanted-refs\" response back to the\n client would omit the duplicates, and it's hard to say what a\n client that accidentally sent a duplicate would do with that. So I\n think we're better off to complain loudly before anybody\n accidentally writes such a client.\n\nLet's also add a note to the protocol documentation clarifying that\nduplicates are forbidden. As discussed above, this was already the\nintent, but it's not very explicit.\n\nSigned-off-by: Jeff King \n---\n Documentation/gitprotocol-v2.txt | 3 ++-\n upload-pack.c | 30 +++++++++++++++++-------------\n 2 files changed, 19 insertions(+), 14 deletions(-)\n\ndiff --git a/Documentation/gitprotocol-v2.txt b/Documentation/gitprotocol-v2.txt\nindex 0b800abd56..837ea6171e 100644\n--- a/Documentation/gitprotocol-v2.txt\n+++ b/Documentation/gitprotocol-v2.txt\n@@ -346,7 +346,8 @@ the 'wanted-refs' section in the server's response as explained below.\n want-ref \n \tIndicates to the server that the client wants to retrieve a\n \tparticular ref, where is the full name of a ref on the\n-\tserver.\n+\tserver. It is a protocol error to send want-ref for the\n+\tsame ref more than once.\n \n If the 'sideband-all' feature is advertised, the following argument can be\n included in the client's request:\ndiff --git a/upload-pack.c b/upload-pack.c\nindex ebad9026d7..8b47576ec7 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -28,6 +28,7 @@\n #include \"shallow.h\"\n #include \"write-or-die.h\"\n #include \"json-writer.h\"\n+#include \"strmap.h\"\n \n /* Remember to update object flag allocation in object.h */\n #define THEY_HAVE\t(1u << 11)\n@@ -61,7 +62,7 @@ struct upload_pack_data {\n \tstruct string_list symref;\t\t\t\t/* v0 only */\n \tstruct object_array want_obj;\n \tstruct object_array have_obj;\n-\tstruct string_list wanted_refs;\t\t\t\t/* v2 only */\n+\tstruct strmap wanted_refs;\t\t\t\t/* v2 only */\n \tstruct strvec hidden_refs;\n \n \tstruct object_array shallows;\n@@ -120,7 +121,7 @@ struct upload_pack_data {\n static void upload_pack_data_init(struct upload_pack_data *data)\n {\n \tstruct string_list symref = STRING_LIST_INIT_DUP;\n-\tstruct string_list wanted_refs = STRING_LIST_INIT_DUP;\n+\tstruct strmap wanted_refs = STRMAP_INIT;\n \tstruct strvec hidden_refs = STRVEC_INIT;\n \tstruct object_array want_obj = OBJECT_ARRAY_INIT;\n \tstruct object_array have_obj = OBJECT_ARRAY_INIT;\n@@ -153,7 +154,7 @@ static void upload_pack_data_init(struct upload_pack_data *data)\n static void upload_pack_data_clear(struct upload_pack_data *data)\n {\n \tstring_list_clear(&data->symref, 1);\n-\tstring_list_clear(&data->wanted_refs, 1);\n+\tstrmap_clear(&data->wanted_refs, 1);\n \tstrvec_clear(&data->hidden_refs);\n \tobject_array_clear(&data->want_obj);\n \tobject_array_clear(&data->have_obj);\n@@ -1488,14 +1489,13 @@ static int parse_want(struct packet_writer *writer, const char *line,\n }\n \n static int parse_want_ref(struct packet_writer *writer, const char *line,\n-\t\t\t struct string_list *wanted_refs,\n+\t\t\t struct strmap *wanted_refs,\n \t\t\t struct strvec *hidden_refs,\n \t\t\t struct object_array *want_obj)\n {\n \tconst char *refname_nons;\n \tif (skip_prefix(line, \"want-ref \", &refname_nons)) {\n \t\tstruct object_id oid;\n-\t\tstruct string_list_item *item;\n \t\tstruct object *o = NULL;\n \t\tstruct strbuf refname = STRBUF_INIT;\n \n@@ -1507,8 +1507,11 @@ static int parse_want_ref(struct packet_writer *writer, const char *line,\n \t\t}\n \t\tstrbuf_release(&refname);\n \n-\t\titem = string_list_append(wanted_refs, refname_nons);\n-\t\titem->util = oiddup(&oid);\n+\t\tif (strmap_put(wanted_refs, refname_nons, oiddup(&oid))) {\n+\t\t\tpacket_writer_error(writer, \"duplicate want-ref %s\",\n+\t\t\t\t\t refname_nons);\n+\t\t\tdie(\"duplicate want-ref %s\", refname_nons);\n+\t\t}\n \n \t\tif (!starts_with(refname_nons, \"refs/tags/\")) {\n \t\t\tstruct commit *commit = lookup_commit_in_graph(the_repository, &oid);\n@@ -1551,7 +1554,7 @@ static void trace2_fetch_info(struct upload_pack_data *data)\n \tjw_object_begin(&jw, 0);\n \tjw_object_intmax(&jw, \"haves\", data->have_obj.nr);\n \tjw_object_intmax(&jw, \"wants\", data->want_obj.nr);\n-\tjw_object_intmax(&jw, \"want-refs\", data->wanted_refs.nr);\n+\tjw_object_intmax(&jw, \"want-refs\", strmap_get_size(&data->wanted_refs));\n \tjw_object_intmax(&jw, \"depth\", data->depth);\n \tjw_object_intmax(&jw, \"shallows\", data->shallows.nr);\n \tjw_object_bool(&jw, \"deepen-since\", data->deepen_since);\n@@ -1705,17 +1708,18 @@ static int process_haves_and_send_acks(struct upload_pack_data *data)\n \n static void send_wanted_ref_info(struct upload_pack_data *data)\n {\n-\tconst struct string_list_item *item;\n+\tstruct hashmap_iter iter;\n+\tconst struct strmap_entry *e;\n \n-\tif (!data->wanted_refs.nr)\n+\tif (strmap_empty(&data->wanted_refs))\n \t\treturn;\n \n \tpacket_writer_write(&data->writer, \"wanted-refs\\n\");\n \n-\tfor_each_string_list_item(item, &data->wanted_refs) {\n+\tstrmap_for_each_entry(&data->wanted_refs, &iter, e) {\n \t\tpacket_writer_write(&data->writer, \"%s %s\\n\",\n-\t\t\t\t oid_to_hex(item->util),\n-\t\t\t\t item->string);\n+\t\t\t\t oid_to_hex(e->value),\n+\t\t\t\t e->key);\n \t}\n \n \tpacket_writer_delim(&data->writer);\n-- \n2.44.0.rc2.424.gbdbf4d014b\n\n"},{"id":"489631","messageId":"20240228223846.GE1158131@coredump.intra.peff.net","threadId":"61025","inReplyTo":"20240228223700.GA1157826@coredump.intra.peff.net","subject":"[PATCH 5/9] upload-pack: accept only a single packfile-uri line","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-02-28T22:38:46Z","receivedAt":"2024-02-28T22:38:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we see a packfile-uri line from the client, we use\nstring_list_split() to split it on commas and store the result in a\nstring_list. A single packfile-uri line is therefore limited to storing\n~64kb, the size of a pkt-line.\n\nBut we'll happily accept multiple such lines, and each line appends to\nthe string list, growing without bound.\n\nIn theory this could be useful, making:\n\n 0017packfile-uris http\n 0018packfile-uris https\n\nequivalent to:\n\n 001dpackfile-uris http,https\n\nBut the protocol documentation doesn't indicate that this should work\n(and indeed, refers to this in the singular as \"the following argument\ncan be included in the client's request\"). And the client-side\nimplementation in fetch-pack has always sent a single line (JGit appears\nto understand the line on the server side but has no client-side\nimplementation, and libgit2 understands neither).\n\nIf we were worried about compatibility, we could instead just put a\nlimit on the maximum number of values we'd accept. The current client\nimplementation limits itself to only two values: \"http\" and \"https\", so\nsomething like \"256\" would be more than enough. But accepting only a\nsingle line seems more in line with the protocol documentation, and\nmatches other parts of the protocol (e.g., we will not accept a second\n\"filter\" line).\n\nWe'll also make this more explicit in the protocol documentation; as\nabove, I think this was always the intent, but there's no harm in making\nit clear.\n\nSigned-off-by: Jeff King \n---\n Documentation/gitprotocol-v2.txt | 3 ++-\n upload-pack.c | 3 +++\n 2 files changed, 5 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/gitprotocol-v2.txt b/Documentation/gitprotocol-v2.txt\nindex 837ea6171e..414bc625d5 100644\n--- a/Documentation/gitprotocol-v2.txt\n+++ b/Documentation/gitprotocol-v2.txt\n@@ -362,7 +362,8 @@ included in the client's request:\n If the 'packfile-uris' feature is advertised, the following argument\n can be included in the client's request as well as the potential\n addition of the 'packfile-uris' section in the server's response as\n-explained below.\n+explained below. Note that at most one `packfile-uris` line can be sent\n+to the server.\n \n packfile-uris \n \tIndicates to the server that the client is willing to receive\ndiff --git a/upload-pack.c b/upload-pack.c\nindex 8b47576ec7..2a5c52666e 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -1646,6 +1646,9 @@ static void process_args(struct packet_reader *request,\n \t\t}\n \n \t\tif (skip_prefix(arg, \"packfile-uris \", &p)) {\n+\t\t\tif (data->uri_protocols.nr)\n+\t\t\t\tsend_err_and_die(data,\n+\t\t\t\t\t\t \"multiple packfile-uris lines forbidden\");\n \t\t\tstring_list_split(&data->uri_protocols, p, ',', -1);\n \t\t\tcontinue;\n \t\t}\n-- \n2.44.0.rc2.424.gbdbf4d014b\n\n"},{"id":"489632","messageId":"20240228223858.GF1158131@coredump.intra.peff.net","threadId":"61025","inReplyTo":"20240228223700.GA1157826@coredump.intra.peff.net","subject":"[PATCH 6/9] upload-pack: disallow object-info capability by default","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-02-28T22:38:58Z","receivedAt":"2024-02-28T22:38:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"From: Taylor Blau \n\nWe added an \"object-info\" capability to the v2 upload-pack protocol in\na2ba162cda (object-info: support for retrieving object info,\n2021-04-20). In the almost 3 years since, we have not added any\nclient-side support, and it does not appear to exist in other\nimplementations either (JGit understands the verb on the server side,\nbut not on the client side).\n\nSince this largely unused code is accessible over the network by\ndefault, it increases the attack surface of upload-pack. I don't know of\nany particularly severe problem, but one issue is that because of the\nrequest/response nature of the v2 protocol, it will happily read an\nunbounded number of packets, adding each one to a string list (without\nregard to whether they are objects we know about, duplicates, etc).\n\nThis may be something we want to improve in the long run, but in the\nshort term it makes sense to disable the feature entirely. We'll add a\nconfig option as an escape hatch for anybody who wants to develop the\nfeature further.\n\nA more gentle option would be to add the config option to let people\ndisable it manually, but leave it enabled by default. But given that\nthere's no client side support, that seems like the wrong balance with\nsecurity.\n\nDisabling by default will slow adoption a bit once client-side support\ndoes become available (there were some patches[1] in 2022, but nothing\ngot merged and there's been nothing since). But clients have to deal\nwith older servers that do not understand the option anyway (and the\ncapability system handles that), so it will just be a matter of servers\nflipping their config at that point (and hopefully once any unbounded\nallocations have been addressed).\n\n[jk: this is a patch that GitHub has been running for several years, but\n rebased forward and with a new commit message for upstream]\n\n[1] https://lore.kernel.org/git/20220208231911.725273-1-calvinwan@google.com/\n\nSigned-off-by: Taylor Blau \nSigned-off-by: Jeff King \n---\n Documentation/config/transfer.txt | 4 ++++\n serve.c | 14 +++++++++++++-\n t/t5555-http-smart-common.sh | 1 -\n t/t5701-git-serve.sh | 24 +++++++++++++++++++++++-\n 4 files changed, 40 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/config/transfer.txt b/Documentation/config/transfer.txt\nindex a9cbdb88a1..f1ce50f4a6 100644\n--- a/Documentation/config/transfer.txt\n+++ b/Documentation/config/transfer.txt\n@@ -121,3 +121,7 @@ transfer.bundleURI::\n \tinformation from the remote server (if advertised) and download\n \tbundles before continuing the clone through the Git protocol.\n \tDefaults to `false`.\n+\n+transfer.advertiseObjectInfo::\n+\tWhen `true`, the `object-info` capability is advertised by\n+\tservers. Defaults to false.\ndiff --git a/serve.c b/serve.c\nindex a1d71134d4..aa651b73e9 100644\n--- a/serve.c\n+++ b/serve.c\n@@ -12,6 +12,7 @@\n #include \"trace2.h\"\n \n static int advertise_sid = -1;\n+static int advertise_object_info = -1;\n static int client_hash_algo = GIT_HASH_SHA1;\n \n static int always_advertise(struct repository *r UNUSED,\n@@ -67,6 +68,17 @@ static void session_id_receive(struct repository *r UNUSED,\n \ttrace2_data_string(\"transfer\", NULL, \"client-sid\", client_sid);\n }\n \n+static int object_info_advertise(struct repository *r, struct strbuf *value UNUSED)\n+{\n+\tif (advertise_object_info == -1 &&\n+\t repo_config_get_bool(r, \"transfer.advertiseobjectinfo\",\n+\t\t\t\t &advertise_object_info)) {\n+\t\t/* disabled by default */\n+\t\tadvertise_object_info = 0;\n+\t}\n+\treturn advertise_object_info;\n+}\n+\n struct protocol_capability {\n \t/*\n \t * The name of the capability. The server uses this name when\n@@ -135,7 +147,7 @@ static struct protocol_capability capabilities[] = {\n \t},\n \t{\n \t\t.name = \"object-info\",\n-\t\t.advertise = always_advertise,\n+\t\t.advertise = object_info_advertise,\n \t\t.command = cap_object_info,\n \t},\n \t{\ndiff --git a/t/t5555-http-smart-common.sh b/t/t5555-http-smart-common.sh\nindex b1cfe8b7db..3dcb3340a3 100755\n--- a/t/t5555-http-smart-common.sh\n+++ b/t/t5555-http-smart-common.sh\n@@ -131,7 +131,6 @@ test_expect_success 'git upload-pack --advertise-refs: v2' '\n \tfetch=shallow wait-for-done\n \tserver-option\n \tobject-format=$(test_oid algo)\n-\tobject-info\n \t0000\n \tEOF\n \ndiff --git a/t/t5701-git-serve.sh b/t/t5701-git-serve.sh\nindex 3591bc2417..c48830de8f 100755\n--- a/t/t5701-git-serve.sh\n+++ b/t/t5701-git-serve.sh\n@@ -20,7 +20,6 @@ test_expect_success 'test capability advertisement' '\n \tfetch=shallow wait-for-done\n \tserver-option\n \tobject-format=$(test_oid algo)\n-\tobject-info\n \tEOF\n \tcat >expect.trailer <<-EOF &&\n \t0000\n@@ -323,6 +322,8 @@ test_expect_success 'unexpected lines are not allowed in fetch request' '\n # Test the basics of object-info\n #\n test_expect_success 'basics of object-info' '\n+\ttest_config transfer.advertiseObjectInfo true &&\n+\n \ttest-tool pkt-line pack >in <<-EOF &&\n \tcommand=object-info\n \tobject-format=$(test_oid algo)\n@@ -380,4 +381,25 @@ test_expect_success 'basics of bundle-uri: dies if not enabled' '\n \ttest_must_be_empty out\n '\n \n+test_expect_success 'object-info missing from capabilities when disabled' '\n+\ttest_config transfer.advertiseObjectInfo false &&\n+\n+\tGIT_TEST_SIDEBAND_ALL=0 test-tool serve-v2 \\\n+\t\t--advertise-capabilities >out &&\n+\ttest-tool pkt-line unpack actual &&\n+\n+\t! grep object.info actual\n+'\n+\n+test_expect_success 'object-info commands rejected when disabled' '\n+\ttest_config transfer.advertiseObjectInfo false &&\n+\n+\ttest-tool pkt-line pack >in <<-EOF &&\n+\tcommand=object-info\n+\tEOF\n+\n+\ttest_must_fail test-tool serve-v2 --stateless-rpc err &&\n+\tgrep invalid.command err\n+'\n+\n test_done\n-- \n2.44.0.rc2.424.gbdbf4d014b\n\n"},{"id":"489633","messageId":"20240228223900.GG1158131@coredump.intra.peff.net","threadId":"61025","inReplyTo":"20240228223700.GA1157826@coredump.intra.peff.net","subject":"[PATCH 7/9] upload-pack: always turn off save_commit_buffer","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-02-28T22:39:00Z","receivedAt":"2024-02-28T22:39:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When the client sends us \"want $oid\" lines, we call parse_object($oid)\nto get an object struct. It's important to parse the commits because we\nneed to traverse them in the negotiation phase. But of course we don't\nneed to hold on to the commit messages for each one.\n\nWe've turned off the save_commit_buffer flag in get_common_commits() for\na long time, since f0243f26f6 (git-upload-pack: More efficient usage of\nthe has_sha1 array, 2005-10-28). That helps with the commits we see\nwhile actually traversing. But:\n\n 1. That function is only used by the v0 protocol. I think the v2\n protocol's code path leaves the flag on (and thus pays the extra\n memory penalty), though I didn't measure it specifically.\n\n 2. If the client sends us a bunch of \"want\" lines, that happens before\n the negotiation phase. So we'll hold on to all of those commit\n messages. Generally the number of \"want\" lines scales with the\n refs, not with the number of objects in the repo. But a malicious\n client could send a lot in order to waste memory.\n\nAs an example of (2), if I generate a request to fetch all commits in\ngit.git like this:\n\n pktline() {\n local msg=\"$*\"\n printf \"%04x%s\\n\" $((1+4+${#msg})) \"$msg\"\n }\n\n want_commits() {\n pktline command=fetch\n printf 0001\n git cat-file --batch-all-objects --batch-check='%(objectname) %(objecttype)' |\n while read oid type; do\n test \"$type\" = \"commit\" || continue\n pktline want $oid\n done\n pktline done\n printf 0000\n }\n\n want_commits | GIT_PROTOCOL=version=2 valgrind --tool=massif git-upload-pack . >/dev/null\n\nbefore this patch upload-pack peaks at ~125MB, and after at ~35MB. The\ndifference is not coincidentally about the same as the sum of all commit\nobject sizes as computed by:\n\n git cat-file --batch-all-objects --batch-check='%(objecttype) %(objectsize)' |\n perl -alne '$v += $F[1] if $F[0] eq \"commit\"; END { print $v }'\n\nIn a larger repository like linux.git, that number is ~1GB.\n\nIn a repository with a full commit-graph file this will have no impact\n(and the commit graph would save us from parsing at all, so is a much\nbetter solution!). But it's easy to do, might help a little in\nreal-world cases (where even if you have a commit graph it might not be\nfully up to date), and helps a lot for a worst-case malicious request.\n\nSigned-off-by: Jeff King \n---\n builtin/upload-pack.c | 2 ++\n upload-pack.c | 2 --\n 2 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/upload-pack.c b/builtin/upload-pack.c\nindex 9b021ef026..15afb97260 100644\n--- a/builtin/upload-pack.c\n+++ b/builtin/upload-pack.c\n@@ -8,6 +8,7 @@\n #include \"replace-object.h\"\n #include \"upload-pack.h\"\n #include \"serve.h\"\n+#include \"commit.h\"\n \n static const char * const upload_pack_usage[] = {\n \tN_(\"git-upload-pack [--[no-]strict] [--timeout=] [--stateless-rpc]\\n\"\n@@ -37,6 +38,7 @@ int cmd_upload_pack(int argc, const char **argv, const char *prefix)\n \n \tpacket_trace_identity(\"upload-pack\");\n \tdisable_replace_refs();\n+\tsave_commit_buffer = 0;\n \n \targc = parse_options(argc, argv, prefix, options, upload_pack_usage, 0);\n \ndiff --git a/upload-pack.c b/upload-pack.c\nindex 2a5c52666e..3970bb9b30 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -526,8 +526,6 @@ static int get_common_commits(struct upload_pack_data *data,\n \tint got_other = 0;\n \tint sent_ready = 0;\n \n-\tsave_commit_buffer = 0;\n-\n \tfor (;;) {\n \t\tconst char *arg;\n \n-- \n2.44.0.rc2.424.gbdbf4d014b\n\n"},{"id":"489634","messageId":"20240228223903.GH1158131@coredump.intra.peff.net","threadId":"61025","inReplyTo":"20240228223700.GA1157826@coredump.intra.peff.net","subject":"[PATCH 8/9] upload-pack: use PARSE_OBJECT_SKIP_HASH_CHECK in more places","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-02-28T22:39:03Z","receivedAt":"2024-02-28T22:39:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"In commit 0bc2557951 (upload-pack: skip parse-object re-hashing of\n\"want\" objects, 2022-09-06), we optimized the parse_object() calls for\nv2 \"want\" lines from the client so that they avoided parsing blobs, and\nso that they used the commit-graph rather than parsing commit objects\nfrom scratch.\n\nWe should extend that to two other spots:\n\n 1. We parse \"have\" objects in the got_oid() function. These won't\n generally be non-commits (unlike \"want\" lines from a partial\n clone). But we still benefit from the use of the commit-graph.\n\n 2. For v0, the \"want\" lines are parsed in receive_needs(). These are\n also less likely to be non-commits because by default they have to\n be ref tips. There are config options you might set to allow\n non-tip objects, but you'd mostly do so to support partial clones,\n and clients recent enough to support partial clone will generally\n speak v2 anyway.\n\nSo I don't expect this change to improve performance much for day-to-day\noperations. But both are possible denial-of-service vectors, where an\nattacker can waste our time by sending over a large number of objects to\nparse (of course we may waste even more time serving a pack to them, but\nwe try as much as possible to optimize that in pack-objects; we should\ndo what we can here in upload-pack, too).\n\nWith this patch, running p5600 with GIT_TEST_PROTOCOL_VERSION=0 shows\nsimilar results to what we saw in 0bc2557951 (which ran with the v2\nprotocol by default). Here are the numbers for linux.git:\n\n Test HEAD^ HEAD\n -----------------------------------------------------------------------------\n 5600.3: checkout of result 50.91(87.95+2.93) 41.75(79.00+3.18) -18.0%\n\nOr for a more extreme (and malicious) case, we can claim to \"have\" every\nblob in git.git over the v0 protocol:\n\n $ {\n echo \"0032want $(git rev-parse HEAD)\"\n printf 0000\n git cat-file --batch-all-objects --batch-check='%(objectname) %(objecttype)' |\n perl -alne 'print \"0032have $F[0]\" if $F[1] eq \"blob\"'\n } >input\n\n $ time ./git.old upload-pack . /dev/null\n real\t0m52.951s\n user\t0m51.633s\n sys\t0m1.304s\n\n $ time ./git.new upload-pack . /dev/null\n real\t0m0.261s\n user\t0m0.156s\n sys\t0m0.105s\n\n(Note that these don't actually compute a pack because of the hacky\nprotocol usage, so those numbers are representing the raw blob-parsing\neffort done by upload-pack).\n\nSigned-off-by: Jeff King \n---\n upload-pack.c | 6 ++++--\n 1 file changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/upload-pack.c b/upload-pack.c\nindex 3970bb9b30..b721155442 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -469,7 +469,8 @@ static void create_pack_file(struct upload_pack_data *pack_data,\n static int do_got_oid(struct upload_pack_data *data, const struct object_id *oid)\n {\n \tint we_knew_they_have = 0;\n-\tstruct object *o = parse_object(the_repository, oid);\n+\tstruct object *o = parse_object_with_flags(the_repository, oid,\n+\t\t\t\t\t\t PARSE_OBJECT_SKIP_HASH_CHECK);\n \n \tif (!o)\n \t\tdie(\"oops (%s)\", oid_to_hex(oid));\n@@ -1148,7 +1149,8 @@ static void receive_needs(struct upload_pack_data *data,\n \t\t\tfree(client_sid);\n \t\t}\n \n-\t\to = parse_object(the_repository, &oid_buf);\n+\t\to = parse_object_with_flags(the_repository, &oid_buf,\n+\t\t\t\t\t PARSE_OBJECT_SKIP_HASH_CHECK);\n \t\tif (!o) {\n \t\t\tpacket_writer_error(&data->writer,\n \t\t\t\t\t \"upload-pack: not our ref %s\",\n-- \n2.44.0.rc2.424.gbdbf4d014b\n\n"},{"id":"489635","messageId":"20240228223907.GI1158131@coredump.intra.peff.net","threadId":"61025","inReplyTo":"20240228223700.GA1157826@coredump.intra.peff.net","subject":"[PATCH 9/9] upload-pack: free tree buffers after parsing","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-02-28T22:39:07Z","receivedAt":"2024-02-28T22:39:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When a client sends us a \"want\" or \"have\" line, we call parse_object()\nto get an object struct. If the object is a tree, then the parsed state\nmeans that tree->buffer points to the uncompressed contents of the tree.\nBut we don't really care about it. We only really need to parse commits\nand tags; for trees and blobs, the important output is just a \"struct\nobject\" with the correct type.\n\nBut much worse, we do not ever free that tree buffer. It's not leaked in\nthe traditional sense, in that we still have a pointer to it from the\nglobal object hash. But if the client requests many trees, we'll hold\nall of their contents in memory at the same time.\n\nNobody really noticed because it's rare for clients to directly request\na tree. It might happen for a lightweight tag pointing straight at a\ntree, or it might happen for a \"tree:depth\" partial clone filling in\nmissing trees.\n\nBut it's also possible for a malicious client to request a lot of trees,\ncausing upload-pack's memory to balloon. For example, without this\npatch, requesting every tree in git.git like:\n\n pktline() {\n local msg=\"$*\"\n printf \"%04x%s\\n\" $((1+4+${#msg})) \"$msg\"\n }\n\n want_trees() {\n pktline command=fetch\n printf 0001\n git cat-file --batch-all-objects --batch-check='%(objectname) %(objecttype)' |\n while read oid type; do\n test \"$type\" = \"tree\" || continue\n pktline want $oid\n done\n pktline done\n printf 0000\n }\n\n want_trees | GIT_PROTOCOL=version=2 valgrind --tool=massif ./git upload-pack . >/dev/null\n\nshows a peak heap usage of ~3.7GB. Which is just about the sum of the\nsizes of all of the uncompressed trees. For linux.git, it's closer to\n17GB.\n\nSo the obvious thing to do is to call free_tree_buffer() after we\nrealize that we've parsed a tree. We know that upload-pack won't need it\nlater. But let's push the logic into parse_object_with_flags(), telling\nit to discard the tree buffer immediately. There are two reasons for\nthis. One, all of the relevant call-sites already call the with_options\nvariant to pass the SKIP_HASH flag. So it actually ends up as less code\nthan manually free-ing in each spot. And two, it enables an extra\noptimization that I'll discuss below.\n\nI've touched all of the sites that currently use SKIP_HASH in\nupload-pack. That drops the peak heap of the upload-pack invocation\nabove from 3.7GB to ~24MB.\n\nI've also modified the caller in get_reference(); a partial clone\nbenefits from its use in pack-objects for the reasons given in\n0bc2557951 (upload-pack: skip parse-object re-hashing of \"want\" objects,\n2022-09-06), where we were measuring blob requests. But note that the\nresults of get_reference() are used for traversing, as well; so we\nreally would _eventually_ use the tree contents. That makes this at\nfirst glance a space/time tradeoff: we won't hold all of the trees in\nmemory at once, but we'll have to reload them each when it comes time to\ntraverse.\n\nAnd here's where our extra optimization comes in. If the caller is not\ngoing to immediately look at the tree contents, and it doesn't care\nabout checking the hash, then parse_object() can simply skip loading the\ntree entirely, just like we do for blobs! And now it's not a space/time\ntradeoff in get_reference() anymore. It's just a lazy-load: we're\ndelaying reading the tree contents until it's time to actually traverse\nthem one by one.\n\nAnd of course for upload-pack, this optimization means we never load the\ntrees at all, saving lots of CPU time. Timing the \"every tree from\ngit.git\" request above shows upload-pack dropping from 32 seconds of CPU\nto 19 (the remainder is mostly due to pack-objects actually sending the\npack; timing just the upload-pack portion shows we go from 13s to\n~0.28s).\n\nThese are all highly gamed numbers, of course. For real-world\npartial-clone requests we're saving only a small bit of time in\npractice. But it does help harden upload-pack against malicious\ndenial-of-service attacks.\n\nSigned-off-by: Jeff King \n---\n object.c | 14 ++++++++++++++\n object.h | 1 +\n revision.c | 3 ++-\n upload-pack.c | 9 ++++++---\n 4 files changed, 23 insertions(+), 4 deletions(-)\n\ndiff --git a/object.c b/object.c\nindex e6a1c4d905..f11c59ac0c 100644\n--- a/object.c\n+++ b/object.c\n@@ -271,6 +271,7 @@ struct object *parse_object_with_flags(struct repository *r,\n \t\t\t\t enum parse_object_flags flags)\n {\n \tint skip_hash = !!(flags & PARSE_OBJECT_SKIP_HASH_CHECK);\n+\tint discard_tree = !!(flags & PARSE_OBJECT_DISCARD_TREE);\n \tunsigned long size;\n \tenum object_type type;\n \tint eaten;\n@@ -298,6 +299,17 @@ struct object *parse_object_with_flags(struct repository *r,\n \t\treturn lookup_object(r, oid);\n \t}\n \n+\t/*\n+\t * If the caller does not care about the tree buffer and does not\n+\t * care about checking the hash, we can simply verify that we\n+\t * have the on-disk object with the correct type.\n+\t */\n+\tif (skip_hash && discard_tree &&\n+\t (!obj || obj->type == OBJ_TREE) &&\n+\t oid_object_info(r, oid, NULL) == OBJ_TREE) {\n+\t\treturn &lookup_tree(r, oid)->object;\n+\t}\n+\n \tbuffer = repo_read_object_file(r, oid, &type, &size);\n \tif (buffer) {\n \t\tif (!skip_hash &&\n@@ -311,6 +323,8 @@ struct object *parse_object_with_flags(struct repository *r,\n \t\t\t\t\t buffer, &eaten);\n \t\tif (!eaten)\n \t\t\tfree(buffer);\n+\t\tif (discard_tree && type == OBJ_TREE)\n+\t\t\tfree_tree_buffer((struct tree *)obj);\n \t\treturn obj;\n \t}\n \treturn NULL;\ndiff --git a/object.h b/object.h\nindex 114d45954d..c7123cade6 100644\n--- a/object.h\n+++ b/object.h\n@@ -197,6 +197,7 @@ void *object_as_type(struct object *obj, enum object_type type, int quiet);\n */\n enum parse_object_flags {\n \tPARSE_OBJECT_SKIP_HASH_CHECK = 1 << 0,\n+\tPARSE_OBJECT_DISCARD_TREE = 1 << 1,\n };\n struct object *parse_object(struct repository *r, const struct object_id *oid);\n struct object *parse_object_with_flags(struct repository *r,\ndiff --git a/revision.c b/revision.c\nindex 2424c9bd67..b10f63a607 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -381,7 +381,8 @@ static struct object *get_reference(struct rev_info *revs, const char *name,\n \n \tobject = parse_object_with_flags(revs->repo, oid,\n \t\t\t\t\t revs->verify_objects ? 0 :\n-\t\t\t\t\t PARSE_OBJECT_SKIP_HASH_CHECK);\n+\t\t\t\t\t PARSE_OBJECT_SKIP_HASH_CHECK |\n+\t\t\t\t\t PARSE_OBJECT_DISCARD_TREE);\n \n \tif (!object) {\n \t\tif (revs->ignore_missing)\ndiff --git a/upload-pack.c b/upload-pack.c\nindex b721155442..761af4a532 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -470,7 +470,8 @@ static int do_got_oid(struct upload_pack_data *data, const struct object_id *oid\n {\n \tint we_knew_they_have = 0;\n \tstruct object *o = parse_object_with_flags(the_repository, oid,\n-\t\t\t\t\t\t PARSE_OBJECT_SKIP_HASH_CHECK);\n+\t\t\t\t\t\t PARSE_OBJECT_SKIP_HASH_CHECK |\n+\t\t\t\t\t\t PARSE_OBJECT_DISCARD_TREE);\n \n \tif (!o)\n \t\tdie(\"oops (%s)\", oid_to_hex(oid));\n@@ -1150,7 +1151,8 @@ static void receive_needs(struct upload_pack_data *data,\n \t\t}\n \n \t\to = parse_object_with_flags(the_repository, &oid_buf,\n-\t\t\t\t\t PARSE_OBJECT_SKIP_HASH_CHECK);\n+\t\t\t\t\t PARSE_OBJECT_SKIP_HASH_CHECK |\n+\t\t\t\t\t PARSE_OBJECT_DISCARD_TREE);\n \t\tif (!o) {\n \t\t\tpacket_writer_error(&data->writer,\n \t\t\t\t\t \"upload-pack: not our ref %s\",\n@@ -1467,7 +1469,8 @@ static int parse_want(struct packet_writer *writer, const char *line,\n \t\t\t \"expected to get oid, not '%s'\", line);\n \n \t\to = parse_object_with_flags(the_repository, &oid,\n-\t\t\t\t\t PARSE_OBJECT_SKIP_HASH_CHECK);\n+\t\t\t\t\t PARSE_OBJECT_SKIP_HASH_CHECK |\n+\t\t\t\t\t PARSE_OBJECT_DISCARD_TREE);\n \n \t\tif (!o) {\n \t\t\tpacket_writer_error(writer,\n-- \n2.44.0.rc2.424.gbdbf4d014b\n"},{"id":"489639","messageId":"xmqqa5nkjje8.fsf@gitster.g","threadId":"61025","inReplyTo":"20240228223700.GA1157826@coredump.intra.peff.net","subject":"Re: [PATCH 0/9] bound upload-pack memory allocations","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-02-28T22:47:43Z","receivedAt":"2024-02-28T22:47:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> We're not doing a coordinated disclosure or special release for this.\n> Even after these patches, it's possible to get upload-pack to allocate\n> quite a bit of memory, especially for a large repository. Not to mention\n> that pack-objects may also allocate quite a bit to serve the pack\n> itself. So while this is low-hanging fruit, a public-facing Git site\n> probably still needs to have some kind of external tooling to kill\n> hungry processes (even if it's just OOM-killing them so they don't hurt\n> other clients).\n>\n> [1/9]: upload-pack: drop separate v2 \"haves\" array\n> [2/9]: upload-pack: switch deepen-not list to an oid_array\n> [3/9]: upload-pack: use oidset for deepen_not list\n> [4/9]: upload-pack: use a strmap for want-ref lines\n> [5/9]: upload-pack: accept only a single packfile-uri line\n> [6/9]: upload-pack: disallow object-info capability by default\n> [7/9]: upload-pack: always turn off save_commit_buffer\n> [8/9]: upload-pack: use PARSE_OBJECT_SKIP_HASH_CHECK in more places\n> [9/9]: upload-pack: free tree buffers after parsing\n\nI saw them when they were posted to the security list and they\nlooked good already. Will queue. Thanks.\n"},{"id":"489844","messageId":"ZeWHdaZnhOHKs5QP@tanuki","threadId":"61025","inReplyTo":"20240228223907.GI1158131@coredump.intra.peff.net","subject":"Re: [PATCH 9/9] upload-pack: free tree buffers after parsing","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-03-04T08:33:57Z","receivedAt":"2024-03-04T08:34:03Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Feb 28, 2024 at 05:39:07PM -0500, Jeff King wrote:\n> When a client sends us a \"want\" or \"have\" line, we call parse_object()\n> to get an object struct. If the object is a tree, then the parsed state\n> means that tree->buffer points to the uncompressed contents of the tree.\n> But we don't really care about it. We only really need to parse commits\n> and tags; for trees and blobs, the important output is just a \"struct\n> object\" with the correct type.\n> \n> But much worse, we do not ever free that tree buffer. It's not leaked in\n> the traditional sense, in that we still have a pointer to it from the\n> global object hash. But if the client requests many trees, we'll hold\n> all of their contents in memory at the same time.\n> \n> Nobody really noticed because it's rare for clients to directly request\n> a tree. It might happen for a lightweight tag pointing straight at a\n> tree, or it might happen for a \"tree:depth\" partial clone filling in\n> missing trees.\n> \n> But it's also possible for a malicious client to request a lot of trees,\n> causing upload-pack's memory to balloon. For example, without this\n> patch, requesting every tree in git.git like:\n> \n> pktline() {\n> local msg=\"$*\"\n> printf \"%04x%s\\n\" $((1+4+${#msg})) \"$msg\"\n> }\n> \n> want_trees() {\n> pktline command=fetch\n> printf 0001\n> git cat-file --batch-all-objects --batch-check='%(objectname) %(objecttype)' |\n> while read oid type; do\n> test \"$type\" = \"tree\" || continue\n> pktline want $oid\n> done\n> pktline done\n> printf 0000\n> }\n> \n> want_trees | GIT_PROTOCOL=version=2 valgrind --tool=massif ./git upload-pack . >/dev/null\n> \n> shows a peak heap usage of ~3.7GB. Which is just about the sum of the\n> sizes of all of the uncompressed trees. For linux.git, it's closer to\n> 17GB.\n> \n> So the obvious thing to do is to call free_tree_buffer() after we\n> realize that we've parsed a tree. We know that upload-pack won't need it\n> later. But let's push the logic into parse_object_with_flags(), telling\n> it to discard the tree buffer immediately. There are two reasons for\n> this. One, all of the relevant call-sites already call the with_options\n> variant to pass the SKIP_HASH flag. So it actually ends up as less code\n> than manually free-ing in each spot. And two, it enables an extra\n> optimization that I'll discuss below.\n> \n> I've touched all of the sites that currently use SKIP_HASH in\n> upload-pack. That drops the peak heap of the upload-pack invocation\n> above from 3.7GB to ~24MB.\n> \n> I've also modified the caller in get_reference(); a partial clone\n> benefits from its use in pack-objects for the reasons given in\n> 0bc2557951 (upload-pack: skip parse-object re-hashing of \"want\" objects,\n> 2022-09-06), where we were measuring blob requests. But note that the\n> results of get_reference() are used for traversing, as well; so we\n> really would _eventually_ use the tree contents. That makes this at\n> first glance a space/time tradeoff: we won't hold all of the trees in\n> memory at once, but we'll have to reload them each when it comes time to\n> traverse.\n> \n> And here's where our extra optimization comes in. If the caller is not\n> going to immediately look at the tree contents, and it doesn't care\n> about checking the hash, then parse_object() can simply skip loading the\n> tree entirely, just like we do for blobs! And now it's not a space/time\n> tradeoff in get_reference() anymore. It's just a lazy-load: we're\n> delaying reading the tree contents until it's time to actually traverse\n> them one by one.\n> \n> And of course for upload-pack, this optimization means we never load the\n> trees at all, saving lots of CPU time. Timing the \"every tree from\n> git.git\" request above shows upload-pack dropping from 32 seconds of CPU\n> to 19 (the remainder is mostly due to pack-objects actually sending the\n> pack; timing just the upload-pack portion shows we go from 13s to\n> ~0.28s).\n> \n> These are all highly gamed numbers, of course. For real-world\n> partial-clone requests we're saving only a small bit of time in\n> practice. But it does help harden upload-pack against malicious\n> denial-of-service attacks.\n> \n> Signed-off-by: Jeff King \n> ---\n> object.c | 14 ++++++++++++++\n> object.h | 1 +\n> revision.c | 3 ++-\n> upload-pack.c | 9 ++++++---\n> 4 files changed, 23 insertions(+), 4 deletions(-)\n> \n> diff --git a/object.c b/object.c\n> index e6a1c4d905..f11c59ac0c 100644\n> --- a/object.c\n> +++ b/object.c\n> @@ -271,6 +271,7 @@ struct object *parse_object_with_flags(struct repository *r,\n> \t\t\t\t enum parse_object_flags flags)\n> {\n> \tint skip_hash = !!(flags & PARSE_OBJECT_SKIP_HASH_CHECK);\n> +\tint discard_tree = !!(flags & PARSE_OBJECT_DISCARD_TREE);\n> \tunsigned long size;\n> \tenum object_type type;\n> \tint eaten;\n> @@ -298,6 +299,17 @@ struct object *parse_object_with_flags(struct repository *r,\n> \t\treturn lookup_object(r, oid);\n> \t}\n> \n> +\t/*\n> +\t * If the caller does not care about the tree buffer and does not\n> +\t * care about checking the hash, we can simply verify that we\n> +\t * have the on-disk object with the correct type.\n> +\t */\n> +\tif (skip_hash && discard_tree &&\n> +\t (!obj || obj->type == OBJ_TREE) &&\n> +\t oid_object_info(r, oid, NULL) == OBJ_TREE) {\n> +\t\treturn &lookup_tree(r, oid)->object;\n> +\t}\n\nThe other condition for blobs does the same, but the condition here\nconfuses me. Why do we call `oid_object_info()` if we have already\nfigured out that `obj->type == OBJ_TREE`? Feels like wasted effort if\nthe in-memory object has been determined to be a tree already anyway.\n\nI'd rather have expected it to look like the following:\n\nif (skip_hash && discard_tree &&\n ((obj && obj->type == OBJ_TREE) ||\n (!obj && oid_object_info(r, oid, NULL)) == OBJ_TREE)) {\n\t\treturn &lookup_tree(r, oid)->object;\n}\n\nAm I missing some side effect that `oid_object_info()` provides?\n\nPatrick\n\n> \tbuffer = repo_read_object_file(r, oid, &type, &size);\n> \tif (buffer) {\n> \t\tif (!skip_hash &&\n> @@ -311,6 +323,8 @@ struct object *parse_object_with_flags(struct repository *r,\n> \t\t\t\t\t buffer, &eaten);\n> \t\tif (!eaten)\n> \t\t\tfree(buffer);\n> +\t\tif (discard_tree && type == OBJ_TREE)\n> +\t\t\tfree_tree_buffer((struct tree *)obj);\n> \t\treturn obj;\n> \t}\n> \treturn NULL;\n> diff --git a/object.h b/object.h\n> index 114d45954d..c7123cade6 100644\n> --- a/object.h\n> +++ b/object.h\n> @@ -197,6 +197,7 @@ void *object_as_type(struct object *obj, enum object_type type, int quiet);\n> */\n> enum parse_object_flags {\n> \tPARSE_OBJECT_SKIP_HASH_CHECK = 1 << 0,\n> +\tPARSE_OBJECT_DISCARD_TREE = 1 << 1,\n> };\n> struct object *parse_object(struct repository *r, const struct object_id *oid);\n> struct object *parse_object_with_flags(struct repository *r,\n> diff --git a/revision.c b/revision.c\n> index 2424c9bd67..b10f63a607 100644\n> --- a/revision.c\n> +++ b/revision.c\n> @@ -381,7 +381,8 @@ static struct object *get_reference(struct rev_info *revs, const char *name,\n> \n> \tobject = parse_object_with_flags(revs->repo, oid,\n> \t\t\t\t\t revs->verify_objects ? 0 :\n> -\t\t\t\t\t PARSE_OBJECT_SKIP_HASH_CHECK);\n> +\t\t\t\t\t PARSE_OBJECT_SKIP_HASH_CHECK |\n> +\t\t\t\t\t PARSE_OBJECT_DISCARD_TREE);\n> \n> \tif (!object) {\n> \t\tif (revs->ignore_missing)\n> diff --git a/upload-pack.c b/upload-pack.c\n> index b721155442..761af4a532 100644\n> --- a/upload-pack.c\n> +++ b/upload-pack.c\n> @@ -470,7 +470,8 @@ static int do_got_oid(struct upload_pack_data *data, const struct object_id *oid\n> {\n> \tint we_knew_they_have = 0;\n> \tstruct object *o = parse_object_with_flags(the_repository, oid,\n> -\t\t\t\t\t\t PARSE_OBJECT_SKIP_HASH_CHECK);\n> +\t\t\t\t\t\t PARSE_OBJECT_SKIP_HASH_CHECK |\n> +\t\t\t\t\t\t PARSE_OBJECT_DISCARD_TREE);\n> \n> \tif (!o)\n> \t\tdie(\"oops (%s)\", oid_to_hex(oid));\n> @@ -1150,7 +1151,8 @@ static void receive_needs(struct upload_pack_data *data,\n> \t\t}\n> \n> \t\to = parse_object_with_flags(the_repository, &oid_buf,\n> -\t\t\t\t\t PARSE_OBJECT_SKIP_HASH_CHECK);\n> +\t\t\t\t\t PARSE_OBJECT_SKIP_HASH_CHECK |\n> +\t\t\t\t\t PARSE_OBJECT_DISCARD_TREE);\n> \t\tif (!o) {\n> \t\t\tpacket_writer_error(&data->writer,\n> \t\t\t\t\t \"upload-pack: not our ref %s\",\n> @@ -1467,7 +1469,8 @@ static int parse_want(struct packet_writer *writer, const char *line,\n> \t\t\t \"expected to get oid, not '%s'\", line);\n> \n> \t\to = parse_object_with_flags(the_repository, &oid,\n> -\t\t\t\t\t PARSE_OBJECT_SKIP_HASH_CHECK);\n> +\t\t\t\t\t PARSE_OBJECT_SKIP_HASH_CHECK |\n> +\t\t\t\t\t PARSE_OBJECT_DISCARD_TREE);\n> \n> \t\tif (!o) {\n> \t\t\tpacket_writer_error(writer,\n> -- \n> 2.44.0.rc2.424.gbdbf4d014b\n> \n"},{"id":"489845","messageId":"ZeWHlknuWMvRiFtC@tanuki","threadId":"61025","inReplyTo":"20240228223858.GF1158131@coredump.intra.peff.net","subject":"Re: [PATCH 6/9] upload-pack: disallow object-info capability by default","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-03-04T08:34:30Z","receivedAt":"2024-03-04T08:34:34Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Feb 28, 2024 at 05:38:58PM -0500, Jeff King wrote:\n> From: Taylor Blau \n> \n> We added an \"object-info\" capability to the v2 upload-pack protocol in\n> a2ba162cda (object-info: support for retrieving object info,\n> 2021-04-20). In the almost 3 years since, we have not added any\n> client-side support, and it does not appear to exist in other\n> implementations either (JGit understands the verb on the server side,\n> but not on the client side).\n> \n> Since this largely unused code is accessible over the network by\n> default, it increases the attack surface of upload-pack. I don't know of\n> any particularly severe problem, but one issue is that because of the\n> request/response nature of the v2 protocol, it will happily read an\n> unbounded number of packets, adding each one to a string list (without\n> regard to whether they are objects we know about, duplicates, etc).\n> \n> This may be something we want to improve in the long run, but in the\n> short term it makes sense to disable the feature entirely. We'll add a\n> config option as an escape hatch for anybody who wants to develop the\n> feature further.\n> \n> A more gentle option would be to add the config option to let people\n> disable it manually, but leave it enabled by default. But given that\n> there's no client side support, that seems like the wrong balance with\n> security.\n> \n> Disabling by default will slow adoption a bit once client-side support\n> does become available (there were some patches[1] in 2022, but nothing\n> got merged and there's been nothing since). But clients have to deal\n> with older servers that do not understand the option anyway (and the\n> capability system handles that), so it will just be a matter of servers\n> flipping their config at that point (and hopefully once any unbounded\n> allocations have been addressed).\n> \n> [jk: this is a patch that GitHub has been running for several years, but\n> rebased forward and with a new commit message for upstream]\n> \n> [1] https://lore.kernel.org/git/20220208231911.725273-1-calvinwan@google.com/\n> \n> Signed-off-by: Taylor Blau \n> Signed-off-by: Jeff King \n> ---\n> Documentation/config/transfer.txt | 4 ++++\n> serve.c | 14 +++++++++++++-\n> t/t5555-http-smart-common.sh | 1 -\n> t/t5701-git-serve.sh | 24 +++++++++++++++++++++++-\n> 4 files changed, 40 insertions(+), 3 deletions(-)\n> \n> diff --git a/Documentation/config/transfer.txt b/Documentation/config/transfer.txt\n> index a9cbdb88a1..f1ce50f4a6 100644\n> --- a/Documentation/config/transfer.txt\n> +++ b/Documentation/config/transfer.txt\n> @@ -121,3 +121,7 @@ transfer.bundleURI::\n> \tinformation from the remote server (if advertised) and download\n> \tbundles before continuing the clone through the Git protocol.\n> \tDefaults to `false`.\n> +\n> +transfer.advertiseObjectInfo::\n> +\tWhen `true`, the `object-info` capability is advertised by\n> +\tservers. Defaults to false.\n> diff --git a/serve.c b/serve.c\n> index a1d71134d4..aa651b73e9 100644\n> --- a/serve.c\n> +++ b/serve.c\n> @@ -12,6 +12,7 @@\n> #include \"trace2.h\"\n> \n> static int advertise_sid = -1;\n> +static int advertise_object_info = -1;\n> static int client_hash_algo = GIT_HASH_SHA1;\n> \n> static int always_advertise(struct repository *r UNUSED,\n> @@ -67,6 +68,17 @@ static void session_id_receive(struct repository *r UNUSED,\n> \ttrace2_data_string(\"transfer\", NULL, \"client-sid\", client_sid);\n> }\n> \n> +static int object_info_advertise(struct repository *r, struct strbuf *value UNUSED)\n> +{\n> +\tif (advertise_object_info == -1 &&\n> +\t repo_config_get_bool(r, \"transfer.advertiseobjectinfo\",\n> +\t\t\t\t &advertise_object_info)) {\n> +\t\t/* disabled by default */\n> +\t\tadvertise_object_info = 0;\n> +\t}\n> +\treturn advertise_object_info;\n> +}\n> +\n> struct protocol_capability {\n> \t/*\n> \t * The name of the capability. The server uses this name when\n> @@ -135,7 +147,7 @@ static struct protocol_capability capabilities[] = {\n> \t},\n> \t{\n> \t\t.name = \"object-info\",\n> -\t\t.advertise = always_advertise,\n> +\t\t.advertise = object_info_advertise,\n> \t\t.command = cap_object_info,\n> \t},\n> \t{\n> diff --git a/t/t5555-http-smart-common.sh b/t/t5555-http-smart-common.sh\n> index b1cfe8b7db..3dcb3340a3 100755\n> --- a/t/t5555-http-smart-common.sh\n> +++ b/t/t5555-http-smart-common.sh\n> @@ -131,7 +131,6 @@ test_expect_success 'git upload-pack --advertise-refs: v2' '\n> \tfetch=shallow wait-for-done\n> \tserver-option\n> \tobject-format=$(test_oid algo)\n> -\tobject-info\n> \t0000\n> \tEOF\n> \n> diff --git a/t/t5701-git-serve.sh b/t/t5701-git-serve.sh\n> index 3591bc2417..c48830de8f 100755\n> --- a/t/t5701-git-serve.sh\n> +++ b/t/t5701-git-serve.sh\n> @@ -20,7 +20,6 @@ test_expect_success 'test capability advertisement' '\n> \tfetch=shallow wait-for-done\n> \tserver-option\n> \tobject-format=$(test_oid algo)\n> -\tobject-info\n> \tEOF\n> \tcat >expect.trailer <<-EOF &&\n> \t0000\n> @@ -323,6 +322,8 @@ test_expect_success 'unexpected lines are not allowed in fetch request' '\n> # Test the basics of object-info\n> #\n> test_expect_success 'basics of object-info' '\n> +\ttest_config transfer.advertiseObjectInfo true &&\n> +\n> \ttest-tool pkt-line pack >in <<-EOF &&\n> \tcommand=object-info\n> \tobject-format=$(test_oid algo)\n> @@ -380,4 +381,25 @@ test_expect_success 'basics of bundle-uri: dies if not enabled' '\n> \ttest_must_be_empty out\n> '\n> \n> +test_expect_success 'object-info missing from capabilities when disabled' '\n> +\ttest_config transfer.advertiseObjectInfo false &&\n> +\n> +\tGIT_TEST_SIDEBAND_ALL=0 test-tool serve-v2 \\\n> +\t\t--advertise-capabilities >out &&\n> +\ttest-tool pkt-line unpack actual &&\n> +\n> +\t! grep object.info actual\n> +'\n\nIs it intentional that you grep for \"object.info\" instead of\n\"object-info\"?\n\nPatrick\n\n> +test_expect_success 'object-info commands rejected when disabled' '\n> +\ttest_config transfer.advertiseObjectInfo false &&\n> +\n> +\ttest-tool pkt-line pack >in <<-EOF &&\n> +\tcommand=object-info\n> +\tEOF\n> +\n> +\ttest_must_fail test-tool serve-v2 --stateless-rpc err &&\n> +\tgrep invalid.command err\n> +'\n> +\n> test_done\n> -- \n> 2.44.0.rc2.424.gbdbf4d014b\n> \n> \n"},{"id":"489860","messageId":"20240304095736.GA3723539@coredump.intra.peff.net","threadId":"61025","inReplyTo":"ZeWHdaZnhOHKs5QP@tanuki","subject":"Re: [PATCH 9/9] upload-pack: free tree buffers after parsing","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-03-04T09:57:36Z","receivedAt":"2024-03-04T09:57:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Mar 04, 2024 at 09:33:57AM +0100, Patrick Steinhardt wrote:\n\n> > +\tif (skip_hash && discard_tree &&\n> > +\t (!obj || obj->type == OBJ_TREE) &&\n> > +\t oid_object_info(r, oid, NULL) == OBJ_TREE) {\n> > +\t\treturn &lookup_tree(r, oid)->object;\n> > +\t}\n> \n> The other condition for blobs does the same, but the condition here\n> confuses me. Why do we call `oid_object_info()` if we have already\n> figured out that `obj->type == OBJ_TREE`? Feels like wasted effort if\n> the in-memory object has been determined to be a tree already anyway.\n> \n> I'd rather have expected it to look like the following:\n> \n> if (skip_hash && discard_tree &&\n> ((obj && obj->type == OBJ_TREE) ||\n> (!obj && oid_object_info(r, oid, NULL)) == OBJ_TREE)) {\n> \t\treturn &lookup_tree(r, oid)->object;\n> }\n> \n> Am I missing some side effect that `oid_object_info()` provides?\n\nCalling oid_object_info() will make sure the on-disk object exists and\nhas the expected type. Keep in mind that an in-memory \"struct object\"\nmay have a type that was just implied by another reference. E.g., if a\ncommit references some object X in its tree field, then we'll call\nlookup_tree(X) to get a \"struct tree\" without actually touching the odb\nat all. When it comes time to parse that object, that's when we'll see\nif we really have it and if it's a tree.\n\nIn the case of skip_hash (and discard_tree) it might be OK to skip both\nof those checks. If we do, I think we should probably do the same for\nblobs (in the skip_hash case, we could just return the object we found\nalready).\n\nBut I'd definitely prefer to do that as a separate step (if at all).\n\n-Peff\n"},{"id":"489861","messageId":"20240304095907.GB3723539@coredump.intra.peff.net","threadId":"61025","inReplyTo":"ZeWHlknuWMvRiFtC@tanuki","subject":"Re: [PATCH 6/9] upload-pack: disallow object-info capability by default","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-03-04T09:59:07Z","receivedAt":"2024-03-04T09:59:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Mar 04, 2024 at 09:34:30AM +0100, Patrick Steinhardt wrote:\n\n> > +test_expect_success 'object-info missing from capabilities when disabled' '\n> > +\ttest_config transfer.advertiseObjectInfo false &&\n> > +\n> > +\tGIT_TEST_SIDEBAND_ALL=0 test-tool serve-v2 \\\n> > +\t\t--advertise-capabilities >out &&\n> > +\ttest-tool pkt-line unpack actual &&\n> > +\n> > +\t! grep object.info actual\n> > +'\n> \n> Is it intentional that you grep for \"object.info\" instead of\n> \"object-info\"?\n\nI didn't even notice this. It should be equivalent because of the regex,\nbut I don't think there's a particular reason to be more loose (and\nunlike single-quote, which we sometimes match with \".\" for shell\nreadability, it should be fine to say \"object-info\" here).\n\n+cc Taylor, who wrote the original.\n\n-Peff\n"},{"id":"489863","messageId":"ZeWbsVL0NF32rYAt@tanuki","threadId":"61025","inReplyTo":"20240304095736.GA3723539@coredump.intra.peff.net","subject":"Re: [PATCH 9/9] upload-pack: free tree buffers after parsing","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-03-04T10:00:17Z","receivedAt":"2024-03-04T10:00:21Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Mar 04, 2024 at 04:57:36AM -0500, Jeff King wrote:\n> On Mon, Mar 04, 2024 at 09:33:57AM +0100, Patrick Steinhardt wrote:\n> \n> > > +\tif (skip_hash && discard_tree &&\n> > > +\t (!obj || obj->type == OBJ_TREE) &&\n> > > +\t oid_object_info(r, oid, NULL) == OBJ_TREE) {\n> > > +\t\treturn &lookup_tree(r, oid)->object;\n> > > +\t}\n> > \n> > The other condition for blobs does the same, but the condition here\n> > confuses me. Why do we call `oid_object_info()` if we have already\n> > figured out that `obj->type == OBJ_TREE`? Feels like wasted effort if\n> > the in-memory object has been determined to be a tree already anyway.\n> > \n> > I'd rather have expected it to look like the following:\n> > \n> > if (skip_hash && discard_tree &&\n> > ((obj && obj->type == OBJ_TREE) ||\n> > (!obj && oid_object_info(r, oid, NULL)) == OBJ_TREE)) {\n> > \t\treturn &lookup_tree(r, oid)->object;\n> > }\n> > \n> > Am I missing some side effect that `oid_object_info()` provides?\n> \n> Calling oid_object_info() will make sure the on-disk object exists and\n> has the expected type. Keep in mind that an in-memory \"struct object\"\n> may have a type that was just implied by another reference. E.g., if a\n> commit references some object X in its tree field, then we'll call\n> lookup_tree(X) to get a \"struct tree\" without actually touching the odb\n> at all. When it comes time to parse that object, that's when we'll see\n> if we really have it and if it's a tree.\n> \n> In the case of skip_hash (and discard_tree) it might be OK to skip both\n> of those checks. If we do, I think we should probably do the same for\n> blobs (in the skip_hash case, we could just return the object we found\n> already).\n> \n> But I'd definitely prefer to do that as a separate step (if at all).\n\nThanks for the explanation!\n\nPatrick\n"},{"id":"493708","messageId":"ZjAH8kW1UkABsDz3@nand.local","threadId":"61025","inReplyTo":"20240304095907.GB3723539@coredump.intra.peff.net","subject":"Re: [PATCH 6/9] upload-pack: disallow object-info capability by default","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-04-29T20:49:54Z","receivedAt":"2024-04-29T20:49:57Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Mar 04, 2024 at 04:59:07AM -0500, Jeff King wrote:\n> On Mon, Mar 04, 2024 at 09:34:30AM +0100, Patrick Steinhardt wrote:\n>\n> > > +test_expect_success 'object-info missing from capabilities when disabled' '\n> > > +\ttest_config transfer.advertiseObjectInfo false &&\n> > > +\n> > > +\tGIT_TEST_SIDEBAND_ALL=0 test-tool serve-v2 \\\n> > > +\t\t--advertise-capabilities >out &&\n> > > +\ttest-tool pkt-line unpack actual &&\n> > > +\n> > > +\t! grep object.info actual\n> > > +'\n> >\n> > Is it intentional that you grep for \"object.info\" instead of\n> > \"object-info\"?\n>\n> I didn't even notice this. It should be equivalent because of the regex,\n> but I don't think there's a particular reason to be more loose (and\n> unlike single-quote, which we sometimes match with \".\" for shell\n> readability, it should be fine to say \"object-info\" here).\n>\n> +cc Taylor, who wrote the original.\n\nI can't think of any reason why I would have written \"object.info\"\ninstead of \"object-info\". Indeed if the missing character were a \"'\"\nsingle-quote, then I would have used the \".\" wildcard to match it for\nreadability, but that's not the case here.\n\nThanks,\nTaylor\n"}]}