[PATCH ci-fix 0/8] fix various memory leaks (but not all)

[PATCH ci-fix 0/8] fix various memory leaks (but not all)

[Paolo Bonzini]

Hi all, the asan and docker-clang@ubuntu tests are failing due
to memory leaks, mostly in instance_init but some also in
realize.  The former are caught by device-introspect-test,
the latter by boot-serial-test.

This series fixes the simplest ones.  ARM and aarch64 failures
remain so I'm disabling LeakSanitizer tests in CI with the last
patch.

Paolo

Paolo Bonzini (8):
  ide: fix leak from qemu_allocate_irqs
  microblaze: fix leak of fdevice tree blob
  mcf5208: fix leak from qemu_allocate_irqs
  hppa: fix leak from g_strdup_printf
  mips: fix memory leaks in board initialization
  cris: do not leak struct cris_disasm_data
  lm32: do not leak memory on object_new/object_unref
  docker: test-debug: disable LeakSanitizer

 disas/cris.c                | 59 ++++++++++++++++++++++-----------------------
 hw/hppa/dino.c              |  1 +
 hw/hppa/machine.c           |  4 ++-
 hw/ide/cmd646.c             |  1 +
 hw/m68k/mcf5208.c           |  2 ++
 hw/microblaze/boot.c        |  1 +
 hw/mips/mips_int.c          |  1 +
 hw/mips/mips_jazz.c         |  2 ++
 hw/timer/lm32_timer.c       |  6 ++---
 hw/timer/milkymist-sysctl.c | 10 ++++----
 tests/docker/test-debug     |  1 +
 11 files changed, 49 insertions(+), 39 deletions(-)

-- 
1.8.3.1

[PATCH 1/8] ide: fix leak from qemu_allocate_irqs

[Paolo Bonzini]

The array returned by qemu_allocate_irqs is malloced, free it.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/ide/cmd646.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/ide/cmd646.c b/hw/ide/cmd646.c
index f3ccd11..19984d2 100644
--- a/hw/ide/cmd646.c
+++ b/hw/ide/cmd646.c
@@ -300,6 +300,7 @@ static void pci_cmd646_ide_realize(PCIDevice *dev, Error **errp)
         d->bmdma[i].bus = &d->bus[i];
         ide_register_restart_cb(&d->bus[i]);
     }
+    g_free(irq);
 
     vmstate_register(DEVICE(dev), 0, &vmstate_ide_pci, d);
     qemu_register_reset(cmd646_reset, d);
-- 
1.8.3.1

[PATCH 2/8] microblaze: fix leak of fdevice tree blob

[Paolo Bonzini]

The device tree blob returned by load_device_tree is malloced.
Free it before returning.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/microblaze/boot.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/microblaze/boot.c b/hw/microblaze/boot.c
index bade4d2..d1d7dfb 100644
--- a/hw/microblaze/boot.c
+++ b/hw/microblaze/boot.c
@@ -100,6 +100,7 @@ static int microblaze_load_dtb(hwaddr addr,
     }
 
     cpu_physical_memory_write(addr, fdt, fdt_size);
+    g_free(fdt);
     return fdt_size;
 }
 
-- 
1.8.3.1

[PATCH 3/8] mcf5208: fix leak from qemu_allocate_irqs

[Paolo Bonzini]

The array returned by qemu_allocate_irqs is malloced, free it.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/m68k/mcf5208.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/m68k/mcf5208.c b/hw/m68k/mcf5208.c
index 012710d..60c5802 100644
--- a/hw/m68k/mcf5208.c
+++ b/hw/m68k/mcf5208.c
@@ -273,6 +273,8 @@ static void mcf5208evb_init(MachineState *machine)
                      0xfc030000, pic + 36);
     }
 
+    g_free(pic);
+
     /*  0xfc000000 SCM.  */
     /*  0xfc004000 XBS.  */
     /*  0xfc008000 FlexBus CS.  */
-- 
1.8.3.1

[PATCH 4/8] hppa: fix leak from g_strdup_printf

[Paolo Bonzini]

memory_region_init_* takes care of copying the name into memory it owns.
Free it in the caller.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/hppa/dino.c    | 1 +
 hw/hppa/machine.c | 4 +++-
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/hw/hppa/dino.c b/hw/hppa/dino.c
index e0466ee..ab6969b 100644
--- a/hw/hppa/dino.c
+++ b/hw/hppa/dino.c
@@ -485,6 +485,7 @@ PCIBus *dino_init(MemoryRegion *addr_space,
         memory_region_init_alias(&s->pci_mem_alias[i], OBJECT(s),
                                  name, &s->pci_mem, addr,
                                  DINO_MEM_CHUNK_SIZE);
+        g_free(name);
     }
 
     /* Set up PCI view of memory: Bus master address space.  */
