Re: [PATCH v1] perf header: Ensure read strings are '\0' terminated

[sashiko-bot@kernel.org]

Sashiko AI review found 1 potential issue(s):
- [Medium] Casting the unsigned 32-bit length to a signed `int` causes integer underflow and logic failures for large lengths.
--

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

perf header: Ensure read strings are '\0' terminated

This commit adds a check to do_read_string() to ensure that strings read
from perf data are correctly '\0' terminated. If the string is not
correctly terminated or if the length is zero, it returns NULL.

> diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
> --- a/tools/perf/util/header.c
> +++ b/tools/perf/util/header.c

[ ... ]

> @@ -269,7 +272,10 @@ static char *do_read_string(struct feat_fd *ff)
>  		 * thus the actual strlen of buf
>  		 * may be less than len
>  		 */
> -		return buf;
> +		for (int i = (int)len - 1; i >= 0; i--) {

Could this explicit cast of the unsigned 32-bit length to a signed int
cause integer underflow and logic failures for large lengths?

Since do_read_u32() reads the 32-bit unsigned integer len directly from
the perf data file, a length greater than 2GB (e.g., 0x80000001) would
result in a negative number when cast to int.

Would this cause the loop condition i >= 0 to evaluate to false
immediately, skipping the null-terminator check and rejecting the string
by returning NULL?

Additionally, if len is exactly 0x80000000, would subtracting 1 cause
signed integer underflow?

> +			if (buf[i] == '\0')
> +				return buf;
> +		}
>  	}
>  
>  	free(buf);

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