Re: [PATCH v2 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 2 potential issue(s) to consider:
- [Medium] GMainLoop object leaks when `perf_gtk__error` is called re-entrantly due to overwriting the global loop pointer.
- [Low] Missing explicit POSIX libc header inclusions (<stdio.h>, <stdarg.h>) causes musl compilation failures.
--

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

tools: port perf ui from GTK 2 to GTK 4

This commit ports the perf GTK UI from GTK 2 to GTK 4. It updates widget
creation, packing, and event loop logic to account for GTK 4 API changes,
such as removing GtkContainer and gtk_main_quit().

> diff --git a/tools/perf/ui/gtk/progress.c b/tools/perf/ui/gtk/progress.c
> index eea6fcde518a8..28a4b486360de 100644
> --- a/tools/perf/ui/gtk/progress.c
> +++ b/tools/perf/ui/gtk/progress.c
> @@ -3,47 +3,62 @@
>  
>  #include "gtk.h"
>  #include "../progress.h"
> +#include <linux/compiler.h>

[Severity: Low]
This is a pre-existing issue, but can we explicitly include <stdio.h> here?
The snprintf function is used further down in gtk_ui_progress__update(),
and relying on implicit inclusion can cause build failures on systems using
the musl C library.

[ ... ]
> diff --git a/tools/perf/ui/gtk/util.c b/tools/perf/ui/gtk/util.c
> index c47f5c387838c..5096888b7c160 100644
> --- a/tools/perf/ui/gtk/util.c
> +++ b/tools/perf/ui/gtk/util.c
> @@ -4,6 +4,7 @@
>  
>  #include <stdlib.h>
>  #include <string.h>
> +#include <linux/compiler.h>
>  #include <linux/zalloc.h>

[Severity: Low]
Could we also explicitly include <stdarg.h> and <stdio.h> in this file?
The patch introduces the use of va_copy and va_end below, which requires
<stdarg.h>. Using vasprintf, fprintf, and vfprintf without explicitly
including <stdio.h> can cause musl compilation failures.

[ ... ]
> @@ -28,27 +29,60 @@ int perf_gtk__deactivate_context(struct perf_gtk_context **ctx)
[ ... ]
>  static int perf_gtk__error(const char *format, va_list args)
>  {
>  	char *msg;
>  	GtkWidget *dialog;
> +	va_list args_copy;
>  
> +	va_copy(args_copy, args);
>  	if (!perf_gtk__is_active_context(pgctx) ||
> -	    vasprintf(&msg, format, args) < 0) {
> +	    vasprintf(&msg, format, args_copy) < 0) {
> +		va_end(args_copy);
>  		fprintf(stderr, "Error:\n");
>  		vfprintf(stderr, format, args);
>  		fprintf(stderr, "\n");
>  		return -1;
>  	}
> +	va_end(args_copy);
>  
>  	dialog = gtk_message_dialog_new_with_markup(GTK_WINDOW(pgctx->main_window),
>  					GTK_DIALOG_DESTROY_WITH_PARENT,
>  					GTK_MESSAGE_ERROR,
>  					GTK_BUTTONS_CLOSE,
>  					"<b>Error</b>\n\n%s", msg);
> -	gtk_dialog_run(GTK_DIALOG(dialog));
>  
> -	gtk_widget_destroy(dialog);
> +	/*
> +	 * "response" only fires when a button is clicked; DESTROY_WITH_PARENT
> +	 * destroys the dialog directly without it. Quit from "destroy"
> +	 * instead, which fires either way, so the nested loop below can't
> +	 * outlive the dialog and hang.
> +	 */
> +	perf_gtk__error_loop = g_main_loop_new(NULL, FALSE);

[Severity: Medium]
What happens if perf_gtk__error() is called re-entrantly while another error
dialog is already open?

If a signal or callback triggers another error while the first dialog's event
loop is running, the global perf_gtk__error_loop pointer will be overwritten
by this new loop.

> +	g_signal_connect(dialog, "response",
> +			 G_CALLBACK(perf_gtk__dialog_response), NULL);
> +	g_signal_connect_swapped(dialog, "destroy",
> +				 G_CALLBACK(g_main_loop_quit), perf_gtk__error_loop);
> +
> +	gtk_widget_set_visible(dialog, TRUE);
> +	g_main_loop_run(perf_gtk__error_loop);
> +	g_clear_pointer(&perf_gtk__error_loop, g_main_loop_unref);

[Severity: Medium]
When the inner dialog closes, it unrefs its loop and sets the global pointer to
NULL. When the outer dialog subsequently closes, g_clear_pointer() will see a
NULL pointer and fail to unref the outer loop, resulting in a memory leak of
the GMainLoop object. Is there a way to store the loop pointer locally or track
nested loops to prevent this?

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

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