Re: [PATCH v7 4/5] hw/mem/cxl-type3: Add CXL CDAT Data Object Exchange

[Gregory Price]

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);
> +}