{"thread":{"id":"60175","subject":"[PATCH 0/10] tree name and depth limits","startedAt":"2023-08-31T06:17:38Z","lastAt":"2023-08-31T22:31:15Z","messageCount":16,"participants":["Jeff King","Oswald Buddenhagen","Junio C Hamano","rsbecker@nexbridge.com"],"isPatch":true,"patchVersion":1,"patchTotal":10},"messages":[{"id":"481209","messageId":"20230831061735.GA2702156@coredump.intra.peff.net","threadId":"60175","inReplyTo":null,"subject":"[PATCH 0/10] tree name and depth limits","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-08-31T06:17:35Z","receivedAt":"2023-08-31T06:17:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"There aren't currently any limits on the size of pathnames in tree\nobjects, nor of the depth of recursive trees. This can create two\nproblems:\n\n  1. We process pathnames from trees as strings in lots of places, and\n     we have a history of using \"int\" to access strings. I don't know of\n     a specific problem area, but I have little doubt there is a spot\n     where you can convince an \"int\" to overflow with a 2GB tree.\n\n     In the long run it would be nice to fix all of that code to use\n     size_t. And hopefully other audits to use st_add(), etc, for\n     allocations mean that we'd avoid actual buffer overflows. But in\n     the meantime, it sure would be nice to have another layer\n     protecting us.\n\n  2. Because we recurse for most tree algorithms, it's easy for a\n     malicious tree to run you out of stack space and cause a segfault.\n     By itself this isn't a security problem, but it's confusing for\n     users. And if you happen to run a public hosting site and monitor\n     your segfaults, it's awfully annoying to investigate each one to\n     see if it's the first sign of a real attack, or if somebody is just\n     throwing garbage at you.\n\nSo here are some patches I wrote for GitHub several years ago. They add\nlimits on the size of a single pathname component, as well as on the\ndepth we're willing to recurse when handling trees.\n\nI kind of hate them, because I hate arbitrary limits. But I also think\nthey can do some good in practice, and the default limits are set such\nthat I doubt anybody will ever notice, let alone complain (they have\nbeen in place on github.com for 4+ years, and I don't recall ever\ngetting any feedback from users who were not security researchers poking\nat the system).\n\nAnd most operating systems have arbitrary limits (that are much\nsmaller!) anyway. So if you plan to do exotic things like \"run\ngit-checkout\", you'd run into problems way before you hit either of\nthese.\n\nSo IMHO they'll do more good than harm.\n\nThe first three patches are cleanups / prep.\n\nPatch 4 is the name limit.\n\nPatches 5-10 implement the depth limits.\n\n  [01/10]: tree-walk: reduce stack size for recursive functions\n  [02/10]: tree-walk: drop MAX_TRAVERSE_TREES macro\n  [03/10]: tree-walk: rename \"error\" variable\n  [04/10]: fsck: detect very large tree pathnames\n  [05/10]: add core.maxTreeDepth config\n  [06/10]: traverse_trees(): respect max_allowed_tree_depth\n  [07/10]: read_tree(): respect max_allowed_tree_depth\n  [08/10]: list-objects: respect max_allowed_tree_depth\n  [09/10]: tree-diff: respect max_allowed_tree_depth\n  [10/10]: lower core.maxTreeDepth default to 2048\n\n Documentation/config/core.txt |  6 +++\n Documentation/fsck-msgids.txt |  7 +++\n config.c                      |  5 ++\n environment.c                 |  1 +\n environment.h                 |  1 +\n fsck.c                        | 24 ++++++++-\n fsck.h                        |  1 +\n list-objects.c                |  8 +++\n sparse-index.c                |  2 +-\n t/t1450-fsck.sh               | 10 ++++\n t/t6700-tree-depth.sh         | 93 +++++++++++++++++++++++++++++++++++\n tree-diff.c                   | 23 ++++++---\n tree-walk.c                   | 20 +++++---\n tree-walk.h                   |  2 -\n tree.c                        |  9 +++-\n tree.h                        |  1 +\n unpack-trees.c                |  9 +++-\n unpack-trees.h                |  2 +-\n wt-status.c                   |  2 +-\n 19 files changed, 201 insertions(+), 25 deletions(-)\n create mode 100755 t/t6700-tree-depth.sh\n\n"},{"id":"481210","messageId":"20230831061754.GA3185325@coredump.intra.peff.net","threadId":"60175","inReplyTo":"20230831061735.GA2702156@coredump.intra.peff.net","subject":"[PATCH 01/10] tree-walk: reduce stack size for recursive functions","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-08-31T06:17:54Z","receivedAt":"2023-08-31T06:17:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The traverse_trees() and traverse_trees_recursive() functions call each\nother recursively. In a deep tree, this can result in running out of\nstack space and crashing.\n\nThere's obviously going to be some limit here based on available stack,\nbut the problem is exacerbated by a few large structs, many of which we\nover-allocate. For example, in traverse_trees() we store a name_entry\nand tree_desc_x per tree, both of which contain an object_id (which is\nnow 32 bytes). And we allocate 8 of them (from MAX_TRAVERSE_TREES), even\nthough many traversals will only look at 1 or 2.\n\nInterestingly, we used to allocate these on the heap, prior to\n8dd40c0472 (traverse_trees(): use stack array for name entries,\n2020-01-30). That commit was trying to simplify away allocation size\ncomputations, and naively assumed that the sizes were small enough not\nto matter. And they don't in normal cases, but on my stock Debian system\nI see a crash running \"git archive\" on a tree with ~3600 entries.\nThat's deep enough we wouldn't see it in practice, but probably shallow\nenough that we'd prefer not to make it a hard limit. Especially because\nother systems may have even smaller stacks.\n\nWe can replace these stack variables with a few malloc invocations. This\nreduces the stack sizes for the two functions from 1128 and 752 bytes,\nrespectively, down to 40 and 92 bytes. That allows a depth of ~13000 on\nmy machine (the improvement isn't in linear proportion because my\nnumbers don't count the size of parameters and other function overhead).\n\nThe possible downsides are:\n\n  1. We now have to remember to free(). But both functions have an easy\n     single exit (and already had to clean up other bits anyway).\n\n  2. The extra malloc()/free() overhead might be measurable. I tested\n     this by setting up a 3000-depth tree with a single blob and running\n     \"git archive\" on it. After switching to the heap, it consistently\n     runs 2-3% faster! Presumably this is because the 1K+ of wasted\n     stack space penalized memory caches.\n\nOn a more real-world case like linux.git, the speed difference isn't\nmeasurable at all, simply because most trees aren't that deep and\nthere's so much other work going on (like accessing the objects\nthemselves). So the improvement I saw should be taken as evidence that\nwe're not making anything worse, but isn't really that interesting on\nits own. The main motivation here is that we're now less likely to run\nout of stack space and crash.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n tree-walk.c    | 10 ++++++----\n unpack-trees.c |  9 +++++++--\n 2 files changed, 13 insertions(+), 6 deletions(-)\n\ndiff --git a/tree-walk.c b/tree-walk.c\nindex 29ead71be1..ad49d55290 100644\n--- a/tree-walk.c\n+++ b/tree-walk.c\n@@ -442,9 +442,9 @@ int traverse_trees(struct index_state *istate,\n \t\t   struct traverse_info *info)\n {\n \tint error = 0;\n-\tstruct name_entry entry[MAX_TRAVERSE_TREES];\n+\tstruct name_entry *entry;\n \tint i;\n-\tstruct tree_desc_x tx[ARRAY_SIZE(entry)];\n+\tstruct tree_desc_x *tx;\n \tstruct strbuf base = STRBUF_INIT;\n \tint interesting = 1;\n \tchar *traverse_path;\n@@ -455,8 +455,8 @@ int traverse_trees(struct index_state *istate,\n \tif (traverse_trees_cur_depth > traverse_trees_max_depth)\n \t\ttraverse_trees_max_depth = traverse_trees_cur_depth;\n \n-\tif (n >= ARRAY_SIZE(entry))\n-\t\tBUG(\"traverse_trees() called with too many trees (%d)\", n);\n+\tALLOC_ARRAY(entry, n);\n+\tALLOC_ARRAY(tx, n);\n \n \tfor (i = 0; i < n; i++) {\n \t\ttx[i].d = t[i];\n@@ -551,6 +551,8 @@ int traverse_trees(struct index_state *istate,\n \t}\n \tfor (i = 0; i < n; i++)\n \t\tfree_extended_entry(tx + i);\n+\tfree(tx);\n+\tfree(entry);\n \tfree(traverse_path);\n \tinfo->traverse_path = NULL;\n \tstrbuf_release(&base);\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex 87517364dc..a203f9a3d7 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -864,8 +864,8 @@ static int traverse_trees_recursive(int n, unsigned long dirmask,\n \tstruct unpack_trees_options *o = info->data;\n \tint i, ret, bottom;\n \tint nr_buf = 0;\n-\tstruct tree_desc t[MAX_UNPACK_TREES];\n-\tvoid *buf[MAX_UNPACK_TREES];\n+\tstruct tree_desc *t;\n+\tvoid **buf;\n \tstruct traverse_info newinfo;\n \tstruct name_entry *p;\n \tint nr_entries;\n@@ -902,6 +902,9 @@ static int traverse_trees_recursive(int n, unsigned long dirmask,\n \tnewinfo.pathlen = st_add3(newinfo.pathlen, tree_entry_len(p), 1);\n \tnewinfo.df_conflicts |= df_conflicts;\n \n+\tALLOC_ARRAY(t, n);\n+\tALLOC_ARRAY(buf, n);\n+\n \t/*\n \t * Fetch the tree from the ODB for each peer directory in the\n \t * n commits.\n@@ -937,6 +940,8 @@ static int traverse_trees_recursive(int n, unsigned long dirmask,\n \n \tfor (i = 0; i < nr_buf; i++)\n \t\tfree(buf[i]);\n+\tfree(buf);\n+\tfree(t);\n \n \treturn ret;\n }\n-- \n2.42.0.561.gaa987ecc69\n\n"},{"id":"481211","messageId":"20230831061916.GB3185325@coredump.intra.peff.net","threadId":"60175","inReplyTo":"20230831061735.GA2702156@coredump.intra.peff.net","subject":"[PATCH 02/10] tree-walk: drop MAX_TRAVERSE_TREES macro","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-08-31T06:19:16Z","receivedAt":"2023-08-31T06:19:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Since the previous commit dropped the hard-coded limit in\ntraverse_trees(), we don't need this macro there anymore (the code can\nhandle any number of trees in parallel).\n\nWe do define MAX_UNPACK_TREES using MAX_TRAVERSE_TREES, due to\n5290d45134 (tree-walk.c: break circular dependency with unpack-trees,\n2020-02-01). So we can just directly define that as \"8\" now; we know\ntraverse_trees() can handle whatever we throw at it.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI think from previous discussions that this \"8\" may also be excessive.\nBut digging into that should definitely be left for another series.\n\n tree-walk.h    | 2 --\n unpack-trees.h | 2 +-\n 2 files changed, 1 insertion(+), 3 deletions(-)\n\ndiff --git a/tree-walk.h b/tree-walk.h\nindex 74cdceb3fe..a6bfa3da3a 100644\n--- a/tree-walk.h\n+++ b/tree-walk.h\n@@ -6,8 +6,6 @@\n struct index_state;\n struct repository;\n \n-#define MAX_TRAVERSE_TREES 8\n-\n /**\n  * The tree walking API is used to traverse and inspect trees.\n  */\ndiff --git a/unpack-trees.h b/unpack-trees.h\nindex 9b827c307f..5867e26e17 100644\n--- a/unpack-trees.h\n+++ b/unpack-trees.h\n@@ -7,7 +7,7 @@\n #include \"string-list.h\"\n #include \"tree-walk.h\"\n \n-#define MAX_UNPACK_TREES MAX_TRAVERSE_TREES\n+#define MAX_UNPACK_TREES 8\n \n struct cache_entry;\n struct unpack_trees_options;\n-- \n2.42.0.561.gaa987ecc69\n\n"},{"id":"481212","messageId":"20230831061922.GC3185325@coredump.intra.peff.net","threadId":"60175","inReplyTo":"20230831061735.GA2702156@coredump.intra.peff.net","subject":"[PATCH 03/10] tree-walk: rename \"error\" variable","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-08-31T06:19:22Z","receivedAt":"2023-08-31T06:19:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The \"error\" variable in traverse_trees() shadows the global error()\nfunction (meaning we can't call error() from here). Let's call the local\nvariable \"ret\" instead, which matches the idiom in other functions.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n tree-walk.c | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/tree-walk.c b/tree-walk.c\nindex ad49d55290..4efd0fc391 100644\n--- a/tree-walk.c\n+++ b/tree-walk.c\n@@ -441,7 +441,7 @@ int traverse_trees(struct index_state *istate,\n \t\t   int n, struct tree_desc *t,\n \t\t   struct traverse_info *info)\n {\n-\tint error = 0;\n+\tint ret = 0;\n \tstruct name_entry *entry;\n \tint i;\n \tstruct tree_desc_x *tx;\n@@ -539,7 +539,7 @@ int traverse_trees(struct index_state *istate,\n \t\tif (interesting) {\n \t\t\ttrees_used = info->fn(n, mask, dirmask, entry, info);\n \t\t\tif (trees_used < 0) {\n-\t\t\t\terror = trees_used;\n+\t\t\t\tret = trees_used;\n \t\t\t\tif (!info->show_all_errors)\n \t\t\t\t\tbreak;\n \t\t\t}\n@@ -558,7 +558,7 @@ int traverse_trees(struct index_state *istate,\n \tstrbuf_release(&base);\n \n \ttraverse_trees_cur_depth--;\n-\treturn error;\n+\treturn ret;\n }\n \n struct dir_state {\n-- \n2.42.0.561.gaa987ecc69\n\n"},{"id":"481213","messageId":"20230831062001.GD3185325@coredump.intra.peff.net","threadId":"60175","inReplyTo":"20230831061735.GA2702156@coredump.intra.peff.net","subject":"[PATCH 04/10] fsck: detect very large tree pathnames","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-08-31T06:20:01Z","receivedAt":"2023-08-31T06:20:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"In general, Git tries not to arbitrarily limit what it will store, and\nthere are currently no limits at all on the size of the path we find in\na tree. In theory you could have one that is gigabytes long.\n\nBut in practice this freedom is not really helping anybody, and is\npotentially harmful:\n\n  1. Most operating systems have much lower limits for the size of a\n     single pathname component (e.g., on Linux you'll generally get\n     ENAMETOOLONG for anything over 255 bytes). And while you _can_ use\n     Git in a way that never touches the filesystem (manipulating the\n     index and trees directly), it's still probably not a good idea to\n     have gigantic tree names. Many operations load and traverse them,\n     so any clever Git-as-a-database scheme is likely to perform poorly\n     in that case.\n\n  2. We still have a lot of code which assumes strings are reasonably\n     sized, and I won't be at all surprised if you can trigger some\n     interesting integer overflows with gigantic pathnames. Stopping\n     malicious trees from entering the repository provides an extra line\n     of defense, protecting downstream code.\n\nThis patch implements an fsck check so that such trees can be rejected\nby transfer.fsckObjects. I've picked a reasonably high maximum depth\nhere (4096) that hopefully should not bother anybody in practice. I've\nalso made it configurable, as an escape hatch.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n Documentation/fsck-msgids.txt |  7 +++++++\n fsck.c                        | 24 +++++++++++++++++++++++-\n fsck.h                        |  1 +\n t/t1450-fsck.sh               | 10 ++++++++++\n 4 files changed, 41 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/fsck-msgids.txt b/Documentation/fsck-msgids.txt\nindex 12eae8a222..09b0aecbf8 100644\n--- a/Documentation/fsck-msgids.txt\n+++ b/Documentation/fsck-msgids.txt\n@@ -103,6 +103,13 @@\n `hasDotgit`::\n \t(WARN) A tree contains an entry named `.git`.\n \n+`largePathname`::\n+\t(WARN) A tree contains an entry with a very long path name. If\n+\tthe value of `fsck.largePathname` contains a colon, that value\n+\tis used as the maximum allowable length (e.g., \"warn:10\" would\n+\tcomplain about any path component of 11 or more bytes). The\n+\tdefault value is 4096.\n+\n `mailmapSymlink`::\n \t(INFO) `.mailmap` is a symlink.\n \ndiff --git a/fsck.c b/fsck.c\nindex 2b1e348005..6a0bbc5087 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -24,6 +24,8 @@\n #include \"credential.h\"\n #include \"help.h\"\n \n+static ssize_t max_tree_entry_len = 4096;\n+\n #define STR(x) #x\n #define MSG_ID(id, msg_type) { STR(id), NULL, NULL, FSCK_##msg_type },\n static struct {\n@@ -154,15 +156,29 @@ void fsck_set_msg_type(struct fsck_options *options,\n \t\t       const char *msg_id_str, const char *msg_type_str)\n {\n \tint msg_id = parse_msg_id(msg_id_str);\n-\tenum fsck_msg_type msg_type = parse_msg_type(msg_type_str);\n+\tchar *to_free = NULL;\n+\tenum fsck_msg_type msg_type;\n \n \tif (msg_id < 0)\n \t\tdie(\"Unhandled message id: %s\", msg_id_str);\n \n+\tif (msg_id == FSCK_MSG_LARGE_PATHNAME) {\n+\t\tconst char *colon = strchr(msg_type_str, ':');\n+\t\tif (colon) {\n+\t\t\tmsg_type_str = to_free =\n+\t\t\t\txmemdupz(msg_type_str, colon - msg_type_str);\n+\t\t\tcolon++;\n+\t\t\tif (!git_parse_ssize_t(colon, &max_tree_entry_len))\n+\t\t\t\tdie(\"unable to parse max tree entry len: %s\", colon);\n+\t\t}\n+\t}\n+\tmsg_type = parse_msg_type(msg_type_str);\n+\n \tif (msg_type != FSCK_ERROR && msg_id_info[msg_id].msg_type == FSCK_FATAL)\n \t\tdie(\"Cannot demote %s to %s\", msg_id_str, msg_type_str);\n \n \tfsck_set_msg_type_from_ids(options, msg_id, msg_type);\n+\tfree(to_free);\n }\n \n void fsck_set_msg_types(struct fsck_options *options, const char *values)\n@@ -578,6 +594,7 @@ static int fsck_tree(const struct object_id *tree_oid,\n \tint has_bad_modes = 0;\n \tint has_dup_entries = 0;\n \tint not_properly_sorted = 0;\n+\tint has_large_name = 0;\n \tstruct tree_desc desc;\n \tunsigned o_mode;\n \tconst char *o_name;\n@@ -607,6 +624,7 @@ static int fsck_tree(const struct object_id *tree_oid,\n \t\thas_dotdot |= !strcmp(name, \"..\");\n \t\thas_dotgit |= is_hfs_dotgit(name) || is_ntfs_dotgit(name);\n \t\thas_zero_pad |= *(char *)desc.buffer == '0';\n+\t\thas_large_name |= tree_entry_len(&desc.entry) > max_tree_entry_len;\n \n \t\tif (is_hfs_dotgitmodules(name) || is_ntfs_dotgitmodules(name)) {\n \t\t\tif (!S_ISLNK(mode))\n@@ -749,6 +767,10 @@ static int fsck_tree(const struct object_id *tree_oid,\n \t\tretval += report(options, tree_oid, OBJ_TREE,\n \t\t\t\t FSCK_MSG_TREE_NOT_SORTED,\n \t\t\t\t \"not properly sorted\");\n+\tif (has_large_name)\n+\t\tretval += report(options, tree_oid, OBJ_TREE,\n+\t\t\t\t FSCK_MSG_LARGE_PATHNAME,\n+\t\t\t\t \"contains excessively large pathname\");\n \treturn retval;\n }\n \ndiff --git a/fsck.h b/fsck.h\nindex 6359ba359b..e3adf9d911 100644\n--- a/fsck.h\n+++ b/fsck.h\n@@ -73,6 +73,7 @@ enum fsck_msg_type {\n \tFUNC(NULL_SHA1, WARN) \\\n \tFUNC(ZERO_PADDED_FILEMODE, WARN) \\\n \tFUNC(NUL_IN_COMMIT, WARN) \\\n+\tFUNC(LARGE_PATHNAME, WARN) \\\n \t/* infos (reported as warnings, but ignored by default) */ \\\n \tFUNC(BAD_FILEMODE, INFO) \\\n \tFUNC(GITMODULES_PARSE, INFO) \\\ndiff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh\nindex 5805d47eb9..10a539158c 100755\n--- a/t/t1450-fsck.sh\n+++ b/t/t1450-fsck.sh\n@@ -589,6 +589,16 @@ test_expect_success 'fsck notices submodule entry pointing to null sha1' '\n \t)\n '\n \n+test_expect_success 'fsck notices excessively large tree entry name' '\n+\tgit init large-name &&\n+\t(\n+\t\tcd large-name &&\n+\t\ttest_commit a-long-name &&\n+\t\tgit -c fsck.largePathname=warn:10 fsck 2>out &&\n+\t\tgrep \"warning.*large pathname\" out\n+\t)\n+'\n+\n while read name path pretty; do\n \twhile read mode type; do\n \t\t: ${pretty:=$path}\n-- \n2.42.0.561.gaa987ecc69\n\n"},{"id":"481214","messageId":"20230831062100.GE3185325@coredump.intra.peff.net","threadId":"60175","inReplyTo":"20230831061735.GA2702156@coredump.intra.peff.net","subject":"[PATCH 05/10] add core.maxTreeDepth config","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-08-31T06:21:00Z","receivedAt":"2023-08-31T06:21:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Most of our tree traversal algorithms use recursion to visit sub-trees.\nFor pathologically large trees, this can cause us to run out of stack\nspace and abort in an uncontrolled way. Let's put our own limit here so\nthat we can fail gracefully rather than segfaulting.\n\nIn similar cases where we recursed along the commit graph, we rewrote\nthe algorithms to avoid recursion and keep any stack data on the heap.\nBut the commit graph is meant to grow without bound, whereas it's not an\nimposition to put a limit on the maximum size of tree we'll handle.\n\nAnd this has a bonus side effect: coupled with a limit on individual\ntree entry names, this limits the total size of a path we may encounter.\nThis gives us an extra protection against code handling long path names\nwhich may suffer from integer overflows in the size (which could then be\nexploited by malicious trees).\n\nThe default of 4096 is set to be much longer than anybody would care\nabout in the real world. Even with single-letter interior tree names\n(like \"a/b/c\"), such a path is at least 8191 bytes. While most operating\nsystems will let you create such a path incrementally, trying to\nreference the whole thing in a system call (as Git would do when\nactually trying to access it) will result in ENAMETOOLONG. Coupled with\nthe recent fsck.largePathname warning, the maximum total pathname Git\nwill handle is (by default) 16MB.\n\nThis config option doesn't do anything yet; future patches will convert\nvarious algorithms to respect the limit.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n Documentation/config/core.txt | 6 ++++++\n config.c                      | 5 +++++\n environment.c                 | 1 +\n environment.h                 | 1 +\n 4 files changed, 13 insertions(+)\n\ndiff --git a/Documentation/config/core.txt b/Documentation/config/core.txt\nindex dfbdaf00b8..0e8c2832bf 100644\n--- a/Documentation/config/core.txt\n+++ b/Documentation/config/core.txt\n@@ -736,3 +736,9 @@ core.abbrev::\n \tIf set to \"no\", no abbreviation is made and the object names\n \tare shown in their full length.\n \tThe minimum length is 4.\n+\n+core.maxTreeDepth::\n+\tThe maximum depth Git is willing to recurse while traversing a\n+\ttree (e.g., \"a/b/cde/f\" has a depth of 4). This is a fail-safe\n+\tto allow Git to abort cleanly, and should not generally need to\n+\tbe adjusted. The default is 4096.\ndiff --git a/config.c b/config.c\nindex 3846a37be9..2ea39295cd 100644\n--- a/config.c\n+++ b/config.c\n@@ -1801,6 +1801,11 @@ static int git_default_core_config(const char *var, const char *value,\n \t\treturn 0;\n \t}\n \n+\tif (!strcmp(var, \"core.maxtreedepth\")) {\n+\t\tmax_allowed_tree_depth = git_config_int(var, value, ctx->kvi);\n+\t\treturn 0;\n+\t}\n+\n \t/* Add other config variables here and to Documentation/config.txt. */\n \treturn platform_core_config(var, value, ctx, cb);\n }\ndiff --git a/environment.c b/environment.c\nindex f98d76f080..8e25b5ef02 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -81,6 +81,7 @@ int merge_log_config = -1;\n int precomposed_unicode = -1; /* see probe_utf8_pathname_composition() */\n unsigned long pack_size_limit_cfg;\n enum log_refs_config log_all_ref_updates = LOG_REFS_UNSET;\n+int max_allowed_tree_depth = 4096;\n \n #ifndef PROTECT_HFS_DEFAULT\n #define PROTECT_HFS_DEFAULT 0\ndiff --git a/environment.h b/environment.h\nindex c5377473c6..e5351c9dd9 100644\n--- a/environment.h\n+++ b/environment.h\n@@ -132,6 +132,7 @@ extern size_t packed_git_limit;\n extern size_t delta_base_cache_limit;\n extern unsigned long big_file_threshold;\n extern unsigned long pack_size_limit_cfg;\n+extern int max_allowed_tree_depth;\n \n /*\n  * Accessors for the core.sharedrepository config which lazy-load the value\n-- \n2.42.0.561.gaa987ecc69\n\n"},{"id":"481215","messageId":"20230831062140.GF3185325@coredump.intra.peff.net","threadId":"60175","inReplyTo":"20230831061735.GA2702156@coredump.intra.peff.net","subject":"[PATCH 06/10] traverse_trees(): respect max_allowed_tree_depth","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-08-31T06:21:40Z","receivedAt":"2023-08-31T06:22:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The tree-walk.c code walks trees recursively, and may run out of stack\nspace. The easiest way to see this is with git-archive; on my 64-bit\nLinux system it runs out of stack trying to generate a tarfile with a\ntree depth of 13,772.\n\nI've picked 4100 as the depth for our \"big\" test. I ran it with a much\nhigher value to confirm that we do get a segfault without this patch.\nBut really anything over 4096 is sufficient for its stated purpose,\nwhich is to find out if our default limit of 4096 is low enough to\nprevent segfaults on all platforms. Keeping it small saves us time on\nthe test setup.\n\nThe tree-walk code that's touched here underlies unpack_trees(), so this\nprotects any programs which use it, not just git-archive (but archive is\neasy to test, and was what alerted me to this issue in a real-world\ncase).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n t/t6700-tree-depth.sh | 66 +++++++++++++++++++++++++++++++++++++++++++\n tree-walk.c           |  4 +++\n 2 files changed, 70 insertions(+)\n create mode 100755 t/t6700-tree-depth.sh\n\ndiff --git a/t/t6700-tree-depth.sh b/t/t6700-tree-depth.sh\nnew file mode 100755\nindex 0000000000..d4d17db2ae\n--- /dev/null\n+++ b/t/t6700-tree-depth.sh\n@@ -0,0 +1,66 @@\n+#!/bin/sh\n+\n+test_description='handling of deep trees in various commands'\n+. ./test-lib.sh\n+\n+# We'll test against two depths here: a small one that will let us check the\n+# behavior of the config setting easily, and a large one that should be\n+# forbidden by default. Testing the default depth will let us know whether our\n+# default is enough to prevent segfaults on systems that run the tests.\n+small_depth=50\n+big_depth=4100\n+\n+small_ok=\"-c core.maxtreedepth=$small_depth\"\n+small_no=\"-c core.maxtreedepth=$((small_depth-1))\"\n+\n+# usage: mkdeep <name> <depth>\n+#   Create a tag <name> containing a file whose path has depth <depth>.\n+#\n+# We'll use fast-import here for two reasons:\n+#\n+#   1. It's faster than creating $big_depth tree objects.\n+#\n+#   2. As we tighten tree limits, it's more likely to allow large sizes\n+#      than trying to stuff a deep path into the index.\n+mkdeep () {\n+\t{\n+\t\techo \"commit refs/tags/$1\" &&\n+\t\techo \"committer foo <foo@example.com> 1234 -0000\" &&\n+\t\techo \"data <<EOF\" &&\n+\t\techo \"the commit message\" &&\n+\t\techo \"EOF\" &&\n+\n+\t\tprintf 'M 100644 inline ' &&\n+\t\ti=0 &&\n+\t\twhile test $i -lt $2\n+\t\tdo\n+\t\t\tprintf 'a/'\n+\t\t\ti=$((i+1))\n+\t\tdone &&\n+\t\techo \"file\" &&\n+\n+\t\techo \"data <<EOF\" &&\n+\t\techo \"the file contents\" &&\n+\t\techo \"EOF\" &&\n+\t\techo\n+\t} | git fast-import\n+}\n+\n+test_expect_success 'create small tree' '\n+\tmkdeep small $small_depth\n+'\n+\n+test_expect_success 'create big tree' '\n+\tmkdeep big $big_depth\n+'\n+\n+test_expect_success 'limit recursion of git-archive' '\n+\tgit $small_ok archive small >/dev/null &&\n+\ttest_must_fail git $small_no archive small >/dev/null\n+'\n+\n+test_expect_success 'default limit for git-archive fails gracefully' '\n+\ttest_must_fail git archive big >/dev/null\n+'\n+\n+test_done\ndiff --git a/tree-walk.c b/tree-walk.c\nindex 4efd0fc391..b517792ba2 100644\n--- a/tree-walk.c\n+++ b/tree-walk.c\n@@ -9,6 +9,7 @@\n #include \"tree.h\"\n #include \"pathspec.h\"\n #include \"json-writer.h\"\n+#include \"environment.h\"\n \n static const char *get_mode(const char *str, unsigned int *modep)\n {\n@@ -449,6 +450,9 @@ int traverse_trees(struct index_state *istate,\n \tint interesting = 1;\n \tchar *traverse_path;\n \n+\tif (traverse_trees_cur_depth > max_allowed_tree_depth)\n+\t\treturn error(\"exceeded maximum allowed tree depth\");\n+\n \ttraverse_trees_count++;\n \ttraverse_trees_cur_depth++;\n \n-- \n2.42.0.561.gaa987ecc69\n\n"},{"id":"481216","messageId":"20230831062155.GG3185325@coredump.intra.peff.net","threadId":"60175","inReplyTo":"20230831061735.GA2702156@coredump.intra.peff.net","subject":"[PATCH 07/10] read_tree(): respect max_allowed_tree_depth","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-08-31T06:21:55Z","receivedAt":"2023-08-31T06:22:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The read_tree() function reads trees recursively (via its read_tree_at()\nhelper). This can cause it to run out of stack space on very deep trees.\nLet's teach it about the new core.maxTreeDepth option.\n\nThe easiest way to demonstrate this is via \"ls-tree -r\", which the test\ncovers. Note that I needed a tree depth of ~30k to trigger a segfault on\nmy Linux system, not the 4100 used by our \"big\" test in t6700. However,\nthat test still tells us what we want: that the default 4096 limit is\nenough to prevent segfaults on all platforms. We could bump it, but that\nincreases the cost of the test setup for little gain.\n\nAs an interesting side-note: when I originally wrote this patch about 4\nyears ago, I needed a depth of ~50k to segfault. But porting it forward,\nthe number is much lower. Seemingly little things like cf0983213c (hash:\nadd an algo member to struct object_id, 2021-04-26) take it from 32,722\nto 29,080.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n sparse-index.c        | 2 +-\n t/t6700-tree-depth.sh | 9 +++++++++\n tree.c                | 9 +++++++--\n tree.h                | 1 +\n wt-status.c           | 2 +-\n 5 files changed, 19 insertions(+), 4 deletions(-)\n\ndiff --git a/sparse-index.c b/sparse-index.c\nindex 1fdb07a9e6..3578feb283 100644\n--- a/sparse-index.c\n+++ b/sparse-index.c\n@@ -391,7 +391,7 @@ void expand_index(struct index_state *istate, struct pattern_list *pl)\n \t\tstrbuf_setlen(&base, 0);\n \t\tstrbuf_add(&base, ce->name, strlen(ce->name));\n \n-\t\tread_tree_at(istate->repo, tree, &base, &ps,\n+\t\tread_tree_at(istate->repo, tree, &base, 0, &ps,\n \t\t\t     add_path_to_index, &ctx);\n \n \t\t/* free directory entries. full entries are re-used */\ndiff --git a/t/t6700-tree-depth.sh b/t/t6700-tree-depth.sh\nindex d4d17db2ae..93ec41df03 100755\n--- a/t/t6700-tree-depth.sh\n+++ b/t/t6700-tree-depth.sh\n@@ -63,4 +63,13 @@ test_expect_success 'default limit for git-archive fails gracefully' '\n \ttest_must_fail git archive big >/dev/null\n '\n \n+test_expect_success 'limit recursion of ls-tree -r' '\n+\tgit $small_ok ls-tree -r small &&\n+\ttest_must_fail git $small_no ls-tree -r small\n+'\n+\n+test_expect_success 'default limit for ls-tree fails gracefully' '\n+\ttest_must_fail git ls-tree -r big >/dev/null\n+'\n+\n test_done\ndiff --git a/tree.c b/tree.c\nindex c745462f96..990f9c9854 100644\n--- a/tree.c\n+++ b/tree.c\n@@ -10,11 +10,13 @@\n #include \"alloc.h\"\n #include \"tree-walk.h\"\n #include \"repository.h\"\n+#include \"environment.h\"\n \n const char *tree_type = \"tree\";\n \n int read_tree_at(struct repository *r,\n \t\t struct tree *tree, struct strbuf *base,\n+\t\t int depth,\n \t\t const struct pathspec *pathspec,\n \t\t read_tree_fn_t fn, void *context)\n {\n@@ -24,6 +26,9 @@ int read_tree_at(struct repository *r,\n \tint len, oldlen = base->len;\n \tenum interesting retval = entry_not_interesting;\n \n+\tif (depth > max_allowed_tree_depth)\n+\t\treturn error(\"exceeded maximum allowed tree depth\");\n+\n \tif (parse_tree(tree))\n \t\treturn -1;\n \n@@ -74,7 +79,7 @@ int read_tree_at(struct repository *r,\n \t\tstrbuf_add(base, entry.path, len);\n \t\tstrbuf_addch(base, '/');\n \t\tretval = read_tree_at(r, lookup_tree(r, &oid),\n-\t\t\t\t      base, pathspec,\n+\t\t\t\t      base, depth + 1, pathspec,\n \t\t\t\t      fn, context);\n \t\tstrbuf_setlen(base, oldlen);\n \t\tif (retval)\n@@ -89,7 +94,7 @@ int read_tree(struct repository *r,\n \t      read_tree_fn_t fn, void *context)\n {\n \tstruct strbuf sb = STRBUF_INIT;\n-\tint ret = read_tree_at(r, tree, &sb, pathspec, fn, context);\n+\tint ret = read_tree_at(r, tree, &sb, 0, pathspec, fn, context);\n \tstrbuf_release(&sb);\n \treturn ret;\n }\ndiff --git a/tree.h b/tree.h\nindex 1b5ecbda6b..cc6ddf51b3 100644\n--- a/tree.h\n+++ b/tree.h\n@@ -44,6 +44,7 @@ typedef int (*read_tree_fn_t)(const struct object_id *, struct strbuf *, const c\n \n int read_tree_at(struct repository *r,\n \t\t struct tree *tree, struct strbuf *base,\n+\t\t int depth,\n \t\t const struct pathspec *pathspec,\n \t\t read_tree_fn_t fn, void *context);\n \ndiff --git a/wt-status.c b/wt-status.c\nindex 5b1378965c..996f8635c3 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -739,7 +739,7 @@ static void wt_status_collect_changes_initial(struct wt_status *s)\n \t\t\tps.max_depth = -1;\n \n \t\t\tstrbuf_add(&base, ce->name, ce->ce_namelen);\n-\t\t\tread_tree_at(istate->repo, tree, &base, &ps,\n+\t\t\tread_tree_at(istate->repo, tree, &base, 0, &ps,\n \t\t\t\t     add_file_to_list, s);\n \t\t\tcontinue;\n \t\t}\n-- \n2.42.0.561.gaa987ecc69\n\n"},{"id":"481217","messageId":"20230831062203.GH3185325@coredump.intra.peff.net","threadId":"60175","inReplyTo":"20230831061735.GA2702156@coredump.intra.peff.net","subject":"[PATCH 08/10] list-objects: respect max_allowed_tree_depth","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-08-31T06:22:03Z","receivedAt":"2023-08-31T06:23:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The tree traversal in list-objects.c, which is used by \"rev-list\n--objects\", etc, uses recursion and may run out of stack space. Let's\nteach it about the new core.maxTreeDepth config option.\n\nWe unfortunately can't return an error here, as this code doesn't\nproduce an error return at all. We'll die() instead, which matches the\nbehavior when we see an otherwise broken tree.\n\nNote that this will also generally reject such deep trees from entering\nthe repository from a fetch or push, due to the use of rev-list in the\nconnectivity check. But it's not foolproof! We stop traversing when we\nsee an UNINTERESTING object, and the connectivity check marks existing\nref tips as UNINTERESTING. So imagine commit X has a tree\nwith maximum depth N. If you then create a new commit Y with a tree\nentry \"Y:subdir\" that points to \"X^{tree}\", then the depth of Y will be\nN+1. But a connectivity check running \"git rev-list --objects Y --not X\"\nwon't realize that; it will stop traversing at X^{tree}, since that was\nalready reachable.\n\nSo this will stop naive pushes of too-deep trees, but not carefully\ncrafted malicious ones. Doing it robustly and efficiently would require\ncaching the maximum depth of each tree (i.e., the longest path to any\nleaf entry). That's much more complex and not strictly needed. If each\nrecursive algorithm limits itself already, then that's sufficient.\nBlocking the objects from entering the repo would be a nice\nbelt-and-suspenders addition, but it's not worth the extra cost.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n list-objects.c        | 8 ++++++++\n t/t6700-tree-depth.sh | 9 +++++++++\n 2 files changed, 17 insertions(+)\n\ndiff --git a/list-objects.c b/list-objects.c\nindex e60a6cd5b4..c25c72b32c 100644\n--- a/list-objects.c\n+++ b/list-objects.c\n@@ -14,13 +14,15 @@\n #include \"packfile.h\"\n #include \"object-store-ll.h\"\n #include \"trace.h\"\n+#include \"environment.h\"\n \n struct traversal_context {\n \tstruct rev_info *revs;\n \tshow_object_fn show_object;\n \tshow_commit_fn show_commit;\n \tvoid *show_data;\n \tstruct filter *filter;\n+\tint depth;\n };\n \n static void show_commit(struct traversal_context *ctx,\n@@ -118,7 +120,9 @@ static void process_tree_contents(struct traversal_context *ctx,\n \t\t\t\t    entry.path, oid_to_hex(&tree->object.oid));\n \t\t\t}\n \t\t\tt->object.flags |= NOT_USER_GIVEN;\n+\t\t\tctx->depth++;\n \t\t\tprocess_tree(ctx, t, base, entry.path);\n+\t\t\tctx->depth--;\n \t\t}\n \t\telse if (S_ISGITLINK(entry.mode))\n \t\t\t; /* ignore gitlink */\n@@ -156,6 +160,9 @@ static void process_tree(struct traversal_context *ctx,\n \t    !revs->include_check_obj(&tree->object, revs->include_check_data))\n \t\treturn;\n \n+\tif (ctx->depth > max_allowed_tree_depth)\n+\t\tdie(\"exceeded maximum allowed tree depth\");\n+\n \tfailed_parse = parse_tree_gently(tree, 1);\n \tif (failed_parse) {\n \t\tif (revs->ignore_missing_links)\n@@ -349,6 +356,7 @@ static void traverse_non_commits(struct traversal_context *ctx,\n \t\tif (!path)\n \t\t\tpath = \"\";\n \t\tif (obj->type == OBJ_TREE) {\n+\t\t\tctx->depth = 0;\n \t\t\tprocess_tree(ctx, (struct tree *)obj, base, path);\n \t\t\tcontinue;\n \t\t}\ndiff --git a/t/t6700-tree-depth.sh b/t/t6700-tree-depth.sh\nindex 93ec41df03..f5d284b16e 100755\n--- a/t/t6700-tree-depth.sh\n+++ b/t/t6700-tree-depth.sh\n@@ -72,4 +72,13 @@ test_expect_success 'default limit for ls-tree fails gracefully' '\n \ttest_must_fail git ls-tree -r big >/dev/null\n '\n \n+test_expect_success 'limit recursion of rev-list --objects' '\n+\tgit $small_ok rev-list --objects small >/dev/null &&\n+\ttest_must_fail git $small_no rev-list --objects small >/dev/null\n+'\n+\n+test_expect_success 'default limit for rev-list fails gracefully' '\n+\ttest_must_fail git rev-list --objects big >/dev/null\n+'\n+\n test_done\n-- \n2.42.0.561.gaa987ecc69\n\n"},{"id":"481218","messageId":"20230831062208.GI3185325@coredump.intra.peff.net","threadId":"60175","inReplyTo":"20230831061735.GA2702156@coredump.intra.peff.net","subject":"[PATCH 09/10] tree-diff: respect max_allowed_tree_depth","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-08-31T06:22:08Z","receivedAt":"2023-08-31T06:23:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When diffing trees, we recurse to handle subtrees. That means we may run\nout of stack space and segfault. Let's teach this code path about\ncore.maxTreeDepth in order to fail more gracefully.\n\nAs with the previous patch, we have no way to return an error (and other\ntree-loading problems would just cause us to die()). So we'll likewise\ncall die() if we exceed the maximum depth.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n t/t6700-tree-depth.sh |  9 +++++++++\n tree-diff.c           | 23 +++++++++++++++--------\n 2 files changed, 24 insertions(+), 8 deletions(-)\n\ndiff --git a/t/t6700-tree-depth.sh b/t/t6700-tree-depth.sh\nindex f5d284b16e..e410c41234 100755\n--- a/t/t6700-tree-depth.sh\n+++ b/t/t6700-tree-depth.sh\n@@ -81,4 +81,13 @@ test_expect_success 'default limit for rev-list fails gracefully' '\n \ttest_must_fail git rev-list --objects big >/dev/null\n '\n \n+test_expect_success 'limit recursion of diff-tree -r' '\n+\tgit $small_ok diff-tree -r $EMPTY_TREE small &&\n+\ttest_must_fail git $small_no diff-tree -r $EMPTY_TREE small\n+'\n+\n+test_expect_success 'default limit for diff-tree fails gracefully' '\n+\ttest_must_fail git diff-tree -r $EMPTY_TREE big\n+'\n+\n test_done\ndiff --git a/tree-diff.c b/tree-diff.c\nindex 8fc159b86e..46107772d1 100644\n--- a/tree-diff.c\n+++ b/tree-diff.c\n@@ -7,6 +7,7 @@\n #include \"hash.h\"\n #include \"tree.h\"\n #include \"tree-walk.h\"\n+#include \"environment.h\"\n \n /*\n  * Some mode bits are also used internally for computations.\n@@ -45,7 +46,8 @@\n static struct combine_diff_path *ll_diff_tree_paths(\n \tstruct combine_diff_path *p, const struct object_id *oid,\n \tconst struct object_id **parents_oid, int nparent,\n-\tstruct strbuf *base, struct diff_options *opt);\n+\tstruct strbuf *base, struct diff_options *opt,\n+\tint depth);\n static void ll_diff_tree_oid(const struct object_id *old_oid,\n \t\t\t     const struct object_id *new_oid,\n \t\t\t     struct strbuf *base, struct diff_options *opt);\n@@ -196,7 +198,7 @@ static struct combine_diff_path *path_appendnew(struct combine_diff_path *last,\n static struct combine_diff_path *emit_path(struct combine_diff_path *p,\n \tstruct strbuf *base, struct diff_options *opt, int nparent,\n \tstruct tree_desc *t, struct tree_desc *tp,\n-\tint imin)\n+\tint imin, int depth)\n {\n \tunsigned short mode;\n \tconst char *path;\n@@ -302,7 +304,8 @@ static struct combine_diff_path *emit_path(struct combine_diff_path *p,\n \n \t\tstrbuf_add(base, path, pathlen);\n \t\tstrbuf_addch(base, '/');\n-\t\tp = ll_diff_tree_paths(p, oid, parents_oid, nparent, base, opt);\n+\t\tp = ll_diff_tree_paths(p, oid, parents_oid, nparent, base, opt,\n+\t\t\t\t       depth + 1);\n \t\tFAST_ARRAY_FREE(parents_oid, nparent);\n \t}\n \n@@ -423,12 +426,16 @@ static inline void update_tp_entries(struct tree_desc *tp, int nparent)\n static struct combine_diff_path *ll_diff_tree_paths(\n \tstruct combine_diff_path *p, const struct object_id *oid,\n \tconst struct object_id **parents_oid, int nparent,\n-\tstruct strbuf *base, struct diff_options *opt)\n+\tstruct strbuf *base, struct diff_options *opt,\n+\tint depth)\n {\n \tstruct tree_desc t, *tp;\n \tvoid *ttree, **tptree;\n \tint i;\n \n+\tif (depth > max_allowed_tree_depth)\n+\t\tdie(\"exceeded maximum allowed tree depth\");\n+\n \tFAST_ARRAY_ALLOC(tp, nparent);\n \tFAST_ARRAY_ALLOC(tptree, nparent);\n \n@@ -522,7 +529,7 @@ static struct combine_diff_path *ll_diff_tree_paths(\n \n \t\t\t/* D += {δ(t,pi) if pi=p[imin];  \"+a\" if pi > p[imin]} */\n \t\t\tp = emit_path(p, base, opt, nparent,\n-\t\t\t\t\t&t, tp, imin);\n+\t\t\t\t\t&t, tp, imin, depth);\n \n \t\tskip_emit_t_tp:\n \t\t\t/* t↓,  ∀ pi=p[imin]  pi↓ */\n@@ -534,7 +541,7 @@ static struct combine_diff_path *ll_diff_tree_paths(\n \t\telse if (cmp < 0) {\n \t\t\t/* D += \"+t\" */\n \t\t\tp = emit_path(p, base, opt, nparent,\n-\t\t\t\t\t&t, /*tp=*/NULL, -1);\n+\t\t\t\t\t&t, /*tp=*/NULL, -1, depth);\n \n \t\t\t/* t↓ */\n \t\t\tupdate_tree_entry(&t);\n@@ -550,7 +557,7 @@ static struct combine_diff_path *ll_diff_tree_paths(\n \t\t\t}\n \n \t\t\tp = emit_path(p, base, opt, nparent,\n-\t\t\t\t\t/*t=*/NULL, tp, imin);\n+\t\t\t\t\t/*t=*/NULL, tp, imin, depth);\n \n \t\tskip_emit_tp:\n \t\t\t/* ∀ pi=p[imin]  pi↓ */\n@@ -572,7 +579,7 @@ struct combine_diff_path *diff_tree_paths(\n \tconst struct object_id **parents_oid, int nparent,\n \tstruct strbuf *base, struct diff_options *opt)\n {\n-\tp = ll_diff_tree_paths(p, oid, parents_oid, nparent, base, opt);\n+\tp = ll_diff_tree_paths(p, oid, parents_oid, nparent, base, opt, 0);\n \n \t/*\n \t * free pre-allocated last element, if any\n-- \n2.42.0.561.gaa987ecc69\n\n"},{"id":"481219","messageId":"20230831062320.GJ3185325@coredump.intra.peff.net","threadId":"60175","inReplyTo":"20230831061735.GA2702156@coredump.intra.peff.net","subject":"[PATCH 10/10] lower core.maxTreeDepth default to 2048","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-08-31T06:23:20Z","receivedAt":"2023-08-31T06:25:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On my Linux system, all of our recursive tree walking algorithms can run\nup to the 4096 default limit without segfaulting. But not all platforms\nwill have stack sizes as generous (nor might even Linux if we kick off a\nrecursive walk within a thread).\n\nIn particular, several of the tests added in the previous few commits\nfail in our Windows CI environment. Through some guess-and-check\npushing, I found that 3072 is still too much, but 2048 is OK.\n\nThese are obviously vague heuristics, and there is nothing to promise\nthat another system might not have trouble at even lower values. But it\nseems unlikely anybody will be too angry about a 2048-depth limit (this\nis close to the default max-pathname limit on Linux even for a\npathological path like \"a/a/a/...\"). So let's just lower it.\n\nSome alternatives are:\n\n  - configure separate defaults for Windows versus other platforms.\n\n  - just skip the tests on Windows. This leaves Windows users with the\n    annoying case that they can be crashed by running out of stack\n    space, but there shouldn't be any security implications (they can't\n    go deep enough to hit integer overflow problems).\n\nSince the original default was arbitrary, it seems less confusing to\njust lower it, keeping behavior consistent across platforms.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nArguably this could be squashed into patch 5. But I thought that\nfollowing the sequence of logic (from \"4096 is probably OK\" to \"whoops,\nit's not\") had some value to share. AFAIK GitHub is still running with\nthe 4096 limit; I discovered the Windows issue only while preparing this\nfor upstream. But I still find it highly unlikely that somebody would\nrun into it in practice.\n\n environment.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/environment.c b/environment.c\nindex 8e25b5ef02..bb3c2a96a3 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -81,7 +81,7 @@ int merge_log_config = -1;\n int precomposed_unicode = -1; /* see probe_utf8_pathname_composition() */\n unsigned long pack_size_limit_cfg;\n enum log_refs_config log_all_ref_updates = LOG_REFS_UNSET;\n-int max_allowed_tree_depth = 4096;\n+int max_allowed_tree_depth = 2048;\n \n #ifndef PROTECT_HFS_DEFAULT\n #define PROTECT_HFS_DEFAULT 0\n-- \n2.42.0.561.gaa987ecc69\n"},{"id":"481231","messageId":"ZPBt6VYAWHJhv3N2@ugly","threadId":"60175","inReplyTo":"20230831062320.GJ3185325@coredump.intra.peff.net","subject":"Re: [PATCH 10/10] lower core.maxTreeDepth default to 2048","fromName":"Oswald Buddenhagen","fromEmail":"oswald.buddenhagen@gmx.de","sentAt":"2023-08-31T10:39:37Z","receivedAt":"2023-08-31T10:39:43Z","isPatch":true,"sender":{"key":"oswald.buddenhagen@gmx.de","avatar":"https://avatars.githubusercontent.com/u/812380?v=4"},"body":"On Thu, Aug 31, 2023 at 02:23:20AM -0400, Jeff King wrote:\n>But I thought that\n>following the sequence of logic (from \"4096 is probably OK\" to \"whoops,\n>it's not\") had some value to share.\n>\nof course, but you can just integrate that into the squashed commit \nmessage. having it all in one place makes it easier to follow.\n\nregards\n"},{"id":"481250","messageId":"20230831174215.GA3208283@coredump.intra.peff.net","threadId":"60175","inReplyTo":"ZPBt6VYAWHJhv3N2@ugly","subject":"Re: [PATCH 10/10] lower core.maxTreeDepth default to 2048","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-08-31T17:42:15Z","receivedAt":"2023-08-31T17:42:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Aug 31, 2023 at 12:39:37PM +0200, Oswald Buddenhagen wrote:\n\n> On Thu, Aug 31, 2023 at 02:23:20AM -0400, Jeff King wrote:\n> > But I thought that\n> > following the sequence of logic (from \"4096 is probably OK\" to \"whoops,\n> > it's not\") had some value to share.\n> > \n> of course, but you can just integrate that into the squashed commit message.\n> having it all in one place makes it easier to follow.\n\nYes, though I think having it as a separate patch makes it easier to\nrevisit later (e.g., by reverting or by replacing the patch during a\nre-roll).\n\n-Peff\n"},{"id":"481256","messageId":"xmqq7cpaudke.fsf@gitster.g","threadId":"60175","inReplyTo":"20230831174215.GA3208283@coredump.intra.peff.net","subject":"Re: [PATCH 10/10] lower core.maxTreeDepth default to 2048","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-08-31T21:06:25Z","receivedAt":"2023-08-31T21:06:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Thu, Aug 31, 2023 at 12:39:37PM +0200, Oswald Buddenhagen wrote:\n>\n>> On Thu, Aug 31, 2023 at 02:23:20AM -0400, Jeff King wrote:\n>> > But I thought that\n>> > following the sequence of logic (from \"4096 is probably OK\" to \"whoops,\n>> > it's not\") had some value to share.\n>> > \n>> of course, but you can just integrate that into the squashed commit message.\n>> having it all in one place makes it easier to follow.\n>\n> Yes, though I think having it as a separate patch makes it easier to\n> revisit later (e.g., by reverting or by replacing the patch during a\n> re-roll).\n\nI am on the fence.  Having it squashed into the same step as it was\nintroduced may reduce the patch count, but then it would not be easy\nto explain why 2048 is a reasonable default at that step when no\ncode actually uses the variable, so the end result is not all that\neasier to follow and read, as that earlier step would be handwaving\n\"2048 is good at the end of the series, trust me\", unlike having it\nat the end.  When 4096 is introduced as a \"random number that seems\nlarger than large enough\" in the earlier step, it might be worth\nmentioning that it is a tentative default and may turn out to be\nlarger than necessary in which case we may want to shrink it ;-)\n\n\n"},{"id":"481270","messageId":"096e01d9dc56$6d9f12a0$48dd37e0$@nexbridge.com","threadId":"60175","inReplyTo":"xmqq7cpaudke.fsf@gitster.g","subject":"RE: [PATCH 10/10] lower core.maxTreeDepth default to 2048","fromName":"","fromEmail":"rsbecker@nexbridge.com","sentAt":"2023-08-31T21:59:28Z","receivedAt":"2023-08-31T21:59:48Z","isPatch":true,"sender":{"key":"randall.becker@nexbridge.ca","avatar":"https://avatars.githubusercontent.com/u/28956764?v=4"},"body":"On Thursday, August 31, 2023 5:06 PM, Junio C Hamano wrote:\n>Jeff King <peff@peff.net> writes:\n>\n>> On Thu, Aug 31, 2023 at 12:39:37PM +0200, Oswald Buddenhagen wrote:\n>>\n>>> On Thu, Aug 31, 2023 at 02:23:20AM -0400, Jeff King wrote:\n>>> > But I thought that\n>>> > following the sequence of logic (from \"4096 is probably OK\" to\n>>> > \"whoops, it's not\") had some value to share.\n>>> >\n>>> of course, but you can just integrate that into the squashed commit\nmessage.\n>>> having it all in one place makes it easier to follow.\n>>\n>> Yes, though I think having it as a separate patch makes it easier to\n>> revisit later (e.g., by reverting or by replacing the patch during a\n>> re-roll).\n>\n>I am on the fence.  Having it squashed into the same step as it was\nintroduced may\n>reduce the patch count, but then it would not be easy to explain why 2048\nis a\n>reasonable default at that step when no code actually uses the variable, so\nthe end\n>result is not all that easier to follow and read, as that earlier step\nwould be\n>handwaving\n>\"2048 is good at the end of the series, trust me\", unlike having it at the\nend.  When\n>4096 is introduced as a \"random number that seems larger than large enough\"\nin the\n>earlier step, it might be worth mentioning that it is a tentative default\nand may turn\n>out to be larger than necessary in which case we may want to shrink it ;-)\n\nI have been trying to figure out the implications of this and went down the\nwrong rabbit hole. Are we taking about the tree depth of the underlying\nMerkel Tree (no) or the tree-ish thing representing the file system\n(apparently yes). In this case, a practical depth of 2048 hits the exact max\npath size on the NonStop platform, so I have no issue there. My concern is\none of terminology. My assumption of what maxTreeDepth meant, from other\nterminology used in git, seemed (wrongly) to align with the use of --depth=n\nwhere n<maxTreeDepth parameters for commands like fetch. From a user\nintuition (arguably, if I have any here) is that the parameter should be\nmore of a path nomenclature, like maxPathHeight or maxHierarchyHeight rather\nthan what is currently in flight. Just my opinion and I'm fine no matter\nwhich way.\n\n--Randall\n--\nBrief whoami: NonStop&UNIX developer since approximately\nUNIX(421664400)\nNonStop(211288444200000000)\n-- In real life, I talk too much.\n\n\n\n"},{"id":"481273","messageId":"20230831223109.GA952036@coredump.intra.peff.net","threadId":"60175","inReplyTo":"096e01d9dc56$6d9f12a0$48dd37e0$@nexbridge.com","subject":"Re: [PATCH 10/10] lower core.maxTreeDepth default to 2048","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-08-31T22:31:09Z","receivedAt":"2023-08-31T22:31:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Aug 31, 2023 at 05:59:28PM -0400, rsbecker@nexbridge.com wrote:\n\n> I have been trying to figure out the implications of this and went down the\n> wrong rabbit hole. Are we taking about the tree depth of the underlying\n> Merkel Tree (no) or the tree-ish thing representing the file system\n> (apparently yes). In this case, a practical depth of 2048 hits the exact max\n> path size on the NonStop platform, so I have no issue there. My concern is\n> one of terminology. My assumption of what maxTreeDepth meant, from other\n> terminology used in git, seemed (wrongly) to align with the use of --depth=n\n> where n<maxTreeDepth parameters for commands like fetch. From a user\n> intuition (arguably, if I have any here) is that the parameter should be\n> more of a path nomenclature, like maxPathHeight or maxHierarchyHeight rather\n> than what is currently in flight. Just my opinion and I'm fine no matter\n> which way.\n\nThe documentation from the patch is:\n\n  +core.maxTreeDepth::\n  +       The maximum depth Git is willing to recurse while traversing a\n  +       tree (e.g., \"a/b/cde/f\" has a depth of 4). This is a fail-safe\n  +       to allow Git to abort cleanly, and should not generally need to\n  +       be adjusted. The default is 4096.\n\nDoes reading that answer your question and make the meaning clear? If\nnot, can you suggest any changes?\n\nI'd like to stick with \"depth\" here as it's commonly used in other\nplaces to mean the same thing (e.g., git-grep's --max-depth option).\n\nI also think this is something that most people will remain completely\noblivious to, as you'd only hit it for absurd cases.\n\n-Peff\n"}]}