| Message ID | 20230531023433.8732-1-lizhijian@cn.fujitsu.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] hw/cxl: Fix CFMW config memory leak | expand |
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)); ---
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)); > > --- >
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)
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(-)