[Qemu-devel] [PATCH 0/4] fix memory leak

[Qemu-devel] [PATCH 0/4] fix memory leak

[arei.gonglei]

From: Gonglei <arei.gonglei@huawei.com>

Spotted by Coverity.

Gonglei (4):
  cpu: fix memory leak
  ppc/spapr_drc: fix memory leak
  arm/xlnx-zynqmp: fix memory leak
  vl.c: fix memory leak

 hw/arm/xlnx-zynqmp.c | 4 ++--
 hw/ppc/spapr_drc.c   | 1 +
 target-i386/cpu.c    | 2 +-
 vl.c                 | 5 ++++-
 4 files changed, 8 insertions(+), 4 deletions(-)

-- 
1.7.12.4

[Qemu-devel] [PATCH 1/4] cpu: fix memory leak

[arei.gonglei]

From: Gonglei <arei.gonglei@huawei.com>

Failing to save or free storage allocated
by "g_strdup(names[0])" leaks it. Actually,
we can pass names[0] directly.

Signed-off-by: Gonglei <arei.gonglei@huawei.com>
---
 target-i386/cpu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index f9b1788..1e49a14 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -3021,7 +3021,7 @@ static void x86_cpu_register_feature_bit_props(X86CPU *cpu,
 
     for (i = 1; names[i]; i++) {
         feat2prop(names[i]);
-        object_property_add_alias(obj, names[i], obj, g_strdup(names[0]),
+        object_property_add_alias(obj, names[i], obj, names[0],
                                   &error_abort);
     }
 
-- 
1.7.12.4

[Qemu-devel] [PATCH 2/4] ppc/spapr_drc: fix memory leak

[arei.gonglei]

From: Gonglei <arei.gonglei@huawei.com>

fix CID 1311373.

Signed-off-by: Gonglei <arei.gonglei@huawei.com>
---
 hw/ppc/spapr_drc.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
index ef98538..ee87432 100644
--- a/hw/ppc/spapr_drc.c
+++ b/hw/ppc/spapr_drc.c
@@ -422,6 +422,7 @@ static void realize(DeviceState *d, Error **errp)
         error_free(err);
         object_unref(OBJECT(drc));
     }
+    g_free(child_name);
     DPRINTFN("drc realize complete");
 }
 
-- 
1.7.12.4

Re: [Qemu-devel] [Qemu-trivial] [PATCH 2/4] ppc/spapr_drc: fix memory leak

[Michael Roth]

Quoting arei.gonglei@huawei.com (2015-07-09 19:51:28)
> From: Gonglei <arei.gonglei@huawei.com>
> 
> fix CID 1311373.
> 
> Signed-off-by: Gonglei <arei.gonglei@huawei.com>

Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com>

> ---
>  hw/ppc/spapr_drc.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> index ef98538..ee87432 100644
> --- a/hw/ppc/spapr_drc.c
> +++ b/hw/ppc/spapr_drc.c
> @@ -422,6 +422,7 @@ static void realize(DeviceState *d, Error **errp)
>          error_free(err);
>          object_unref(OBJECT(drc));
>      }
> +    g_free(child_name);
>      DPRINTFN("drc realize complete");
>  }
> 
> -- 
> 1.7.12.4
> 
> 
> 

[Qemu-devel] [PATCH 3/4] arm/xlnx-zynqmp: fix memory leak

[arei.gonglei]

From: Gonglei <arei.gonglei@huawei.com>

fix CID 1311372.

Signed-off-by: Gonglei <arei.gonglei@huawei.com>
---
 hw/arm/xlnx-zynqmp.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/arm/xlnx-zynqmp.c b/hw/arm/xlnx-zynqmp.c
index 5e72078..00c5943 100644
--- a/hw/arm/xlnx-zynqmp.c
+++ b/hw/arm/xlnx-zynqmp.c
@@ -144,7 +144,7 @@ static void xlnx_zynqmp_realize(DeviceState *dev, Error **errp)
         } else {
             s->boot_cpu_ptr = &s->apu_cpu[i];
         }
