Re: [PATCH 05/12] perf debuginfo: Show the debuginfod fetch progress and keys in the TUI

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] perf tui: infinite padding loop on narrow terminal width
--

commit bf7df34cfc153e952fa2cf73c078a1a435dfeb1c
Author: Arnaldo Carvalho de Melo <acme@redhat.com>

perf debuginfo: Show the debuginfod fetch progress and keys in the TUI

The stdio only fetch UI left a fetch in progress in the TUI with
nothing on screen, indistinguishable from perf being stuck, with no way
out of it. The browser thread is the one doing the fetch, so it draws a
window over itself with the fetch progress and drains the 's'/'d' keys
from the input queue it owns.

> diff --git a/tools/perf/ui/tui/util.c b/tools/perf/ui/tui/util.c
> index e4d322ce0b54c..c3274b92b0e22 100644
> --- a/tools/perf/ui/tui/util.c
> +++ b/tools/perf/ui/tui/util.c

[ ... ]

> +void ui__progress_window(const char *title, const char *text,
> +			 u64 fetched, u64 total)
> +{
> +	static char lines[PROGRESS_WINDOW_MAX_LINES][256];
> +	char buf_cur[20], buf_tot[20], bytes[64];
> +	size_t len;
> +	int y, height, nr_lines, inner, i;
> +
> +	if (use_browser != 1)
> +		return;
> +
> +	unit_number__scnprintf(buf_cur, sizeof(buf_cur), fetched);
> +	if (total) {
> +		unit_number__scnprintf(buf_tot, sizeof(buf_tot), total);
> +		scnprintf(bytes, sizeof(bytes), "  %s / %s fetched",
> +			  buf_cur, buf_tot);
> +	} else {
> +		scnprintf(bytes, sizeof(bytes),
> +			  "  %s fetched, size unknown", buf_cur);
> +	}
> +
> +	if (progress_window__shown && !strcmp(bytes, progress_window__bytes))
> +		return;
> +
> +	scnprintf(progress_window__bytes, sizeof(progress_window__bytes),
> +		  "%s", bytes);
> +	progress_window__shown = true;
> +
> +	ui__refresh_dimensions(false);
> +	mutex_lock(&ui__lock);
> +	inner = SLtt_Screen_Cols - 2;
> +	if (inner > 255)
> +		inner = 255;
> +	nr_lines = progress_window__wrap(text, inner, lines);

[Severity: Medium]
If the terminal is resized to 0 or 1 columns during a fetch, could
SLtt_Screen_Cols - 2 underflow to a negative value, allowing inner to
become negative without bounds checking here?

> +	height = nr_lines + 3;
> +
> +	SLsmg_set_color(0);
> +	if (progress_window__rows)
> +		SLsmg_fill_region(progress_window__y, 0, progress_window__rows,
> +				  SLtt_Screen_Cols, ' ');
> +	y = (SLtt_Screen_Rows - height) / 2;
> +	if (y < 0)
> +		y = 0;
> +	progress_window__y = y;
> +	progress_window__rows = height;
> +
> +	SLsmg_draw_box(y, 0, height, SLtt_Screen_Cols);
> +	SLsmg_gotorc(y++, 1);
> +	len = strlen(title);
> +	if (len > (size_t)inner)
> +		len = inner;
> +	SLsmg_write_nchars(title, len);

[Severity: Medium]
If inner is negative, won't casting it to size_t result in a huge unsigned
value? This seems like it would bypass the length bounds check and could read
out of bounds.

> +	for (i = 0; i < nr_lines; i++, y++) {
> +		SLsmg_gotorc(y, 1);
> +		SLsmg_write_nstring(lines[i], SLtt_Screen_Cols - 2);
> +	}

[Severity: Medium]
Could passing SLtt_Screen_Cols - 2 as a negative value here convert to a
massive unsigned parameter in the TUI renderer SLsmg_write_nstring()? This
seems like it would cause an endless padding loop that spikes the CPU and
locks up the TUI.

> +	SLsmg_gotorc(y, 1);
> +	SLsmg_write_nstring(bytes, SLtt_Screen_Cols - 2);

[Severity: Medium]
Does this also pass the same potentially underflowed width value to the padding
function?

> +	SLsmg_refresh();
> +	mutex_unlock(&ui__lock);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260916114740.48230-1-acme@kernel.org?part=5