Re: [PATCH 7/9] perf annotate-data: Resolve type DIEs in the debug file they came from

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Unbounded recursive calls in `__die_find_member_cb` when processing anonymous structures can lead to stack exhaustion.
- [High] Missing loop iteration bound in `die_get_member_type` allows an infinite loop on cyclic DWARF structure references.
--

commit ea0e3dfe9a708661f7cdcfe58f9fbe047a513329
Author: Arnaldo Carvalho de Melo <acme@redhat.com>

perf annotate-data: Resolve type DIEs in the debug file they came from

A 'perf report -s type' on a 783 MB AMD IBS data type profiling session
hangs, burning all of a CPU and producing no output. This resolves the
type DIEs in the debug file they came from to prevent hanging on
self-referencing typedefs and qualifiers. It also adds depth and recursion
bounds to type chasers.

> diff --git a/tools/perf/util/annotate-data.c b/tools/perf/util/annotate-data.c
> index 4e4c587640823..06d27868887bd 100644
> --- a/tools/perf/util/annotate-data.c
> +++ b/tools/perf/util/annotate-data.c
[ ... ]
> @@ -221,6 +221,15 @@ static bool data_type_less(struct rb_node *node_a, const struct rb_node *node_b)
>  	return strcmp(a->self.type_name, b->self.type_name) < 0;
>  }
>  
> +/*
> + * Members of struct/union members are added recursively, and the same DIE
> + * that is not what it looks like, the one that makes the type chasers in
> + * util/dwarf-aux.c spin, can make a member's type point back at one of its
> + * own ancestors, recursing until the stack is gone.  Nothing usable comes
> + * out of nesting members this deep anyway.
> + */
> +#define MAX_MEMBER_DEPTH 8
> +
>  /* Recursively add new members for struct/union */
>  static int __add_member_cb(Dwarf_Die *die, void *arg)

[Severity: High]
This is a pre-existing issue, but while we are adding MAX_MEMBER_DEPTH to
prevent stack exhaustion here, what about __die_find_member_cb() in
tools/perf/util/dwarf-aux.c?

If we process a corrupted DWARF file with an anonymous structure member that
forms a cycle, couldn't this initiate an unbounded recursive search down the
anonymous structure member chain and also exhaust the stack?

tools/perf/util/dwarf-aux.c:__die_find_member_cb() {
        ...
        else if (!dwarf_diename(die_mem)) {     /* Unnamed structure */
                Dwarf_Die type_die, tmp_die;
                if (die_get_type(die_mem, &type_die) &&
                    die_find_member(&type_die, name, &tmp_die))
                        return DIE_FIND_CB_END;
        }
        ...
}

> diff --git a/tools/perf/util/dwarf-aux.c b/tools/perf/util/dwarf-aux.c
> index d7160f87ac7d7..7acb431fd34a8 100644
> --- a/tools/perf/util/dwarf-aux.c
> +++ b/tools/perf/util/dwarf-aux.c
[ ... ]
> @@ -266,16 +266,35 @@ Dwarf_Die *die_get_type(Dwarf_Die *vr_die, Dwarf_Die *die_mem)
>  		return NULL;
>  }
>  
> +/*
> + * The chases below cross typedefs and qualifiers to get to the type that
> + * is actually meant, and a DIE that is not what it looks like, e.g. one
> + * parsed at an offset that is not the start of a DIE in the file it was
> + * resolved in, can have a DW_AT_type that refers back to itself, which
> + * makes them spin forever: 'perf report -s type' did exactly that on the
> + * dwz compressed debug info of zlib-ng (libz.so.1), burning all of a CPU
> + * with no output while resolving a hist entry in build_tree().
> + *
> + * No sane chain is this long, so give up instead of hanging, telling about
> + * it so that the broken debug info can be looked at.
> + */
> +#define MAX_TYPE_CHASE 32
> +
>  /* Get a type die, but skip qualifiers */
>  Dwarf_Die *__die_get_real_type(Dwarf_Die *vr_die, Dwarf_Die *die_mem)
>  {
> -	int tag;
> +	int tag, chase = 0;

[Severity: High]
This isn't a bug introduced by this patch, but since we are preventing
infinite spinning on cyclic DWARF types here, is there a similar missing
loop iteration bound in die_get_member_type() in tools/perf/util/dwarf-aux.c?

If a crafted DWARF file contains a cyclic struct/union definition (e.g., an
anonymous embedded struct whose type points back to itself) located at
offset 0, would this traverse downwards infinitely and hang the tool since
offset and tag remain unchanged?

tools/perf/util/dwarf-aux.c:die_get_member_type() {
        ...
        while (tag == DW_TAG_structure_type || tag == DW_TAG_union_type) {
                member = die_find_child(&mb_type, __die_find_member_offset_cb,
                                        (void *)(long)offset, die_mem);
                if (member == NULL)
                        return NULL;

                if (die_get_real_type(member, &mb_type) == NULL)
                        return NULL;

                tag = dwarf_tag(&mb_type);
        ...
}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260914125451.2045-1-acme@kernel.org?part=7