Re: [PATCH 9/9] tools/perf/powerpc: Add logical address in decoded traces

[sashiko-bot@kernel.org]

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

New issues:
- [High] Improper error handling causes memory leak and potential segfault during initialization.
- [High] File write failures cause an infinite loop in trace line parsing.
- [High] String replacement logic corrupts output for trace lines with multiple addresses.
- [Medium] Unmapped physical addresses are incorrectly overwritten with 0x0, destroying trace information.
- [Medium] Trace lines that do not match the event label regex are silently dropped from the translated dump.

Pre-existing issues:
- [High] Malformed physical addresses cause an infinite loop in trace line parsing.
--

commit cce34e6215ec4758fecae703f279c8591debaca6
Author: Athira Rajeev <atrajeev@linux.ibm.com>

tools/perf/powerpc: Add logical address in decoded traces

This patch enhances post-processing to translate physical addresses to
logical addresses in the decoded output. It creates a new .l output file
with these translated addresses. It also allows optional LP index filtering
when dumping traces.

> diff --git a/tools/perf/util/powerpc-htm.c b/tools/perf/util/powerpc-htm.c
> index 050fbceac71eb..1bd2664453c66 100644
> --- a/tools/perf/util/powerpc-htm.c
> +++ b/tools/perf/util/powerpc-htm.c

[ ... ]

> @@ -259,36 +261,60 @@ static struct addr_map *process_trace_file(const char *trace_file,
>  	unsigned long logical_addr;
>  	size_t total_phys = 0;
>  	size_t total_phys_to_logical = 0;
> -
>  	FILE *fp = fopen(trace_file, "r");
> +	size_t prefix_len;
> +	int found_match = 0;
> +	FILE *fout;
> +	int filter_lp = 1;
> +	char *output = malloc(strlen(trace_file) + 3);  /* +3 for ".l" and null */
> +
> +	if (!output) {
> +		pr_err("Failed to allocate memory for output filename\n");
> +		fclose(fp);
> +		return NULL;
> +	}
> +
>  	if (!fp) {
>  		pr_err("Failed to open trace file %s: %s\n", trace_file, strerror(errno));
>  		return NULL;
>  	}

[Severity: High]
Does this logic mishandle failures from fopen() and malloc()?

If trace_file cannot be opened, fopen() returns NULL. If malloc() for output
succeeds, the subsequent if (!fp) block returns NULL without freeing output,
leaking the allocated memory.

Additionally, if malloc() fails when fp is NULL, the if (!output) block calls
fclose(fp), which would dereference the NULL pointer and crash the tool.

[ ... ]

> @@ -303,6 +329,10 @@ static struct addr_map *process_trace_file(const char *trace_file,
>  			while (regexec(&addr_regex, ptr, 1, pmatch, 0) == 0) {
>  				unsigned long long phys_addr = 0;
>  				struct addr_map entry = {0};
> +				char *hex_start = strstr(line, "addr:0x");

[Severity: High]
Can this strstr() call corrupt the output for trace lines with multiple
addresses?

When a trace line contains multiple addresses, the inner loop iterates over
each regex match. However, hex_start is calculated using
strstr(line, "addr:0x"), which always finds the first occurrence in the
entire line, ignoring the regex match bounds. This will cause the same prefix
and suffix to be printed multiple times, duplicating portions of the line.

> +				const char *target = "addr:0x";
> +				char *old_val_ptr;
> +				size_t written;
>  
>  				if (sscanf(ptr + pmatch[0].rm_so + strlen("addr:"),
>  					  "%llx", &phys_addr) != 1) {
>  					pr_debug("Failed to parse phys addr from trace line\n");
>  					continue;
>  				}

[Severity: High]
This is a pre-existing issue, but does this error path cause an infinite loop?

If sscanf() fails to parse a physical address, it executes continue. This
skips the ptr += pmatch[0].rm_eo; offset advancement at the bottom of the loop.
As a result, the next iteration of the while (regexec(&addr_regex, ptr...))
loop will match the exact same string and fail to parse it again, looping
indefinitely.

> @@ -315,7 +345,30 @@ static struct addr_map *process_trace_file(const char *trace_file,
>  				logical_addr = find_logical_addr(phys_addr,
>  							mem_entries_array,
>  							n_entries,
> -							lp_filter);
> +							lp_filter, filter_lp);
> +				/* create output.txt with logical address */
> +				if (dump_trace && hex_start) {
> +					old_val_ptr = hex_start + strlen(target);
> +					prefix_len = hex_start - line;
> +					written = fwrite(line, 1, prefix_len, fout);
> +					if (written != prefix_len) {
> +						pr_err("Failed to write prefix to output file\n");
> +						continue;
> +					}

[Severity: High]
Can file write failures here cause an infinite loop in trace line parsing?

If either the fwrite() or fprintf() calls fail (e.g., due to a full disk),
the code logs an error and executes continue. Similar to the parsing error
above, this skips the ptr += pmatch[0].rm_eo; advancement at the bottom of
the loop, causing regexec() to repeatedly match the same address string
forever.

> +					if (fprintf(fout, "addr:0x%llx\t", (unsigned long long)logical_addr) < 0) {

[Severity: Medium]
Are unmapped physical addresses incorrectly overwritten with 0x0 here?

If find_logical_addr() fails to find a mapping, it returns 0. The code then
unconditionally writes logical_addr to the output file. This replaces valid but
unmapped physical addresses with addr:0x0, rather than preserving the original
address.

> +						pr_err("Failed to write to output file\n");
> +						continue;
> +					}

[ ... ]

> @@ -335,6 +388,12 @@ static struct addr_map *process_trace_file(const char *trace_file,
>  
>  				ptr += pmatch[0].rm_eo;
>  			}
> +			if (dump_trace && (!found_match) && line) {
> +				if (fprintf(fout, "%s", line) < 0) {
> +					pr_err("Failed to write line to output file\n");
> +					continue;
> +				}
> +			}
>  		}
>  	}

[Severity: Medium]
Are lines that do not match the event label regex dropped entirely from the
translated dump?

The logic to dump lines to fout when dump_trace is enabled appears to be
nested completely within this if (regexec(&label_regex, line, 2, pmatch, 0) == 0)
block. There is no corresponding else block, which would cause any
non-matching lines (like headers or metadata) to be silently omitted from
the .l output file.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260701084115.80383-1-atrajeev@linux.ibm.com?part=9