{"thread":{"id":"65318","subject":"[PATCH v1] path-walk: fix NULL pointer dereference in error message","startedAt":"2026-03-20T11:48:31Z","lastAt":"2026-03-23T09:45:59Z","messageCount":7,"participants":["Yuvraj Singh Chauhan","D. Ben Knoble","Derrick Stolee","Junio C Hamano","René Scharfe"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"539528","messageId":"20260320114823.3151961-1-ysinghcin@gmail.com","threadId":"65318","inReplyTo":null,"subject":"[PATCH v1] path-walk: fix NULL pointer dereference in error message","fromName":"Yuvraj Singh Chauhan","fromEmail":"ysinghcin@gmail.com","sentAt":"2026-03-20T11:48:23Z","receivedAt":"2026-03-20T11:48:31Z","isPatch":true,"sender":{"key":"ysinghcin@gmail.com","avatar":"https://avatars.githubusercontent.com/u/133221941?v=4"},"body":"When lookup_tree() or lookup_blob() cannot find a tree entry's object,\n'o' is set to NULL via:\n\n o = child ? &child->object : NULL;\n\nThe subsequent null-check catches this correctly, but then dereferences\n'o' to format the error message:\n\n error(_(\"failed to find object %s\"), oid_to_hex(&o->oid));\n\nThis causes a segfault instead of the intended diagnostic output.\n\nFix this by using &entry.oid instead. 'entry' is the struct name_entry\npopulated by tree_entry() on each loop iteration and holds the OID of\nthe failing lookup -- which is exactly what the error should report.\n\nThis crash is reachable via git-backfill(1) when a tree entry's object\nis absent from the local object database.\n\nSigned-off-by: Yuvraj Singh Chauhan \n---\n path-walk.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/path-walk.c b/path-walk.c\nindex 364e4cfa19..839582380c 100644\n--- a/path-walk.c\n+++ b/path-walk.c\n@@ -171,7 +171,7 @@ static int add_tree_entries(struct path_walk_context *ctx,\n \n \t\tif (!o) {\n \t\t\terror(_(\"failed to find object %s\"),\n-\t\t\t oid_to_hex(&o->oid));\n+\t\t\t oid_to_hex(&entry.oid));\n \t\t\treturn -1;\n \t\t}\n \n-- \n2.53.0.582.gca1db8a0f7\n\n"},{"id":"539558","messageId":"CALnO6CDnwYaAPhp67kaYWtV48ULjWAR6ks1khVXmSs1oWUbRDQ@mail.gmail.com","threadId":"65318","inReplyTo":"20260320114823.3151961-1-ysinghcin@gmail.com","subject":"Re: [PATCH v1] path-walk: fix NULL pointer dereference in error message","fromName":"D. Ben Knoble","fromEmail":"ben.knoble@gmail.com","sentAt":"2026-03-20T15:16:48Z","receivedAt":"2026-03-20T15:17:04Z","isPatch":true,"sender":{"key":"ben.knoble@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22802209?v=4"},"body":"On Fri, Mar 20, 2026 at 7:50 AM Yuvraj Singh Chauhan\n wrote:\n>\n> When lookup_tree() or lookup_blob() cannot find a tree entry's object,\n> 'o' is set to NULL via:\n>\n> o = child ? &child->object : NULL;\n>\n> The subsequent null-check catches this correctly, but then dereferences\n> 'o' to format the error message:\n>\n> error(_(\"failed to find object %s\"), oid_to_hex(&o->oid));\n>\n> This causes a segfault instead of the intended diagnostic output.\n>\n> Fix this by using &entry.oid instead. 'entry' is the struct name_entry\n> populated by tree_entry() on each loop iteration and holds the OID of\n> the failing lookup -- which is exactly what the error should report.\n>\n> This crash is reachable via git-backfill(1) when a tree entry's object\n> is absent from the local object database.\n>\n> Signed-off-by: Yuvraj Singh Chauhan \n> ---\n> path-walk.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/path-walk.c b/path-walk.c\n> index 364e4cfa19..839582380c 100644\n> --- a/path-walk.c\n> +++ b/path-walk.c\n> @@ -171,7 +171,7 @@ static int add_tree_entries(struct path_walk_context *ctx,\n>\n> if (!o) {\n> error(_(\"failed to find object %s\"),\n> - oid_to_hex(&o->oid));\n> + oid_to_hex(&entry.oid));\n> return -1;\n> }\n>\n> --\n> 2.53.0.582.gca1db8a0f7\n\nInteresting find. I was hoping to see an easy way to reproduce hitting\nthis code, and after grepping around a bit I found a few places that\nend up in this code (git-backfill and git-repo being the primary\ncallers of walk_objects_by_path), but on second glance I think \"!o\" is\ncurrent dead code.\n\nWhen we compute \"child\" in either preceding branch using lookup_tree\nor lookup_blob, we only return NULL if !quiet in the object_as_type\ncalls (assuming we hit the \"else\" case there, anyway). But quiet==0 in\nboth callers along this path, so !quiet will be truthy and we'll\nerror() out there instead, never returning to add_tree_entries.\n\nSince I didn't quickly come up with a reproduction, I can't quite\nprove this, anyway. It's also possible my analysis is based on code\nthat has since changed (I happened to have a537e3e6e9 (Merge branch\n'sp/send-email-validate-charset' into next, 2026-03-06) checked out at\nthe moment).\n\nStill, fixing such obviously wrong dereference is good, but I wonder\nif we should go further?\n\nYou mentioned git-backfill with a tree missing from the local odb; do\nyou have a short reproduction script or test-case?\n\n-- \nD. Ben Knoble\n"},{"id":"539562","messageId":"994f92e9-3576-455a-a142-0fefc559131c@gmail.com","threadId":"65318","inReplyTo":"CALnO6CDnwYaAPhp67kaYWtV48ULjWAR6ks1khVXmSs1oWUbRDQ@mail.gmail.com","subject":"Re: [PATCH v1] path-walk: fix NULL pointer dereference in error message","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2026-03-20T16:14:19Z","receivedAt":"2026-03-20T16:14:22Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 3/20/2026 11:16 AM, D. Ben Knoble wrote:\n> On Fri, Mar 20, 2026 at 7:50 AM Yuvraj Singh Chauhan\n> wrote:\n>> @@ -171,7 +171,7 @@ static int add_tree_entries(struct path_walk_context *ctx,\n>>\n>> if (!o) {\n>> error(_(\"failed to find object %s\"),\n>> - oid_to_hex(&o->oid));\n>> + oid_to_hex(&entry.oid));\n>> return -1;\n>> }\n>>\n>> --\n>> 2.53.0.582.gca1db8a0f7\n> \n> Interesting find. I was hoping to see an easy way to reproduce hitting\n> this code, and after grepping around a bit I found a few places that\n> end up in this code (git-backfill and git-repo being the primary\n> callers of walk_objects_by_path), but on second glance I think \"!o\" is\n> current dead code.\n\nI can appreciate that the existing code is clearly incorrect, so\ntooling scanning code for defects would find this even if we can't\neasily create a test case to demonstrate it.\n \n> Still, fixing such obviously wrong dereference is good, but I wonder\n> if we should go further?\n> \n> You mentioned git-backfill with a tree missing from the local odb; do\n> you have a short reproduction script or test-case?\n\nI imagine that it would be difficult to set up such a case, but maybe\nit would follow these steps (based on existing 'git backfill' tests\nthat start with a partial clone):\n\n1. Make a bare, blobless partial clone of the server repo.\n\n2. Explode the client repo's object store into loose objects.\n\n3. Delete a loose tree object, but one that isn't a commit's root\n tree. It must be a child tree.\n\nIn this case, we should hit this issue. Blobless partial clones\nexpect all reachable trees to exist locally and so are not prepared\nto download missing trees on-demand.\n\nThanks,\n-Stolee\n\n\n \n\n"},{"id":"539570","messageId":"xmqqy0jme8ea.fsf@gitster.g","threadId":"65318","inReplyTo":"CALnO6CDnwYaAPhp67kaYWtV48ULjWAR6ks1khVXmSs1oWUbRDQ@mail.gmail.com","subject":"Re: [PATCH v1] path-walk: fix NULL pointer dereference in error message","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-03-20T17:08:13Z","receivedAt":"2026-03-20T17:08:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"D. Ben Knoble\" writes:\n\n> You mentioned git-backfill with a tree missing from the local odb; do\n> you have a short reproduction script or test-case?\n\nInteresting thing to ask. THe code looks correct, though.\n"},{"id":"539577","messageId":"98833ee0-4d63-4d72-9a0c-d668a421ece4@web.de","threadId":"65318","inReplyTo":"CALnO6CDnwYaAPhp67kaYWtV48ULjWAR6ks1khVXmSs1oWUbRDQ@mail.gmail.com","subject":"Re: [PATCH v1] path-walk: fix NULL pointer dereference in error message","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2026-03-20T18:54:21Z","receivedAt":"2026-03-20T18:54:29Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"On 3/20/26 4:16 PM, D. Ben Knoble wrote:\n> \n> When we compute \"child\" in either preceding branch using lookup_tree\n> or lookup_blob, we only return NULL if !quiet in the object_as_type\n> calls (assuming we hit the \"else\" case there, anyway). But quiet==0 in\n> both callers along this path, so !quiet will be truthy and we'll\n> error() out there instead, never returning to add_tree_entries.\n\nerror() just prints a message, it doesn't end the program.\n\n> Since I didn't quickly come up with a reproduction, I can't quite\n> prove this, anyway. It's also possible my analysis is based on code\n> that has since changed (I happened to have a537e3e6e9 (Merge branch\n> 'sp/send-email-validate-charset' into next, 2026-03-06) checked out at\n> the moment).\n\nWe could build a tree referencing an object using a mismatched\ntype to hit that. It's possible by removing the type check from\nbuiltin/mktree.c:mktree_line(), then using the resulting twisted tool:\n\n $ commit=$(git rev-parse HEAD)\n $ tree=$(printf \"100644 blob $commit\\tcommit\\n\" | git_evil mktree)\n\n> Still, fixing such obviously wrong dereference is good, but I wonder\n> if we should go further?\n> \n> You mentioned git-backfill with a tree missing from the local odb; do\n> you have a short reproduction script or test-case?\n\nI don't know about backfill, but this would work:\n\n $ echo $tree | git pack-objects --path-walk --all foo\n\nRené\n\n"},{"id":"539648","messageId":"CALnO6CB6RzTDy3=H2PzD339O6UaZ5GoEZZjK+3ihRABaDfv=VA@mail.gmail.com","threadId":"65318","inReplyTo":"98833ee0-4d63-4d72-9a0c-d668a421ece4@web.de","subject":"Re: [PATCH v1] path-walk: fix NULL pointer dereference in error message","fromName":"D. Ben Knoble","fromEmail":"ben.knoble@gmail.com","sentAt":"2026-03-22T14:21:06Z","receivedAt":"2026-03-22T14:21:18Z","isPatch":true,"sender":{"key":"ben.knoble@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22802209?v=4"},"body":"On Fri, Mar 20, 2026 at 2:54 PM René Scharfe wrote:\n>\n> On 3/20/26 4:16 PM, D. Ben Knoble wrote:\n> >\n> > When we compute \"child\" in either preceding branch using lookup_tree\n> > or lookup_blob, we only return NULL if !quiet in the object_as_type\n> > calls (assuming we hit the \"else\" case there, anyway). But quiet==0 in\n> > both callers along this path, so !quiet will be truthy and we'll\n> > error() out there instead, never returning to add_tree_entries.\n>\n> error() just prints a message, it doesn't end the program.\n\nAh, thanks! The rest is probably moot then.\n\n>\n> > Since I didn't quickly come up with a reproduction, I can't quite\n> > prove this, anyway. It's also possible my analysis is based on code\n> > that has since changed (I happened to have a537e3e6e9 (Merge branch\n> > 'sp/send-email-validate-charset' into next, 2026-03-06) checked out at\n> > the moment).\n>\n> We could build a tree referencing an object using a mismatched\n> type to hit that. It's possible by removing the type check from\n> builtin/mktree.c:mktree_line(), then using the resulting twisted tool:\n>\n> $ commit=$(git rev-parse HEAD)\n> $ tree=$(printf \"100644 blob $commit\\tcommit\\n\" | git_evil mktree)\n>\n> > Still, fixing such obviously wrong dereference is good, but I wonder\n> > if we should go further?\n> >\n> > You mentioned git-backfill with a tree missing from the local odb; do\n> > you have a short reproduction script or test-case?\n>\n> I don't know about backfill, but this would work:\n>\n> $ echo $tree | git pack-objects --path-walk --all foo\n>\n> René\n>\n\nThanks all.\n\n-- \nD. Ben Knoble\n"},{"id":"539721","messageId":"acELx_n-RK3qwz1Z@ThinkPad-E14-Gen-6","threadId":"65318","inReplyTo":"994f92e9-3576-455a-a142-0fefc559131c@gmail.com","subject":"Re: [PATCH v1] path-walk: fix NULL pointer dereference in error message","fromName":"Yuvraj Singh Chauhan","fromEmail":"ysinghcin@gmail.com","sentAt":"2026-03-23T09:45:43Z","receivedAt":"2026-03-23T09:45:59Z","isPatch":true,"sender":{"key":"ysinghcin@gmail.com","avatar":"https://avatars.githubusercontent.com/u/133221941?v=4"},"body":"On Fri, Mar 20, 2026 at 12:14:19PM -0400, Derrick Stolee wrote:\n> On 3/20/2026 11:16 AM, D. Ben Knoble wrote:\n> > On Fri, Mar 20, 2026 at 7:50 AM Yuvraj Singh Chauhan\n> > wrote:\n> >> @@ -171,7 +171,7 @@ static int add_tree_entries(struct path_walk_context *ctx,\n> >>\n> >> if (!o) {\n> >> error(_(\"failed to find object %s\"),\n> >> - oid_to_hex(&o->oid));\n> >> + oid_to_hex(&entry.oid));\n> >> return -1;\n> >> }\n> >>\n> >> --\n> >> 2.53.0.582.gca1db8a0f7\n> > \n> > Interesting find. I was hoping to see an easy way to reproduce hitting\n> > this code, and after grepping around a bit I found a few places that\n> > end up in this code (git-backfill and git-repo being the primary\n> > callers of walk_objects_by_path), but on second glance I think \"!o\" is\n> > current dead code.\n> \n> I can appreciate that the existing code is clearly incorrect, so\n> tooling scanning code for defects would find this even if we can't\n> easily create a test case to demonstrate it.\n> \n> > Still, fixing such obviously wrong dereference is good, but I wonder\n> > if we should go further?\n> > \n> > You mentioned git-backfill with a tree missing from the local odb; do\n> > you have a short reproduction script or test-case?\n> \n> I imagine that it would be difficult to set up such a case, but maybe\n> it would follow these steps (based on existing 'git backfill' tests\n> that start with a partial clone):\n> \n> 1. Make a bare, blobless partial clone of the server repo.\n> \n> 2. Explode the client repo's object store into loose objects.\n> \n> 3. Delete a loose tree object, but one that isn't a commit's root\n> tree. It must be a child tree.\n> \n> In this case, we should hit this issue. Blobless partial clones\n> expect all reachable trees to exist locally and so are not prepared\n> to download missing trees on-demand.\n> \n> Thanks,\n> -Stolee\n\nThanks for the suggestions on how to reproduce this.\nI traced through the code paths and believe the scenario described\n(blobless partial clone, explode objects, delete a child tree) would\nnot actually reach the '!o' branch. Here is my reasoning:\n\nWhen add_tree_entries() encounters a child tree entry whose OID is\nmissing from the ODB, it calls lookup_tree(). Since the OID was never\npreviously registered in the parsed object hash, lookup_object()\nreturns NULL, and lookup_tree() falls through to create_object() which\nallocates a shell object. So 'child' is non-NULL, 'o' is non-NULL, \nand the '!o' check is not reached.\nThe missing object would instead fail later: when the path-walk tries\nto visit the child tree via a subsequent add_tree_entries() call,\nrepo_parse_tree_gently() attempts to read it from the ODB, fails, and\nreturns the \"bad tree object\" error at the top of the function.\n\nThe '!o' path is reachable when the OID in a tree entry was\n\"previously registered\" in the parsed object hash with a different\ntype. In that case, lookup_blob() or lookup_tree() calls\nobject_as_type(), which detects the type conflict and returns NULL.\nThis is what happens with a corrupt/confused tree that references a\ncommit OID as a blob entry, the commit was already registered during\nthe revision walk, so object_as_type(obj, OBJ_BLOB, 0) returns NULL.\nFor v2, I've added a test that exercises exactly this path:\n # Create a tree with a blob entry pointing to a commit object\n commit=$(git rev-parse HEAD)\n bad_tree=$(perl -e 'print \"100644 confused\\0\" . pack(\"H*\", $ARGV[0])' \\\n \"$commit\" | git hash-object -w --stdin -t tree)\n git pack-objects --path-walk --all /tmp/test-pack <<< \"$bad_tree\"\nThis constructs a tree where mode 100644 (blob) has a commit OID.\nDuring --path-walk, the commit is registered as OBJ_COMMIT by the\nrevision walk via --all, so when the tree entry is processed,\nlookup_blob() → object_as_type() returns NULL, triggering the bug.\n\nPlease review\n\nSincerely,\nYuvraj\n"}]}