Re: [PATCH v2 4/4] perf annotate-data: Adjust type offset for flex-array

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] Off-by-one error and tail padding overlap in flex-array offset adjustment causes accesses to be silently dropped and incorrectly attributed.
- [Medium] Calling `strstr()` on `child->type_name` without checking for NULL causes a segmentation fault if memory allocation failed during type name extraction.
- [Medium] The newly added `strstr(child->type_name, "[]")` call reads uninitialized heap memory, leading to out-of-bounds reads and potential crashes when analyzing malformed or unknown DWARF entries.
--

commit 546415e2740f53443f133b3600542636f3d84f31
Author: Namhyung Kim <namhyung@kernel.org>
perf annotate-data: Adjust type offset for flex-array

This commit adjusts the type offset for flexible array members that are located
beyond the original type size. It ensures a field name is found for flex arrays
by adjusting the offset in an array to find a corresponding element.

> diff --git a/tools/perf/util/annotate-data.c b/tools/perf/util/annotate-data.c
> index e8aff6a916c0a..a66b72feb6e2f 100644
> --- a/tools/perf/util/annotate-data.c
> +++ b/tools/perf/util/annotate-data.c

[ ... ]

> @@ -355,15 +376,37 @@ static int fill_member_name(char *buf, size_t sz, struct annotated_member *m,
>  			len = 0;
>  		}
>  
> -		return fill_member_name(buf + len, sz - len, child, offset, first) + len;
> +		return fill_member_name(buf + len, sz - len, child, offset, first,
> +					has_flex_array) + len;
>  	}
> +
>  	return 0;
>  }
>  
>  int annotated_data_type__get_member_name(struct annotated_data_type *adt,
>  					 char *buf, size_t sz, int member_offset)
>  {
> -	return fill_member_name(buf, sz, &adt->self, member_offset, /*first=*/true);
> +	return fill_member_name(buf, sz, &adt->self, member_offset, /*first=*/true,
> +				adt->flex_array);
> +}
> +
> +static struct annotated_member *find_flex_array(struct annotated_member *m)
> +{
> +	struct annotated_member *child;
> +
> +	if (list_empty(&m->children))
> +		return NULL;
> +
> +	child = list_last_entry(&m->children, struct annotated_member, node);
> +	if (strstr(child->type_name, "[]"))

[Severity: Medium]
Could this call to strstr() safely handle scenarios where child->type_name is
NULL or uninitialized?

During DWARF parsing, for instance in __add_member_cb(), if die_get_typename()
fails due to a malformed DWARF entry or memory exhaustion, strbuf_detach()
might return an uninitialized heap buffer or NULL. If that happens, does this
scan risk triggering a segmentation fault or an out-of-bounds read when looking
for the "[]" string?

> +		return child;
> +
> +	return find_flex_array(child);
> +}

[ ... ]

> @@ -1740,6 +1783,7 @@ struct annotated_data_type *find_data_type(struct data_loc_info *dloc)
>  {
>  	struct dso *dso = map__dso(dloc->ms->map);
>  	Dwarf_Die type_die;
> +	struct annotated_data_type *result;
>  
>  	/*
>  	 * The type offset is the same as instruction offset by default.
> @@ -1752,7 +1796,25 @@ struct annotated_data_type *find_data_type(struct data_loc_info *dloc)
>  	if (find_data_type_die(dloc, &type_die) < 0)
>  		return NULL;
>  
> -	return dso__findnew_data_type(dso, &type_die);
> +	result = dso__findnew_data_type(dso, &type_die);
> +	if (result == NULL)
> +		return NULL;
> +
> +	if (result->flex_array && dloc->type_offset > result->self.size) {

[Severity: Medium]
Will this boundary check correctly catch accesses that fall exactly on the
boundary or within the struct's tail padding?

If an event samples a flexible array element whose offset is greater than 
flex_array->offset but less than or equal to result->self.size (the total
padded size of the struct), it appears this condition evaluates to false.
Could this cause the offset adjustment to be skipped, causing valid sample
counts to be silently dropped during data presentation?

> +		struct annotated_member *flex_array = get_flex_array_member(result);
> +
> +		if (flex_array && flex_array->size > 0) {
> +			int offset = dloc->type_offset;
> +
> +			/* adjust offset in the flex array */
> +			offset -= flex_array->offset;
> +			offset %= flex_array->size;
> +			offset += flex_array->offset;
> +
> +			dloc->type_offset = offset;
> +		}
> +	}
> +	return result;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260914064535.1671939-1-namhyung@kernel.org?part=4