diff --git a/hw/hppa/machine.c b/hw/hppa/machine.c
index 2736ce8..7e23675 100644
--- a/hw/hppa/machine.c
+++ b/hw/hppa/machine.c
@@ -78,13 +78,15 @@ static void machine_hppa_init(MachineState *machine)
 
     /* Create CPUs.  */
     for (i = 0; i < smp_cpus; i++) {
+        char *name = g_strdup_printf("cpu%ld-io-eir", i);
         cpu[i] = HPPA_CPU(cpu_create(machine->cpu_type));
 
         cpu_region = g_new(MemoryRegion, 1);
         memory_region_init_io(cpu_region, OBJECT(cpu[i]), &hppa_io_eir_ops,
-                              cpu[i], g_strdup_printf("cpu%ld-io-eir", i), 4);
+                              cpu[i], name, 4);
         memory_region_add_subregion(addr_space, CPU_HPA + i * 0x1000,
                                     cpu_region);
+        g_free(name);
     }
 
     /* Limit main memory. */
-- 
1.8.3.1

[PATCH 5/8] mips: fix memory leaks in board initialization

[Paolo Bonzini]

They are not a big deal, but they upset asan.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/mips/mips_int.c  | 1 +
 hw/mips/mips_jazz.c | 2 ++
 2 files changed, 3 insertions(+)

diff --git a/hw/mips/mips_int.c b/hw/mips/mips_int.c
index 5ebc961..863ed45 100644
--- a/hw/mips/mips_int.c
+++ b/hw/mips/mips_int.c
@@ -81,6 +81,7 @@ void cpu_mips_irq_init_cpu(MIPSCPU *cpu)
     for (i = 0; i < 8; i++) {
         env->irq[i] = qi[i];
     }
+    g_free(qi);
 }
 
 void cpu_mips_soft_irq(CPUMIPSState *env, int irq, int level)
diff --git a/hw/mips/mips_jazz.c b/hw/mips/mips_jazz.c
index c967b97..8d010a0 100644
--- a/hw/mips/mips_jazz.c
+++ b/hw/mips/mips_jazz.c
@@ -362,6 +362,8 @@ static void mips_jazz_init(MachineState *machine,
 
     /* LED indicator */
     sysbus_create_simple("jazz-led", 0x8000f000, NULL);
+
+    g_free(dmas);
 }
 
 static
-- 
1.8.3.1

[PATCH 6/8] cris: do not leak struct cris_disasm_data

[Paolo Bonzini]

Use a stack-allocated struct to avoid a memory leak.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 disas/cris.c | 59 +++++++++++++++++++++++++++++------------------------------
 1 file changed, 29 insertions(+), 30 deletions(-)

diff --git a/disas/cris.c b/disas/cris.c
index 2f43c9b..0b0a3fb 100644
--- a/disas/cris.c
+++ b/disas/cris.c
@@ -1294,24 +1294,17 @@ static int cris_constraint
 /* Parse disassembler options and store state in info.  FIXME: For the
    time being, we abuse static variables.  */
 
