[PATCH v1] path-walk: fix NULL pointer dereference in error message

[PATCH v1] path-walk: fix NULL pointer dereference in error message

[Yuvraj Singh Chauhan]

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 <ysinghcin@gmail.com>
---
 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

Re: [PATCH v1] path-walk: fix NULL pointer dereference in error message

[D. Ben Knoble]

On Fri, Mar 20, 2026 at 7:50 AM Yuvraj Singh Chauhan
<ysinghcin@gmail.com> 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 <ysinghcin@gmail.com>
> ---
>  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

Re: [PATCH v1] path-walk: fix NULL pointer dereference in error message

[Derrick Stolee]

On 3/20/2026 11:16 AM, D. Ben Knoble wrote:
> On Fri, Mar 20, 2026 at 7:50 AM Yuvraj Singh Chauhan
> <ysinghcin@gmail.com> 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


 

Re: [PATCH v1] path-walk: fix NULL pointer dereference in error message

[Yuvraj Singh Chauhan]

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
> > <ysinghcin@gmail.com> 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

Re: [PATCH v1] path-walk: fix NULL pointer dereference in error message

[Junio C Hamano]

"D. Ben Knoble" <ben.knoble@gmail.com> 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.

Re: [PATCH v1] path-walk: fix NULL pointer dereference in error message

[René Scharfe]

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é

Re: [PATCH v1] path-walk: fix NULL pointer dereference in error message

[D. Ben Knoble]

On Fri, Mar 20, 2026 at 2:54 PM René Scharfe <l.s.r@web.de> 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