* [PATCH v2] hw/cxl: Fix CFMW config memory leak @ 2023-05-31 2:34 Li Zhijian 2023-05-31 5:45 ` Philippe Mathieu-Daudé 0 siblings, 1 reply; 3+ messages in thread From: Li Zhijian @ 2023-05-31 2:34 UTC (permalink / raw) To: jonathan.cameron, fan.ni; +Cc: qemu-devel, 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> --- 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); } void cxl_fmws_link_targets(CXLState *cxl_state, Error **errp) -- 2.31.1 ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v2] hw/cxl: Fix CFMW config memory leak 2023-05-31 2:34 [PATCH v2] hw/cxl: Fix CFMW config memory leak Li Zhijian @ 2023-05-31 5:45 ` Philippe Mathieu-Daudé 2023-05-31 6:08 ` Zhijian Li (Fujitsu) 0 siblings, 1 reply; 3+ messages in thread From: Philippe Mathieu-Daudé @ 2023-05-31 5:45 UTC (permalink / raw) To: Li Zhijian, jonathan.cameron, fan.ni; +Cc: qemu-devel 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)); --- ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v2] hw/cxl: Fix CFMW config memory leak 2023-05-31 5:45 ` Philippe Mathieu-Daudé @ 2023-05-31 6:08 ` Zhijian Li (Fujitsu) 0 siblings, 0 replies; 3+ messages in thread From: Zhijian Li (Fujitsu) @ 2023-05-31 6:08 UTC (permalink / raw) To: Philippe Mathieu-Daudé, Zhijian Li (Fujitsu), jonathan.cameron@huawei.com, fan.ni@samsung.com Cc: qemu-devel@nongnu.org On 31/05/2023 13:45, Philippe Mathieu-Daudé wrote: > 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? Yeah, you are right. i will update the patch as you suggest. > > -- >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)); > > --- > ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2023-05-31 6:08 UTC | newest] Thread overview: 3+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-05-31 2:34 [PATCH v2] hw/cxl: Fix CFMW config memory leak Li Zhijian 2023-05-31 5:45 ` Philippe Mathieu-Daudé 2023-05-31 6:08 ` Zhijian Li (Fujitsu)
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).