-static bfd_boolean
-cris_parse_disassembler_options (disassemble_info *info,
+static void
+cris_parse_disassembler_options (struct cris_disasm_data *disdata,
+				 char *disassembler_options,
 				 enum cris_disass_family distype)
 {
-  struct cris_disasm_data *disdata;
-
-  info->private_data = calloc (1, sizeof (struct cris_disasm_data));
-  disdata = (struct cris_disasm_data *) info->private_data;
-  if (disdata == NULL)
-    return false;
-
   /* Default true.  */
   disdata->trace_case
-    = (info->disassembler_options == NULL
-       || (strcmp (info->disassembler_options, "nocase") != 0));
+    = (disassembler_options == NULL
+       || (strcmp (disassembler_options, "nocase") != 0));
 
   disdata->distype = distype;
-  return true;
 }
 
 static const struct cris_spec_reg *
@@ -2736,9 +2729,10 @@ static int
 print_insn_cris_with_register_prefix (bfd_vma vma,
 				      disassemble_info *info)
 {
-  if (info->private_data == NULL
-      && !cris_parse_disassembler_options (info, cris_dis_v0_v10))
-    return -1;
+  struct cris_disasm_data disdata;
+  info->private_data = &disdata;
+  cris_parse_disassembler_options (&disdata, info->disassembler_options,
+				   cris_dis_v0_v10);
   return print_insn_cris_generic (vma, info, true);
 }
 /* Disassemble, prefixing register names with `$'.  CRIS v32.  */
@@ -2747,9 +2741,10 @@ static int
 print_insn_crisv32_with_register_prefix (bfd_vma vma,
 					 disassemble_info *info)
 {
-  if (info->private_data == NULL
-      && !cris_parse_disassembler_options (info, cris_dis_v32))
-    return -1;
+  struct cris_disasm_data disdata;
+  info->private_data = &disdata;
+  cris_parse_disassembler_options (&disdata, info->disassembler_options,
+				   cris_dis_v32);
   return print_insn_cris_generic (vma, info, true);
 }
 
@@ -2761,9 +2756,10 @@ static int
 print_insn_crisv10_v32_with_register_prefix (bfd_vma vma,
 					     disassemble_info *info)
 {
-  if (info->private_data == NULL
-      && !cris_parse_disassembler_options (info, cris_dis_common_v10_v32))
-    return -1;
+  struct cris_disasm_data disdata;
+  info->private_data = &disdata;
+  cris_parse_disassembler_options (&disdata, info->disassembler_options,
+				   cris_dis_common_v10_v32);
   return print_insn_cris_generic (vma, info, true);
 }
 
@@ -2773,9 +2769,10 @@ static int
 print_insn_cris_without_register_prefix (bfd_vma vma,
 					 disassemble_info *info)
 {
-  if (info->private_data == NULL
-      && !cris_parse_disassembler_options (info, cris_dis_v0_v10))
-    return -1;
+  struct cris_disasm_data disdata;
+  info->private_data = &disdata;
+  cris_parse_disassembler_options (&disdata, info->disassembler_options,
+				   cris_dis_v0_v10);
   return print_insn_cris_generic (vma, info, false);
 }
 
@@ -2785,9 +2782,10 @@ static int
 print_insn_crisv32_without_register_prefix (bfd_vma vma,
 					    disassemble_info *info)
 {
-  if (info->private_data == NULL
-      && !cris_parse_disassembler_options (info, cris_dis_v32))
-    return -1;
+  struct cris_disasm_data disdata;
+  info->private_data = &disdata;
+  cris_parse_disassembler_options (&disdata, info->disassembler_options,
+				   cris_dis_v32);
   return print_insn_cris_generic (vma, info, false);
 }
 
@@ -2798,9 +2796,10 @@ static int
 print_insn_crisv10_v32_without_register_prefix (bfd_vma vma,
 						disassemble_info *info)
 {
-  if (info->private_data == NULL
-      && !cris_parse_disassembler_options (info, cris_dis_common_v10_v32))
-    return -1;
+  struct cris_disasm_data disdata;
+  info->private_data = &disdata;
+  cris_parse_disassembler_options (&disdata, info->disassembler_options,
+				   cris_dis_common_v10_v32);
   return print_insn_cris_generic (vma, info, false);
 }
 #endif
-- 
1.8.3.1

[PATCH 7/8] lm32: do not leak memory on object_new/object_unref

[Paolo Bonzini]

Bottom halves and ptimers are malloced, but nothing in these
files is freeing memory allocated by instance_init.  Since
these are sysctl devices that are never unrealized, just moving
the allocations to realize is enough to avoid the leak in
practice (and also to avoid upsetting asan when running
device-introspect-test).

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/timer/lm32_timer.c       |  6 +++---
 hw/timer/milkymist-sysctl.c | 10 +++++-----
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/hw/timer/lm32_timer.c b/hw/timer/lm32_timer.c
index ac3edaf..cf316ed 100644
--- a/hw/timer/lm32_timer.c
+++ b/hw/timer/lm32_timer.c
@@ -186,9 +186,6 @@ static void lm32_timer_init(Object *obj)
 
     sysbus_init_irq(dev, &s->irq);
 
-    s->bh = qemu_bh_new(timer_hit, s);
-    s->ptimer = ptimer_init(s->bh, PTIMER_POLICY_DEFAULT);
-
     memory_region_init_io(&s->iomem, obj, &timer_ops, s,
                           "timer", R_MAX * 4);
     sysbus_init_mmio(dev, &s->iomem);
@@ -198,6 +195,9 @@ static void lm32_timer_realize(DeviceState *dev, Error **errp)
 {
     LM32TimerState *s = LM32_TIMER(dev);
 
+    s->bh = qemu_bh_new(timer_hit, s);
+    s->ptimer = ptimer_init(s->bh, PTIMER_POLICY_DEFAULT);
+
     ptimer_set_freq(s->ptimer, s->freq_hz);
 }
 
diff --git a/hw/timer/milkymist-sysctl.c b/hw/timer/milkymist-sysctl.c
index 9583507..6aedc11 100644
--- a/hw/timer/milkymist-sysctl.c
+++ b/hw/timer/milkymist-sysctl.c
@@ -283,11 +283,6 @@ static void milkymist_sysctl_init(Object *obj)
     sysbus_init_irq(dev, &s->timer0_irq);
     sysbus_init_irq(dev, &s->timer1_irq);
 
-    s->bh0 = qemu_bh_new(timer0_hit, s);
-    s->bh1 = qemu_bh_new(timer1_hit, s);
-    s->ptimer0 = ptimer_init(s->bh0, PTIMER_POLICY_DEFAULT);
-    s->ptimer1 = ptimer_init(s->bh1, PTIMER_POLICY_DEFAULT);
-
     memory_region_init_io(&s->regs_region, obj, &sysctl_mmio_ops, s,
             "milkymist-sysctl", R_MAX * 4);
     sysbus_init_mmio(dev, &s->regs_region);
@@ -297,6 +292,11 @@ static void milkymist_sysctl_realize(DeviceState *dev, Error **errp)
 {
     MilkymistSysctlState *s = MILKYMIST_SYSCTL(dev);
 
+    s->bh0 = qemu_bh_new(timer0_hit, s);
+    s->bh1 = qemu_bh_new(timer1_hit, s);
+    s->ptimer0 = ptimer_init(s->bh0, PTIMER_POLICY_DEFAULT);
+    s->ptimer1 = ptimer_init(s->bh1, PTIMER_POLICY_DEFAULT);
+
     ptimer_set_freq(s->ptimer0, s->freq_hz);
     ptimer_set_freq(s->ptimer1, s->freq_hz);
 }
-- 
1.8.3.1

[PATCH 8/8] docker: test-debug: disable LeakSanitizer

[Paolo Bonzini]

There are just too many leaks in device-introspect-test (especially for
the plethora of arm and aarch64 boards) to make LeakSanitizer useful;
disable it for now.

Whoever is interested in debugging leaks can also use valgrind like this:

   QTEST_QEMU_BINARY=aarch64-softmmu/qemu-system-aarch64 \
   QTEST_QEMU_IMG=qemu-img \
   valgrind --trace-children=yes --leak-check=full \
   tests/device-introspect-test -p /aarch64/device/introspect/concrete/defaults/none

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 tests/docker/test-debug | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tests/docker/test-debug b/tests/docker/test-debug
index 137f4f2..c050fa0 100755
--- a/tests/docker/test-debug
+++ b/tests/docker/test-debug
@@ -21,6 +21,7 @@ cd "$BUILD_DIR"
 OPTS="--cxx=clang++ --cc=clang --host-cc=clang"
 OPTS="--enable-debug --enable-sanitizers $OPTS"
 
+export ASAN_OPTIONS=detect_leaks=0
 build_qemu $OPTS
 check_qemu check V=1
 install_qemu
-- 
1.8.3.1

Re: [PATCH 8/8] docker: test-debug: disable LeakSanitizer

[Philippe Mathieu-Daudé]

On 10/1/19 3:36 PM, Paolo Bonzini wrote:
> There are just too many leaks in device-introspect-test (especially for
> the plethora of arm and aarch64 boards) to make LeakSanitizer useful;
> disable it for now.
> 
> Whoever is interested in debugging leaks can also use valgrind like this:
> 
>     QTEST_QEMU_BINARY=aarch64-softmmu/qemu-system-aarch64 \
>     QTEST_QEMU_IMG=qemu-img \
>     valgrind --trace-children=yes --leak-check=full \
>     tests/device-introspect-test -p /aarch64/device/introspect/concrete/defaults/none
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   tests/docker/test-debug | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/tests/docker/test-debug b/tests/docker/test-debug
> index 137f4f2..c050fa0 100755
> --- a/tests/docker/test-debug
> +++ b/tests/docker/test-debug
> @@ -21,6 +21,7 @@ cd "$BUILD_DIR"
>   OPTS="--cxx=clang++ --cc=clang --host-cc=clang"
>   OPTS="--enable-debug --enable-sanitizers $OPTS"
>   
> +export ASAN_OPTIONS=detect_leaks=0
>   build_qemu $OPTS
>   check_qemu check V=1
>   install_qemu
> 

Can we reduce it only for the arm/aarch64 targets?

Re: [PATCH 4/8] hppa: fix leak from g_strdup_printf

[Philippe Mathieu-Daudé]

On 10/1/19 3:36 PM, Paolo Bonzini wrote:
> memory_region_init_* takes care of copying the name into memory it owns.
> Free it in the caller.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   hw/hppa/dino.c    | 1 +
>   hw/hppa/machine.c | 4 +++-
>   2 files changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/hppa/dino.c b/hw/hppa/dino.c
> index e0466ee..ab6969b 100644
> --- a/hw/hppa/dino.c
> +++ b/hw/hppa/dino.c
> @@ -485,6 +485,7 @@ PCIBus *dino_init(MemoryRegion *addr_space,
>           memory_region_init_alias(&s->pci_mem_alias[i], OBJECT(s),
>                                    name, &s->pci_mem, addr,
>                                    DINO_MEM_CHUNK_SIZE);
> +        g_free(name);
>       }
>   
>       /* Set up PCI view of memory: Bus master address space.  */
> diff --git a/hw/hppa/machine.c b/hw/hppa/machine.c
> index 2736ce8..7e23675 100644
> --- a/hw/hppa/machine.c
> +++ b/hw/hppa/machine.c
> @@ -78,13 +78,15 @@ static void machine_hppa_init(MachineState *machine)
>   
>       /* Create CPUs.  */
>       for (i = 0; i < smp_cpus; i++) {
> +        char *name = g_strdup_printf("cpu%ld-io-eir", i);
>           cpu[i] = HPPA_CPU(cpu_create(machine->cpu_type));
>   
>           cpu_region = g_new(MemoryRegion, 1);
>           memory_region_init_io(cpu_region, OBJECT(cpu[i]), &hppa_io_eir_ops,
> -                              cpu[i], g_strdup_printf("cpu%ld-io-eir", i), 4);
> +                              cpu[i], name, 4);
>           memory_region_add_subregion(addr_space, CPU_HPA + i * 0x1000,
>                                       cpu_region);
> +        g_free(name);
>       }
>   
>       /* Limit main memory. */
> 

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Re: [PATCH 2/8] microblaze: fix leak of fdevice tree blob

