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

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

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.