diff mbox series

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

Message ID 20240408095217.57239-3-philmd@linaro.org
State Superseded
Headers show
Series hw/misc/applesmc: Fix memory leak | expand

Commit Message

Philippe Mathieu-Daudé April 8, 2024, 9:52 a.m. UTC 
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(+)

Comments

Peter Maydell April 8, 2024, 10:34 a.m. UTC | #1 
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
Philippe Mathieu-Daudé April 8, 2024, 11:17 a.m. UTC | #2 
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.
diff mbox series

Patch

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;