[Philippe Mathieu-Daudé]

On 10/1/19 3:36 PM, Paolo Bonzini wrote:
> The device tree blob returned by load_device_tree is malloced.
> Free it before returning.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   hw/microblaze/boot.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/hw/microblaze/boot.c b/hw/microblaze/boot.c
> index bade4d2..d1d7dfb 100644
> --- a/hw/microblaze/boot.c
> +++ b/hw/microblaze/boot.c
> @@ -100,6 +100,7 @@ static int microblaze_load_dtb(hwaddr addr,
>       }
>   
>       cpu_physical_memory_write(addr, fdt, fdt_size);
> +    g_free(fdt);
>       return fdt_size;
>   }
>   
> 

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Re: [PATCH 7/8] lm32: do not leak memory on object_new/object_unref

[Philippe Mathieu-Daudé]

On 10/1/19 3:36 PM, Paolo Bonzini wrote:
> Bottom halves and ptimers are malloced, but nothing in these
> files is freeing memory allocated by instance_init.  Since
> these are sysctl devices that are never unrealized, just moving
> the allocations to realize is enough to avoid the leak in
> practice (and also to avoid upsetting asan when running
> device-introspect-test).
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   hw/timer/lm32_timer.c       |  6 +++---
>   hw/timer/milkymist-sysctl.c | 10 +++++-----
>   2 files changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/timer/lm32_timer.c b/hw/timer/lm32_timer.c
> index ac3edaf..cf316ed 100644
> --- a/hw/timer/lm32_timer.c
> +++ b/hw/timer/lm32_timer.c
> @@ -186,9 +186,6 @@ static void lm32_timer_init(Object *obj)
>   
>       sysbus_init_irq(dev, &s->irq);
>   
> -    s->bh = qemu_bh_new(timer_hit, s);
> -    s->ptimer = ptimer_init(s->bh, PTIMER_POLICY_DEFAULT);
> -
>       memory_region_init_io(&s->iomem, obj, &timer_ops, s,
>                             "timer", R_MAX * 4);
>       sysbus_init_mmio(dev, &s->iomem);
> @@ -198,6 +195,9 @@ static void lm32_timer_realize(DeviceState *dev, Error **errp)
>   {
>       LM32TimerState *s = LM32_TIMER(dev);
>   
> +    s->bh = qemu_bh_new(timer_hit, s);
> +    s->ptimer = ptimer_init(s->bh, PTIMER_POLICY_DEFAULT);
> +
>       ptimer_set_freq(s->ptimer, s->freq_hz);
>   }
>   
> diff --git a/hw/timer/milkymist-sysctl.c b/hw/timer/milkymist-sysctl.c
> index 9583507..6aedc11 100644
> --- a/hw/timer/milkymist-sysctl.c
> +++ b/hw/timer/milkymist-sysctl.c
> @@ -283,11 +283,6 @@ static void milkymist_sysctl_init(Object *obj)
>       sysbus_init_irq(dev, &s->timer0_irq);
>       sysbus_init_irq(dev, &s->timer1_irq);
>   
> -    s->bh0 = qemu_bh_new(timer0_hit, s);
> -    s->bh1 = qemu_bh_new(timer1_hit, s);
> -    s->ptimer0 = ptimer_init(s->bh0, PTIMER_POLICY_DEFAULT);
> -    s->ptimer1 = ptimer_init(s->bh1, PTIMER_POLICY_DEFAULT);
> -
>       memory_region_init_io(&s->regs_region, obj, &sysctl_mmio_ops, s,
>               "milkymist-sysctl", R_MAX * 4);
>       sysbus_init_mmio(dev, &s->regs_region);
> @@ -297,6 +292,11 @@ static void milkymist_sysctl_realize(DeviceState *dev, Error **errp)
>   {
>       MilkymistSysctlState *s = MILKYMIST_SYSCTL(dev);
>   
> +    s->bh0 = qemu_bh_new(timer0_hit, s);
> +    s->bh1 = qemu_bh_new(timer1_hit, s);
> +    s->ptimer0 = ptimer_init(s->bh0, PTIMER_POLICY_DEFAULT);
> +    s->ptimer1 = ptimer_init(s->bh1, PTIMER_POLICY_DEFAULT);
> +
>       ptimer_set_freq(s->ptimer0, s->freq_hz);
>       ptimer_set_freq(s->ptimer1, s->freq_hz);
>   }
> 

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Re: [PATCH 1/8] ide: fix leak from qemu_allocate_irqs

