Re: [PATCH 05/15] 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:
- [High] perf tui: SLang data race and input corruption during concurrent debuginfod fetch
--

commit 8d18f5ec06006e4e275aaf1773a82812d40a2e7f
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 TUI fetch in progress with nothing on
    screen, indistinguishable from a stuck perf, with no way out of it.
    Draw a window over the browser with the fetch progress, and drain the
    's'/'d' keys from the TUI input queue, through the new
    ui__key_pending()/ui__key_read()/ui__progress_window()/
    ui__progress_window_end() primitives declared in ui/util.h and
    implemented in ui/tui/util.c, with no-ops for builds without slang:
    slang stays behind the ui/ layer and callers stay free of #ifdefs.

> diff --git a/tools/perf/ui/tui/util.c b/tools/perf/ui/tui/util.c
> index e4d322ce0b54c..280865e980ef5 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);
> +	/*
> +	 * SLsmg_write_nstring() takes the width as an unsigned int, so never
> +	 * let a narrow terminal make it negative.  Clamp it.
> +	 */
> +	inner = SLtt_Screen_Cols - 2;
> +	if (inner < 0)
> +		inner = 0;
> +	else if (inner > 255)
> +		inner = 255;
> +	nr_lines = progress_window__wrap(text, inner, lines);

[Severity: High]
Does this code introduce data races on the static buffers?

The ui__progress_window() function updates static variables like
progress_window__bytes, progress_window__shown, and the static lines array
before acquiring the ui__lock.

Since debuginfod fetch callbacks can execute on the process_thread while the
display_thread is running, can concurrent updates corrupt these buffers and
state flags?

[ ... ]
> +/*
> + * The keys typed while the fetch blocked the browser are in the TUI
> + * input queue: nobody else is reading it, drain them through these.
> + */
> +bool ui__key_pending(void)
> +{
> +	return use_browser == 1 && SLang_input_pending(0) > 0;
> +}

[Severity: High]
Can calling this corrupt the terminal state when debuginfod fetches occur?

This calls SLang_input_pending() directly without synchronization. Since
ui__key_pending() can be triggered via debuginfod__tui_poll_cancel_keys()
from the process_thread during a background fetch, it can race with the
display_thread running ui__getch().

> +
> +int ui__key_read(void)
> +{
> +	return SLang_getkey();
> +}
> diff --git a/tools/perf/util/debuginfo.c b/tools/perf/util/debuginfo.c
> index 0ec11e1ad7508..737dd445d42bb 100644
> --- a/tools/perf/util/debuginfo.c
> +++ b/tools/perf/util/debuginfo.c
[ ... ]
> +/*
> + * The browser thread is the one doing the fetch, so its input queue has
> + * the keys typed while it was busy, drain those.
> + */
> +static void debuginfod__tui_poll_cancel_keys(void)
> +{
> +	while (ui__key_pending())
> +		debuginfod__cancel_key(ui__key_read());
> +}

[Severity: High]
Does this loop silently drop any keystrokes other than 's' or 'd'?

When debuginfod__cancel_key() processes input, it only handles 's' and 'd',
meaning any other keys read from the queue are discarded rather than being
passed back to the TUI.

When this runs on the process_thread, could it drop user input intended for
the display_thread?

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