# [PATCH v1] path-walk: fix NULL pointer dereference in error message 7 messages from 2026-03-20 to 2026-03-23. Participants: Yuvraj Singh Chauhan, D. Ben Knoble, Derrick Stolee, Junio C Hamano, René Scharfe. Thread: https://gitlist.dev/t/65318 ## Yuvraj Singh Chauhan, 2026-03-20 11:48 Subject: [PATCH v1] path-walk: fix NULL pointer dereference in error message Message-ID: <20260320114823.3151961-1-ysinghcin@gmail.com> URL: https://gitlist.dev/e/20260320114823.3151961-1-ysinghcin%40gmail.com ``` When lookup_tree() or lookup_blob() cannot find a tree entry's object, 'o' is set to NULL via: o = child ? &child->object : NULL; The subsequent null-check catches this correctly, but then dereferences 'o' to format the error message: error(_("failed to find object %s"), oid_to_hex(&o->oid)); This causes a segfault instead of the intended diagnostic output. Fix this by using &entry.oid instead. 'entry' is the struct name_entry populated by tree_entry() on each loop iteration and holds the OID of the failing lookup -- which is exactly what the error should report. This crash is reachable via git-backfill(1) when a tree entry's object is absent from the local object database. Signed-off-by: Yuvraj Singh Chauhan --- path-walk.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/path-walk.c b/path-walk.c index 364e4cfa19..839582380c 100644 --- a/path-walk.c +++ b/path-walk.c @@ -171,7 +171,7 @@ static int add_tree_entries(struct path_walk_context *ctx, if (!o) { error(_("failed to find object %s"), - oid_to_hex(&o->oid)); + oid_to_hex(&entry.oid)); return -1; } -- 2.53.0.582.gca1db8a0f7 ``` ## D. Ben Knoble, 2026-03-20 15:16 Subject: Re: [PATCH v1] path-walk: fix NULL pointer dereference in error message Message-ID: URL: https://gitlist.dev/e/CALnO6CDnwYaAPhp67kaYWtV48ULjWAR6ks1khVXmSs1oWUbRDQ%40mail.gmail.com In-Reply-To: <20260320114823.3151961-1-ysinghcin@gmail.com> ``` On Fri, Mar 20, 2026 at 7:50 AM Yuvraj Singh Chauhan wrote: > > When lookup_tree() or lookup_blob() cannot find a tree entry's object, > 'o' is set to NULL via: > > o = child ? &child->object : NULL; > > The subsequent null-check catches this correctly, but then dereferences > 'o' to format the error message: > > error(_("failed to find object %s"), oid_to_hex(&o->oid)); > > This causes a segfault instead of the intended diagnostic output. > > Fix this by using &entry.oid instead. 'entry' is the struct name_entry > populated by tree_entry() on each loop iteration and holds the OID of > the failing lookup -- which is exactly what the error should report. > > This crash is reachable via git-backfill(1) when a tree entry's object > is absent from the local object database. > > Signed-off-by: Yuvraj Singh Chauhan > --- > path-walk.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/path-walk.c b/path-walk.c > index 364e4cfa19..839582380c 100644 > --- a/path-walk.c > +++ b/path-walk.c > @@ -171,7 +171,7 @@ static int add_tree_entries(struct path_walk_context *ctx, > > if (!o) { > error(_("failed to find object %s"), > - oid_to_hex(&o->oid)); > + oid_to_hex(&entry.oid)); > return -1; > } > > -- > 2.53.0.582.gca1db8a0f7 Interesting find. I was hoping to see an easy way to reproduce hitting this code, and after grepping around a bit I found a few places that end up in this code (git-backfill and git-repo being the primary callers of walk_objects_by_path), but on second glance I think "!o" is current dead code. When we compute "child" in either preceding branch using lookup_tree or lookup_blob, we only return NULL if !quiet in the object_as_type calls (assuming we hit the "else" case there, anyway). But quiet==0 in both callers along this path, so !quiet will be truthy and we'll error() out there instead, never returning to add_tree_entries. Since I didn't quickly come up with a reproduction, I can't quite prove this, anyway. It's also possible my analysis is based on code that has since changed (I happened to have a537e3e6e9 (Merge branch 'sp/send-email-validate-charset' into next, 2026-03-06) checked out at the moment). Still, fixing such obviously wrong dereference is good, but I wonder if we should go further? You mentioned git-backfill with a tree missing from the local odb; do you have a short reproduction script or test-case? -- D. Ben Knoble ``` ## Derrick Stolee, 2026-03-20 16:14 Subject: Re: [PATCH v1] path-walk: fix NULL pointer dereference in error message Message-ID: <994f92e9-3576-455a-a142-0fefc559131c@gmail.com> URL: https://gitlist.dev/e/994f92e9-3576-455a-a142-0fefc559131c%40gmail.com In-Reply-To: ``` On 3/20/2026 11:16 AM, D. Ben Knoble wrote: > On Fri, Mar 20, 2026 at 7:50 AM Yuvraj Singh Chauhan > wrote: >> @@ -171,7 +171,7 @@ static int add_tree_entries(struct path_walk_context *ctx, >> >> if (!o) { >> error(_("failed to find object %s"), >> - oid_to_hex(&o->oid)); >> + oid_to_hex(&entry.oid)); >> return -1; >> } >> >> -- >> 2.53.0.582.gca1db8a0f7 > > Interesting find. I was hoping to see an easy way to reproduce hitting > this code, and after grepping around a bit I found a few places that > end up in this code (git-backfill and git-repo being the primary > callers of walk_objects_by_path), but on second glance I think "!o" is > current dead code. I can appreciate that the existing code is clearly incorrect, so tooling scanning code for defects would find this even if we can't easily create a test case to demonstrate it. > Still, fixing such obviously wrong dereference is good, but I wonder > if we should go further? > > You mentioned git-backfill with a tree missing from the local odb; do > you have a short reproduction script or test-case? I imagine that it would be difficult to set up such a case, but maybe it would follow these steps (based on existing 'git backfill' tests that start with a partial clone): 1. Make a bare, blobless partial clone of the server repo. 2. Explode the client repo's object store into loose objects. 3. Delete a loose tree object, but one that isn't a commit's root tree. It must be a child tree. In this case, we should hit this issue. Blobless partial clones expect all reachable trees to exist locally and so are not prepared to download missing trees on-demand. Thanks, -Stolee ``` ## Junio C Hamano, 2026-03-20 17:08 Subject: Re: [PATCH v1] path-walk: fix NULL pointer dereference in error message Message-ID: URL: https://gitlist.dev/e/xmqqy0jme8ea.fsf%40gitster.g In-Reply-To: ``` "D. Ben Knoble" writes: > You mentioned git-backfill with a tree missing from the local odb; do > you have a short reproduction script or test-case? Interesting thing to ask. THe code looks correct, though. ``` ## René Scharfe, 2026-03-20 18:54 Subject: Re: [PATCH v1] path-walk: fix NULL pointer dereference in error message Message-ID: <98833ee0-4d63-4d72-9a0c-d668a421ece4@web.de> URL: https://gitlist.dev/e/98833ee0-4d63-4d72-9a0c-d668a421ece4%40web.de In-Reply-To: ``` On 3/20/26 4:16 PM, D. Ben Knoble wrote: > > When we compute "child" in either preceding branch using lookup_tree > or lookup_blob, we only return NULL if !quiet in the object_as_type > calls (assuming we hit the "else" case there, anyway). But quiet==0 in > both callers along this path, so !quiet will be truthy and we'll > error() out there instead, never returning to add_tree_entries. error() just prints a message, it doesn't end the program. > Since I didn't quickly come up with a reproduction, I can't quite > prove this, anyway. It's also possible my analysis is based on code > that has since changed (I happened to have a537e3e6e9 (Merge branch > 'sp/send-email-validate-charset' into next, 2026-03-06) checked out at > the moment). We could build a tree referencing an object using a mismatched type to hit that. It's possible by removing the type check from builtin/mktree.c:mktree_line(), then using the resulting twisted tool: $ commit=$(git rev-parse HEAD) $ tree=$(printf "100644 blob $commit\tcommit\n" | git_evil mktree) > Still, fixing such obviously wrong dereference is good, but I wonder > if we should go further? > > You mentioned git-backfill with a tree missing from the local odb; do > you have a short reproduction script or test-case? I don't know about backfill, but this would work: $ echo $tree | git pack-objects --path-walk --all foo René ``` ## D. Ben Knoble, 2026-03-22 14:21 Subject: Re: [PATCH v1] path-walk: fix NULL pointer dereference in error message Message-ID: URL: https://gitlist.dev/e/CALnO6CB6RzTDy3%3DH2PzD339O6UaZ5GoEZZjK%2B3ihRABaDfv%3DVA%40mail.gmail.com In-Reply-To: <98833ee0-4d63-4d72-9a0c-d668a421ece4@web.de> ``` On Fri, Mar 20, 2026 at 2:54 PM René Scharfe wrote: > > On 3/20/26 4:16 PM, D. Ben Knoble wrote: > > > > When we compute "child" in either preceding branch using lookup_tree > > or lookup_blob, we only return NULL if !quiet in the object_as_type > > calls (assuming we hit the "else" case there, anyway). But quiet==0 in > > both callers along this path, so !quiet will be truthy and we'll > > error() out there instead, never returning to add_tree_entries. > > error() just prints a message, it doesn't end the program. Ah, thanks! The rest is probably moot then. > > > Since I didn't quickly come up with a reproduction, I can't quite > > prove this, anyway. It's also possible my analysis is based on code > > that has since changed (I happened to have a537e3e6e9 (Merge branch > > 'sp/send-email-validate-charset' into next, 2026-03-06) checked out at > > the moment). > > We could build a tree referencing an object using a mismatched > type to hit that. It's possible by removing the type check from > builtin/mktree.c:mktree_line(), then using the resulting twisted tool: > > $ commit=$(git rev-parse HEAD) > $ tree=$(printf "100644 blob $commit\tcommit\n" | git_evil mktree) > > > Still, fixing such obviously wrong dereference is good, but I wonder > > if we should go further? > > > > You mentioned git-backfill with a tree missing from the local odb; do > > you have a short reproduction script or test-case? > > I don't know about backfill, but this would work: > > $ echo $tree | git pack-objects --path-walk --all foo > > René > Thanks all. -- D. Ben Knoble ``` ## Yuvraj Singh Chauhan, 2026-03-23 09:45 Subject: Re: [PATCH v1] path-walk: fix NULL pointer dereference in error message Message-ID: URL: https://gitlist.dev/e/acELx_n-RK3qwz1Z%40ThinkPad-E14-Gen-6 In-Reply-To: <994f92e9-3576-455a-a142-0fefc559131c@gmail.com> ``` On Fri, Mar 20, 2026 at 12:14:19PM -0400, Derrick Stolee wrote: > On 3/20/2026 11:16 AM, D. Ben Knoble wrote: > > On Fri, Mar 20, 2026 at 7:50 AM Yuvraj Singh Chauhan > > wrote: > >> @@ -171,7 +171,7 @@ static int add_tree_entries(struct path_walk_context *ctx, > >> > >> if (!o) { > >> error(_("failed to find object %s"), > >> - oid_to_hex(&o->oid)); > >> + oid_to_hex(&entry.oid)); > >> return -1; > >> } > >> > >> -- > >> 2.53.0.582.gca1db8a0f7 > > > > Interesting find. I was hoping to see an easy way to reproduce hitting > > this code, and after grepping around a bit I found a few places that > > end up in this code (git-backfill and git-repo being the primary > > callers of walk_objects_by_path), but on second glance I think "!o" is > > current dead code. > > I can appreciate that the existing code is clearly incorrect, so > tooling scanning code for defects would find this even if we can't > easily create a test case to demonstrate it. > > > Still, fixing such obviously wrong dereference is good, but I wonder > > if we should go further? > > > > You mentioned git-backfill with a tree missing from the local odb; do > > you have a short reproduction script or test-case? > > I imagine that it would be difficult to set up such a case, but maybe > it would follow these steps (based on existing 'git backfill' tests > that start with a partial clone): > > 1. Make a bare, blobless partial clone of the server repo. > > 2. Explode the client repo's object store into loose objects. > > 3. Delete a loose tree object, but one that isn't a commit's root > tree. It must be a child tree. > > In this case, we should hit this issue. Blobless partial clones > expect all reachable trees to exist locally and so are not prepared > to download missing trees on-demand. > > Thanks, > -Stolee Thanks for the suggestions on how to reproduce this. I traced through the code paths and believe the scenario described (blobless partial clone, explode objects, delete a child tree) would not actually reach the '!o' branch. Here is my reasoning: When add_tree_entries() encounters a child tree entry whose OID is missing from the ODB, it calls lookup_tree(). Since the OID was never previously registered in the parsed object hash, lookup_object() returns NULL, and lookup_tree() falls through to create_object() which allocates a shell object. So 'child' is non-NULL, 'o' is non-NULL, and the '!o' check is not reached. The missing object would instead fail later: when the path-walk tries to visit the child tree via a subsequent add_tree_entries() call, repo_parse_tree_gently() attempts to read it from the ODB, fails, and returns the "bad tree object" error at the top of the function. The '!o' path is reachable when the OID in a tree entry was "previously registered" in the parsed object hash with a different type. In that case, lookup_blob() or lookup_tree() calls object_as_type(), which detects the type conflict and returns NULL. This is what happens with a corrupt/confused tree that references a commit OID as a blob entry, the commit was already registered during the revision walk, so object_as_type(obj, OBJ_BLOB, 0) returns NULL. For v2, I've added a test that exercises exactly this path: # Create a tree with a blob entry pointing to a commit object commit=$(git rev-parse HEAD) bad_tree=$(perl -e 'print "100644 confused\0" . pack("H*", $ARGV[0])' \ "$commit" | git hash-object -w --stdin -t tree) git pack-objects --path-walk --all /tmp/test-pack <<< "$bad_tree" This constructs a tree where mode 100644 (blob) has a commit OID. During --path-walk, the commit is registered as OBJ_COMMIT by the revision walk via --all, so when the tree entry is processed, lookup_blob() → object_as_type() returns NULL, triggering the bug. Please review Sincerely, Yuvraj ```