[PATCH] hw/clx: Fix CFMW config memory leak

[PATCH] hw/clx: Fix CFMW config memory leak

[Li Zhijian]

Only 'fw' pointer is marked as g_autofree, so we shoud free other
resource manually in error path.

Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com>
---
 hw/cxl/cxl-host.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/hw/cxl/cxl-host.c b/hw/cxl/cxl-host.c
index 034c7805b3e..dd1a7c83f71 100644
--- a/hw/cxl/cxl-host.c
+++ b/hw/cxl/cxl-host.c
@@ -48,7 +48,7 @@ static void cxl_fixed_memory_window_config(CXLState *cxl_state,
     if (object->size % (256 * MiB)) {
         error_setg(errp,
                    "Size of a CXL fixed memory window must be a multiple of 256MiB");
-        return;
+        goto err_free;
     }
     fw->size = object->size;
 
@@ -57,7 +57,7 @@ static void cxl_fixed_memory_window_config(CXLState *cxl_state,
             cxl_interleave_granularity_enc(object->interleave_granularity,
                                            errp);
         if (*errp) {
-            return;
+            goto err_free;
         }
     } else {
         /* Default to 256 byte interleave */
@@ -68,6 +68,12 @@ static void cxl_fixed_memory_window_config(CXLState *cxl_state,
                                              g_steal_pointer(&fw));
 
     return;
+
+err_free:
+    for (i = 0; i < fw->num_targets && fw->targets[i]; i++) {
+        g_free(fw->targets[i]);
+    }
+    g_free(fw->targets);
 }
 
 void cxl_fmws_link_targets(CXLState *cxl_state, Error **errp)
-- 
2.31.1

Re: [PATCH] hw/clx: Fix CFMW config memory leak

[Jonathan Cameron via]

On Mon, 29 May 2023 15:59:56 +0800
Li Zhijian <lizhijian@cn.fujitsu.com> wrote:

> Only 'fw' pointer is marked as g_autofree, so we shoud free other
> resource manually in error path.
> 
Good spot.

Patch title typo
hw/cxl: 

It's a bit annoying we can't handle this with more autofree magic.
That would work for fw->targets but not for the string duplicates.

One other comment inline,

Thanks,

Jonathan


> Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com>
> ---
>  hw/cxl/cxl-host.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/cxl/cxl-host.c b/hw/cxl/cxl-host.c
> index 034c7805b3e..dd1a7c83f71 100644
> --- a/hw/cxl/cxl-host.c
> +++ b/hw/cxl/cxl-host.c
> @@ -48,7 +48,7 @@ static void cxl_fixed_memory_window_config(CXLState *cxl_state,
>      if (object->size % (256 * MiB)) {
>          error_setg(errp,
>                     "Size of a CXL fixed memory window must be a multiple of 256MiB");
> -        return;
> +        goto err_free;


>      }
>      fw->size = object->size;
>  
> @@ -57,7 +57,7 @@ static void cxl_fixed_memory_window_config(CXLState *cxl_state,
>              cxl_interleave_granularity_enc(object->interleave_granularity,
>                                             errp);
>          if (*errp) {
> -            return;
> +            goto err_free;
>          }
>      } else {
>          /* Default to 256 byte interleave */
> @@ -68,6 +68,12 @@ static void cxl_fixed_memory_window_config(CXLState *cxl_state,
>                                               g_steal_pointer(&fw));
>  
>      return;
> +
> +err_free:
> +    for (i = 0; i < fw->num_targets && fw->targets[i]; i++) {

No need to check fw->targets[i]

> +        g_free(fw->targets[i]);
> +    }
> +    g_free(fw->targets);
>  }
>  
>  void cxl_fmws_link_targets(CXLState *cxl_state, Error **errp)

Re: [PATCH] hw/clx: Fix CFMW config memory leak

[Zhijian Li (Fujitsu)]

On 30/05/2023 23:14, Jonathan Cameron via wrote:
> On Mon, 29 May 2023 15:59:56 +0800
> Li Zhijian <lizhijian@cn.fujitsu.com> wrote:
> 
>> Only 'fw' pointer is marked as g_autofree, so we shoud free other
>> resource manually in error path.
>>
> Good spot.
> 
> Patch title typo
> hw/cxl:> 

OMG, it was the 2nd time i have this typo. i hate my stupid finger.



> It's a bit annoying we can't handle this with more autofree magic.
> That would work for fw->targets but not for the string duplicates.
> 
> One other comment inline,
> 
> Thanks,
> 
> Jonathan
> 
> 
>> Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com>
>> ---
>>   hw/cxl/cxl-host.c | 10 ++++++++--
>>   1 file changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/cxl/cxl-host.c b/hw/cxl/cxl-host.c
>> index 034c7805b3e..dd1a7c83f71 100644
>> --- a/hw/cxl/cxl-host.c
>> +++ b/hw/cxl/cxl-host.c
>> @@ -48,7 +48,7 @@ static void cxl_fixed_memory_window_config(CXLState *cxl_state,
>>       if (object->size % (256 * MiB)) {
>>           error_setg(errp,
>>                      "Size of a CXL fixed memory window must be a multiple of 256MiB");
>> -        return;
>> +        goto err_free;
> 
> 
>>       }
>>       fw->size = object->size;
>>   
>> @@ -57,7 +57,7 @@ static void cxl_fixed_memory_window_config(CXLState *cxl_state,
>>               cxl_interleave_granularity_enc(object->interleave_granularity,
>>                                              errp);
>>           if (*errp) {
>> -            return;
>> +            goto err_free;
>>           }
>>       } else {
>>           /* Default to 256 byte interleave */
>> @@ -68,6 +68,12 @@ static void cxl_fixed_memory_window_config(CXLState *cxl_state,
>>                                                g_steal_pointer(&fw));
>>   
>>       return;
>> +
>> +err_free:
>> +    for (i = 0; i < fw->num_targets && fw->targets[i]; i++) {
> 
> No need to check fw->targets[i]


I knew it's safe to free a NULL pointer. The initial idea was just to make the
loop end as soon as possible :)

Of course, I would also be happy to delete this.


Thanks
Zhijian

> 
>> +        g_free(fw->targets[i]);
>> +    }
>> +    g_free(fw->targets);
>>   }
>>   
>>   void cxl_fmws_link_targets(CXLState *cxl_state, Error **errp)
> 
>