-
+        g_free(name);
         object_property_set_int(OBJECT(&s->apu_cpu[i]), GIC_BASE_ADDR,
                                 "reset-cbar", &err);
         if (err) {
@@ -181,7 +181,7 @@ static void xlnx_zynqmp_realize(DeviceState *dev, Error **errp)
         } else {
             s->boot_cpu_ptr = &s->rpu_cpu[i];
         }
-
+        g_free(name);
         object_property_set_bool(OBJECT(&s->rpu_cpu[i]), true, "reset-hivecs",
                                  &err);
         if (err != NULL) {
-- 
1.7.12.4

Re: [Qemu-devel] [PATCH 3/4] arm/xlnx-zynqmp: fix memory leak

[Peter Crosthwaite]

On Thu, Jul 9, 2015 at 5:51 PM,  <arei.gonglei@huawei.com> wrote:
> From: Gonglei <arei.gonglei@huawei.com>
>
> fix CID 1311372.
>
> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> ---
>  hw/arm/xlnx-zynqmp.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/hw/arm/xlnx-zynqmp.c b/hw/arm/xlnx-zynqmp.c
> index 5e72078..00c5943 100644
> --- a/hw/arm/xlnx-zynqmp.c
> +++ b/hw/arm/xlnx-zynqmp.c
> @@ -144,7 +144,7 @@ static void xlnx_zynqmp_realize(DeviceState *dev, Error **errp)
>          } else {
>              s->boot_cpu_ptr = &s->apu_cpu[i];
>          }
> -

Thanks,

Please keep this blank line here and below. The g_gree should group
with the above code.

Otherwise,

Reviewed-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>

Regards,
Peter

