[Qemu-devel] [PATCH 2/5] spapr_cpu_core: fix potential leak in spapr_cpu_core_realize()

Greg Kurz posted 5 patches 7 years, 8 months ago
[Qemu-devel] [PATCH 2/5] spapr_cpu_core: fix potential leak in spapr_cpu_core_realize()
Posted by Greg Kurz 7 years, 8 months ago
Commit 94ad93bd97684 (QEMU 2.12) switched to instantiate CPUs separately
but it missed to adapt the error path accordingly. If something fails in
the CPU creation loop, then the CPU object that was just created is leaked.

The error paths in this function are a bit obfuscated, and adding
yet another label to free this CPU object makes it worse. We should
move the block of the loop to a separate function, with a proper
rollback path, but this is a bigger cleanup.

For now, let's just fix the bug by adding the missing calls to
object_unref(). This will allow easier backport to older QEMU
versions.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/ppc/spapr_cpu_core.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
index 27602245fd55..003c4c5a79d2 100644
--- a/hw/ppc/spapr_cpu_core.c
+++ b/hw/ppc/spapr_cpu_core.c
@@ -201,6 +201,7 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
         cs->cpu_index = cc->core_id + i;
         spapr_set_vcpu_id(cpu, cs->cpu_index, &local_err);
         if (local_err) {
+            object_unref(obj);
             goto err;
         }
 
@@ -212,6 +213,7 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
         object_property_add_child(OBJECT(sc), id, obj, &local_err);
         g_free(id);
         if (local_err) {
+            object_unref(obj);
             goto err;
         }
         object_unref(obj);


Re: [Qemu-devel] [PATCH 2/5] spapr_cpu_core: fix potential leak in spapr_cpu_core_realize()
Posted by David Gibson 7 years, 8 months ago
On Thu, Jun 14, 2018 at 11:50:27PM +0200, Greg Kurz wrote:
> Commit 94ad93bd97684 (QEMU 2.12) switched to instantiate CPUs separately
> but it missed to adapt the error path accordingly. If something fails in
> the CPU creation loop, then the CPU object that was just created is leaked.
> 
> The error paths in this function are a bit obfuscated, and adding
> yet another label to free this CPU object makes it worse. We should
> move the block of the loop to a separate function, with a proper
> rollback path, but this is a bigger cleanup.
> 
> For now, let's just fix the bug by adding the missing calls to
> object_unref(). This will allow easier backport to older QEMU
> versions.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>

Applied to ppc-for-3.0, thanks.

> ---
>  hw/ppc/spapr_cpu_core.c |    2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> index 27602245fd55..003c4c5a79d2 100644
> --- a/hw/ppc/spapr_cpu_core.c
> +++ b/hw/ppc/spapr_cpu_core.c
> @@ -201,6 +201,7 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
>          cs->cpu_index = cc->core_id + i;
>          spapr_set_vcpu_id(cpu, cs->cpu_index, &local_err);
>          if (local_err) {
> +            object_unref(obj);
>              goto err;
>          }
>  
> @@ -212,6 +213,7 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
>          object_property_add_child(OBJECT(sc), id, obj, &local_err);
>          g_free(id);
>          if (local_err) {
> +            object_unref(obj);
>              goto err;
>          }
>          object_unref(obj);
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson