This code contains heap corruption on free, and I think should be refactored to pre-allocate all the entries we're interested in putting into the table. This would flatten the code and simplify the error handling steps.
Also, should we consider making a union with all the possible entries to make entry allocation easier? It may eat a few extra bytes of memory, but it would simplify the allocation/cleanup code here further. Given that every allocation has to be checked, i'm also not convinced the use of g_autofree is worth the potential footguns associated with it. > diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c > index 568c9d62f5..3fa5d70662 100644 > --- a/hw/mem/cxl_type3.c > +++ b/hw/mem/cxl_type3.c > @@ -12,9 +12,218 @@ > +static int ct3_build_cdat_table(CDATSubHeader ***cdat_table, > + void *priv) > +{ (snip) > + /* For now, no memory side cache, plausiblish numbers */ > + dslbis_nonvolatile = g_malloc(sizeof(*dslbis_nonvolatile) * > dslbis_nonvolatile_num); > + if (!dslbis_nonvolatile) > + return -ENOMEM; this allocation creates a table of entries, which is later freed incorrectly > + > + *cdat_table = g_malloc0(len * sizeof(*cdat_table)); this allocation needs to be checked > + /* Header always at start of structure */ > + if (dsmas_nonvolatile) { > + (*cdat_table)[i++] = g_steal_pointer(&dsmas_nonvolatile); > + } > + if (dslbis_nonvolatile) { > + CDATDslbis *dslbis = g_steal_pointer(&dslbis_nonvolatile); using a local reference used to avoid a g_autofree footgun suggests we should not use g_autofree here, and possibly reconsider the overall strategy for allocation and cleanup > + int j; > + > + for (j = 0; j < dslbis_nonvolatile_num; j++) { > + (*cdat_table)[i++] = (CDATSubHeader *)&dslbis[j]; > + } this fills the CDAT table with sub-references to the table allocated above, which leads to heap corruption with the current code, or complicated cleanup if we decide to keep it > + > + return len; > +} > + > +static void ct3_free_cdat_table(CDATSubHeader **cdat_table, int num, void > *priv) > +{ > + int i; > + And here we free every entry of the table, which can/will cause heap corruption when the sub-table entries are freed > + for (i = 0; i < num; i++) { > + g_free(cdat_table[i]); > + } > + g_free(cdat_table); > +}