[Thomas Huth]

On 01/10/2019 15.36, Paolo Bonzini wrote:
> The array returned by qemu_allocate_irqs is malloced, free it.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/ide/cmd646.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/hw/ide/cmd646.c b/hw/ide/cmd646.c
> index f3ccd11..19984d2 100644
> --- a/hw/ide/cmd646.c
> +++ b/hw/ide/cmd646.c
> @@ -300,6 +300,7 @@ static void pci_cmd646_ide_realize(PCIDevice *dev, Error **errp)
>          d->bmdma[i].bus = &d->bus[i];
>          ide_register_restart_cb(&d->bus[i]);
>      }
> +    g_free(irq);
>  
>      vmstate_register(DEVICE(dev), 0, &vmstate_ide_pci, d);
>      qemu_register_reset(cmd646_reset, d);
> 

Maybe you could also update the description of qemu_allocate_irqs() to
state that the returned pointer should be g_free'd later?

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH 3/8] mcf5208: fix leak from qemu_allocate_irqs

[Thomas Huth]

On 01/10/2019 15.36, Paolo Bonzini wrote:
> The array returned by qemu_allocate_irqs is malloced, free it.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/m68k/mcf5208.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/hw/m68k/mcf5208.c b/hw/m68k/mcf5208.c
> index 012710d..60c5802 100644
> --- a/hw/m68k/mcf5208.c
> +++ b/hw/m68k/mcf5208.c
> @@ -273,6 +273,8 @@ static void mcf5208evb_init(MachineState *machine)
>                       0xfc030000, pic + 36);
>      }
>  
> +    g_free(pic);
> +
>      /*  0xfc000000 SCM.  */
>      /*  0xfc004000 XBS.  */
>      /*  0xfc008000 FlexBus CS.  */
> 

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH 5/8] mips: fix memory leaks in board initialization

