[PATCH] hw/m68k/mcf5206: Don't leak IRQs in mcf5206_mbar_realize()

[PATCH] hw/m68k/mcf5206: Don't leak IRQs in mcf5206_mbar_realize()

[Peter Maydell]

Coverity points out that the realize function for the TYPE_MCF5206_MBAR
device leaks the IRQ array it allocates with qemu_allocate_irqs().
Keep a pointer to it in the device state struct to avoid the leak.
(Since it needs to stay around for the life of the simulation there
is no need to actually free it, and the leak was harmless.)

Fixes: Coverity CID 1432412
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---

We don't need to put this in 5.2, I'm just slowly working through
some of the Coverity issues that are various kinds of harmless leak
involving qemu_allocate_irqs().

A more thorough overhaul of this code would probably split the
current MBAR device into multiple individual devices:
 * timers
 * uarts
 * interrupt controller proper
 * a container device that wires up all the above
In that design instead of using qemu_allocate_irqs(), the
interrupt-controller device would create gpio_in lines and the
container would wire them up to the timers and uarts.
---
 hw/m68k/mcf5206.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/hw/m68k/mcf5206.c b/hw/m68k/mcf5206.c
index 51d2e0da1c9..92a194dbc46 100644
--- a/hw/m68k/mcf5206.c
+++ b/hw/m68k/mcf5206.c
@@ -164,6 +164,7 @@ typedef struct {
 
     M68kCPU *cpu;
     MemoryRegion iomem;
+    qemu_irq *pic;
     m5206_timer_state *timer[2];
     void *uart[2];
     uint8_t scr;
@@ -588,17 +589,16 @@ static const MemoryRegionOps m5206_mbar_ops = {
 static void mcf5206_mbar_realize(DeviceState *dev, Error **errp)
 {
     m5206_mbar_state *s = MCF5206_MBAR(dev);
-    qemu_irq *pic;
 
     memory_region_init_io(&s->iomem, NULL, &m5206_mbar_ops, s,
                           "mbar", 0x00001000);
     sysbus_init_mmio(SYS_BUS_DEVICE(dev), &s->iomem);
 
-    pic = qemu_allocate_irqs(m5206_mbar_set_irq, s, 14);
-    s->timer[0] = m5206_timer_init(pic[9]);
-    s->timer[1] = m5206_timer_init(pic[10]);
-    s->uart[0] = mcf_uart_init(pic[12], serial_hd(0));
-    s->uart[1] = mcf_uart_init(pic[13], serial_hd(1));
+    s->pic = qemu_allocate_irqs(m5206_mbar_set_irq, s, 14);
+    s->timer[0] = m5206_timer_init(s->pic[9]);
+    s->timer[1] = m5206_timer_init(s->pic[10]);
+    s->uart[0] = mcf_uart_init(s->pic[12], serial_hd(0));
+    s->uart[1] = mcf_uart_init(s->pic[13], serial_hd(1));
     s->cpu = M68K_CPU(qemu_get_cpu(0));
 }
 
-- 
2.20.1

Re: [PATCH] hw/m68k/mcf5206: Don't leak IRQs in mcf5206_mbar_realize()

[Philippe Mathieu-Daudé]

On 11/20/20 6:23 PM, Peter Maydell wrote:
> Coverity points out that the realize function for the TYPE_MCF5206_MBAR
> device leaks the IRQ array it allocates with qemu_allocate_irqs().
> Keep a pointer to it in the device state struct to avoid the leak.
> (Since it needs to stay around for the life of the simulation there
> is no need to actually free it, and the leak was harmless.)
> 
> Fixes: Coverity CID 1432412
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> 
> We don't need to put this in 5.2, I'm just slowly working through
> some of the Coverity issues that are various kinds of harmless leak
> involving qemu_allocate_irqs().

As you said, harmless, so this could go for 5.2 IMO.

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> 
> A more thorough overhaul of this code would probably split the
> current MBAR device into multiple individual devices:
>  * timers
>  * uarts
>  * interrupt controller proper
>  * a container device that wires up all the above
> In that design instead of using qemu_allocate_irqs(), the
> interrupt-controller device would create gpio_in lines and the
> container would wire them up to the timers and uarts.

Agreed, but this only makes sense if there is activity in this
model, else your patch is sufficient (else, what about the
OMAP1/2?).

> ---
>  hw/m68k/mcf5206.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/m68k/mcf5206.c b/hw/m68k/mcf5206.c
> index 51d2e0da1c9..92a194dbc46 100644
> --- a/hw/m68k/mcf5206.c
> +++ b/hw/m68k/mcf5206.c
> @@ -164,6 +164,7 @@ typedef struct {
>  
>      M68kCPU *cpu;
>      MemoryRegion iomem;
> +    qemu_irq *pic;
>      m5206_timer_state *timer[2];
>      void *uart[2];
>      uint8_t scr;
> @@ -588,17 +589,16 @@ static const MemoryRegionOps m5206_mbar_ops = {
>  static void mcf5206_mbar_realize(DeviceState *dev, Error **errp)
>  {
>      m5206_mbar_state *s = MCF5206_MBAR(dev);
> -    qemu_irq *pic;
>  
>      memory_region_init_io(&s->iomem, NULL, &m5206_mbar_ops, s,
>                            "mbar", 0x00001000);
>      sysbus_init_mmio(SYS_BUS_DEVICE(dev), &s->iomem);
>  
> -    pic = qemu_allocate_irqs(m5206_mbar_set_irq, s, 14);
> -    s->timer[0] = m5206_timer_init(pic[9]);
> -    s->timer[1] = m5206_timer_init(pic[10]);
> -    s->uart[0] = mcf_uart_init(pic[12], serial_hd(0));
> -    s->uart[1] = mcf_uart_init(pic[13], serial_hd(1));
> +    s->pic = qemu_allocate_irqs(m5206_mbar_set_irq, s, 14);
> +    s->timer[0] = m5206_timer_init(s->pic[9]);
> +    s->timer[1] = m5206_timer_init(s->pic[10]);
> +    s->uart[0] = mcf_uart_init(s->pic[12], serial_hd(0));
> +    s->uart[1] = mcf_uart_init(s->pic[13], serial_hd(1));
>      s->cpu = M68K_CPU(qemu_get_cpu(0));
>  }
>  
> 

Re: [PATCH] hw/m68k/mcf5206: Don't leak IRQs in mcf5206_mbar_realize()

[Thomas Huth]

Am Fri, 20 Nov 2020 17:23:14 +0000
schrieb Peter Maydell <peter.maydell@linaro.org>:

> Coverity points out that the realize function for the
> TYPE_MCF5206_MBAR device leaks the IRQ array it allocates with
> qemu_allocate_irqs(). Keep a pointer to it in the device state struct
> to avoid the leak. (Since it needs to stay around for the life of the
> simulation there is no need to actually free it, and the leak was
> harmless.)
> 
> Fixes: Coverity CID 1432412
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Thomas Huth <huth@tuxfamily.org>