> +        g_free(name);
>          object_property_set_int(OBJECT(&s->apu_cpu[i]), GIC_BASE_ADDR,
>                                  "reset-cbar", &err);
>          if (err) {
> @@ -181,7 +181,7 @@ static void xlnx_zynqmp_realize(DeviceState *dev, Error **errp)
>          } else {
>              s->boot_cpu_ptr = &s->rpu_cpu[i];
>          }
> -
> +        g_free(name);
>          object_property_set_bool(OBJECT(&s->rpu_cpu[i]), true, "reset-hivecs",
>                                   &err);
>          if (err != NULL) {
> --
> 1.7.12.4
>
>
>

[Qemu-devel] [PATCH 4/4] vl.c: fix memory leak

[arei.gonglei]

From: Gonglei <arei.gonglei@huawei.com>

Failing to save or free storage allocated
by "g_strdup(cmd)" leaks it. Let's use a
variable to storage it.

Signed-off-by: Gonglei <arei.gonglei@huawei.com>
---
 vl.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/vl.c b/vl.c
index 3f269dc..399e816 100644
--- a/vl.c
+++ b/vl.c
@@ -1326,16 +1326,19 @@ static int add_semihosting_arg(void *opaque,
 static inline void semihosting_arg_fallback(const char *file, const char *cmd)
 {
     char *cmd_token;
+    char *cmd_str;
 
     /* argv[0] */
     add_semihosting_arg(&semihosting, "arg", file, NULL);
 
+    cmd_str = g_strdup(cmd);
     /* split -append and initialize argv[1..n] */
-    cmd_token = strtok(g_strdup(cmd), " ");
+    cmd_token = strtok(cmd_str, " ");
     while (cmd_token) {
         add_semihosting_arg(&semihosting, "arg", cmd_token, NULL);
         cmd_token = strtok(NULL, " ");
     }
+    g_free(cmd_str);
 }
 
 /***********************************************************/
-- 
1.7.12.4

Re: [Qemu-devel] [PATCH 4/4] vl.c: fix memory leak

[Leon Alrae]

On 10/07/2015 01:51, arei.gonglei@huawei.com wrote:
> From: Gonglei <arei.gonglei@huawei.com>
> 
> Failing to save or free storage allocated
> by "g_strdup(cmd)" leaks it. Let's use a
> variable to storage it.
> 
> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> ---
>  vl.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/vl.c b/vl.c
> index 3f269dc..399e816 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -1326,16 +1326,19 @@ static int add_semihosting_arg(void *opaque,
>  static inline void semihosting_arg_fallback(const char *file, const char *cmd)
>  {
>      char *cmd_token;
> +    char *cmd_str;
>  
>      /* argv[0] */
>      add_semihosting_arg(&semihosting, "arg", file, NULL);
>  
> +    cmd_str = g_strdup(cmd);
>      /* split -append and initialize argv[1..n] */
> -    cmd_token = strtok(g_strdup(cmd), " ");
> +    cmd_token = strtok(cmd_str, " ");
>      while (cmd_token) {
>          add_semihosting_arg(&semihosting, "arg", cmd_token, NULL);
>          cmd_token = strtok(NULL, " ");
>      }
> +    g_free(cmd_str);
>  }
>  
>  /***********************************************************/
> 

I don't think this is correct as there's no leak here. This duplicated string
is modified (i.e. split into tokens) and each pointer to the beginning of a
token is saved in the global semihosting.argv[] array which is used later in
target semihosting code. It shouldn't be freed.

Regards,
Leon

Re: [Qemu-devel] [PATCH 4/4] vl.c: fix memory leak

[Gonglei]

On 2015/7/10 17:28, Leon Alrae wrote:
> On 10/07/2015 01:51, arei.gonglei@huawei.com wrote:
>> From: Gonglei <arei.gonglei@huawei.com>
>>
>> Failing to save or free storage allocated
>> by "g_strdup(cmd)" leaks it. Let's use a
>> variable to storage it.
>>
>> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
>> ---
>>  vl.c | 5 ++++-
>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/vl.c b/vl.c
>> index 3f269dc..399e816 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -1326,16 +1326,19 @@ static int add_semihosting_arg(void *opaque,
>>  static inline void semihosting_arg_fallback(const char *file, const char *cmd)
>>  {
>>      char *cmd_token;
>> +    char *cmd_str;
>>  
>>      /* argv[0] */
>>      add_semihosting_arg(&semihosting, "arg", file, NULL);
>>  
>> +    cmd_str = g_strdup(cmd);
>>      /* split -append and initialize argv[1..n] */
>> -    cmd_token = strtok(g_strdup(cmd), " ");
>> +    cmd_token = strtok(cmd_str, " ");
>>      while (cmd_token) {
>>          add_semihosting_arg(&semihosting, "arg", cmd_token, NULL);
>>          cmd_token = strtok(NULL, " ");
>>      }
>> +    g_free(cmd_str);
>>  }
>>  
>>  /***********************************************************/
>>
> 
> I don't think this is correct as there's no leak here. This duplicated string
> is modified (i.e. split into tokens) and each pointer to the beginning of a
> token is saved in the global semihosting.argv[] array which is used later in
> target semihosting code. It shouldn't be freed.
> 
Yep, I look over the logic, and you are right. :)
False positive report. NACK please.

Thanks,
-Gonglei

Re: [Qemu-devel] [PATCH 0/4] fix memory leak

[Paolo Bonzini]

On 10/07/2015 02:51, arei.gonglei@huawei.com wrote:
> From: Gonglei <arei.gonglei@huawei.com>
> 
> Spotted by Coverity.
> 
> Gonglei (4):
>   cpu: fix memory leak
>   ppc/spapr_drc: fix memory leak
>   arm/xlnx-zynqmp: fix memory leak
>   vl.c: fix memory leak
> 
>  hw/arm/xlnx-zynqmp.c | 4 ++--
>  hw/ppc/spapr_drc.c   | 1 +
>  target-i386/cpu.c    | 2 +-
>  vl.c                 | 5 ++++-
>  4 files changed, 8 insertions(+), 4 deletions(-)
> 

Eduardo posted a patch for 1, too.

Paolo