[PATCH] hw/ppc/pegasos: Fix memory leak

[PATCH] hw/ppc/pegasos: Fix memory leak

[BALATON Zoltan]

Commit 9099b430a4 introduced an early return that caused a leak of a
GString. Allocate it later to avoid the leak.

Fixes: 9099b430a4 (hw/ppc/pegasos2: Change device tree generation)
Resolves: Coverity CID 1642027
Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/ppc/pegasos.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/ppc/pegasos.c b/hw/ppc/pegasos.c
index 3a498edd16..8ce185de3e 100644
--- a/hw/ppc/pegasos.c
+++ b/hw/ppc/pegasos.c
@@ -847,7 +847,7 @@ static struct {
 static void add_pci_device(PCIBus *bus, PCIDevice *d, void *opaque)
 {
     FDTInfo *fi = opaque;
-    GString *node = g_string_new(NULL);
+    GString *node;
     uint32_t cells[(PCI_NUM_REGIONS + 1) * 5];
     int i, j;
     const char *name = NULL;
@@ -871,6 +871,7 @@ static void add_pci_device(PCIBus *bus, PCIDevice *d, void *opaque)
             break;
         }
     }
+    node = g_string_new(NULL);
     g_string_printf(node, "%s/%s@%x", fi->path, (name ?: pn),
                     PCI_SLOT(d->devfn));
     if (PCI_FUNC(d->devfn)) {
-- 
2.41.3

Re: [PATCH] hw/ppc/pegasos: Fix memory leak

[Peter Maydell]

On Sat, 1 Nov 2025 at 16:53, BALATON Zoltan <balaton@eik.bme.hu> wrote:
>
> Commit 9099b430a4 introduced an early return that caused a leak of a
> GString. Allocate it later to avoid the leak.
>
> Fixes: 9099b430a4 (hw/ppc/pegasos2: Change device tree generation)
> Resolves: Coverity CID 1642027
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [PATCH] hw/ppc/pegasos: Fix memory leak

[Harsh Prateek Bora]

Hi Balaton,

Thanks for taking care of this ...

On 11/1/25 22:22, BALATON Zoltan wrote:
> Commit 9099b430a4 introduced an early return that caused a leak of a
> GString. Allocate it later to avoid the leak.
> 

I think we also want to mention:

Reported-by: Peter Maydell <peter.maydell@linaro.org>

> Fixes: 9099b430a4 (hw/ppc/pegasos2: Change device tree generation)
> Resolves: Coverity CID 1642027
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
>   hw/ppc/pegasos.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/ppc/pegasos.c b/hw/ppc/pegasos.c
> index 3a498edd16..8ce185de3e 100644
> --- a/hw/ppc/pegasos.c
> +++ b/hw/ppc/pegasos.c
> @@ -847,7 +847,7 @@ static struct {
>   static void add_pci_device(PCIBus *bus, PCIDevice *d, void *opaque)
>   {
>       FDTInfo *fi = opaque;
> -    GString *node = g_string_new(NULL);
> +    GString *node;

Curious to know if there were any technical reasons for not using 
g_autoptr which Peter initially suggested ?

Anyways, it fixes the leak, so

Reviewed-by: Harsh Prateek Bora <harshpb@linux.ibm.com>


>       uint32_t cells[(PCI_NUM_REGIONS + 1) * 5];
>       int i, j;
>       const char *name = NULL;
> @@ -871,6 +871,7 @@ static void add_pci_device(PCIBus *bus, PCIDevice *d, void *opaque)
>               break;
>           }
>       }
> +    node = g_string_new(NULL);
>       g_string_printf(node, "%s/%s@%x", fi->path, (name ?: pn),
>                       PCI_SLOT(d->devfn));
>       if (PCI_FUNC(d->devfn)) {

Re: [PATCH] hw/ppc/pegasos: Fix memory leak

[Peter Maydell]

On Mon, 3 Nov 2025 at 02:21, Harsh Prateek Bora <harshpb@linux.ibm.com> wrote:
>
> Hi Balaton,
>
> Thanks for taking care of this ...
>
> On 11/1/25 22:22, BALATON Zoltan wrote:
> > Commit 9099b430a4 introduced an early return that caused a leak of a
> > GString. Allocate it later to avoid the leak.
> >
>
> I think we also want to mention:
>
> Reported-by: Peter Maydell <peter.maydell@linaro.org>

You can if you like, but it's not essential -- I just
forward the info from the coverity reports.

-- PMM

Re: [PATCH] hw/ppc/pegasos: Fix memory leak

[Harsh Prateek Bora]

On 11/3/25 15:09, Peter Maydell wrote:
> On Mon, 3 Nov 2025 at 02:21, Harsh Prateek Bora <harshpb@linux.ibm.com> wrote:
>>
>> Hi Balaton,
>>
>> Thanks for taking care of this ...
>>
>> On 11/1/25 22:22, BALATON Zoltan wrote:
>>> Commit 9099b430a4 introduced an early return that caused a leak of a
>>> GString. Allocate it later to avoid the leak.
>>>
>>
>> I think we also want to mention:
>>
>> Reported-by: Peter Maydell <peter.maydell@linaro.org>
> 
> You can if you like, but it's not essential -- I just
> forward the info from the coverity reports.

Sure, thanks Peter.
I think now that I mentioned it, b4 am will pick it anyway.

regards,
Harsh
> 
> -- PMM

Re: [PATCH] hw/ppc/pegasos: Fix memory leak

[BALATON Zoltan]

On Mon, 3 Nov 2025, Harsh Prateek Bora wrote:
> Hi Balaton,
>
> Thanks for taking care of this ...
>
> On 11/1/25 22:22, BALATON Zoltan wrote:
>> Commit 9099b430a4 introduced an early return that caused a leak of a
>> GString. Allocate it later to avoid the leak.
>> 
>
> I think we also want to mention:
>
> Reported-by: Peter Maydell <peter.maydell@linaro.org>

You can add it on merge.

>> Fixes: 9099b430a4 (hw/ppc/pegasos2: Change device tree generation)
>> Resolves: Coverity CID 1642027
>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>> ---
>>   hw/ppc/pegasos.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>> 
>> diff --git a/hw/ppc/pegasos.c b/hw/ppc/pegasos.c
>> index 3a498edd16..8ce185de3e 100644
>> --- a/hw/ppc/pegasos.c
>> +++ b/hw/ppc/pegasos.c
>> @@ -847,7 +847,7 @@ static struct {
>>   static void add_pci_device(PCIBus *bus, PCIDevice *d, void *opaque)
>>   {
>>       FDTInfo *fi = opaque;
>> -    GString *node = g_string_new(NULL);
>> +    GString *node;
>
> Curious to know if there were any technical reasons for not using g_autoptr 
> which Peter initially suggested ?

Just thought it's simpler and more straight forward this way and saves a 
bit of unnecessary complication. We don't even allocate the string now 
when we exit, with g_autoptr it might do some additional operations 
unnecessarily. As this function otherwise does not have multiple exit 
points just one free at the end works.

> Anyways, it fixes the leak, so
>
> Reviewed-by: Harsh Prateek Bora <harshpb@linux.ibm.com>

Thanks,
BALATON Zoltan

>
>>       uint32_t cells[(PCI_NUM_REGIONS + 1) * 5];
>>       int i, j;
>>       const char *name = NULL;
>> @@ -871,6 +871,7 @@ static void add_pci_device(PCIBus *bus, PCIDevice *d, 
>> void *opaque)
>>               break;
>>           }
>>       }
>> +    node = g_string_new(NULL);
>>       g_string_printf(node, "%s/%s@%x", fi->path, (name ?: pn),
>>                       PCI_SLOT(d->devfn));
>>       if (PCI_FUNC(d->devfn)) {
>
>

Re: [PATCH] hw/ppc/pegasos: Fix memory leak

[Harsh Prateek Bora]

On 11/3/25 18:03, BALATON Zoltan wrote:
> On Mon, 3 Nov 2025, Harsh Prateek Bora wrote:
>> Hi Balaton,
>>
>> Thanks for taking care of this ...
>>
>> On 11/1/25 22:22, BALATON Zoltan wrote:
>>> Commit 9099b430a4 introduced an early return that caused a leak of a
>>> GString. Allocate it later to avoid the leak.
>>>
>>
>> I think we also want to mention:
>>
>> Reported-by: Peter Maydell <peter.maydell@linaro.org>
> 
> You can add it on merge.
> 
>>> Fixes: 9099b430a4 (hw/ppc/pegasos2: Change device tree generation)
>>> Resolves: Coverity CID 1642027
>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>>> ---
>>>   hw/ppc/pegasos.c | 3 ++-
>>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/hw/ppc/pegasos.c b/hw/ppc/pegasos.c
>>> index 3a498edd16..8ce185de3e 100644
>>> --- a/hw/ppc/pegasos.c
>>> +++ b/hw/ppc/pegasos.c
>>> @@ -847,7 +847,7 @@ static struct {
>>>   static void add_pci_device(PCIBus *bus, PCIDevice *d, void *opaque)
>>>   {
>>>       FDTInfo *fi = opaque;
>>> -    GString *node = g_string_new(NULL);
>>> +    GString *node;
>>
>> Curious to know if there were any technical reasons for not using 
>> g_autoptr which Peter initially suggested ?
> 
> Just thought it's simpler and more straight forward this way and saves a 
> bit of unnecessary complication. We don't even allocate the string now 
> when we exit, with g_autoptr it might do some additional operations 
> unnecessarily. As this function otherwise does not have multiple exit 
> points just one free at the end works.

Queued, thanks.