Re: [PATCH perf-tools-next v3 1/5] perf ftrace: Optimise __cmd_ftrace() stack frame with strbuf streaming

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] perf: dropped stdout error check causes infinite loop on EPIPE
- [Medium] perf: uninitialized heap memory read in __cmd_ftrace() strbuf
--

commit 0a06afad8e6a1e6bf21a4b8a3c19582ed3d99cd1
Author: Aaron Tomlin <atomlin@atomlin.com>

perf ftrace: Optimise __cmd_ftrace() stack frame with strbuf streaming

This commit refactors __cmd_ftrace() to use dynamic heap allocation
for the read buffer and a strbuf to accumulate streamed characters
into lines. This reduces the stack frame size by over 96%.

> diff --git a/tools/perf/builtin-ftrace.c b/tools/perf/builtin-ftrace.c
> index 6017493ff1797..d6604be593a34 100644
> --- a/tools/perf/builtin-ftrace.c
> +++ b/tools/perf/builtin-ftrace.c

[ ... ]

> @@ -810,11 +821,19 @@ static int __cmd_ftrace(struct perf_ftrace *ftrace)
>  			break;
>  
>  		if (pollfd.revents & POLLIN) {
> -			int n = read(trace_fd, buf, sizeof(buf));
> -			if (n < 0)
> -				break;
> -			if (fwrite(buf, n, 1, stdout) != 1)
> +			int n = read(trace_fd, buf, TRACE_BUF_SIZE);
> +
> +			if (n <= 0)
>  				break;
> +			for (int i = 0; i < n; i++) {
> +				if (buf[i] == '\n') {
> +					fprintf(stdout, "%s\n", linebuf.buf);

[Severity: Medium]
Can this print uninitialized heap memory?

Earlier in this function, strbuf_init() allocates the buffer but does not
explicitly null-terminate it:

tools/perf/util/strbuf.c:strbuf_grow() {
    ...
    buf = realloc(sb->alloc ? sb->buf : NULL, nr * sizeof(*buf));
    if (!buf)
        return -ENOMEM;

    sb->buf = buf;
    sb->alloc = nr;
    return 0;
}

If the very first character read from trace_pipe is '\n' (for example, from an
empty trace line), linebuf.buf is passed to fprintf() as a string before any
null byte has been written to it.

[Severity: Medium]
Does the removal of the stdout error check cause an infinite loop?

The original code explicitly checked if fwrite() failed to gracefully exit
the polling loop if stdout closes (for example, when the pager exits or
when piped to head).

Because trace_pipe provides endless continuous data while tracing is active,
ignoring the fprintf() error forces the tool to endlessly read trace data
and discard it, never terminating.

> +					strbuf_setlen(&linebuf, 0);
> +				} else {
> +					if (strbuf_addch(&linebuf, buf[i]) < 0)
> +						goto out_close_fd;
> +				}
> +			}
>  			/* flush output since stdout is in full buffering mode due to pager */
>  			fflush(stdout);
>  		}

[ ... ]

> @@ -832,15 +851,31 @@ static int __cmd_ftrace(struct perf_ftrace *ftrace)
>  
>  	/* read remaining buffer contents */
>  	while (true) {
> -		int n = read(trace_fd, buf, sizeof(buf));
> +		int n = read(trace_fd, buf, TRACE_BUF_SIZE);
> +
>  		if (n <= 0)
>  			break;
> -		if (fwrite(buf, n, 1, stdout) != 1)
> -			break;
> +		for (int i = 0; i < n; i++) {
> +			if (buf[i] == '\n') {
> +				fprintf(stdout, "%s\n", linebuf.buf);

[Severity: Medium]
Does the lack of a return value check on fprintf() also cause an infinite
loop here on a broken pipe?

The previous code broke out of the loop if fwrite() failed. Without
checking the return value of fprintf(), an error writing to stdout is
ignored.

> +				strbuf_setlen(&linebuf, 0);
> +			} else {
> +				if (strbuf_addch(&linebuf, buf[i]) < 0)
> +					goto out_close_fd;
> +			}
> +		}
> +	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261012003016.263063-1-atomlin@atomlin.com?part=1