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];
     }
-- 
2.19.1

Keqian Zhu <zhukeqian1@huawei.com> 于2020年7月27日周一 下午9:03写道：
>
> Avoid covering object refcount of qemu_irq, otherwise it may causes
> memory leak.

Any reproducer?

Thanks,
Li Qiang

>
> 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];
>      }
> --
> 2.19.1
>

Hi Qiang,

On 2020/7/27 22:37, Li Qiang wrote:
> Keqian Zhu <zhukeqian1@huawei.com> 于2020年7月27日周一 下午9:03写道：
>>
>> Avoid covering object refcount of qemu_irq, otherwise it may causes
>> memory leak.
> 
> Any reproducer?
> 
In mainline Qemu. this function is only used in qtest. One of our internal
self-developed module also use this function. The memory leak is reported
by ASAN.

Thanks,
Keqian

> Thanks,
> Li Qiang
> 
>>
>> 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];
>>      }
>> --
>> 2.19.1
>>
> .
> 

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.

Is there a specific bug you're trying to fix here?

thanks
-- PMM

Hi Peter,

On 2020/7/27 22: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.
One of our internal self-developed module also use this function, and we
implemented a function to remove intercepting, so there is no memory leak
after this bugfix.

I suggest to merge this bugfix to prepare for future code which may invoke
this function.

> 
> Is there a specific bug you're trying to fix here?
The memory leak is reported by ASAN.
> 

Thanks,
Keqian
> thanks
> -- PMM
> .
> 

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?

 Thomas

Hi Thomas,

On 2020/7/28 16:48, Thomas Huth 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?
Yeah, this can save some memory. But I think it does not solve the refcount covering
problem.
> 
Thanks
Keqian
>  Thomas
> 
> 
> 

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