Re: [PATCH v3 5/6] perf annotate-data: Fix libdw API contract violations

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Setting `loc = 0` upon `dwarf_formudata` failure silently corrupts struct member offsets for DWARF location expressions.
- [Medium] Redundant validation of `dwarf_diename` introduces dead code and fails to provide the safety claimed in the commit message.
--

commit c23bc4d691e2495e7ddd491d7f1b7b532eb95f50
Author: Ian Rogers <irogers@google.com>

perf annotate-data: Fix libdw API contract violations

This commit adds return value checks for dwarf_aggregate_size and
dwarf_formudata. It also attempts to validate dwarf_diename before calling
strdup to prevent potential crashes.

> diff --git a/tools/perf/util/annotate-data.c b/tools/perf/util/annotate-data.c
> index 1eff0a27237d..e881a40a4885 100644
> --- a/tools/perf/util/annotate-data.c
> +++ b/tools/perf/util/annotate-data.c

[ ... ]

> @@ -250,9 +251,10 @@ static int __add_member_cb(Dwarf_Die *die, void *arg)
>  	if (dwarf_aggregate_size(&die_mem, &size) < 0)
>  		size = 0;
>  
> -	if (dwarf_attr_integrate(die, DW_AT_data_member_location, &attr))
> -		dwarf_formudata(&attr, &loc);
> -	else {
> +	if (dwarf_attr_integrate(die, DW_AT_data_member_location, &attr)) {
> +		if (dwarf_formudata(&attr, &loc) != 0)
> +			loc = 0;

Since DW_AT_data_member_location can be encoded as a location expression block
(such as DW_OP_plus_uconst) rather than just a constant form, dwarf_formudata
will return an error for those blocks.

Will setting loc to 0 here deterministically corrupt the struct member offsets
for these location expressions?

Should this code perhaps evaluate the location expression block using a helper
like die_get_data_member_location() instead of unconditionally setting it to
0?

> +	} else {
>  		/* bitfield member */
>  		if (dwarf_attr_integrate(die, DW_AT_data_bit_offset, &attr) &&
>  		    dwarf_formudata(&attr, &loc) == 0)

[ ... ]

> @@ -273,7 +275,9 @@ static int __add_member_cb(Dwarf_Die *die, void *arg)
>  				     dwarf_diename(die), (long)bit_size) < 0)
>  				member->var_name = NULL;
>  		} else {
> -			member->var_name = strdup(dwarf_diename(die));
> +			const char *name = dwarf_diename(die);
> +
> +			member->var_name = strdup(name ?: "unknown");

The commit message states: "Validate dwarf_diename before strdup to prevent
potential crashes."

Looking at the larger context of __add_member_cb(), this block is already
guarded by an outer if statement checking the same value:

    if (dwarf_diename(die)) {
        if (bit_size) {
            ...
        } else {
            const char *name = dwarf_diename(die);
            member->var_name = strdup(name ?: "unknown");
        }
    }

Does this make the fallback to "unknown" dead code since name is guaranteed
to be non-NULL here?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260503003552.1063540-1-irogers@google.com?part=5