Re: [PATCH] bugfix: irq: Avoid covering object refcount of qemu_irq

[Peter Maydell]

On Tue, 28 Jul 2020 at 09:48, Thomas Huth <thuth@redhat.com> wrote:
>
> On 27/07/2020 16.41, Peter Maydell wrote:
> > On Mon, 27 Jul 2020 at 14:03, Keqian Zhu <zhukeqian1@huawei.com> wrote:
> >>
> >> Avoid covering object refcount of qemu_irq, otherwise it may causes
> >> memory leak.
> >>
> >> Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com>
> >> ---
> >>  hw/core/irq.c | 4 +++-
> >>  1 file changed, 3 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/hw/core/irq.c b/hw/core/irq.c
> >> index fb3045b912..59af4dfc74 100644
> >> --- a/hw/core/irq.c
> >> +++ b/hw/core/irq.c
> >> @@ -125,7 +125,9 @@ void qemu_irq_intercept_in(qemu_irq *gpio_in, 
> >> qemu_irq_handler handler, int n)
> >>      int i;
> >>      qemu_irq *old_irqs = qemu_allocate_irqs(NULL, NULL, n);
> >>      for (i = 0; i < n; i++) {
> >> -        *old_irqs[i] = *gpio_in[i];
> >> +        old_irqs[i]->handler = gpio_in[i]->handler;
> >> +        old_irqs[i]->opaque = gpio_in[i]->opaque;
> >> +
> >>          gpio_in[i]->handler = handler;
> >>          gpio_in[i]->opaque = &old_irqs[i];
> >>      }
> >
> > This function is leaky by design, because it doesn't do anything
> > with the old_irqs array and there's no function for un-intercepting
> > the IRQs (which would need to free that memory). This is not ideal
> > but OK because it's only used in the test suite.
>
> I think this could better be done without calling qemu_allocate_irqs():
> Simply call qemu_allocate_irq() (without "s" at the end) within the
> for-loop for each irq instead. What do you think?

Well, what are we trying to do with the function? I think that
your suggestion still doesn't really fix the leak in the sense
that there's no mechanism for undoing the operation and freeing
the memory allocated by qemu_allocate_irq(). The whole concept
of the function is pretty dubious because it's arbitrarily
re-plugging IRQs, which in the whole of the rest of QEMU are
assumed to be something that's wired up by board code or by
an SoC device that "owns" the two devices it's connecting.
That's fine for test-case purposes, and for the qtest code that
is the only in-tree user of this function we could construct a
(nominally) leak-free version of the function by having it return
some kind of handle that could be passed to a
qemu_irq_remove_intercept_in() to clean up [this would fix a
Coverity nag, which would be the main benefit...].

I agree that copying the refcount here looks wrong, but in
what situation is the refcount on any of the qemu_irqs
involved changing anyway?

For non-testing uses of this function it gets trickier (what if the
device on one end or the other is destroyed while the intercept is
in place, for instance?) So I'm a bit sceptical of ideas about
extending the uses of this function to non-test-code, and I think
I'd want to see what the intended non-test uses were...

thanks
-- PMM