[Thomas Huth]

On 01/10/2019 15.36, Paolo Bonzini wrote:
> They are not a big deal, but they upset asan.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/mips/mips_int.c  | 1 +
>  hw/mips/mips_jazz.c | 2 ++
>  2 files changed, 3 insertions(+)
> 
> diff --git a/hw/mips/mips_int.c b/hw/mips/mips_int.c
> index 5ebc961..863ed45 100644
> --- a/hw/mips/mips_int.c
> +++ b/hw/mips/mips_int.c
> @@ -81,6 +81,7 @@ void cpu_mips_irq_init_cpu(MIPSCPU *cpu)
>      for (i = 0; i < 8; i++) {
>          env->irq[i] = qi[i];
>      }
> +    g_free(qi);
>  }
>  
>  void cpu_mips_soft_irq(CPUMIPSState *env, int irq, int level)
> diff --git a/hw/mips/mips_jazz.c b/hw/mips/mips_jazz.c
> index c967b97..8d010a0 100644
> --- a/hw/mips/mips_jazz.c
> +++ b/hw/mips/mips_jazz.c
> @@ -362,6 +362,8 @@ static void mips_jazz_init(MachineState *machine,
>  
>      /* LED indicator */
>      sysbus_create_simple("jazz-led", 0x8000f000, NULL);
> +
> +    g_free(dmas);
>  }
>  
>  static
> 

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH 1/8] ide: fix leak from qemu_allocate_irqs

[Eric Blake]

On 10/1/19 8:36 AM, Paolo Bonzini wrote:
> The array returned by qemu_allocate_irqs is malloced, free it.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   hw/ide/cmd646.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/hw/ide/cmd646.c b/hw/ide/cmd646.c
> index f3ccd11..19984d2 100644
> --- a/hw/ide/cmd646.c
> +++ b/hw/ide/cmd646.c
> @@ -300,6 +300,7 @@ static void pci_cmd646_ide_realize(PCIDevice *dev, Error **errp)
>           d->bmdma[i].bus = &d->bus[i];
>           ide_register_restart_cb(&d->bus[i]);
>       }
> +    g_free(irq);

How many of these patches could be fixed by using g_autofree or similar 
instead?

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH 1/8] ide: fix leak from qemu_allocate_irqs

[Peter Maydell]

