Re: [PATCH 2/8] plugins: fix memory leak while parsing options

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

On 19/5/23 19:04, Alex Bennée wrote:
> It was hard to track down this leak as it was an internal allocation
> by glib and the backtraces did not give much away. The autofree was
> freeing the allocation with g_free() but not taking care of the
> individual strings. They should have been freed with g_strfreev()
> instead.
> 
> Searching the glib source code for the correct string free function
> led to:
> 
>    G_DEFINE_AUTO_CLEANUP_FREE_FUNC(GStrv, g_strfreev, NULL)
> 
> and indeed if you read to the bottom of the documentation page you
> will find:
> 
>    typedef gchar** GStrv;
> 
>    A typedef alias for gchar**. This is mostly useful when used together with g_auto().

So possibly glib could improve by declaring g_strsplit()
(and co) returning a GStrv instead of a gchar** type?

> So fix up all the g_autofree g_strsplit case that smugly thought they
> had de-allocation covered.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>   contrib/plugins/cache.c     | 2 +-
>   contrib/plugins/drcov.c     | 2 +-
>   contrib/plugins/execlog.c   | 2 +-
>   contrib/plugins/hotblocks.c | 2 +-
>   contrib/plugins/hotpages.c  | 2 +-
>   contrib/plugins/howvec.c    | 2 +-
>   contrib/plugins/hwprofile.c | 2 +-
>   contrib/plugins/lockstep.c  | 2 +-
>   tests/plugin/bb.c           | 2 +-
>   tests/plugin/insn.c         | 2 +-
>   tests/plugin/mem.c          | 2 +-
>   tests/plugin/syscall.c      | 2 +-
>   12 files changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/contrib/plugins/cache.c b/contrib/plugins/cache.c
> index 2e25184a7f..5036213f1b 100644
> --- a/contrib/plugins/cache.c
> +++ b/contrib/plugins/cache.c
> @@ -772,7 +772,7 @@ int qemu_plugin_install(qemu_plugin_id_t id, const qemu_info_t *info,
>   
>       for (i = 0; i < argc; i++) {
>           char *opt = argv[i];
> -        g_autofree char **tokens = g_strsplit(opt, "=", 2);
> +        g_auto(GStrv) tokens = g_strsplit(opt, "=", 2);

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>