[PATCH-for-9.0? 0/2] hw/misc/applesmc: Fix memory leak

[PATCH-for-9.0? 0/2] hw/misc/applesmc: Fix memory leak

[Philippe Mathieu-Daudé]

Fix for https://gitlab.com/qemu-project/qemu/-/issues/2272

Philippe Mathieu-Daudé (2):
  hw/misc/applesmc: Do not call DeviceReset() from DeviceRealize()
  hw/misc/applesmc: Fix memory leak in reset() handler

 hw/misc/applesmc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

-- 
2.41.0

[PATCH-for-9.0? 1/2] hw/misc/applesmc: Do not call DeviceReset() from DeviceRealize()

[Philippe Mathieu-Daudé]

QDev core layer always call DeviceReset() after DeviceRealize(),
no need to do it manually. Remove the extra call.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/misc/applesmc.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/hw/misc/applesmc.c b/hw/misc/applesmc.c
index 72300d0cbc..8e65816da6 100644
--- a/hw/misc/applesmc.c
+++ b/hw/misc/applesmc.c
@@ -342,7 +342,6 @@ static void applesmc_isa_realize(DeviceState *dev, Error **errp)
     }
 
     QLIST_INIT(&s->data_def);
-    qdev_applesmc_isa_reset(dev);
 }
 
 static Property applesmc_isa_properties[] = {
-- 
2.41.0

Re: [PATCH-for-9.0? 1/2] hw/misc/applesmc: Do not call DeviceReset() from DeviceRealize()

[Peter Maydell]

On Mon, 8 Apr 2024 at 10:52, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> QDev core layer always call DeviceReset() after DeviceRealize(),
> no need to do it manually. Remove the extra call.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  hw/misc/applesmc.c | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/hw/misc/applesmc.c b/hw/misc/applesmc.c
> index 72300d0cbc..8e65816da6 100644
> --- a/hw/misc/applesmc.c
> +++ b/hw/misc/applesmc.c
> @@ -342,7 +342,6 @@ static void applesmc_isa_realize(DeviceState *dev, Error **errp)
>      }
>
>      QLIST_INIT(&s->data_def);
> -    qdev_applesmc_isa_reset(dev);
>  }
>
>  static Property applesmc_isa_properties[] = {
> --

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

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

[Philippe Mathieu-Daudé]

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;
-- 
2.41.0

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

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

[Philippe Mathieu-Daudé]

On 8/4/24 12:34, Peter Maydell wrote:
> 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.

This was my first approach, moving the applesmc_add_key()
calls to the realize() handler, and freeing them in a
unrealize() one:

-- >8 --
diff --git a/hw/misc/applesmc.c b/hw/misc/applesmc.c
index 14e3ef667d..59a4899312 100644
--- a/hw/misc/applesmc.c
+++ b/hw/misc/applesmc.c
@@ -145,7 +145,7 @@ static void applesmc_io_cmd_write(void *opaque, 
hwaddr addr, uint64_t val,
      s->data_pos = 0;
  }

-static struct AppleSMCData *applesmc_find_key(AppleSMCState *s)
+static const struct AppleSMCData *applesmc_find_key(AppleSMCState *s)
  {
      struct AppleSMCData *d;

@@ -161,7 +161,7 @@ static void applesmc_io_data_write(void *opaque, 
hwaddr addr, uint64_t val,
                                     unsigned size)
  {
      AppleSMCState *s = opaque;
-    struct AppleSMCData *d;
+    const struct AppleSMCData *d;

      smc_debug("DATA received: 0x%02x\n", (uint8_t)val);
      switch (s->cmd) {
@@ -269,23 +269,10 @@ static void applesmc_add_key(AppleSMCState *s, 
const char *key,
  static void qdev_applesmc_isa_reset(DeviceState *dev)
  {
      AppleSMCState *s = APPLE_SMC(dev);
-    struct AppleSMCData *d, *next;

-    /* 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;
      s->last_ret = 0x00;
-
-    applesmc_add_key(s, "REV ", 6, "\x01\x13\x0f\x00\x00\x03");
-    applesmc_add_key(s, "OSK0", 32, s->osk);
-    applesmc_add_key(s, "OSK1", 32, s->osk + 32);
-    applesmc_add_key(s, "NATJ", 1, "\0");
-    applesmc_add_key(s, "MSSP", 1, "\0");
-    applesmc_add_key(s, "MSSD", 1, "\0x3");
  }

  static const MemoryRegionOps applesmc_data_io_ops = {
@@ -343,6 +330,24 @@ static void applesmc_isa_realize(DeviceState *dev, 
Error **errp)
      }

      QLIST_INIT(&s->data_def);
+    applesmc_add_key(s, "REV ", 6, "\x01\x13\x0f\x00\x00\x03");
+    applesmc_add_key(s, "OSK0", 32, s->osk);
+    applesmc_add_key(s, "OSK1", 32, s->osk + 32);
+    applesmc_add_key(s, "NATJ", 1, "\0");
+    applesmc_add_key(s, "MSSP", 1, "\0");
+    applesmc_add_key(s, "MSSD", 1, "\0x3");
+}
+
+static void applesmc_unrealize(DeviceState *dev)
+{
+    AppleSMCState *s = APPLE_SMC(dev);
+    struct AppleSMCData *d, *next;
+
+    /* Remove existing entries */
+    QLIST_FOREACH_SAFE(d, &s->data_def, node, next) {
+        QLIST_REMOVE(d, node);
+        g_free(d);
+    }
  }

  static Property applesmc_isa_properties[] = {
@@ -377,6 +382,7 @@ static void qdev_applesmc_class_init(ObjectClass 
*klass, void *data)
      AcpiDevAmlIfClass *adevc = ACPI_DEV_AML_IF_CLASS(klass);

      dc->realize = applesmc_isa_realize;
+    dc->unrealize = applesmc_unrealize;
      dc->reset = qdev_applesmc_isa_reset;
      device_class_set_props(dc, applesmc_isa_properties);
      set_bit(DEVICE_CATEGORY_MISC, dc->categories);

---

But since a bit too much changes for the next release, I kept
it as a separate patch.

Thanks for the review,

Phil.