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

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

Hi Li,

On 31/5/23 04:34, Li Zhijian wrote:
> 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>
> ---
> V2: Delete unnecesarry check
> ---
>   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..787a2e779d2 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; i++) {
> +        g_free(fw->targets[i]);
> +    }
> +    g_free(fw->targets);
>   }

IIUC we don't need targets[] before checking errors. What about simply
allocate when we can't fail?

-- >8 --
diff --git a/hw/cxl/cxl-host.c b/hw/cxl/cxl-host.c
index 034c7805b3..f0920da956 100644
--- a/hw/cxl/cxl-host.c
+++ b/hw/cxl/cxl-host.c
@@ -39,12 +39,6 @@ static void cxl_fixed_memory_window_config(CXLState 
*cxl_state,
          return;
      }

-    fw->targets = g_malloc0_n(fw->num_targets, sizeof(*fw->targets));
-    for (i = 0, target = object->targets; target; i++, target = 
target->next) {
-        /* This link cannot be resolved yet, so stash the name for now */
-        fw->targets[i] = g_strdup(target->value);
-    }
-
      if (object->size % (256 * MiB)) {
          error_setg(errp,
                     "Size of a CXL fixed memory window must be a 
multiple of 256MiB");
@@ -64,6 +58,12 @@ static void cxl_fixed_memory_window_config(CXLState 
*cxl_state,
          fw->enc_int_gran = 0;
      }

+    fw->targets = g_malloc0_n(fw->num_targets, sizeof(*fw->targets));
+    for (i = 0, target = object->targets; target; i++, target = 
target->next) {
+        /* This link cannot be resolved yet, so stash the name for now */
+        fw->targets[i] = g_strdup(target->value);
+    }
+
      cxl_state->fixed_windows = g_list_append(cxl_state->fixed_windows,
                                               g_steal_pointer(&fw));

---