On Tue, 1 Oct 2019 at 14:38, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> The array returned by qemu_allocate_irqs is malloced, free it.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/ide/cmd646.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/hw/ide/cmd646.c b/hw/ide/cmd646.c
> index f3ccd11..19984d2 100644
> --- a/hw/ide/cmd646.c
> +++ b/hw/ide/cmd646.c
> @@ -300,6 +300,7 @@ static void pci_cmd646_ide_realize(PCIDevice *dev, Error **errp)
>          d->bmdma[i].bus = &d->bus[i];
>          ide_register_restart_cb(&d->bus[i]);
>      }
> +    g_free(irq);
>
>      vmstate_register(DEVICE(dev), 0, &vmstate_ide_pci, d);
>      qemu_register_reset(cmd646_reset, d);
> --
> 1.8.3.1

It's a bit weird to be calling qemu_allocate_irqs() here in the
first place -- usually you'd just have a qemu_irq irqs[2] array
in the device state struct, qemu_allocate_irq(irq[i], ...) and
pass irq[i] to the ide_init2() function to tell it what
qemu_irq to use. Or else you'd keep the pointer to the
allocated irqs array in the device state struct, so as
to be able to free it on any theoretical hot-unplug your
device might support. Calling qemu_allocate_irqs() and then
immediately freeing the array means that there are now
two actual qemu_irqs floating around which are supposed
to be owned by this device but which it has no way to
get hold of any more. This is only not a leak because
the cmd646 doesn't support hot-unplug.

