{"thread":{"id":"63950","subject":"Potential Null Pointer Dereference detected by static analysis tool","startedAt":"2025-08-13T07:13:18Z","lastAt":"2025-08-20T06:30:37Z","messageCount":28,"participants":["Cheng","Phillip Wood","Jeff King","René Scharfe","Junio C Hamano","Patrick Steinhardt"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"524114","messageId":"AAkArwD3JXZP4EIjvKF0Waow.1.1755044612233.Hmail.2201111603@stu.pku.edu.cn","threadId":"63950","inReplyTo":null,"subject":"Potential Null Pointer Dereference detected by static analysis tool","fromName":"Cheng","fromEmail":"prophecheng@stu.pku.edu.cn","sentAt":"2025-08-13T00:23:32Z","receivedAt":"2025-08-13T07:13:18Z","isPatch":false,"sender":{"key":"prophecheng@stu.pku.edu.cn","avatar":null},"body":"\n\nline 326 in builtin/describe.cdescribe.c, which is located in the function describe_commit. In the following code, cmit could be NULL passed to the call, which then causes a NULL dereference. Seems should be replaced lookup_commit_reference with lookup_commit_or_die.\n\n\n```cpp\ncmit = lookup_commit_reference(the_repository, oid);\nn = find_commit_name(&cmit->object.oid);\n```\n \n\n\nThe NULL value seems to come from function lookup_commit_reference_gently where:\n\n- 1. call to deref_tag may return NULL.\n\n- 2. call to object_as_type may return NULL.\n\n\nIn this repository, other calls  lookup_commit_reference are followed by a null check. So this seems to lead to NULL dereference. Can I confirm with you whether this is a true positive bug report?\n\n\n\n\n\n\n\n\n"},{"id":"524122","messageId":"5303c45e-d95b-4bc8-9cd1-bf4efe6bfbae@gmail.com","threadId":"63950","inReplyTo":"AAkArwD3JXZP4EIjvKF0Waow.1.1755044612233.Hmail.2201111603@stu.pku.edu.cn","subject":"Re: Potential Null Pointer Dereference detected by static analysis tool","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2025-08-13T13:19:14Z","receivedAt":"2025-08-13T13:19:18Z","isPatch":false,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Cheng\n\nOn 13/08/2025 01:23, Cheng wrote:\n> \n> \n> line 326 in builtin/describe.cdescribe.c, which is located in the function describe_commit. In the following code, cmit could be NULL passed to the call, which then causes a NULL dereference. Seems should be replaced lookup_commit_reference with lookup_commit_or_die.\n> \n> \n> ```cpp\n> cmit = lookup_commit_reference(the_repository, oid);\n> n = find_commit_name(&cmit->object.oid);\n> ```\n> \n> \n> \n> The NULL value seems to come from function lookup_commit_reference_gently where:\n> \n> - 1. call to deref_tag may return NULL.\n> \n> - 2. call to object_as_type may return NULL.\n> \n> \n> In this repository, other calls  lookup_commit_reference are followed by a null check. So this seems to lead to NULL dereference. Can I confirm with you whether this is a true positive bug report?\n\nI had a quick look at the callers of describe_commit() and they all seem \nto use an oid that they get from looking up a commit so I'm not sure \nunder what circumstances this call to lookup_commit_reference() can fail.\n\nThanks\n\nPhillip\n\n"},{"id":"524199","messageId":"20250814232644.GC2937@coredump.intra.peff.net","threadId":"63950","inReplyTo":"5303c45e-d95b-4bc8-9cd1-bf4efe6bfbae@gmail.com","subject":"Re: Potential Null Pointer Dereference detected by static analysis tool","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-08-14T23:26:44Z","receivedAt":"2025-08-14T23:26:45Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Aug 13, 2025 at 02:19:14PM +0100, Phillip Wood wrote:\n\n> I had a quick look at the callers of describe_commit() and they all seem to\n> use an oid that they get from looking up a commit so I'm not sure under what\n> circumstances this call to lookup_commit_reference() can fail.\n\nI wonder if it would make sense for describe_commit() to just take a\n\"struct commit\" pointer. Then it could skip the call to turn the oid\ninto a commit entirely, and the compiler would make sure we always have\na commit. :)\n\nSomething like this (totally untested, and not something I'm planning to\nfollow up on, but maybe inspirational):\n\ndiff --git a/builtin/describe.c b/builtin/describe.c\nindex 32f5bf513f..3e8691a4c4 100644\n--- a/builtin/describe.c\n+++ b/builtin/describe.c\n@@ -352,26 +352,24 @@ static void append_suffix(int depth, const struct object_id *oid, struct strbuf\n \t\t repo_find_unique_abbrev(the_repository, oid, abbrev));\n }\n \n-static void describe_commit(struct object_id *oid, struct strbuf *dst)\n+static void describe_commit(struct commit *cmit, struct strbuf *dst)\n {\n-\tstruct commit *cmit, *gave_up_on = NULL;\n+\tstruct commit *gave_up_on = NULL;\n \tstruct lazy_queue queue = LAZY_QUEUE_INIT;\n \tstruct commit_name *n;\n \tstruct possible_tag all_matches[MAX_TAGS];\n \tunsigned int match_cnt = 0, annotated_cnt = 0, cur_match;\n \tunsigned long seen_commits = 0;\n \tunsigned int unannotated_cnt = 0;\n \n-\tcmit = lookup_commit_reference(the_repository, oid);\n-\n \tn = find_commit_name(&cmit->object.oid);\n \tif (n && (tags || all || n->prio == 2)) {\n \t\t/*\n \t\t * Exact match to an existing ref.\n \t\t */\n \t\tappend_name(n, dst);\n \t\tif (n->misnamed || longformat)\n-\t\t\tappend_suffix(0, n->tag ? get_tagged_oid(n->tag) : oid, dst);\n+\t\t\tappend_suffix(0, n->tag ? get_tagged_oid(n->tag) : &cmit->object.oid, dst);\n \t\tif (suffix)\n \t\t\tstrbuf_addstr(dst, suffix);\n \t\treturn;\n@@ -528,7 +526,7 @@ static void describe_commit(struct object_id *oid, struct strbuf *dst)\n }\n \n struct process_commit_data {\n-\tstruct object_id current_commit;\n+\tstruct commit *current_commit;\n \tstruct object_id looking_for;\n \tstruct strbuf *dst;\n \tstruct rev_info *revs;\n@@ -537,7 +535,7 @@ struct process_commit_data {\n static void process_commit(struct commit *commit, void *data)\n {\n \tstruct process_commit_data *pcd = data;\n-\tpcd->current_commit = commit->object.oid;\n+\tpcd->current_commit = commit;\n }\n \n static void process_object(struct object *obj, const char *path, void *data)\n@@ -546,7 +544,7 @@ static void process_object(struct object *obj, const char *path, void *data)\n \n \tif (oideq(&pcd->looking_for, &obj->oid) && !pcd->dst->len) {\n \t\treset_revision_walk();\n-\t\tdescribe_commit(&pcd->current_commit, pcd->dst);\n+\t\tdescribe_commit(pcd->current_commit, pcd->dst);\n \t\tstrbuf_addf(pcd->dst, \":%s\", path);\n \t\tclear_prio_queue(&pcd->revs->commits);\n \t}\n@@ -556,7 +554,7 @@ static void describe_blob(struct object_id oid, struct strbuf *dst)\n {\n \tstruct rev_info revs;\n \tstruct strvec args = STRVEC_INIT;\n-\tstruct process_commit_data pcd = { *null_oid(the_hash_algo), oid, dst, &revs};\n+\tstruct process_commit_data pcd = { NULL, oid, dst, &revs};\n \n \tstrvec_pushl(&args, \"internal: The first arg is not parsed\",\n \t\t \"--objects\", \"--in-commit-order\", \"--reverse\", \"HEAD\",\n@@ -589,7 +587,7 @@ static void describe(const char *arg, int last_one)\n \tcmit = lookup_commit_reference_gently(the_repository, &oid, 1);\n \n \tif (cmit)\n-\t\tdescribe_commit(&oid, &sb);\n+\t\tdescribe_commit(cmit, &sb);\n \telse if (odb_read_object_info(the_repository->objects,\n \t\t\t\t &oid, NULL) == OBJ_BLOB)\n \t\tdescribe_blob(oid, &sb);\n"},{"id":"524254","messageId":"2560a90d-4015-4087-97f1-3733a58261e4@gmail.com","threadId":"63950","inReplyTo":"20250814232644.GC2937@coredump.intra.peff.net","subject":"Re: Potential Null Pointer Dereference detected by static analysis tool","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2025-08-15T15:49:12Z","receivedAt":"2025-08-15T15:49:15Z","isPatch":false,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Peff\n\nOn 15/08/2025 00:26, Jeff King wrote:\n> On Wed, Aug 13, 2025 at 02:19:14PM +0100, Phillip Wood wrote:\n> \n>> I had a quick look at the callers of describe_commit() and they all seem to\n>> use an oid that they get from looking up a commit so I'm not sure under what\n>> circumstances this call to lookup_commit_reference() can fail.\n> \n> I wonder if it would make sense for describe_commit() to just take a\n> \"struct commit\" pointer. Then it could skip the call to turn the oid\n> into a commit entirely, and the compiler would make sure we always have\n> a commit. :)\n\nI think that's a good idea, it would be clearer to the reader that we've \nalready looked up the commit before calling describe_commit() as well.\n\nThanks\n\nPhillip\n\n> Something like this (totally untested, and not something I'm planning to\n> follow up on, but maybe inspirational):\n> \n> diff --git a/builtin/describe.c b/builtin/describe.c\n> index 32f5bf513f..3e8691a4c4 100644\n> --- a/builtin/describe.c\n> +++ b/builtin/describe.c\n> @@ -352,26 +352,24 @@ static void append_suffix(int depth, const struct object_id *oid, struct strbuf\n> \t\t repo_find_unique_abbrev(the_repository, oid, abbrev));\n> }\n> \n> -static void describe_commit(struct object_id *oid, struct strbuf *dst)\n> +static void describe_commit(struct commit *cmit, struct strbuf *dst)\n> {\n> -\tstruct commit *cmit, *gave_up_on = NULL;\n> +\tstruct commit *gave_up_on = NULL;\n> \tstruct lazy_queue queue = LAZY_QUEUE_INIT;\n> \tstruct commit_name *n;\n> \tstruct possible_tag all_matches[MAX_TAGS];\n> \tunsigned int match_cnt = 0, annotated_cnt = 0, cur_match;\n> \tunsigned long seen_commits = 0;\n> \tunsigned int unannotated_cnt = 0;\n> \n> -\tcmit = lookup_commit_reference(the_repository, oid);\n> -\n> \tn = find_commit_name(&cmit->object.oid);\n> \tif (n && (tags || all || n->prio == 2)) {\n> \t\t/*\n> \t\t * Exact match to an existing ref.\n> \t\t */\n> \t\tappend_name(n, dst);\n> \t\tif (n->misnamed || longformat)\n> -\t\t\tappend_suffix(0, n->tag ? get_tagged_oid(n->tag) : oid, dst);\n> +\t\t\tappend_suffix(0, n->tag ? get_tagged_oid(n->tag) : &cmit->object.oid, dst);\n> \t\tif (suffix)\n> \t\t\tstrbuf_addstr(dst, suffix);\n> \t\treturn;\n> @@ -528,7 +526,7 @@ static void describe_commit(struct object_id *oid, struct strbuf *dst)\n> }\n> \n> struct process_commit_data {\n> -\tstruct object_id current_commit;\n> +\tstruct commit *current_commit;\n> \tstruct object_id looking_for;\n> \tstruct strbuf *dst;\n> \tstruct rev_info *revs;\n> @@ -537,7 +535,7 @@ struct process_commit_data {\n> static void process_commit(struct commit *commit, void *data)\n> {\n> \tstruct process_commit_data *pcd = data;\n> -\tpcd->current_commit = commit->object.oid;\n> +\tpcd->current_commit = commit;\n> }\n> \n> static void process_object(struct object *obj, const char *path, void *data)\n> @@ -546,7 +544,7 @@ static void process_object(struct object *obj, const char *path, void *data)\n> \n> \tif (oideq(&pcd->looking_for, &obj->oid) && !pcd->dst->len) {\n> \t\treset_revision_walk();\n> -\t\tdescribe_commit(&pcd->current_commit, pcd->dst);\n> +\t\tdescribe_commit(pcd->current_commit, pcd->dst);\n> \t\tstrbuf_addf(pcd->dst, \":%s\", path);\n> \t\tclear_prio_queue(&pcd->revs->commits);\n> \t}\n> @@ -556,7 +554,7 @@ static void describe_blob(struct object_id oid, struct strbuf *dst)\n> {\n> \tstruct rev_info revs;\n> \tstruct strvec args = STRVEC_INIT;\n> -\tstruct process_commit_data pcd = { *null_oid(the_hash_algo), oid, dst, &revs};\n> +\tstruct process_commit_data pcd = { NULL, oid, dst, &revs};\n> \n> \tstrvec_pushl(&args, \"internal: The first arg is not parsed\",\n> \t\t \"--objects\", \"--in-commit-order\", \"--reverse\", \"HEAD\",\n> @@ -589,7 +587,7 @@ static void describe(const char *arg, int last_one)\n> \tcmit = lookup_commit_reference_gently(the_repository, &oid, 1);\n> \n> \tif (cmit)\n> -\t\tdescribe_commit(&oid, &sb);\n> +\t\tdescribe_commit(cmit, &sb);\n> \telse if (odb_read_object_info(the_repository->objects,\n> \t\t\t\t &oid, NULL) == OBJ_BLOB)\n> \t\tdescribe_blob(oid, &sb);\n\n"},{"id":"524316","messageId":"7f289d4f-0a9d-480d-a5a9-7f4d4d24626b@web.de","threadId":"63950","inReplyTo":"20250814232644.GC2937@coredump.intra.peff.net","subject":"Re: Potential Null Pointer Dereference detected by static analysis tool","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2025-08-17T09:27:12Z","receivedAt":"2025-08-17T09:27:21Z","isPatch":false,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"On 8/15/25 1:26 AM, Jeff King wrote:\n> On Wed, Aug 13, 2025 at 02:19:14PM +0100, Phillip Wood wrote:\n> \n>> I had a quick look at the callers of describe_commit() and they all seem to\n>> use an oid that they get from looking up a commit so I'm not sure under what\n>> circumstances this call to lookup_commit_reference() can fail.\n> \n> I wonder if it would make sense for describe_commit() to just take a\n> \"struct commit\" pointer.\n\nYes, a lot.\n\n> Then it could skip the call to turn the oid\n> into a commit entirely, and the compiler would make sure we always have\n> a commit. :)\n> \n> Something like this (totally untested, and not something I'm planning to\n> follow up on, but maybe inspirational):\n> \n> diff --git a/builtin/describe.c b/builtin/describe.c\n> index 32f5bf513f..3e8691a4c4 100644\n> --- a/builtin/describe.c\n> +++ b/builtin/describe.c\n> @@ -352,26 +352,24 @@ static void append_suffix(int depth, const struct object_id *oid, struct strbuf\n> \t\t repo_find_unique_abbrev(the_repository, oid, abbrev));\n> }\n> \n> -static void describe_commit(struct object_id *oid, struct strbuf *dst)\n> +static void describe_commit(struct commit *cmit, struct strbuf *dst)\n> {\n> -\tstruct commit *cmit, *gave_up_on = NULL;\n> +\tstruct commit *gave_up_on = NULL;\n> \tstruct lazy_queue queue = LAZY_QUEUE_INIT;\n> \tstruct commit_name *n;\n> \tstruct possible_tag all_matches[MAX_TAGS];\n> \tunsigned int match_cnt = 0, annotated_cnt = 0, cur_match;\n> \tunsigned long seen_commits = 0;\n> \tunsigned int unannotated_cnt = 0;\n> \n> -\tcmit = lookup_commit_reference(the_repository, oid);\n> -\n> \tn = find_commit_name(&cmit->object.oid);\n> \tif (n && (tags || all || n->prio == 2)) {\n> \t\t/*\n> \t\t * Exact match to an existing ref.\n> \t\t */\n> \t\tappend_name(n, dst);\n> \t\tif (n->misnamed || longformat)\n> -\t\t\tappend_suffix(0, n->tag ? get_tagged_oid(n->tag) : oid, dst);\n> +\t\t\tappend_suffix(0, n->tag ? get_tagged_oid(n->tag) : &cmit->object.oid, dst);\n> \t\tif (suffix)\n> \t\t\tstrbuf_addstr(dst, suffix);\n> \t\treturn;\n> @@ -528,7 +526,7 @@ static void describe_commit(struct object_id *oid, struct strbuf *dst)\n> }\n> \n> struct process_commit_data {\n> -\tstruct object_id current_commit;\n> +\tstruct commit *current_commit;\n> \tstruct object_id looking_for;\n> \tstruct strbuf *dst;\n> \tstruct rev_info *revs;\n> @@ -537,7 +535,7 @@ struct process_commit_data {\n> static void process_commit(struct commit *commit, void *data)\n> {\n> \tstruct process_commit_data *pcd = data;\n> -\tpcd->current_commit = commit->object.oid;\n> +\tpcd->current_commit = commit;\n> }\n> \n> static void process_object(struct object *obj, const char *path, void *data)\n> @@ -546,7 +544,7 @@ static void process_object(struct object *obj, const char *path, void *data)\n> \n> \tif (oideq(&pcd->looking_for, &obj->oid) && !pcd->dst->len) {\n> \t\treset_revision_walk();\n> -\t\tdescribe_commit(&pcd->current_commit, pcd->dst);\n> +\t\tdescribe_commit(pcd->current_commit, pcd->dst);\n\npcd->current_commit is initialized to NULL below, but\ntraverse_commit_list() without a filter must have set it via our\nprocess_commit() callback before we get to the describe_commit() call.\n\nOr are there weird repositories (e.g., just a blob, just a tag) that can\ncause traverse_commit_list() to call its show_object() callback without\never calling its show_commit() callback? I don't see how, but may be\nmissing some way.\n\n> \t\tstrbuf_addf(pcd->dst, \":%s\", path);\n> \t\tclear_prio_queue(&pcd->revs->commits);\n> \t}\n> @@ -556,7 +554,7 @@ static void describe_blob(struct object_id oid, struct strbuf *dst)\n> {\n> \tstruct rev_info revs;\n> \tstruct strvec args = STRVEC_INIT;\n> -\tstruct process_commit_data pcd = { *null_oid(the_hash_algo), oid, dst, &revs};\n> +\tstruct process_commit_data pcd = { NULL, oid, dst, &revs};\n> \n> \tstrvec_pushl(&args, \"internal: The first arg is not parsed\",\n> \t\t \"--objects\", \"--in-commit-order\", \"--reverse\", \"HEAD\",\n> @@ -589,7 +587,7 @@ static void describe(const char *arg, int last_one)\n> \tcmit = lookup_commit_reference_gently(the_repository, &oid, 1);\n> \n> \tif (cmit)\n> -\t\tdescribe_commit(&oid, &sb);\n> +\t\tdescribe_commit(cmit, &sb);\n> \telse if (odb_read_object_info(the_repository->objects,\n> \t\t\t\t &oid, NULL) == OBJ_BLOB)\n> \t\tdescribe_blob(oid, &sb);\n\n"},{"id":"524334","messageId":"20250818044807.GA51803@coredump.intra.peff.net","threadId":"63950","inReplyTo":"7f289d4f-0a9d-480d-a5a9-7f4d4d24626b@web.de","subject":"Re: Potential Null Pointer Dereference detected by static analysis tool","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-08-18T04:48:07Z","receivedAt":"2025-08-18T04:48:16Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Aug 17, 2025 at 11:27:12AM +0200, René Scharfe wrote:\n\n> > @@ -546,7 +544,7 @@ static void process_object(struct object *obj, const char *path, void *data)\n> > \n> > \tif (oideq(&pcd->looking_for, &obj->oid) && !pcd->dst->len) {\n> > \t\treset_revision_walk();\n> > -\t\tdescribe_commit(&pcd->current_commit, pcd->dst);\n> > +\t\tdescribe_commit(pcd->current_commit, pcd->dst);\n> \n> pcd->current_commit is initialized to NULL below, but\n> traverse_commit_list() without a filter must have set it via our\n> process_commit() callback before we get to the describe_commit() call.\n> \n> Or are there weird repositories (e.g., just a blob, just a tag) that can\n> cause traverse_commit_list() to call its show_object() callback without\n> ever calling its show_commit() callback? I don't see how, but may be\n> missing some way.\n\nIf there are, then I think the current code would segfault, too. It\ninitializes &pcd->current_commit to the null oid, and then\ndescribe_commit() resolves that via lookup_commit_reference(). That\nwould return NULL, and the next line dereferencing the result would\nsegfault.\n\nAnd it would be a counter-example to the claim that the call to\nlookup_commit_reference() in describe_commit() never fails. ;)\n\nI think your intuition is right that we could get the traversal code to\ncall show_object() without show_commit() in general. E.g. just:\n\n git init\n git tag foo $(echo bar | git hash-object -w --stdin)\n git rev-list --all --objects\n\nwould do so. But in this code our traversal always starts from HEAD,\nwhich must be a commit (and this is enforced by the refs code). So you'd\nhave to corrupt your repository like:\n\n # do this in two steps since redirecting to .git/HEAD breaks the\n # repository for a moment!\n tag=$(git rev-parse foo)\n echo $tag >.git/HEAD\n\nAnd indeed, the current code does then segfault on \"git describe foo\" at\nthe spot I mentioned. Even though the repository state here is\nunexpected and corrupt, I do think we should probably be more defensive\nand avoid the segfault.\n\nI also find it a bit funny that describing a blob only walks from HEAD\n(and not say, all refs or ones matching --match/--exclude, etc). It is\ndocumented that way, though. Actually, I find the whole feature a bit\npointless now that we have the diff \"--find-object\" option. Reading\n15af58c1ad (diffcore: add a pickaxe option to find a specific blob,\n2018-01-04), I think the git-describe behavior is mostly considered a\nmistake (which we have to keep around for historical reasons). I guess\nanother possible candidate for removing in Git 3.0. :)\n\n-Peff\n"},{"id":"524335","messageId":"20250818050545.GA62231@coredump.intra.peff.net","threadId":"63950","inReplyTo":"20250818044807.GA51803@coredump.intra.peff.net","subject":"Re: Potential Null Pointer Dereference detected by static analysis tool","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-08-18T05:05:45Z","receivedAt":"2025-08-18T05:05:47Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 18, 2025 at 12:48:07AM -0400, Jeff King wrote:\n\n> And indeed, the current code does then segfault on \"git describe foo\" at\n> the spot I mentioned. Even though the repository state here is\n> unexpected and corrupt, I do think we should probably be more defensive\n> and avoid the segfault.\n\nSo you almost nerd-sniped me into making a series. But the more I dug\ninto the rabbit-hole, the more I turned away in disgust. :)\n\nThe set of problems I found are:\n\n 1. What should happen when traversing from HEAD does not find the blob\n in question? Right now we print a blank line, which is...weird.\n Probably we should either print nothing, or return an error. If we\n return an error, should we respect --always? Are we stuck with the\n current dumb behavior because it's a plumbing command?\n\n 2. When we are on an unborn branch, we print a confusing message:\n\n $ git init\n $ git commit --allow-empty -m foo\n $ git tag foo\n $ git symbolic-ref HEAD refs/heads/unborn\n $ git describe $(echo blob | git hash-object -w --stdin)\n fatal: ambiguous argument 'HEAD': unknown revision or path not in the working tree.\n Use '--' to separate paths from revisions, like this:\n 'git [...] -- [...]'\n\n We should probably resolve HEAD ourselves and either bail with an\n empty output or an error (depending on what we do for (1) above).\n\n 3. When we do traverse, if process_object() sees that we didn't find a\n commit, we should detect that and either return an empty result or\n an error (again, depending on the behavior of (2) above). This is\n done by checking is_null_oid(&pcd->current_commit) there.\n\n 4. Then we can teach describe_commit() to take a commit rather than an\n oid (and the is_null_oid() check becomes a NULL check).\n\nSo it all depends on what to do with (1), and for a feature that IMHO\nshould not even exist in the first place, I had trouble summoning the\nwill-power to make this 4-patch series.\n\n-Peff\n"},{"id":"524360","messageId":"194415e5-2adc-4b3f-98d3-0fde4a288b5c@web.de","threadId":"63950","inReplyTo":"20250818050545.GA62231@coredump.intra.peff.net","subject":"Re: Potential Null Pointer Dereference detected by static analysis tool","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2025-08-18T19:56:35Z","receivedAt":"2025-08-18T19:56:50Z","isPatch":false,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"On 8/18/25 7:05 AM, Jeff King wrote:\n> On Mon, Aug 18, 2025 at 12:48:07AM -0400, Jeff King wrote:\n> \n>> And indeed, the current code does then segfault on \"git describe foo\" at\n>> the spot I mentioned. Even though the repository state here is\n>> unexpected and corrupt, I do think we should probably be more defensive\n>> and avoid the segfault.\n> \n> So you almost nerd-sniped me into making a series. But the more I dug\n> into the rabbit-hole, the more I turned away in disgust. :)\n> \n> The set of problems I found are:\n> \n> 1. What should happen when traversing from HEAD does not find the blob\n> in question? Right now we print a blank line, which is...weird.\n\nWeird indeed.\n\n> Probably we should either print nothing, or return an error.\n\nThe latter, consistent with \"git describe \".\n\n> If we return an error, should we respect --always?\n\nThe documentation says \"git describe \" takes no options. It could\nlearn some, of course. But does it have to? Perhaps better keep that\nthing contained.\n\n> Are we stuck with the\n> current dumb behavior because it's a plumbing command?\n\ngit describe is a porcelain command.\n\nThe documentation doesn't say what happens when the blob is not found\nin HEAD's ancestry.\n\nI can't imagine why someone would use \"git describe \" to check\nwhether a particular blob is linked to, but it _is_ slightly faster than\n\"git rev-list --objects --no-object-names HEAD | grep \" for me,\nand of course easier to type. \"git log --find-object \" is slower\nthan either.\n\n> 2. When we are on an unborn branch, we print a confusing message:\n> \n> $ git init\n> $ git commit --allow-empty -m foo\n> $ git tag foo\n> $ git symbolic-ref HEAD refs/heads/unborn\n> $ git describe $(echo blob | git hash-object -w --stdin)\n> fatal: ambiguous argument 'HEAD': unknown revision or path not in the working tree.\n> Use '--' to separate paths from revisions, like this:\n> 'git [...] -- [...]'\n> \n> We should probably resolve HEAD ourselves and either bail with an\n> empty output or an error (depending on what we do for (1) above).\n\nIt already is an error, just needs a better message. It should still\nreport an error even if we were to stick with showing blank lines for\nunrelated blobs.\n\n> 3. When we do traverse, if process_object() sees that we didn't find a\n> commit, we should detect that and either return an empty result or\n> an error (again, depending on the behavior of (2) above). This is\n> done by checking is_null_oid(&pcd->current_commit) there.\n\nOK, ending the search right there might be the best option. Traversing\ndeeper into the forest that we then know to be cursed would be the\nunappealing alternative.\n\n> 4. Then we can teach describe_commit() to take a commit rather than an\n> oid (and the is_null_oid() check becomes a NULL check).\n\n 5. When process_object() has a commit, but it is indescribable, it\n shows an error:\n\n $ git describe 5afbe6da1d6ab0b8939343166636b828e561bf35\n fatal: No tags can describe '3b681e255cd8577a77983958ef7f566b05806cd0'.\n Try --always, or create some tags.\n\n It's not immediately clear that the reported hash belongs to the\n found commit. And that suggestion to try --always is misleading,\n as \"git describe \" takes no options according to the\n documentation. I'm not sure I like it in general -- can't tell\n if the command is being snarky with me.\n\n> So it all depends on what to do with (1), and for a feature that IMHO\n> should not even exist in the first place, I had trouble summoning the\n> will-power to make this 4-patch series.\n\n644eb60bd0 (builtin/describe.c: describe a blob, 2017-11-15) and\n15af58c1ad (diffcore: add a pickaxe option to find a specific blob,\n2018-01-04) confuse me; the latter's commit message sounds like the\nformer wasn't (supposed to be?) merged.\n\nI think the issues you listed are independent, though. Or what's wrong\nwith this demo that addresses point 3 in process_object() and 1 in\ndescribe_blob(). If we want a blank line for 1 then we apply only\nthe first hunk. Or am I missing something?\n\nRené\n\n\ndiff --git a/builtin/describe.c b/builtin/describe.c\nindex d7dd8139de..9e485240aa 100644\n--- a/builtin/describe.c\n+++ b/builtin/describe.c\n@@ -507,8 +507,10 @@ static void process_object(struct object *obj, const char *path, void *data)\n \n \tif (oideq(&pcd->looking_for, &obj->oid) && !pcd->dst->len) {\n \t\treset_revision_walk();\n-\t\tdescribe_commit(&pcd->current_commit, pcd->dst);\n-\t\tstrbuf_addf(pcd->dst, \":%s\", path);\n+\t\tif (!is_null_oid(&pcd->current_commit)) {\n+\t\t\tdescribe_commit(&pcd->current_commit, pcd->dst);\n+\t\t\tstrbuf_addf(pcd->dst, \":%s\", path);\n+\t\t}\n \t\tfree_commit_list(pcd->revs->commits);\n \t\tpcd->revs->commits = NULL;\n \t}\n@@ -519,6 +521,7 @@ static void describe_blob(struct object_id oid, struct strbuf *dst)\n \tstruct rev_info revs;\n \tstruct strvec args = STRVEC_INIT;\n \tstruct process_commit_data pcd = { *null_oid(the_hash_algo), oid, dst, &revs};\n+\tsize_t orig_len = dst->len;\n \n \tstrvec_pushl(&args, \"internal: The first arg is not parsed\",\n \t\t \"--objects\", \"--in-commit-order\", \"--reverse\", \"HEAD\",\n@@ -532,6 +535,8 @@ static void describe_blob(struct object_id oid, struct strbuf *dst)\n \t\tdie(\"revision walk setup failed\");\n \n \ttraverse_commit_list(&revs, process_commit, process_object, &pcd);\n+\tif (dst->len == orig_len)\n+\t\tdie(_(\"unable to describe blob '%s'\"), oid_to_hex(&oid));\n \treset_revision_walk();\n \trelease_revisions(&revs);\n \tstrvec_clear(&args);\n\n"},{"id":"524362","messageId":"20250818202140.GA1014353@coredump.intra.peff.net","threadId":"63950","inReplyTo":"194415e5-2adc-4b3f-98d3-0fde4a288b5c@web.de","subject":"Re: Potential Null Pointer Dereference detected by static analysis tool","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-08-18T20:21:40Z","receivedAt":"2025-08-18T20:21:43Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 18, 2025 at 09:56:35PM +0200, René Scharfe wrote:\n\n> > 1. What should happen when traversing from HEAD does not find the blob\n> > in question? Right now we print a blank line, which is...weird.\n> \n> Weird indeed.\n> \n> > Probably we should either print nothing, or return an error.\n> \n> The latter, consistent with \"git describe \".\n\nCertainly if we were starting from scratch, that's my thought. I just\nwondered if we were stuck with the behavior for historical reasons.\n\n> > If we return an error, should we respect --always?\n> \n> The documentation says \"git describe \" takes no options. It could\n> learn some, of course. But does it have to? Perhaps better keep that\n> thing contained.\n\nOK. It is easy to just treat --always like we do in the commit case, but\nI'm not sure it's actually useful. Just bailing early to say the option\nis incompatible is reasonable.\n\n> > 2. When we are on an unborn branch, we print a confusing message:\n> > \n> > $ git init\n> > $ git commit --allow-empty -m foo\n> > $ git tag foo\n> > $ git symbolic-ref HEAD refs/heads/unborn\n> > $ git describe $(echo blob | git hash-object -w --stdin)\n> > fatal: ambiguous argument 'HEAD': unknown revision or path not in the working tree.\n> > Use '--' to separate paths from revisions, like this:\n> > 'git [...] -- [...]'\n> > \n> > We should probably resolve HEAD ourselves and either bail with an\n> > empty output or an error (depending on what we do for (1) above).\n> \n> It already is an error, just needs a better message. It should still\n> report an error even if we were to stick with showing blank lines for\n> unrelated blobs.\n\nI think an error here is OK. But if we quietly return no output for (1),\nthen I think this should do the same. If we return an error for (1),\nthen yeah, it should remain one here and just get a better message.\n\n> > 3. When we do traverse, if process_object() sees that we didn't find a\n> > commit, we should detect that and either return an empty result or\n> > an error (again, depending on the behavior of (2) above). This is\n> > done by checking is_null_oid(&pcd->current_commit) there.\n> \n> OK, ending the search right there might be the best option. Traversing\n> deeper into the forest that we then know to be cursed would be the\n> unappealing alternative.\n\nRight. There is no real \"deeper\" because we know we will never find a\ncommit.\n\n> 5. When process_object() has a commit, but it is indescribable, it\n> shows an error:\n> \n> $ git describe 5afbe6da1d6ab0b8939343166636b828e561bf35\n> fatal: No tags can describe '3b681e255cd8577a77983958ef7f566b05806cd0'.\n> Try --always, or create some tags.\n> \n> It's not immediately clear that the reported hash belongs to the\n> found commit. And that suggestion to try --always is misleading,\n> as \"git describe \" takes no options according to the\n> documentation. I'm not sure I like it in general -- can't tell\n> if the command is being snarky with me.\n\nYeah, describe_commit()'s messages are not really set up to handle\ndescribing an arbitrary commit that the user did not specify.\n\n> > So it all depends on what to do with (1), and for a feature that IMHO\n> > should not even exist in the first place, I had trouble summoning the\n> > will-power to make this 4-patch series.\n> \n> 644eb60bd0 (builtin/describe.c: describe a blob, 2017-11-15) and\n> 15af58c1ad (diffcore: add a pickaxe option to find a specific blob,\n> 2018-01-04) confuse me; the latter's commit message sounds like the\n> former wasn't (supposed to be?) merged.\n> \n> I think the issues you listed are independent, though. Or what's wrong\n> with this demo that addresses point 3 in process_object() and 1 in\n> describe_blob(). If we want a blank line for 1 then we apply only\n> the first hunk. Or am I missing something?\n\nYeah, I came up with a similar patch. The question was more of what the\nbehavior _should_ be, and whether we wanted to align that with an unborn\nHEAD. And of course tests for all of these cases.\n\nI'll send out a few patches in a moment (I guess I summoned some\nwillpower in the interim).\n\n-Peff\n"},{"id":"524364","messageId":"20250818205608.GA1018532@coredump.intra.peff.net","threadId":"63950","inReplyTo":"20250818202140.GA1014353@coredump.intra.peff.net","subject":"Re: Potential Null Pointer Dereference detected by static analysis tool","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-08-18T20:56:08Z","receivedAt":"2025-08-18T20:56:09Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 18, 2025 at 04:21:40PM -0400, Jeff King wrote:\n\n> > 5. When process_object() has a commit, but it is indescribable, it\n> > shows an error:\n> > \n> > $ git describe 5afbe6da1d6ab0b8939343166636b828e561bf35\n> > fatal: No tags can describe '3b681e255cd8577a77983958ef7f566b05806cd0'.\n> > Try --always, or create some tags.\n> > \n> > It's not immediately clear that the reported hash belongs to the\n> > found commit. And that suggestion to try --always is misleading,\n> > as \"git describe \" takes no options according to the\n> > documentation. I'm not sure I like it in general -- can't tell\n> > if the command is being snarky with me.\n> \n> Yeah, describe_commit()'s messages are not really set up to handle\n> describing an arbitrary commit that the user did not specify.\n\nOh, there's one other related case I _thought_ was bug, but maybe isn't.\nIf you don't have any tags at all, then we'll bail immediately:\n\n $ git init\n $ echo foo >file && git add file && git commit -m foo\n $ git describe $(git rev-parse HEAD:file)\n fatal: No names found, cannot describe anything.\n\nBut we're traversing from HEAD, not the tags, so my initial thought was\nthat this should work. But of course it doesn't because we try to\ndescribe the containing commit, which _does_ require tags.\n\nAnd that leads back to: \"well, you could pass --always\". Which disagrees\nwith the documentation, but I think does actually do something useful\nhere.\n\n-Peff\n"},{"id":"524365","messageId":"20250818205812.GA1018043@coredump.intra.peff.net","threadId":"63950","inReplyTo":"20250818202140.GA1014353@coredump.intra.peff.net","subject":"[PATCH 0/5] fix segfault and other oddities describing blobs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-08-18T20:58:12Z","receivedAt":"2025-08-18T20:58:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 18, 2025 at 04:21:40PM -0400, Jeff King wrote:\n\n> I'll send out a few patches in a moment (I guess I summoned some\n> willpower in the interim).\n\nHere's what I came up with.\n\n [1/5]: describe: pass oid struct by const pointer\n [2/5]: describe: error if blob not found\n [3/5]: describe: catch unborn branch in describe_blob()\n [4/5]: describe: handle blob traversal with no commits\n [5/5]: describe: pass commit to describe_commit()\n\n builtin/describe.c | 41 +++++++++++++++++++++++++----------------\n t/t6120-describe.sh | 14 ++++++++++++++\n 2 files changed, 39 insertions(+), 16 deletions(-)\n\n-Peff\n"},{"id":"524366","messageId":"20250818205929.GA1024556@coredump.intra.peff.net","threadId":"63950","inReplyTo":"20250818205812.GA1018043@coredump.intra.peff.net","subject":"[PATCH 1/5] describe: pass oid struct by const pointer","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-08-18T20:59:29Z","receivedAt":"2025-08-18T20:59:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We pass a \"struct object_id\" to describe_blob() by value. This isn't\nwrong, as an oid is composed only of copy-able values. But it's unusual;\ntypically we pass structs by const pointer, including object_ids. Let's\ndo so.\n\nIt similarly makes sense for us to hold that pointer in the callback\ndata (rather than yet another copy of the oid).\n\nSigned-off-by: Jeff King \n---\nNot strictly related, but I noticed while in the area and remembered a\nrecent discussion in this direction.\n\n builtin/describe.c | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/describe.c b/builtin/describe.c\nindex d7dd8139de..383d3e6b9a 100644\n--- a/builtin/describe.c\n+++ b/builtin/describe.c\n@@ -490,7 +490,7 @@ static void describe_commit(struct object_id *oid, struct strbuf *dst)\n \n struct process_commit_data {\n \tstruct object_id current_commit;\n-\tstruct object_id looking_for;\n+\tconst struct object_id *looking_for;\n \tstruct strbuf *dst;\n \tstruct rev_info *revs;\n };\n@@ -505,7 +505,7 @@ static void process_object(struct object *obj, const char *path, void *data)\n {\n \tstruct process_commit_data *pcd = data;\n \n-\tif (oideq(&pcd->looking_for, &obj->oid) && !pcd->dst->len) {\n+\tif (oideq(pcd->looking_for, &obj->oid) && !pcd->dst->len) {\n \t\treset_revision_walk();\n \t\tdescribe_commit(&pcd->current_commit, pcd->dst);\n \t\tstrbuf_addf(pcd->dst, \":%s\", path);\n@@ -514,7 +514,7 @@ static void process_object(struct object *obj, const char *path, void *data)\n \t}\n }\n \n-static void describe_blob(struct object_id oid, struct strbuf *dst)\n+static void describe_blob(const struct object_id *oid, struct strbuf *dst)\n {\n \tstruct rev_info revs;\n \tstruct strvec args = STRVEC_INIT;\n@@ -554,7 +554,7 @@ static void describe(const char *arg, int last_one)\n \t\tdescribe_commit(&oid, &sb);\n \telse if (odb_read_object_info(the_repository->objects,\n \t\t\t\t &oid, NULL) == OBJ_BLOB)\n-\t\tdescribe_blob(oid, &sb);\n+\t\tdescribe_blob(&oid, &sb);\n \telse\n \t\tdie(_(\"%s is neither a commit nor blob\"), arg);\n \n-- \n2.51.0.326.gecbb38d78e\n\n"},{"id":"524367","messageId":"20250818210125.GB1024556@coredump.intra.peff.net","threadId":"63950","inReplyTo":"20250818205812.GA1018043@coredump.intra.peff.net","subject":"[PATCH 2/5] describe: error if blob not found","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-08-18T21:01:25Z","receivedAt":"2025-08-18T21:01:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"If describe_blob() does not find the blob in question, it returns an\nempty strbuf, and we print an empty line. This differs from\ndescribe_commit(), which always either returns an answer or calls die()\nitself. As the blob function was bolted onto the command afterwards, I\nthink its behavior is not intentional, and it is just a bug that it does\nnot report an error.\n\nSigned-off-by: Jeff King \n---\nThis one is perhaps the most controversial, as it is a change in\nbehavior. But the current behavior just really seems like a bug to me.\n\nUnlike what René posted earlier, I didn't record the dst strbuf's\noriginal size and compare against that. This is a static function with\nonly one caller that passes in an empty strbuf, so being overly\ndefensive didn't seem worth it (arguably these functions should just\nreturn an allocated buffer anyway).\n\n builtin/describe.c | 3 +++\n t/t6120-describe.sh | 6 ++++++\n 2 files changed, 9 insertions(+)\n\ndiff --git a/builtin/describe.c b/builtin/describe.c\nindex 383d3e6b9a..06e413d937 100644\n--- a/builtin/describe.c\n+++ b/builtin/describe.c\n@@ -535,6 +535,9 @@ static void describe_blob(const struct object_id *oid, struct strbuf *dst)\n \treset_revision_walk();\n \trelease_revisions(&revs);\n \tstrvec_clear(&args);\n+\n+\tif (!dst->len)\n+\t\tdie(_(\"blob '%s' not reachable from HEAD\"), oid_to_hex(oid));\n }\n \n static void describe(const char *arg, int last_one)\ndiff --git a/t/t6120-describe.sh b/t/t6120-describe.sh\nindex 256ccaefb7..470631d17d 100755\n--- a/t/t6120-describe.sh\n+++ b/t/t6120-describe.sh\n@@ -409,6 +409,12 @@ test_expect_success 'describe tag object' '\n \ttest_grep \"fatal: test-blob-1 is neither a commit nor blob\" actual\n '\n \n+test_expect_success 'describe an unreachable blob' '\n+\tblob=$(echo not-found-anywhere | git hash-object -w --stdin) &&\n+\ttest_must_fail git describe $blob 2>actual &&\n+\ttest_grep \"blob .$blob. not reachable from HEAD\" actual\n+'\n+\n test_expect_success ULIMIT_STACK_SIZE 'name-rev works in a deep repo' '\n \ti=1 &&\n \twhile test $i -lt 8000\n-- \n2.51.0.326.gecbb38d78e\n\n"},{"id":"524368","messageId":"20250818210154.GC1024556@coredump.intra.peff.net","threadId":"63950","inReplyTo":"20250818205812.GA1018043@coredump.intra.peff.net","subject":"[PATCH 3/5] describe: catch unborn branch in describe_blob()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-08-18T21:01:54Z","receivedAt":"2025-08-18T21:01:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When describing a blob, we search for it by traversing from HEAD. We do\nthis by feeding the name HEAD to setup_revisions(). But if we are on an\nunborn branch, this will fail with a confusing message:\n\n $ git describe $blob\n fatal: ambiguous argument 'HEAD': unknown revision or path not in the working tree.\n Use '--' to separate paths from revisions, like this:\n 'git [...] -- [...]'\n\nIt is OK for this to be an error (we cannot find $blob in an empty\ntraversal, so we'd eventually complain about that). But the error\nmessage could be more helpful.\n\nLet's resolve HEAD ourselves and pass the resolved object id to\nsetup_revisions(). If resolving fails, then we can print a more useful\nmessage.\n\nSigned-off-by: Jeff King \n---\n builtin/describe.c | 8 +++++++-\n t/t6120-describe.sh | 8 ++++++++\n 2 files changed, 15 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/describe.c b/builtin/describe.c\nindex 06e413d937..f7bea3c8c5 100644\n--- a/builtin/describe.c\n+++ b/builtin/describe.c\n@@ -518,10 +518,16 @@ static void describe_blob(const struct object_id *oid, struct strbuf *dst)\n {\n \tstruct rev_info revs;\n \tstruct strvec args = STRVEC_INIT;\n+\tstruct object_id head_oid;\n \tstruct process_commit_data pcd = { *null_oid(the_hash_algo), oid, dst, &revs};\n \n+\tif (repo_get_oid(the_repository, \"HEAD\", &head_oid))\n+\t\tdie(_(\"cannot search for blob '%s' on an unborn branch\"),\n+\t\t oid_to_hex(oid));\n+\n \tstrvec_pushl(&args, \"internal: The first arg is not parsed\",\n-\t\t \"--objects\", \"--in-commit-order\", \"--reverse\", \"HEAD\",\n+\t\t \"--objects\", \"--in-commit-order\", \"--reverse\",\n+\t\t oid_to_hex(&head_oid),\n \t\t NULL);\n \n \trepo_init_revisions(the_repository, &revs, NULL);\ndiff --git a/t/t6120-describe.sh b/t/t6120-describe.sh\nindex 470631d17d..feec57bcbc 100755\n--- a/t/t6120-describe.sh\n+++ b/t/t6120-describe.sh\n@@ -415,6 +415,14 @@ test_expect_success 'describe an unreachable blob' '\n \ttest_grep \"blob .$blob. not reachable from HEAD\" actual\n '\n \n+test_expect_success 'describe blob on an unborn branch' '\n+\toldbranch=$(git symbolic-ref HEAD) &&\n+\ttest_when_finished \"git symbolic-ref HEAD $oldbranch\" &&\n+\tgit symbolic-ref HEAD refs/heads/does-not-exist &&\n+\ttest_must_fail git describe test-blob 2>actual &&\n+\ttest_grep \"cannot search .* on an unborn branch\" actual\n+'\n+\n test_expect_success ULIMIT_STACK_SIZE 'name-rev works in a deep repo' '\n \ti=1 &&\n \twhile test $i -lt 8000\n-- \n2.51.0.326.gecbb38d78e\n\n"},{"id":"524369","messageId":"20250818210312.GD1024556@coredump.intra.peff.net","threadId":"63950","inReplyTo":"20250818205812.GA1018043@coredump.intra.peff.net","subject":"[PATCH 4/5] describe: handle blob traversal with no commits","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-08-18T21:03:12Z","receivedAt":"2025-08-18T21:03:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When describing a blob, we traverse from HEAD, remembering each commit\nwe saw, and then checking each blob to report the containing commit.\nBut if we haven't seen any commits at all, we'll segfault (we store the\n\"current\" commit as an oid initialized to the null oid, causing\nlookup_commit_reference() to return NULL).\n\nThis shouldn't be able to happen normally. We always start our traversal\nat HEAD, which must be a commit (a property which is enforced by the\nrefs code). But you can trigger the segfault like this:\n\n blob=$(echo foo | git hash-object -w --stdin)\n echo $blob >.git/HEAD\n git describe $blob\n\nWe can instead catch this case and return an empty result, which hits\nthe usual \"we didn't find $blob while traversing HEAD\" error.\n\nThis is a minor lie in that we did \"find\" the blob. And this even hints\nat a bigger problem in this code: what if the traversal pointed to the\nblob as _not_ part of a commit at all, but we had previously filled in\nthe recorded \"current commit\"? One could imagine this happening due to a\ntag pointing directly to the blob in question.\n\nBut that can't happen, because we only traverse from HEAD, never from\nany other refs. And the intent of the blob-describing code is to find\nblobs within commits.\n\nSo I think this matches the original intent as closely as we can (and\nagain, this segfault cannot be triggered without corrupting your\nrepository!).\n\nI didn't include a test here because it requires corrupting the\nrepository in a way that is only easy to do using the files ref backend.\nIt doesn't seem worth carrying a REFFILES test just for this oddity.\n\nSigned-off-by: Jeff King \n---\n builtin/describe.c | 6 ++++--\n 1 file changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/describe.c b/builtin/describe.c\nindex f7bea3c8c5..72b2e1162c 100644\n--- a/builtin/describe.c\n+++ b/builtin/describe.c\n@@ -507,8 +507,10 @@ static void process_object(struct object *obj, const char *path, void *data)\n \n \tif (oideq(pcd->looking_for, &obj->oid) && !pcd->dst->len) {\n \t\treset_revision_walk();\n-\t\tdescribe_commit(&pcd->current_commit, pcd->dst);\n-\t\tstrbuf_addf(pcd->dst, \":%s\", path);\n+\t\tif (!is_null_oid(&pcd->current_commit)) {\n+\t\t\tdescribe_commit(&pcd->current_commit, pcd->dst);\n+\t\t\tstrbuf_addf(pcd->dst, \":%s\", path);\n+\t\t}\n \t\tfree_commit_list(pcd->revs->commits);\n \t\tpcd->revs->commits = NULL;\n \t}\n-- \n2.51.0.326.gecbb38d78e\n\n"},{"id":"524370","messageId":"20250818210417.GE1024556@coredump.intra.peff.net","threadId":"63950","inReplyTo":"20250818205812.GA1018043@coredump.intra.peff.net","subject":"[PATCH 5/5] describe: pass commit to describe_commit()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-08-18T21:04:17Z","receivedAt":"2025-08-18T21:04:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"There's a call in describe_commit() to lookup_commit_reference(), but we\ndon't check the return value. If it returns NULL, we'll segfault as we\nimmediately dereference the result.\n\nIn practice this can never happen, since all callers pass an oid which\ncame from a \"struct commit\" already. So we can make this more obvious\nby just taking that commit struct in the first place.\n\nReported-by: Cheng \nSigned-off-by: Jeff King \n---\n builtin/describe.c | 20 +++++++++-----------\n 1 file changed, 9 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/describe.c b/builtin/describe.c\nindex 72b2e1162c..04df89d56b 100644\n--- a/builtin/describe.c\n+++ b/builtin/describe.c\n@@ -313,26 +313,24 @@ static void append_suffix(int depth, const struct object_id *oid, struct strbuf\n \t\t repo_find_unique_abbrev(the_repository, oid, abbrev));\n }\n \n-static void describe_commit(struct object_id *oid, struct strbuf *dst)\n+static void describe_commit(struct commit *cmit, struct strbuf *dst)\n {\n-\tstruct commit *cmit, *gave_up_on = NULL;\n+\tstruct commit *gave_up_on = NULL;\n \tstruct commit_list *list;\n \tstruct commit_name *n;\n \tstruct possible_tag all_matches[MAX_TAGS];\n \tunsigned int match_cnt = 0, annotated_cnt = 0, cur_match;\n \tunsigned long seen_commits = 0;\n \tunsigned int unannotated_cnt = 0;\n \n-\tcmit = lookup_commit_reference(the_repository, oid);\n-\n \tn = find_commit_name(&cmit->object.oid);\n \tif (n && (tags || all || n->prio == 2)) {\n \t\t/*\n \t\t * Exact match to an existing ref.\n \t\t */\n \t\tappend_name(n, dst);\n \t\tif (n->misnamed || longformat)\n-\t\t\tappend_suffix(0, n->tag ? get_tagged_oid(n->tag) : oid, dst);\n+\t\t\tappend_suffix(0, n->tag ? get_tagged_oid(n->tag) : &cmit->object.oid, dst);\n \t\tif (suffix)\n \t\t\tstrbuf_addstr(dst, suffix);\n \t\treturn;\n@@ -489,7 +487,7 @@ static void describe_commit(struct object_id *oid, struct strbuf *dst)\n }\n \n struct process_commit_data {\n-\tstruct object_id current_commit;\n+\tstruct commit *current_commit;\n \tconst struct object_id *looking_for;\n \tstruct strbuf *dst;\n \tstruct rev_info *revs;\n@@ -498,7 +496,7 @@ struct process_commit_data {\n static void process_commit(struct commit *commit, void *data)\n {\n \tstruct process_commit_data *pcd = data;\n-\tpcd->current_commit = commit->object.oid;\n+\tpcd->current_commit = commit;\n }\n \n static void process_object(struct object *obj, const char *path, void *data)\n@@ -507,8 +505,8 @@ static void process_object(struct object *obj, const char *path, void *data)\n \n \tif (oideq(pcd->looking_for, &obj->oid) && !pcd->dst->len) {\n \t\treset_revision_walk();\n-\t\tif (!is_null_oid(&pcd->current_commit)) {\n-\t\t\tdescribe_commit(&pcd->current_commit, pcd->dst);\n+\t\tif (pcd->current_commit) {\n+\t\t\tdescribe_commit(pcd->current_commit, pcd->dst);\n \t\t\tstrbuf_addf(pcd->dst, \":%s\", path);\n \t\t}\n \t\tfree_commit_list(pcd->revs->commits);\n@@ -521,7 +519,7 @@ static void describe_blob(const struct object_id *oid, struct strbuf *dst)\n \tstruct rev_info revs;\n \tstruct strvec args = STRVEC_INIT;\n \tstruct object_id head_oid;\n-\tstruct process_commit_data pcd = { *null_oid(the_hash_algo), oid, dst, &revs};\n+\tstruct process_commit_data pcd = { NULL, oid, dst, &revs};\n \n \tif (repo_get_oid(the_repository, \"HEAD\", &head_oid))\n \t\tdie(_(\"cannot search for blob '%s' on an unborn branch\"),\n@@ -562,7 +560,7 @@ static void describe(const char *arg, int last_one)\n \tcmit = lookup_commit_reference_gently(the_repository, &oid, 1);\n \n \tif (cmit)\n-\t\tdescribe_commit(&oid, &sb);\n+\t\tdescribe_commit(cmit, &sb);\n \telse if (odb_read_object_info(the_repository->objects,\n \t\t\t\t &oid, NULL) == OBJ_BLOB)\n \t\tdescribe_blob(&oid, &sb);\n-- \n2.51.0.326.gecbb38d78e\n"},{"id":"524371","messageId":"xmqqfrdoibmn.fsf@gitster.g","threadId":"63950","inReplyTo":"20250818205929.GA1024556@coredump.intra.peff.net","subject":"Re: [PATCH 1/5] describe: pass oid struct by const pointer","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-08-18T21:05:36Z","receivedAt":"2025-08-18T21:05:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> We pass a \"struct object_id\" to describe_blob() by value. This isn't\n> wrong, as an oid is composed only of copy-able values. But it's unusual;\n> typically we pass structs by const pointer, including object_ids. Let's\n> do so.\n>\n> It similarly makes sense for us to hold that pointer in the callback\n> data (rather than yet another copy of the oid).\n>\n> Signed-off-by: Jeff King \n> ---\n> Not strictly related, but I noticed while in the area and remembered a\n> recent discussion in this direction.\n\nThanks for making this part of the code less odd.\n\n> builtin/describe.c | 8 ++++----\n> 1 file changed, 4 insertions(+), 4 deletions(-)\n>\n> diff --git a/builtin/describe.c b/builtin/describe.c\n> index d7dd8139de..383d3e6b9a 100644\n> --- a/builtin/describe.c\n> +++ b/builtin/describe.c\n> @@ -490,7 +490,7 @@ static void describe_commit(struct object_id *oid, struct strbuf *dst)\n> \n> struct process_commit_data {\n> \tstruct object_id current_commit;\n> -\tstruct object_id looking_for;\n> +\tconst struct object_id *looking_for;\n> \tstruct strbuf *dst;\n> \tstruct rev_info *revs;\n> };\n> @@ -505,7 +505,7 @@ static void process_object(struct object *obj, const char *path, void *data)\n> {\n> \tstruct process_commit_data *pcd = data;\n> \n> -\tif (oideq(&pcd->looking_for, &obj->oid) && !pcd->dst->len) {\n> +\tif (oideq(pcd->looking_for, &obj->oid) && !pcd->dst->len) {\n> \t\treset_revision_walk();\n> \t\tdescribe_commit(&pcd->current_commit, pcd->dst);\n> \t\tstrbuf_addf(pcd->dst, \":%s\", path);\n> @@ -514,7 +514,7 @@ static void process_object(struct object *obj, const char *path, void *data)\n> \t}\n> }\n> \n> -static void describe_blob(struct object_id oid, struct strbuf *dst)\n> +static void describe_blob(const struct object_id *oid, struct strbuf *dst)\n> {\n> \tstruct rev_info revs;\n> \tstruct strvec args = STRVEC_INIT;\n> @@ -554,7 +554,7 @@ static void describe(const char *arg, int last_one)\n> \t\tdescribe_commit(&oid, &sb);\n> \telse if (odb_read_object_info(the_repository->objects,\n> \t\t\t\t &oid, NULL) == OBJ_BLOB)\n> -\t\tdescribe_blob(oid, &sb);\n> +\t\tdescribe_blob(&oid, &sb);\n> \telse\n> \t\tdie(_(\"%s is neither a commit nor blob\"), arg);\n"},{"id":"524372","messageId":"xmqqbjocibbk.fsf@gitster.g","threadId":"63950","inReplyTo":"20250818210125.GB1024556@coredump.intra.peff.net","subject":"Re: [PATCH 2/5] describe: error if blob not found","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-08-18T21:12:15Z","receivedAt":"2025-08-18T21:12:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> If describe_blob() does not find the blob in question, it returns an\n> empty strbuf, and we print an empty line. This differs from\n> describe_commit(), which always either returns an answer or calls die()\n> itself. As the blob function was bolted onto the command afterwards, I\n> think its behavior is not intentional, and it is just a bug that it does\n> not report an error.\n\nYes, let's do so. Silently succeeding without returning anything\nuseful is not what we usually do in this system.\n\n> This one is perhaps the most controversial, as it is a change in\n> behavior. But the current behavior just really seems like a bug to me.\n>\n> Unlike what René posted earlier, I didn't record the dst strbuf's\n> original size and compare against that. This is a static function with\n> only one caller that passes in an empty strbuf, so being overly\n> defensive didn't seem worth it (arguably these functions should just\n> return an allocated buffer anyway).\n\nSounds sensible.\n\nThanks.\n\n> diff --git a/builtin/describe.c b/builtin/describe.c\n> index 383d3e6b9a..06e413d937 100644\n> --- a/builtin/describe.c\n> +++ b/builtin/describe.c\n> @@ -535,6 +535,9 @@ static void describe_blob(const struct object_id *oid, struct strbuf *dst)\n> \treset_revision_walk();\n> \trelease_revisions(&revs);\n> \tstrvec_clear(&args);\n> +\n> +\tif (!dst->len)\n> +\t\tdie(_(\"blob '%s' not reachable from HEAD\"), oid_to_hex(oid));\n> }\n> \n> static void describe(const char *arg, int last_one)\n> diff --git a/t/t6120-describe.sh b/t/t6120-describe.sh\n> index 256ccaefb7..470631d17d 100755\n> --- a/t/t6120-describe.sh\n> +++ b/t/t6120-describe.sh\n> @@ -409,6 +409,12 @@ test_expect_success 'describe tag object' '\n> \ttest_grep \"fatal: test-blob-1 is neither a commit nor blob\" actual\n> '\n> \n> +test_expect_success 'describe an unreachable blob' '\n> +\tblob=$(echo not-found-anywhere | git hash-object -w --stdin) &&\n> +\ttest_must_fail git describe $blob 2>actual &&\n> +\ttest_grep \"blob .$blob. not reachable from HEAD\" actual\n> +'\n> +\n> test_expect_success ULIMIT_STACK_SIZE 'name-rev works in a deep repo' '\n> \ti=1 &&\n> \twhile test $i -lt 8000\n"},{"id":"524373","messageId":"xmqqy0rggwep.fsf@gitster.g","threadId":"63950","inReplyTo":"20250818210154.GC1024556@coredump.intra.peff.net","subject":"Re: [PATCH 3/5] describe: catch unborn branch in describe_blob()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-08-18T21:19:42Z","receivedAt":"2025-08-18T21:19:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> --- a/builtin/describe.c\n> +++ b/builtin/describe.c\n> @@ -518,10 +518,16 @@ static void describe_blob(const struct object_id *oid, struct strbuf *dst)\n> {\n> \tstruct rev_info revs;\n> \tstruct strvec args = STRVEC_INIT;\n> +\tstruct object_id head_oid;\n> \tstruct process_commit_data pcd = { *null_oid(the_hash_algo), oid, dst, &revs};\n\nAn unrelated tangent, but it seems that we are copying the object\nname for the first member of this struct, even though 1/5 changed\nthe second one.\n\n> +\tif (repo_get_oid(the_repository, \"HEAD\", &head_oid))\n> +\t\tdie(_(\"cannot search for blob '%s' on an unborn branch\"),\n> +\t\t oid_to_hex(oid));\n\nMakes sense. I briefly wondered if there is really a point in doing\nthe traversal only from HEAD, but this topic is not about enhancing\nand making the \"describe \" more useful, but it is a strict\nimprovement. When you first mentioned \"resolve HEAD ourselves\", I\nsomehow expected you to ask the ref subsystem to resolve HEAD, but\nthis should do fine, thanks to ref_rev_parse_rules[].\n\n> diff --git a/t/t6120-describe.sh b/t/t6120-describe.sh\n> index 470631d17d..feec57bcbc 100755\n> --- a/t/t6120-describe.sh\n> +++ b/t/t6120-describe.sh\n> @@ -415,6 +415,14 @@ test_expect_success 'describe an unreachable blob' '\n> \ttest_grep \"blob .$blob. not reachable from HEAD\" actual\n> '\n> \n> +test_expect_success 'describe blob on an unborn branch' '\n> +\toldbranch=$(git symbolic-ref HEAD) &&\n> +\ttest_when_finished \"git symbolic-ref HEAD $oldbranch\" &&\n> +\tgit symbolic-ref HEAD refs/heads/does-not-exist &&\n> +\ttest_must_fail git describe test-blob 2>actual &&\n> +\ttest_grep \"cannot search .* on an unborn branch\" actual\n> +'\n> +\n> test_expect_success ULIMIT_STACK_SIZE 'name-rev works in a deep repo' '\n> \ti=1 &&\n> \twhile test $i -lt 8000\n"},{"id":"524376","messageId":"20250818230717.GA1027509@coredump.intra.peff.net","threadId":"63950","inReplyTo":"xmqqy0rggwep.fsf@gitster.g","subject":"Re: [PATCH 3/5] describe: catch unborn branch in describe_blob()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-08-18T23:07:17Z","receivedAt":"2025-08-18T23:07:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 18, 2025 at 02:19:42PM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > --- a/builtin/describe.c\n> > +++ b/builtin/describe.c\n> > @@ -518,10 +518,16 @@ static void describe_blob(const struct object_id *oid, struct strbuf *dst)\n> > {\n> > \tstruct rev_info revs;\n> > \tstruct strvec args = STRVEC_INIT;\n> > +\tstruct object_id head_oid;\n> > \tstruct process_commit_data pcd = { *null_oid(the_hash_algo), oid, dst, &revs};\n> \n> An unrelated tangent, but it seems that we are copying the object\n> name for the first member of this struct, even though 1/5 changed\n> the second one.\n\nYep, agreed. It goes away in the final patch, though.\n\n> > +\tif (repo_get_oid(the_repository, \"HEAD\", &head_oid))\n> > +\t\tdie(_(\"cannot search for blob '%s' on an unborn branch\"),\n> > +\t\t oid_to_hex(oid));\n> \n> Makes sense. I briefly wondered if there is really a point in doing\n> the traversal only from HEAD, but this topic is not about enhancing\n> and making the \"describe \" more useful, but it is a strict\n> improvement.\n\nYeah. We do document the traversal from HEAD, so I wondered if anybody\nwould be upset at being more inclusive. So it might need a new option.\nSomebody who is more interested in the option is welcome to pick up that\ntopic. :)\n\n> When you first mentioned \"resolve HEAD ourselves\", I somehow expected\n> you to ask the ref subsystem to resolve HEAD, but this should do fine,\n> thanks to ref_rev_parse_rules[].\n\nI hadn't really thought about the distinction. If you grep for '\"HEAD\"',\nlooks like we have a mix of both types. I don't think it should matter\nmuch in practice.\n\nSome even call lookup_commit_reference_by_name(). So we could perhaps do\nthat, in which case the segfault fix in patch 4 just happens naturally,\nbecause we know we are always traversing from at least one commit. I\ndon't see any difference between the approaches compelling enough to\nswitch, though.\n\n-Peff\n"},{"id":"524389","messageId":"aKQwNFMo-N5VwB2v@pks.im","threadId":"63950","inReplyTo":"xmqqbjocibbk.fsf@gitster.g","subject":"Re: [PATCH 2/5] describe: error if blob not found","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-08-19T08:05:08Z","receivedAt":"2025-08-19T08:05:22Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Aug 18, 2025 at 02:12:15PM -0700, Junio C Hamano wrote:\n> Jeff King writes:\n> \n> > If describe_blob() does not find the blob in question, it returns an\n> > empty strbuf, and we print an empty line. This differs from\n> > describe_commit(), which always either returns an answer or calls die()\n> > itself. As the blob function was bolted onto the command afterwards, I\n> > think its behavior is not intentional, and it is just a bug that it does\n> > not report an error.\n> \n> Yes, let's do so. Silently succeeding without returning anything\n> useful is not what we usually do in this system.\n\nSeconded. It might be breaking backwards compatibility, but it really\nsounds like just another bug in our codebase that ought to be fixed.\n\nPatrick\n"},{"id":"524390","messageId":"aKQwP01nulsquhzi@pks.im","threadId":"63950","inReplyTo":"20250818210312.GD1024556@coredump.intra.peff.net","subject":"Re: [PATCH 4/5] describe: handle blob traversal with no commits","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-08-19T08:05:19Z","receivedAt":"2025-08-19T08:05:25Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Aug 18, 2025 at 05:03:12PM -0400, Jeff King wrote:\n> When describing a blob, we traverse from HEAD, remembering each commit\n> we saw, and then checking each blob to report the containing commit.\n> But if we haven't seen any commits at all, we'll segfault (we store the\n> \"current\" commit as an oid initialized to the null oid, causing\n> lookup_commit_reference() to return NULL).\n> \n> This shouldn't be able to happen normally. We always start our traversal\n> at HEAD, which must be a commit (a property which is enforced by the\n> refs code). But you can trigger the segfault like this:\n> \n> blob=$(echo foo | git hash-object -w --stdin)\n> echo $blob >.git/HEAD\n> git describe $blob\n\nI bet that the enforcement is only of partial nature, and that there are\nways to do the above e.g. via git-update-ref(1) or by playing around\nwith symrefs.\n\n[snip]\n> I didn't include a test here because it requires corrupting the\n> repository in a way that is only easy to do using the files ref backend.\n> It doesn't seem worth carrying a REFFILES test just for this oddity.\n\nTrue:\n\n $ git update-ref HEAD HEAD^{tree}\n fatal: update_ref failed for ref 'HEAD': trying to write non-commit object 4b825dc642cb6eb9a060e54bf8d69288fbee4904 to branch 'HEAD'\n\nBut:\n\n $ git update-ref refs/some/tree HEAD^{tree}\n $ git symbolic-ref HEAD refs/some/tree\n $ git show\n tree HEAD\n\nSo that should allow you to write a test, right?\n\nPatrick\n"},{"id":"524391","messageId":"aKQwRaX94uwTwiQP@pks.im","threadId":"63950","inReplyTo":"20250818210417.GE1024556@coredump.intra.peff.net","subject":"Re: [PATCH 5/5] describe: pass commit to describe_commit()","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-08-19T08:05:25Z","receivedAt":"2025-08-19T08:05:31Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Aug 18, 2025 at 05:04:17PM -0400, Jeff King wrote:\n> There's a call in describe_commit() to lookup_commit_reference(), but we\n> don't check the return value. If it returns NULL, we'll segfault as we\n> immediately dereference the result.\n> \n> In practice this can never happen, since all callers pass an oid which\n> came from a \"struct commit\" already. So we can make this more obvious\n> by just taking that commit struct in the first place.\n\nI was wondering a bit about commit-graphs. We had the case in the past\nwhere it was possible to look up commits via the graph even though they\ndon't exist in the ODB. So we might actually end up with a missing\nobject if `GIT_COMMIT_GRAPH_PARANOIA=false`, which is the default value.\nBut that might be fine? No idea without digging further.\n\nIn any case, the refactoring makes sense regardless from my point of\nview.\n\nPatrick\n"},{"id":"524454","messageId":"20250819165947.GA1050577@coredump.intra.peff.net","threadId":"63950","inReplyTo":"aKQwP01nulsquhzi@pks.im","subject":"Re: [PATCH 4/5] describe: handle blob traversal with no commits","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-08-19T16:59:47Z","receivedAt":"2025-08-19T16:59:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 19, 2025 at 10:05:19AM +0200, Patrick Steinhardt wrote:\n\n> > I didn't include a test here because it requires corrupting the\n> > repository in a way that is only easy to do using the files ref backend.\n> > It doesn't seem worth carrying a REFFILES test just for this oddity.\n> \n> True:\n> \n> $ git update-ref HEAD HEAD^{tree}\n> fatal: update_ref failed for ref 'HEAD': trying to write non-commit object 4b825dc642cb6eb9a060e54bf8d69288fbee4904 to branch 'HEAD'\n> \n> But:\n> \n> $ git update-ref refs/some/tree HEAD^{tree}\n> $ git symbolic-ref HEAD refs/some/tree\n> $ git show\n> tree HEAD\n> \n> So that should allow you to write a test, right?\n\nHrm, that seems like a bug. I thought we insisted that HEAD point at\nrefs/heads.\n\nAh, no. We did that in b229d18a80 (validate_headref: tighten\nref-matching to just branches, 2009-01-29), but had to revert it in\ne9cc02f0e4 (symbolic-ref: allow refs/ in HEAD, 2009-02-13) to\nkeep compatibility for topgit. :(\n\nStill, I'm not sure it's something I'd want to base a test on. Maybe if\nthere is a big comment that says \"It is OK to invalidate and remove this\ntest if we ever tighten symbolic-ref\" it would be OK?\n\n-Peff\n"},{"id":"524455","messageId":"20250819170255.GB1050577@coredump.intra.peff.net","threadId":"63950","inReplyTo":"aKQwRaX94uwTwiQP@pks.im","subject":"Re: [PATCH 5/5] describe: pass commit to describe_commit()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-08-19T17:02:55Z","receivedAt":"2025-08-19T17:02:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 19, 2025 at 10:05:25AM +0200, Patrick Steinhardt wrote:\n\n> On Mon, Aug 18, 2025 at 05:04:17PM -0400, Jeff King wrote:\n> > There's a call in describe_commit() to lookup_commit_reference(), but we\n> > don't check the return value. If it returns NULL, we'll segfault as we\n> > immediately dereference the result.\n> > \n> > In practice this can never happen, since all callers pass an oid which\n> > came from a \"struct commit\" already. So we can make this more obvious\n> > by just taking that commit struct in the first place.\n> \n> I was wondering a bit about commit-graphs. We had the case in the past\n> where it was possible to look up commits via the graph even though they\n> don't exist in the ODB. So we might actually end up with a missing\n> object if `GIT_COMMIT_GRAPH_PARANOIA=false`, which is the default value.\n> But that might be fine? No idea without digging further.\n\nI don't think existence matters here. Any call to parse_object() will\ncall lookup_object() and find the same commit struct we already had, and\nthen exit immediately because its \"parsed\" flag is set. So we'd never\nget a NULL return from lookup_commit_reference().\n\n> In any case, the refactoring makes sense regardless from my point of\n> view.\n\nBut yeah. Even if I am wrong above, this would fix it. ;)\n\n-Peff\n"},{"id":"524462","messageId":"efa90ac9-8190-4af5-9e63-ffbd04c3c9c1@web.de","threadId":"63950","inReplyTo":"20250818210125.GB1024556@coredump.intra.peff.net","subject":"Re: [PATCH 2/5] describe: error if blob not found","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2025-08-19T18:32:03Z","receivedAt":"2025-08-19T18:32:06Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"On 8/18/25 11:01 PM, Jeff King wrote:\n> If describe_blob() does not find the blob in question, it returns an\n> empty strbuf, and we print an empty line. This differs from\n> describe_commit(), which always either returns an answer or calls die()\n> itself. As the blob function was bolted onto the command afterwards, I\n> think its behavior is not intentional, and it is just a bug that it does\n> not report an error.\n> \n> Signed-off-by: Jeff King \n> ---\n> This one is perhaps the most controversial, as it is a change in\n> behavior. But the current behavior just really seems like a bug to me.\n> \n> Unlike what René posted earlier, I didn't record the dst strbuf's\n> original size and compare against that. This is a static function with\n> only one caller that passes in an empty strbuf, so being overly\n> defensive didn't seem worth it\n\nMakes sense.\n\n> (arguably these functions should just\n> return an allocated buffer anyway).\n\nOr even print results directly.\n\n> builtin/describe.c | 3 +++\n> t/t6120-describe.sh | 6 ++++++\n> 2 files changed, 9 insertions(+)\n> \n> diff --git a/builtin/describe.c b/builtin/describe.c\n> index 383d3e6b9a..06e413d937 100644\n> --- a/builtin/describe.c\n> +++ b/builtin/describe.c\n> @@ -535,6 +535,9 @@ static void describe_blob(const struct object_id *oid, struct strbuf *dst)\n> \treset_revision_walk();\n> \trelease_revisions(&revs);\n> \tstrvec_clear(&args);\n> +\n> +\tif (!dst->len)\n> +\t\tdie(_(\"blob '%s' not reachable from HEAD\"), oid_to_hex(oid));\n\nI like the clarity and precision of this message. \n\nThe rest of the patches look good to me as well. The first is a nice\nlittle bonus.\n\nRené\n\n> }\n> \n> static void describe(const char *arg, int last_one)\n> diff --git a/t/t6120-describe.sh b/t/t6120-describe.sh\n> index 256ccaefb7..470631d17d 100755\n> --- a/t/t6120-describe.sh\n> +++ b/t/t6120-describe.sh\n> @@ -409,6 +409,12 @@ test_expect_success 'describe tag object' '\n> \ttest_grep \"fatal: test-blob-1 is neither a commit nor blob\" actual\n> '\n> \n> +test_expect_success 'describe an unreachable blob' '\n> +\tblob=$(echo not-found-anywhere | git hash-object -w --stdin) &&\n> +\ttest_must_fail git describe $blob 2>actual &&\n> +\ttest_grep \"blob .$blob. not reachable from HEAD\" actual\n> +'\n> +\n> test_expect_success ULIMIT_STACK_SIZE 'name-rev works in a deep repo' '\n> \ti=1 &&\n> \twhile test $i -lt 8000\n\n"},{"id":"524501","messageId":"aKVQSNgYgt2RO8hd@pks.im","threadId":"63950","inReplyTo":"20250819165947.GA1050577@coredump.intra.peff.net","subject":"Re: [PATCH 4/5] describe: handle blob traversal with no commits","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-08-20T04:34:16Z","receivedAt":"2025-08-20T04:34:25Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Aug 19, 2025 at 12:59:47PM -0400, Jeff King wrote:\n> On Tue, Aug 19, 2025 at 10:05:19AM +0200, Patrick Steinhardt wrote:\n> \n> > > I didn't include a test here because it requires corrupting the\n> > > repository in a way that is only easy to do using the files ref backend.\n> > > It doesn't seem worth carrying a REFFILES test just for this oddity.\n> > \n> > True:\n> > \n> > $ git update-ref HEAD HEAD^{tree}\n> > fatal: update_ref failed for ref 'HEAD': trying to write non-commit object 4b825dc642cb6eb9a060e54bf8d69288fbee4904 to branch 'HEAD'\n> > \n> > But:\n> > \n> > $ git update-ref refs/some/tree HEAD^{tree}\n> > $ git symbolic-ref HEAD refs/some/tree\n> > $ git show\n> > tree HEAD\n> > \n> > So that should allow you to write a test, right?\n> \n> Hrm, that seems like a bug. I thought we insisted that HEAD point at\n> refs/heads.\n> \n> Ah, no. We did that in b229d18a80 (validate_headref: tighten\n> ref-matching to just branches, 2009-01-29), but had to revert it in\n> e9cc02f0e4 (symbolic-ref: allow refs/ in HEAD, 2009-02-13) to\n> keep compatibility for topgit. :(\n\nWell, that's certainly from before my time in the Git project :) I guess\nchanging semantics now would be quite risky. Reintroducing this change\nfeels out of the picture, but an alternative one could think about is to\nvalidate that HEAD always points to a commit(-ish?).\n\nBut ultimately I'm not sure it's even worth it. If people really want to\nshoot themselves into the foot they'll find a way to do so.\n\n> Still, I'm not sure it's something I'd want to base a test on. Maybe if\n> there is a big comment that says \"It is OK to invalidate and remove this\n> test if we ever tighten symbolic-ref\" it would be OK?\n\nThat sounds reasonable to me, yeah.\n\nPatrick\n"},{"id":"524510","messageId":"20250820063034.GA1087703@coredump.intra.peff.net","threadId":"63950","inReplyTo":"aKVQSNgYgt2RO8hd@pks.im","subject":"[replacement PATCH 4/5] describe: handle blob traversal with no commits","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-08-20T06:30:34Z","receivedAt":"2025-08-20T06:30:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Aug 20, 2025 at 06:34:16AM +0200, Patrick Steinhardt wrote:\n\n> > Ah, no. We did that in b229d18a80 (validate_headref: tighten\n> > ref-matching to just branches, 2009-01-29), but had to revert it in\n> > e9cc02f0e4 (symbolic-ref: allow refs/ in HEAD, 2009-02-13) to\n> > keep compatibility for topgit. :(\n> \n> Well, that's certainly from before my time in the Git project :) I guess\n> changing semantics now would be quite risky. Reintroducing this change\n> feels out of the picture, but an alternative one could think about is to\n> validate that HEAD always points to a commit(-ish?).\n\nYeah, that's _probably_ OK. I don't remember how topgit works at all,\nbut I think its custom refs do at least point to commits.\n\n> But ultimately I'm not sure it's even worth it. If people really want to\n> shoot themselves into the foot they'll find a way to do so.\n\nYeah, agreed that it's probably not urgent.\n\n> > Still, I'm not sure it's something I'd want to base a test on. Maybe if\n> > there is a big comment that says \"It is OK to invalidate and remove this\n> > test if we ever tighten symbolic-ref\" it would be OK?\n> \n> That sounds reasonable to me, yeah.\n\nOK. Here's a replacement for patch 4, then, with a test. Nothing else in\nthe series should need to be touched.\n\n-- >8 --\nSubject: [PATCH] describe: handle blob traversal with no commits\n\nWhen describing a blob, we traverse from HEAD, remembering each commit\nwe saw, and then checking each blob to report the containing commit.\nBut if we haven't seen any commits at all, we'll segfault (we store the\n\"current\" commit as an oid initialized to the null oid, causing\nlookup_commit_reference() to return NULL).\n\nThis shouldn't be able to happen normally. We always start our traversal\nat HEAD, which must be a commit (a property which is enforced by the\nrefs code). But you can trigger the segfault like this:\n\n blob=$(echo foo | git hash-object -w --stdin)\n echo $blob >.git/HEAD\n git describe $blob\n\nWe can instead catch this case and return an empty result, which hits\nthe usual \"we didn't find $blob while traversing HEAD\" error.\n\nThis is a minor lie in that we did \"find\" the blob. And this even hints\nat a bigger problem in this code: what if the traversal pointed to the\nblob as _not_ part of a commit at all, but we had previously filled in\nthe recorded \"current commit\"? One could imagine this happening due to a\ntag pointing directly to the blob in question.\n\nBut that can't happen, because we only traverse from HEAD, never from\nany other refs. And the intent of the blob-describing code is to find\nblobs within commits.\n\nSo I think this matches the original intent as closely as we can (and\nagain, this segfault cannot be triggered without corrupting your\nrepository!).\n\nThe test here does not use the formula above, which works only for the\nfiles backend (and not reftables). Instead we use another loophole to\ncreate the bogus state using only Git commands. See the comment in the\ntest for details.\n\nSigned-off-by: Jeff King \n---\n builtin/describe.c | 6 ++++--\n t/t6120-describe.sh | 16 ++++++++++++++++\n 2 files changed, 20 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/describe.c b/builtin/describe.c\nindex f7bea3c8c5..72b2e1162c 100644\n--- a/builtin/describe.c\n+++ b/builtin/describe.c\n@@ -507,8 +507,10 @@ static void process_object(struct object *obj, const char *path, void *data)\n \n \tif (oideq(pcd->looking_for, &obj->oid) && !pcd->dst->len) {\n \t\treset_revision_walk();\n-\t\tdescribe_commit(&pcd->current_commit, pcd->dst);\n-\t\tstrbuf_addf(pcd->dst, \":%s\", path);\n+\t\tif (!is_null_oid(&pcd->current_commit)) {\n+\t\t\tdescribe_commit(&pcd->current_commit, pcd->dst);\n+\t\t\tstrbuf_addf(pcd->dst, \":%s\", path);\n+\t\t}\n \t\tfree_commit_list(pcd->revs->commits);\n \t\tpcd->revs->commits = NULL;\n \t}\ndiff --git a/t/t6120-describe.sh b/t/t6120-describe.sh\nindex feec57bcbc..2c70cc561a 100755\n--- a/t/t6120-describe.sh\n+++ b/t/t6120-describe.sh\n@@ -423,6 +423,22 @@ test_expect_success 'describe blob on an unborn branch' '\n \ttest_grep \"cannot search .* on an unborn branch\" actual\n '\n \n+# This test creates a repository state that we generally try to disallow: HEAD\n+# is pointing to an object that is not a commit. The ref update code forbids\n+# non-commit writes directly to HEAD or to any branch in refs/heads/. But we\n+# can use the loophole of pointing HEAD to another non-branch ref (something we\n+# should forbid, but don't for historical reasons).\n+#\n+# Do not take this test as an endorsement of the loophole! If we ever tighten\n+# it, it is reasonable to just drop this test entirely.\n+test_expect_success 'describe blob on a non-commit HEAD' '\n+\toldbranch=$(git symbolic-ref HEAD) &&\n+\ttest_when_finished \"git symbolic-ref HEAD $oldbranch\" &&\n+\tgit symbolic-ref HEAD refs/tags/test-blob &&\n+\ttest_must_fail git describe test-blob 2>actual &&\n+\ttest_grep \"blob .* not reachable from HEAD\" actual\n+'\n+\n test_expect_success ULIMIT_STACK_SIZE 'name-rev works in a deep repo' '\n \ti=1 &&\n \twhile test $i -lt 8000\n-- \n2.51.0.326.gecbb38d78e\n\n"}]}