On Fri, Apr 21, 2023 at 02:20:20PM +0100, Jonathan Cameron via wrote: > The failure paths in CDAT file loading did not clear up properly. > Change to using g_auto_free and a local pointer for the buffer to > ensure this function has no side effects on error. > Also drop some unnecessary checks that can not fail. > > Cleanup properly after a failure to load a CDAT file. > > Suggested-by: Peter Maydell <peter.mayd...@linaro.org> > Signed-off-by: Jonathan Cameron <jonathan.came...@huawei.com>
Reviewed-by: Fan Ni <fan...@samsung.com> > --- > hw/cxl/cxl-cdat.c | 33 ++++++++++++++++++--------------- > hw/mem/cxl_type3.c | 4 ++++ > hw/pci-bridge/cxl_upstream.c | 3 +++ > 3 files changed, 25 insertions(+), 15 deletions(-) > > diff --git a/hw/cxl/cxl-cdat.c b/hw/cxl/cxl-cdat.c > index 056711d63d..d246d6885b 100644 > --- a/hw/cxl/cxl-cdat.c > +++ b/hw/cxl/cxl-cdat.c > @@ -108,6 +108,7 @@ static void ct3_build_cdat(CDATObject *cdat, Error **errp) > static void ct3_load_cdat(CDATObject *cdat, Error **errp) > { > g_autofree CDATEntry *cdat_st = NULL; > + g_autofree char *buf = NULL; > uint8_t sum = 0; > int num_ent; > int i = 0, ent = 1; > @@ -116,7 +117,7 @@ static void ct3_load_cdat(CDATObject *cdat, Error **errp) > GError *error = NULL; > > /* Read CDAT file and create its cache */ > - if (!g_file_get_contents(cdat->filename, (gchar **)&cdat->buf, > + if (!g_file_get_contents(cdat->filename, (gchar **)&buf, > &file_size, &error)) { > error_setg(errp, "CDAT: File read failed: %s", error->message); > g_error_free(error); > @@ -129,9 +130,17 @@ static void ct3_load_cdat(CDATObject *cdat, Error **errp) > i = sizeof(CDATTableHeader); > num_ent = 1; > while (i < file_size) { > - hdr = (CDATSubHeader *)(cdat->buf + i); > + hdr = (CDATSubHeader *)(buf + i); > + if (i + sizeof(CDATSubHeader) > file_size) { > + error_setg(errp, "CDAT: Truncated table"); > + return; > + } > cdat_len_check(hdr, errp); > i += hdr->length; > + if (i > file_size) { > + error_setg(errp, "CDAT: Truncated table"); > + return; > + } > num_ent++; > } > if (i != file_size) { > @@ -139,33 +148,26 @@ static void ct3_load_cdat(CDATObject *cdat, Error > **errp) > return; > } > > - cdat_st = g_malloc0(sizeof(*cdat_st) * num_ent); > - if (!cdat_st) { > - error_setg(errp, "CDAT: Failed to allocate entry array"); > - return; > - } > + cdat_st = g_new0(CDATEntry, num_ent); > > /* Set CDAT header, Entry = 0 */ > - cdat_st[0].base = cdat->buf; > + cdat_st[0].base = buf; > cdat_st[0].length = sizeof(CDATTableHeader); > i = 0; > > while (i < cdat_st[0].length) { > - sum += cdat->buf[i++]; > + sum += buf[i++]; > } > > /* Read CDAT structures */ > while (i < file_size) { > - hdr = (CDATSubHeader *)(cdat->buf + i); > - cdat_len_check(hdr, errp); > - > + hdr = (CDATSubHeader *)(buf + i); > cdat_st[ent].base = hdr; > cdat_st[ent].length = hdr->length; > > - while (cdat->buf + i < > - (uint8_t *)cdat_st[ent].base + cdat_st[ent].length) { > + while (buf + i < (char *)cdat_st[ent].base + cdat_st[ent].length) { > assert(i < file_size); > - sum += cdat->buf[i++]; > + sum += buf[i++]; > } > > ent++; > @@ -176,6 +178,7 @@ static void ct3_load_cdat(CDATObject *cdat, Error **errp) > } > cdat->entry_len = num_ent; > cdat->entry = g_steal_pointer(&cdat_st); > + cdat->buf = g_steal_pointer(&buf); > } > > void cxl_doe_cdat_init(CXLComponentState *cxl_cstate, Error **errp) > diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c > index abe60b362c..7647122cc6 100644 > --- a/hw/mem/cxl_type3.c > +++ b/hw/mem/cxl_type3.c > @@ -593,6 +593,9 @@ static void ct3_realize(PCIDevice *pci_dev, Error **errp) > cxl_cstate->cdat.free_cdat_table = ct3_free_cdat_table; > cxl_cstate->cdat.private = ct3d; > cxl_doe_cdat_init(cxl_cstate, errp); > + if (*errp) { > + goto err_free_special_ops; > + } > > pcie_cap_deverr_init(pci_dev); > /* Leave a bit of room for expansion */ > @@ -605,6 +608,7 @@ static void ct3_realize(PCIDevice *pci_dev, Error **errp) > > err_release_cdat: > cxl_doe_cdat_release(cxl_cstate); > +err_free_special_ops: > g_free(regs->special_ops); > err_address_space_free: > address_space_destroy(&ct3d->hostmem_as); > diff --git a/hw/pci-bridge/cxl_upstream.c b/hw/pci-bridge/cxl_upstream.c > index 9df436cb73..ef47e5d625 100644 > --- a/hw/pci-bridge/cxl_upstream.c > +++ b/hw/pci-bridge/cxl_upstream.c > @@ -346,6 +346,9 @@ static void cxl_usp_realize(PCIDevice *d, Error **errp) > cxl_cstate->cdat.free_cdat_table = free_default_cdat_table; > cxl_cstate->cdat.private = d; > cxl_doe_cdat_init(cxl_cstate, errp); > + if (*errp) { > + goto err_cap; > + } > > return; > > -- > 2.37.2 > > >