Re: [PATCH 1/2] tools: port perf ui from GTK 2 to GTK 4

[sashiko-bot@kernel.org]

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

New issues:
- [High] The custom nested GMainLoop in `perf_gtk__error()` will hang indefinitely if the parent window is closed or a process signal is received while the error dialog is open.
- [Low] The code uses GTK APIs that have been removed in GTK4, causing build failures when GTK info bar support is enabled.

Pre-existing issues:
- [High] User-triggered Use-After-Free in the progress dialog if closed manually.
- [High] Reusing an exhausted `va_list` after `vasprintf` fails leads to Undefined Behavior and arbitrary memory reads.
--

commit b3b8000cd456db5d6741b6a000f3706d6fd6d929
Author: Matt Turner <mattst88@gmail.com>

tools: port perf ui from GTK 2 to GTK 4

Port straight to GTK 4 rather than the intermediate GTK 3, since GTK 4
is where new development happens and GTK 3 is now old itself.

GTK 4 removes several APIs perf's GTK UI relied on, rather than just
renaming them, so this is a bigger change than a GTK 2 to GTK 3 port
would have been.

[ ... ]

> diff --git a/tools/perf/ui/gtk/browser.c b/tools/perf/ui/gtk/browser.c
> index d2dadf3873fbd..ee69e6a2bfbb0 100644
> --- a/tools/perf/ui/gtk/browser.c
> +++ b/tools/perf/ui/gtk/browser.c
[ ... ]
> @@ -50,15 +82,15 @@ GtkWidget *perf_gtk__setup_info_bar(void)
>  	GtkWidget *content_area;
>  
>  	info_bar = gtk_info_bar_new();
> -	gtk_widget_set_no_show_all(info_bar, TRUE);
> +	gtk_widget_set_visible(info_bar, FALSE);
>  
>  	label = gtk_label_new("");
>  	gtk_widget_show(label);
>  
>  	content_area = gtk_info_bar_get_content_area(GTK_INFO_BAR(info_bar));

[Severity: Low]
Does compiling with GTK4 enabled fail here? While test-gtk4-infobar.c still
finds gtk_info_bar_new(), APIs like gtk_widget_show(),
gtk_info_bar_get_content_area(), and gtk_widget_hide() were removed in GTK4
and replaced, which might cause build failures.

> -	gtk_container_add(GTK_CONTAINER(content_area), label);
> +	gtk_box_append(GTK_BOX(content_area), label);
>  
> -	gtk_info_bar_add_button(GTK_INFO_BAR(info_bar), GTK_STOCK_OK,
> +	gtk_info_bar_add_button(GTK_INFO_BAR(info_bar), "_OK",
>  				GTK_RESPONSE_OK);
>  	g_signal_connect(info_bar, "response",
>  			 G_CALLBACK(gtk_widget_hide), NULL);

> diff --git a/tools/perf/ui/gtk/progress.c b/tools/perf/ui/gtk/progress.c
> index eea6fcde518a8..1a73a6ba259f0 100644
> --- a/tools/perf/ui/gtk/progress.c
> +++ b/tools/perf/ui/gtk/progress.c
> @@ -13,37 +13,41 @@ static void gtk_ui_progress__update(struct ui_progress *p)
>  	char buf[1024];
>  
>  	if (dialog == NULL) {
> -		GtkWidget *vbox = gtk_vbox_new(TRUE, 5);
> +		GtkWidget *vbox = gtk_box_new(GTK_ORIENTATION_VERTICAL, 5);
>  		GtkWidget *label = gtk_label_new(p->title);
>  
> -		dialog = gtk_window_new(GTK_WINDOW_TOPLEVEL);
> +		dialog = gtk_window_new();
>  		progress = gtk_progress_bar_new();
>  
> -		gtk_box_pack_start(GTK_BOX(vbox), label, TRUE, FALSE, 3);
> -		gtk_box_pack_start(GTK_BOX(vbox), progress, TRUE, TRUE, 3);
> +		gtk_widget_set_vexpand(label, TRUE);
> +		gtk_box_append(GTK_BOX(vbox), label);
> +		gtk_widget_set_vexpand(progress, TRUE);
> +		gtk_box_append(GTK_BOX(vbox), progress);
>  
> -		gtk_container_add(GTK_CONTAINER(dialog), vbox);
> +		gtk_window_set_child(GTK_WINDOW(dialog), vbox);
>  
>  		gtk_window_set_title(GTK_WINDOW(dialog), "perf");
> -		gtk_window_resize(GTK_WINDOW(dialog), 300, 80);
> -		gtk_window_set_position(GTK_WINDOW(dialog), GTK_WIN_POS_CENTER);
> +		gtk_window_set_default_size(GTK_WINDOW(dialog), 300, 80);
>  
> -		gtk_widget_show_all(dialog);
> +		gtk_widget_set_visible(dialog, TRUE);
>  	}
>  
>  	gtk_progress_bar_set_fraction(GTK_PROGRESS_BAR(progress), fraction);

[Severity: High]
This isn't a bug introduced by this patch, but can this cause a
use-after-free if the user manually closes the progress dialog?

If the window manager destroys the dialog, the dialog and progress widget
pointers aren't set to NULL. The next call to gtk_progress_bar_set_fraction()
might dereference a freed widget pointer.

> diff --git a/tools/perf/ui/gtk/util.c b/tools/perf/ui/gtk/util.c
> index c47f5c387838c..7df80cf6b2b5a 100644
> --- a/tools/perf/ui/gtk/util.c
> +++ b/tools/perf/ui/gtk/util.c
[ ... ]
> @@ -46,9 +58,15 @@ static int perf_gtk__error(const char *format, va_list args)

[Severity: High]
This is a pre-existing issue, but could this lead to undefined behavior and
out-of-bounds stack reads?

In the early fallback paths of perf_gtk__error() and similar functions, if
vasprintf() fails it returns a negative value after potentially exhausting
args. The fallback error path immediately calls vfprintf(stderr, format, args)
without resetting the va_list via va_copy(), which could cause vfprintf() to
read past the end of the provided arguments.

>  					GTK_MESSAGE_ERROR,
>  					GTK_BUTTONS_CLOSE,
>  					"<b>Error</b>\n\n%s", msg);
> -	gtk_dialog_run(GTK_DIALOG(dialog));
>  
> -	gtk_widget_destroy(dialog);
> +	loop = g_main_loop_new(NULL, FALSE);
> +	g_signal_connect(dialog, "response",
> +			 G_CALLBACK(perf_gtk__dialog_response), loop);
> +
> +	gtk_widget_set_visible(dialog, TRUE);
> +	g_main_loop_run(loop);

[Severity: High]
If the parent window is closed or a signal is received while the error dialog
is open, will this nested main loop hang indefinitely?

Since the dialog is created with GTK_DIALOG_DESTROY_WITH_PARENT, destroying
the parent will programmatically destroy the dialog without emitting the
'response' signal, leaving the loop blocked forever.

> +	g_main_loop_unref(loop);
> +
>  	free(msg);
>  	return 0;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260906-perf-gtk2-v1-0-7564bf8523a9@gmail.com?part=1