(Hot take version : we should be getting rid of qemu_allocate_irqs()
entirely: it is essentially a "pre-QOM" API and there are better
ways to phrase code that's currently calling it.)

thanks
-- PMM

Re: [PATCH 1/8] ide: fix leak from qemu_allocate_irqs

[Paolo Bonzini]

On 01/10/19 17:58, Peter Maydell wrote:
> On Tue, 1 Oct 2019 at 14:38, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>
>> The array returned by qemu_allocate_irqs is malloced, free it.
>>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>>  hw/ide/cmd646.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/hw/ide/cmd646.c b/hw/ide/cmd646.c
>> index f3ccd11..19984d2 100644
>> --- a/hw/ide/cmd646.c
>> +++ b/hw/ide/cmd646.c
>> @@ -300,6 +300,7 @@ static void pci_cmd646_ide_realize(PCIDevice *dev, Error **errp)
>>          d->bmdma[i].bus = &d->bus[i];
>>          ide_register_restart_cb(&d->bus[i]);
>>      }
>> +    g_free(irq);
>>
>>      vmstate_register(DEVICE(dev), 0, &vmstate_ide_pci, d);
>>      qemu_register_reset(cmd646_reset, d);
>> --
>> 1.8.3.1
> 
> It's a bit weird to be calling qemu_allocate_irqs() here in the
> first place -- usually you'd just have a qemu_irq irqs[2] array
> in the device state struct, qemu_allocate_irq(irq[i], ...) and
> pass irq[i] to the ide_init2() function to tell it what
> qemu_irq to use. Or else you'd keep the pointer to the
> allocated irqs array in the device state struct, so as
> to be able to free it on any theoretical hot-unplug your
> device might support. Calling qemu_allocate_irqs() and then
> immediately freeing the array means that there are now
> two actual qemu_irqs floating around which are supposed
> to be owned by this device but which it has no way to
> get hold of any more. This is only not a leak because
> the cmd646 doesn't support hot-unplug.
> 
> (Hot take version : we should be getting rid of qemu_allocate_irqs()
> entirely: it is essentially a "pre-QOM" API and there are better
> ways to phrase code that's currently calling it.)

Yes, I agree, and it's why I didn't mind doing the quick fix that gets
rid of the boot-serial-test leak the easy way.

Paolo

Re: [PATCH 8/8] docker: test-debug: disable LeakSanitizer

[Paolo Bonzini]

On 01/10/19 15:59, Philippe Mathieu-Daudé wrote:
> On 10/1/19 3:36 PM, Paolo Bonzini wrote:
>> There are just too many leaks in device-introspect-test (especially for
>> the plethora of arm and aarch64 boards) to make LeakSanitizer useful;
>> disable it for now.
>>
>> Whoever is interested in debugging leaks can also use valgrind like this:
>>
>>     QTEST_QEMU_BINARY=aarch64-softmmu/qemu-system-aarch64 \
>>     QTEST_QEMU_IMG=qemu-img \
>>     valgrind --trace-children=yes --leak-check=full \
>>     tests/device-introspect-test -p
>> /aarch64/device/introspect/concrete/defaults/none
>>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>>   tests/docker/test-debug | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/tests/docker/test-debug b/tests/docker/test-debug
>> index 137f4f2..c050fa0 100755
>> --- a/tests/docker/test-debug
>> +++ b/tests/docker/test-debug
>> @@ -21,6 +21,7 @@ cd "$BUILD_DIR"
>>   OPTS="--cxx=clang++ --cc=clang --host-cc=clang"
>>   OPTS="--enable-debug --enable-sanitizers $OPTS"
>>   +export ASAN_OPTIONS=detect_leaks=0
>>   build_qemu $OPTS
>>   check_qemu check V=1
>>   install_qemu
> 
> Can we reduce it only for the arm/aarch64 targets?

Not easily, since check_qemu runs tests for all targets.  It's not
possible AFAICT to set ASAN_OPTIONS from inside the test, for example.

Paolo

Re: [PATCH ci-fix 0/8] fix various memory leaks (but not all)

[no-reply]

Patchew URL: https://patchew.org/QEMU/1569936988-635-1-git-send-email-pbonzini@redhat.com/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 1569936988-635-1-git-send-email-pbonzini@redhat.com
Subject: [PATCH ci-fix 0/8] fix various memory leaks (but not all)

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
82a01e4 docker: test-debug: disable LeakSanitizer
a641027 lm32: do not leak memory on object_new/object_unref
4371ce1 cris: do not leak struct cris_disasm_data
4528288 mips: fix memory leaks in board initialization
e06bae0 hppa: fix leak from g_strdup_printf
becd3c3 mcf5208: fix leak from qemu_allocate_irqs
8b51d7a microblaze: fix leak of fdevice tree blob
02eaa60 ide: fix leak from qemu_allocate_irqs

=== OUTPUT BEGIN ===
1/8 Checking commit 02eaa60b9e2a (ide: fix leak from qemu_allocate_irqs)
2/8 Checking commit 8b51d7aff31f (microblaze: fix leak of fdevice tree blob)
3/8 Checking commit becd3c365fdf (mcf5208: fix leak from qemu_allocate_irqs)
4/8 Checking commit e06bae052258 (hppa: fix leak from g_strdup_printf)
5/8 Checking commit 4528288f43ef (mips: fix memory leaks in board initialization)
6/8 Checking commit 4371ce1f2f81 (cris: do not leak struct cris_disasm_data)
ERROR: space prohibited between function name and open parenthesis '('
#23: FILE: disas/cris.c:1298:
+cris_parse_disassembler_options (struct cris_disasm_data *disdata,

ERROR: code indent should never use tabs
#24: FILE: disas/cris.c:1299:
+^I^I^I^I char *disassembler_options,$

ERROR: space prohibited between function name and open parenthesis '('
#39: FILE: disas/cris.c:1305:
+       || (strcmp (disassembler_options, "nocase") != 0));

ERROR: space prohibited between function name and open parenthesis '('
#55: FILE: disas/cris.c:2734:
+  cris_parse_disassembler_options (&disdata, info->disassembler_options,

ERROR: code indent should never use tabs
#56: FILE: disas/cris.c:2735:
+^I^I^I^I   cris_dis_v0_v10);$

ERROR: space prohibited between function name and open parenthesis '('
#69: FILE: disas/cris.c:2746:
+  cris_parse_disassembler_options (&disdata, info->disassembler_options,

ERROR: code indent should never use tabs
#70: FILE: disas/cris.c:2747:
+^I^I^I^I   cris_dis_v32);$

ERROR: space prohibited between function name and open parenthesis '('
#83: FILE: disas/cris.c:2761:
+  cris_parse_disassembler_options (&disdata, info->disassembler_options,

ERROR: code indent should never use tabs
#84: FILE: disas/cris.c:2762:
+^I^I^I^I   cris_dis_common_v10_v32);$

ERROR: space prohibited between function name and open parenthesis '('
#97: FILE: disas/cris.c:2774:
+  cris_parse_disassembler_options (&disdata, info->disassembler_options,

ERROR: code indent should never use tabs
#98: FILE: disas/cris.c:2775:
+^I^I^I^I   cris_dis_v0_v10);$

ERROR: space prohibited between function name and open parenthesis '('
#111: FILE: disas/cris.c:2787:
+  cris_parse_disassembler_options (&disdata, info->disassembler_options,

ERROR: code indent should never use tabs
#112: FILE: disas/cris.c:2788:
+^I^I^I^I   cris_dis_v32);$

ERROR: space prohibited between function name and open parenthesis '('
#125: FILE: disas/cris.c:2801:
+  cris_parse_disassembler_options (&disdata, info->disassembler_options,

ERROR: code indent should never use tabs
#126: FILE: disas/cris.c:2802:
+^I^I^I^I   cris_dis_common_v10_v32);$

total: 15 errors, 0 warnings, 107 lines checked

Patch 6/8 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

7/8 Checking commit a64102750d5f (lm32: do not leak memory on object_new/object_unref)
8/8 Checking commit 82a01e41c267 (docker: test-debug: disable LeakSanitizer)
=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/1569936988-635-1-git-send-email-pbonzini@redhat.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com