On Wed, 12 Oct 2022 12:01:54 -0400 Gregory Price <gregory.pr...@memverge.com> wrote:
> 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 I just realized that sizeof should be sizeof(**cdat_table) I've moved to a local autofree pointer after factoring out the guts of the code so this gets simpler anyway (and was more obviously wrong!) Jonathan