From: D. Ben Knoble <ben.knoble@gmail.com>
Date: Fri, 20 Mar 2026 15:16:48 GMT
Subject: Re: [PATCH v1] path-walk: fix NULL pointer dereference in error message
Message-ID: <CALnO6CDnwYaAPhp67kaYWtV48ULjWAR6ks1khVXmSs1oWUbRDQ@mail.gmail.com>
In-Reply-To: <20260320114823.3151961-1-ysinghcin@gmail.com>

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