Re: [PATCH-for-9.0? 2/2] hw/misc/applesmc: Fix memory leak in reset() handler

[Peter Maydell]

On Mon, 8 Apr 2024 at 10:53, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> AppleSMCData is allocated with g_new0() in applesmc_add_key():
> release it with g_free().
>
> Leaked since commit 1ddda5cd36 ("AppleSMC device emulation").
>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2272
> Reported-by: Zheyu Ma <zheyuma97@gmail.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  hw/misc/applesmc.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/hw/misc/applesmc.c b/hw/misc/applesmc.c
> index 8e65816da6..14e3ef667d 100644
> --- a/hw/misc/applesmc.c
> +++ b/hw/misc/applesmc.c
> @@ -274,6 +274,7 @@ static void qdev_applesmc_isa_reset(DeviceState *dev)
>      /* Remove existing entries */
>      QLIST_FOREACH_SAFE(d, &s->data_def, node, next) {
>          QLIST_REMOVE(d, node);
> +        g_free(d);
>      }
>      s->status = 0x00;
>      s->status_1e = 0x00;
> --

Cc stable?

This is the right minimal fix for the leak, so
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

but overall this is a bit odd. We don't change either the
keys or their values at runtime, they seem to be a fixed
set defined by the device properties, so why are we tearing
them down and readding them every reset? It would be
simpler to create the data structure once at device realize.

thanks
-- PMM