patchpath-walk: fix NULL pointer dereference in error message
5 messages between Mar 20, 2026 and Mar 21, 2026, from Yuvraj Singh Chauhan, Tian Yuchen, Junio C Hamano.
Plain Markdown or JSON for tools and agents. Diffs are folded; open one to read it.
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
---
path-walk.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to path-walk.c +1 −1
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] path-walk: fix NULL pointer dereference in error message
Hello,
Thanks for the patch!
On 3/20/26 19:45, Yuvraj Singh Chauhan wrote:
Show 14 quoted lines
> 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 lookupChecking for NULL and then dereference the pointer, a segfault is bound to occur. I believe this is indeed a bug.
Show 17 quoted lines
> ---
> 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;
> }
> The change itself looks good to me.
However, I have a slight concern about the original code implementation:
> o = child ? &child->object : NULL;
This means that 'child = NULL' is the expected failure path. But why is 'NULL' returned? Does the object truly not exist, or was it simply not parsed? Is 'failed to find object' sufficient to describe the cause of the failure? I think that's debatable. (I’m not 'suggesting' that you make this change; I just hope we can all think about it together ;)
Also, it looks like you forgot to include 'Signed-off-by' line when you committed the changes. When you've gathered enough feedback to commit v2, please remember to include it.
Regards,
Yuchen
Re: [PATCH] path-walk: fix NULL pointer dereference in error message
Although this isn’t covered in this patch, I’d still like to add a few more comments. Please allow me to elaborate a bit further:
This is the implementation of object_as_type() in object.c:
Show 19 quoted lines
> void *object_as_type(struct object *obj, enum object_type type, int quiet)
> {
> if (obj->type == type)
> return obj;
> else if (obj->type == OBJ_NONE) {
> if (type == OBJ_COMMIT)
> init_commit_node((struct commit *) obj);
> else
> obj->type = type;
> return obj;
> }
> else {
> if (!quiet)
> error(_("object %s is a %s, not a %s"),
> oid_to_hex(&obj->oid),
> type_name(obj->type), type_name(type));
> return NULL;
> }
> }There are at least two possible scenarios: the object doesn't exist, or the type doesn't match, right?
Then the message 'failed to find object' is misleading when user encounters the second scenario. Wouldn’t it be confusing if a user saw this message, checked the object using 'git cat-file -t', and discovered that the object actually exists?
Regards,
Yuchen
Re: [PATCH] path-walk: fix NULL pointer dereference in error message
Tian Yuchen <a3205153416@gmail.com> writes:
> Then the message 'failed to find object' is misleading when user
> encounters the second scenario. Wouldn’t it be confusing if a user saw
> this message, checked the object using 'git cat-file -t', and discovered
> that the object actually exists?
For that, you'd need to remove the block in question and duplicate the message generation, perhaps like so, to allow the message properly localized.
But that is way outside the scope of the patch that was posted, which was a surgical fix for a reference to an incorrect pointer, I would have to say.
path-walk.c | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
Show changes to diff +8 −8
diff --git c/path-walk.c w/path-walk.c
index 364e4cfa19..bd2c47079e 100644
--- c/path-walk.c
+++ w/path-walk.c
@@ -161,20 +161,20 @@ static int add_tree_entries(struct path_walk_context *ctx,
if (type == OBJ_TREE) {
struct tree *child = lookup_tree(ctx->repo, &entry.oid);
- o = child ? &child->object : NULL;
+ if (!child)
+ return error(_("failed to find tree object %s"),
+ oid_to_hex(&entry.oid));
+ o = &child->object;
} else if (type == OBJ_BLOB) {
struct blob *child = lookup_blob(ctx->repo, &entry.oid);
- o = child ? &child->object : NULL;
+ if (!child)
+ return error(_("failed to find blob object %s"),
+ oid_to_hex(&entry.oid));
+ o = &child->object;
} else {
BUG("invalid type for tree entry: %d", type);
}
- if (!o) {
- error(_("failed to find object %s"),
- oid_to_hex(&o->oid));
- return -1;
- }
-
/* Skip this object if already seen. */
if (o->flags & SEEN)
continue;
Re: [PATCH] path-walk: fix NULL pointer dereference in error message
On 3/21/26 01:47, Junio C Hamano wrote:
Show 7 quoted lines
> For that, you'd need to remove the block in question and duplicate
> the message generation, perhaps like so, to allow the message
> properly localized.
>
> But that is way outside the scope of the patch that was posted,
> which was a surgical fix for a reference to an incorrect pointer, I
> would have to say.
Thank you for pointing that out, but I believe I already mentioned that this is not what the patch is intended to address, and I did not suggest making that change.
Yuchen
Yuvraj Singh Chauhan
